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 database names #913

Merged
merged 25 commits into from
Jul 20, 2022
Merged

Validate database names #913

merged 25 commits into from
Jul 20, 2022

Conversation

seeforschauer
Copy link
Contributor

@seeforschauer seeforschauer commented Jul 18, 2022

Description

This PR is a part of #807.

  • removed TestFindAndModifyEmptyCollectionName test as the same test already done in TestCollectionName.
  • database test names generation changed from sign - to sign _ to conform the rules.

Error codes

Test Case MongoDB FerretDB Comment
too long name Invalid namespace specified, quoted Invalid namespace, not quoted specified word in MongoDB, quotation differs
$^%*& Invalid namespace, not quoted Invalid namespace, not quoted equal

Dance PR FerretDB/dance#176.

Checklist

  • I added tests for new functionality or bugfixes.
  • I ran task all, and it passed.
  • I added/updated comments for both exported and unexported top-level declarations (functions, types, etc).
  • I checked comments rendering with task godocs.
  • (for maintainers only) I set Reviewers, Assignee, Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@seeforschauer seeforschauer self-assigned this Jul 18, 2022
@seeforschauer seeforschauer added the code/feature Some user-visible feature is not implemented yet label Jul 18, 2022
@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #913 (3aa5611) into main (38f0537) will increase coverage by 0.00%.
The diff coverage is 53.57%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #913   +/-   ##
=======================================
  Coverage   59.70%   59.70%           
=======================================
  Files         224      224           
  Lines       10184    10201   +17     
=======================================
+ Hits         6080     6091   +11     
- Misses       3393     3400    +7     
+ Partials      711      710    -1     
Impacted Files Coverage Δ
internal/handlers/pg/msg_insert.go 39.39% <0.00%> (-2.55%) ⬇️
internal/handlers/pg/msg_update.go 50.94% <0.00%> (-1.32%) ⬇️
internal/handlers/pg/pgdb/settings.go 39.53% <ø> (ø)
internal/handlers/tigris/msg_update.go 0.00% <ø> (ø)
internal/handlers/pg/pgdb/databases.go 60.00% <62.50%> (+0.29%) ⬆️
internal/handlers/pg/msg_create.go 75.71% <100.00%> (+4.07%) ⬆️
internal/handlers/pg/pgdb/collections.go 60.17% <100.00%> (+1.91%) ⬆️
internal/handlers/pg/pgdb/tables.go 64.70% <100.00%> (ø)
internal/util/testutil/db.go 100.00% <100.00%> (ø)
internal/util/version/version.go 55.00% <0.00%> (-5.00%) ⬇️
... and 5 more
Flag Coverage Δ
integration 61.89% <50.00%> (-0.01%) ⬇️
mongodb 18.88% <14.28%> (-0.08%) ⬇️
pg 61.85% <50.00%> (-0.01%) ⬇️
tigris 49.37% <ø> (ø)
unit 25.13% <21.42%> (-0.05%) ⬇️

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

@seeforschauer seeforschauer linked an issue Jul 19, 2022 that may be closed by this pull request
4 tasks
@seeforschauer seeforschauer marked this pull request as ready for review July 19, 2022 05:23
@seeforschauer seeforschauer requested review from a team and AlekSi as code owners July 19, 2022 05:23
@seeforschauer seeforschauer enabled auto-merge (squash) July 19, 2022 05:24
w84thesun
w84thesun previously approved these changes Jul 19, 2022
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@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.

A few things are not really clear to me, asked questions.

README.md Show resolved Hide resolved
integration/basic_test.go Show resolved Hide resolved
integration/basic_test.go Outdated Show resolved Hide resolved
integration/findandmodify_test.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/databases.go Outdated Show resolved Hide resolved
@rumyantseva
Copy link
Member

P.S. @seeforschauer could you also check MsgUpdate? I think it calls collection creation somewhere under the hood as well.

@seeforschauer seeforschauer removed the request for review from w84thesun July 19, 2022 10:17
noisersup
noisersup previously approved these changes Jul 19, 2022
Copy link
Member

@noisersup noisersup 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 19, 2022
rumyantseva
rumyantseva previously approved these changes Jul 19, 2022
Copy link
Member

@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.

I have a couple of minor comments, but in general it looks good to me.

internal/handlers/pg/msg_update.go Show resolved Hide resolved
Co-authored-by: Elena Grahovac <elena@grahovac.me>
@seeforschauer
Copy link
Contributor Author

seeforschauer commented Jul 19, 2022

I committed the suggested change and all 3 approvals were dismissed. can we do anything with It?

rumyantseva
rumyantseva previously approved these changes Jul 19, 2022
Copy link
Member

@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.

I don't have critical comments, but I guess this PR might need a final look from @AlekSi

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

:shipit:

@seeforschauer seeforschauer merged commit 1ea7936 into FerretDB:main Jul 20, 2022
@seeforschauer seeforschauer deleted the issue-807-validate-db-names branch July 20, 2022 10:37
seeforschauer added a commit to FerretDB/dance that referenced this pull request Jul 21, 2022
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 database names
5 participants