Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate collection names #844

Merged
merged 32 commits into from
Jul 7, 2022
Merged

Validate collection names #844

merged 32 commits into from
Jul 7, 2022

Conversation

seeforschauer
Copy link
Contributor

@seeforschauer seeforschauer commented Jul 5, 2022

This PR refs #806.

Difference to be documented in dance tests:

  • dots
  • dashes
  • max length in FerretDB: 120, in MongoDB: 255.
  • FerretDB reserved prefix is _ferretdb_, MongoDB - system..

@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #844 (11495ad) into main (ced68a5) will decrease coverage by 0.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   59.09%   59.07%   -0.02%     
==========================================
  Files         218      218              
  Lines        9845     9855      +10     
==========================================
+ Hits         5818     5822       +4     
- Misses       3318     3322       +4     
- Partials      709      711       +2     
Impacted Files Coverage Δ
internal/handlers/common/error.go 82.65% <ø> (ø)
internal/handlers/pg/pgdb/settings.go 37.20% <ø> (ø)
internal/handlers/pg/pgdb/pool.go 57.88% <57.14%> (-0.10%) ⬇️
internal/handlers/pg/msg_create.go 69.35% <100.00%> (+2.11%) ⬆️
internal/util/testutil/pg.go 40.74% <100.00%> (ø)
internal/util/version/version.go 55.00% <0.00%> (-5.00%) ⬇️
Flag Coverage Δ
integration 61.51% <76.92%> (-0.17%) ⬇️
mongodb 18.76% <23.07%> (-0.06%) ⬇️
pg 61.44% <76.92%> (-0.17%) ⬇️
unit 24.73% <15.38%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@seeforschauer seeforschauer self-assigned this Jul 5, 2022
@AlekSi AlekSi linked an issue Jul 5, 2022 that may be closed by this pull request
4 tasks
@AlekSi AlekSi added this to the v0.5.0 milestone Jul 5, 2022
@AlekSi AlekSi added the code/feature Some user-visible feature is not implemented yet label Jul 5, 2022
@seeforschauer seeforschauer marked this pull request as ready for review July 5, 2022 14:19
@seeforschauer seeforschauer requested review from a team and noisersup and removed request for a team July 5, 2022 14:19
@seeforschauer seeforschauer requested review from AlekSi and w84thesun July 6, 2022 07:09
w84thesun
w84thesun previously approved these changes Jul 6, 2022
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the PR body there is a case max length in FerretDB: 255, in MongoDB: 63.. Should it be max length in FerretDB: 119? or what 119 is about? :)

  2. Are dance cases in the scope of this task? Or is there a separate issue for it?

  3. It's great that there is a new piece of doc for README. It's not 100% full/accurate (I added a couple of comments about it).

  4. Special thanks for updating CreateCollection godoc.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
@seeforschauer
Copy link
Contributor Author

@rumyantseva

  1. In the PR body there is a case max length in FerretDB: 255, in MongoDB: 63..

Please pay attention to the caption - it's a difference between FerretDB and MongoDB.
I listed these differences in the PR body to remember what cases to cover, document, and mention in the dance tests.

Should it be max length in FerretDB: 119? or what 119 is about? :)

Not sure about 119, it's a magic number for now. And yes, this magic number is not in the constant somewhere, I follow the task guideline that we can change it later.

  1. Are dance cases in the scope of this task? Or is there a separate issue for it?

It is in the scope of the task and it is not in the scope of the PR since the repository is FerretDB.

  1. It's great that there is a new piece of doc for README. It's not 100% full/accurate (I added a couple of comments about it).

Added, thank you for noting this.

  1. Special thanks for updating CreateCollection godoc.

You are welcome ^ - ^. I'm glad I have a command to check what docs look like (thanks to @AlekSi).

rumyantseva
rumyantseva previously approved these changes Jul 6, 2022
Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

w84thesun
w84thesun previously approved these changes Jul 6, 2022
README.md Outdated Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
@AlekSi AlekSi enabled auto-merge (squash) July 7, 2022 18:33
@AlekSi AlekSi merged commit fba5737 into FerretDB:main Jul 7, 2022
@seeforschauer seeforschauer deleted the issues-806 branch July 8, 2022 07:49
@seeforschauer seeforschauer mentioned this pull request Jul 19, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/feature Some user-visible feature is not implemented yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate collection names
4 participants