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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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 •
edited
Loading

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
Validate database names #807
Closed
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
View reviewed changes
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

rumyantseva
rumyantseva suggested changes Jul 19, 2022
View reviewed changes
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.

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
Contributor

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
View reviewed changes
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

@seeforschauer seeforschauer mentioned this pull request Jul 19, 2022
Merged
w84thesun
w84thesun previously approved these changes Jul 19, 2022
View reviewed changes
rumyantseva
rumyantseva previously approved these changes Jul 19, 2022
View reviewed changes
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.

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
@seeforschauer @rumyantseva
Update internal/handlers/pg/pgdb/settings.go
0b59b3f 
Co-authored-by: Elena Grahovac <elena@grahovac.me>
@seeforschauer seeforschauer dismissed stale reviews from rumyantseva, w84thesun, and noisersup via 0b59b3f July 19, 2022 17:53
@seeforschauer
Copy link
Contributor Author

seeforschauer commented Jul 19, 2022 •
edited
Loading

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
View reviewed changes
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.

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

AlekSi
AlekSi reviewed Jul 19, 2022
View reviewed changes
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.

https://github.com/FerretDB/FerretDB/pull/913/files#r924907975

AlekSi
AlekSi approved these changes Jul 20, 2022
View reviewed changes
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
@seeforschauer
Validate database names (#176)
af18f9a 
Closes FerretDB/FerretDB#807.

Refs FerretDB/FerretDB#913.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w84thesun w84thesun w84thesun approved these changes

@AlekSi AlekSi AlekSi approved these changes

@noisersup noisersup Awaiting requested review from noisersup

@rumyantseva rumyantseva Awaiting requested review from rumyantseva

Assignees

@seeforschauer seeforschauer

Labels
code/feature Some user-visible feature is not implemented yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate database names
5 participants
@seeforschauer @rumyantseva @AlekSi @w84thesun @noisersup