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 and collection names for SQLite handler #2868

Merged
merged 89 commits into from Jul 26, 2023
Merged

Validate database and collection names for SQLite handler #2868

merged 89 commits into from Jul 26, 2023

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented Jun 20, 2023 •
edited

Description

Closes #2749.

This PR is a part of the issue, database validation will be handled in second PR.

TODO

  • Verify insert and update test cases
[user@nixpc:~/FerretDB]$ task test-integration-sqlite TEST_RUN='TestCollectionName'
task: [test-integration-sqlite] go test -count=1 -run='TestCollectionName' -timeout=35m -race=true -tags=ferretdb_debug,ferretdb_tigris,ferretdb_hana -shuffle=on -coverpkg=../... -coverprofile=integration-sqlite.txt . -target-backend=ferretdb-sqlite -target-tls -compat-url='mongodb://username:password@127.0.0.1:47018/?tls=true&tlsCertificateKeyFile=../build/certs/client.pem&tlsCaFile=../build/certs/rootCA-cert.pem' -disable-filter-pushdown

ok      github.com/FerretDB/FerretDB/integration        1.453s  coverage: 40.0% of statements in ../...

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@noisersup noisersup self-assigned this Jun 20, 2023
@noisersup noisersup added code/chore Code maintenance improvements not ready Issues that are not ready to be worked on; PRs that should skip CI backend/sqlite Issues about SQLite backend labels Jun 20, 2023
@noisersup noisersup changed the title Validate database and collection names for SQLite handler Validate collection names for SQLite handler Jun 23, 2023
@noisersup noisersup removed the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023 •
edited

Codecov Report

Merging #2868 (c39f316) into main (ee41035) will decrease coverage by 1.12%.
The diff coverage is 51.51%.

❗ Current head c39f316 differs from pull request most recent head e266e35. Consider uploading reports for the commit e266e35 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2868      +/-   ##
==========================================
- Coverage   76.56%   75.44%   -1.12%     
==========================================
  Files         390      391       +1     
  Lines       21419    21594     +175     
==========================================
- Hits        16399    16292     -107     
- Misses       4098     4346     +248     
- Partials      922      956      +34
Files Changed Coverage Δ
internal/backends/collection.go 100.00% <ø> (ø)
internal/backends/error.go 54.28% <ø> (-8.58%) ⬇️
internal/backends/errorcode_string.go 50.00% <ø> (ø)
internal/handlers/pg/msg_insert.go 88.42% <0.00%> (ø)
internal/handlers/pg/msg_update.go 78.40% <0.00%> (ø)
internal/handlers/pg/pgdb/collections.go 71.20% <ø> (ø)
internal/handlers/sqlite/msg_count.go 0.00% <0.00%> (-81.25%) ⬇️
internal/handlers/sqlite/msg_dropdatabase.go 63.88% <0.00%> (-1.83%) ⬇️
internal/handlers/sqlite/msg_listcollections.go 61.76% <14.28%> (-14.05%) ⬇️
internal/handlers/sqlite/msg_drop.go 54.71% <18.18%> (-24.83%) ⬇️
... and 14 more

... and 18 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 71.93% <51.51%> (-1.17%) ⬇️
mongodb 5.46% <0.00%> (-0.05%) ⬇️
pg 65.99% <0.75%> (-0.53%) ⬇️
shard-1 49.54% <0.00%> (-6.35%) ⬇️
shard-2 53.53% <51.51%> (-0.32%) ⬇️
shard-3 52.93% <0.00%> (-6.28%) ⬇️
sqlite 35.68% <50.75%> (-10.76%) ⬇️
unit 23.93% <0.00%> (-0.21%) ⬇️

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

@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2023

@noisersup this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Jun 23, 2023
@AlekSi
Copy link
Member

AlekSi commented Jun 26, 2023

CleanShot 2023-06-26 at 09 59 32@2x

PR is not an issue

@AlekSi AlekSi removed the backend/sqlite Issues about SQLite backend label Jun 26, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label Jun 26, 2023
@AlekSi AlekSi added not ready Issues that are not ready to be worked on; PRs that should skip CI and removed do not merge PRs that should not be merged labels Jul 20, 2023
@AlekSi AlekSi removed the not ready Issues that are not ready to be worked on; PRs that should skip CI label Jul 26, 2023
noisersup
noisersup commented Jul 26, 2023
View reviewed changes
Copy link
Member Author

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

The name validation on the contract level enforces more code related to error handling, although it seems like the most reasonable approach.

noisersup
noisersup commented Jul 26, 2023
View reviewed changes
internal/handlers/sqlite/msg_dropdatabase.go
@@ -59,6 +59,8 @@ func (h *Handler) MsgDropDatabase(ctx context.Context, msg *wire.OpMsg) (*wire.O
switch {
case err == nil:
res.Set("dropped", dbName)
case backends.ErrorCodeIs(err, backends.ErrorCodeDatabaseNameIsInvalid):
// nothing?
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can decide based on what MongoDB does for dropping invalid names, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, a test would be needed there, but that's not super important

@AlekSi AlekSi changed the title Validate collection names for SQLite handler Validate database and collection names for SQLite handler Jul 26, 2023
@AlekSi AlekSi added code/enhancement Some user-visible feature could work better and removed code/chore Code maintenance improvements labels Jul 26, 2023
@AlekSi AlekSi marked this pull request as ready for review July 26, 2023 14:52
@AlekSi AlekSi merged commit a398e31 into FerretDB:main Jul 26, 2023
23 of 42 checks passed
yonarw pushed a commit to yonarw/FerretDB that referenced this pull request Aug 31, 2023
@noisersup @yonarw
Validate database and collection names for SQLite handler (FerretDB#2868
5362fc9 
)

Co-authored-by: Dmitry <dmitry.eremenko@ferretdb.io>
Co-authored-by: Alexey Palazhchenko <alexey.palazhchenko@ferretdb.io>

Closes FerretDB#2749.
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

@rumyantseva rumyantseva rumyantseva left review comments

@chilagrow chilagrow chilagrow left review comments

@AlekSi AlekSi Awaiting requested review from AlekSi AlekSi is a code owner

Assignees

@AlekSi AlekSi

Labels
code/enhancement Some user-visible feature could work better
Projects
Current Engineering
Archived in project
Milestone
v1.7.0
Development

Successfully merging this pull request may close these issues.

Validate database and collection names
5 participants
@noisersup @AlekSi @w84thesun @rumyantseva @chilagrow