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

Verify that client metadata not being mutated #3194

Merged
merged 45 commits into from
Sep 28, 2023
Merged

Verify that client metadata not being mutated #3194

merged 45 commits into from
Sep 28, 2023

Conversation

kropidlowsky
Copy link
Contributor

@kropidlowsky kropidlowsky commented Aug 10, 2023 •
edited by noisersup
Loading

Description

Closes #2989.

Added support for verifying client metadata not being mutated

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • 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.

@kropidlowsky kropidlowsky requested a review from a team as a code owner August 10, 2023 17:07
@codecov
Copy link

codecov bot commented Aug 10, 2023 •
edited
Loading

Codecov Report

Merging #3194 (4e77bb8) into main (7ca090f) will decrease coverage by 0.95%.
The diff coverage is 61.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3194      +/-   ##
==========================================
- Coverage   75.43%   74.49%   -0.95%     
==========================================
  Files         420      421       +1     
  Lines       26179    26238      +59     
==========================================
- Hits        19748    19545     -203     
- Misses       5299     5518     +219     
- Partials     1132     1175      +43
Files Coverage Δ
internal/clientconn/conninfo/conn_info.go 94.28% <100.00%> (+2.28%) ⬆️
internal/handlers/common/client_metadata.go 100.00% <100.00%> (ø)
internal/handlers/commonerrors/error.go 88.23% <ø> (ø)
internal/handlers/commonerrors/errorcode_string.go 80.00% <ø> (ø)
internal/handlers/pg/cmd_query.go 19.04% <100.00%> (ø)
internal/handlers/sqlite/cmd_query.go 32.00% <100.00%> (ø)
internal/handlers/common/ismaster.go 86.95% <25.00%> (-13.05%) ⬇️
internal/handlers/hana/cmd_query.go 0.00% <0.00%> (ø)
internal/handlers/sqlite/msg_hello.go 88.00% <57.14%> (-12.00%) ⬇️
internal/handlers/sqlite/msg_ismaster.go 78.57% <62.50%> (-21.43%) ⬇️
... and 2 more

... and 36 files with indirect coverage changes

Flag Coverage Δ
filter-true 71.13% <61.90%> (-0.85%) ⬇️
hana-1 ?
integration 71.13% <61.90%> (-0.85%) ⬇️
mongodb-1 4.61% <0.00%> (-0.02%) ⬇️
pg-1 41.35% <36.50%> (+0.47%) ⬆️
pg-2 42.46% <34.92%> (-0.90%) ⬇️
pg-3 ?
postgresql-1 ∅ <ø> (∅)
postgresql-2 40.12% <36.50%> (-0.72%) ⬇️
postgresql-3 41.49% <57.14%> (+1.13%) ⬆️
sort-false 71.13% <61.90%> (-0.85%) ⬇️
sqlite-1 40.38% <38.09%> (+0.48%) ⬆️
sqlite-2 41.65% <36.50%> (-1.02%) ⬇️
sqlite-3 43.24% <57.14%> (+1.07%) ⬆️
unit 24.53% <39.68%> (+0.02%) ⬆️

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

@noisersup noisersup requested review from b1ron, a team and noisersup August 10, 2023 20:53
rumyantseva
rumyantseva suggested changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@rumyantseva rumyantseva left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After fixing the problem with closure, you can run the new test against MongoDB only:

go test -count=1 -run='TestMutatingClientMetadata' -target-url=mongodb://127.0.0.1:47017/ -target-backend=mongodb .

It seems to me that the test fails.

@kropidlowsky
Copy link
Contributor Author

Hi @rumyantseva thanks!
I have pushed requires changes.

With regard to running the single integration test I was trying to follow previous approach - task test-integration-pg TEST_RUN=TestName/TestCaseName which used to be in CONTRIBUTING.md.

rumyantseva
rumyantseva reviewed Aug 14, 2023
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.

Thank you very much for contributing! I tried the implementation and left my thoughts.

integration/basic_test.go Show resolved Hide resolved
internal/handlers/common/common.go Outdated Show resolved Hide resolved
@kropidlowsky
Copy link
Contributor Author

Hi @rumyantseva,

Thank you very much for the review!

I have pushed the required changes.

@kropidlowsky
Copy link
Contributor Author

@noisersup PTAL

@mergify mergify bot added the conflict PRs that have merge conflicts label Sep 19, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label Sep 19, 2023
AlekSi
AlekSi reviewed Sep 20, 2023
View reviewed changes
internal/clientconn/conninfo/conn_info.go Outdated Show resolved Hide resolved
internal/handlers/common/metadata.go Outdated Show resolved Hide resolved
@AlekSi
Copy link
Member

AlekSi commented Sep 25, 2023

@kropidlowsky please re-request review if it is ready :)

@kropidlowsky
Copy link
Contributor Author

@AlekSi integration tests continue to explode. Can you please advise? I would appreciate it

@AlekSi AlekSi modified the milestones: v1.11.0, Next Sep 25, 2023
@AlekSi AlekSi self-requested a review September 26, 2023 15:31
@AlekSi AlekSi requested review from a team September 27, 2023 12:12
@AlekSi
Copy link
Member

AlekSi commented Sep 27, 2023

@kropidlowsky the root cause is fixed in be944e7

@AlekSi AlekSi enabled auto-merge (squash) September 27, 2023 12:13
chilagrow
chilagrow approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for your contribution 🤗

@AlekSi
Copy link
Member

AlekSi commented Sep 28, 2023

@rumyantseva PTAL

rumyantseva
rumyantseva approved these changes Sep 28, 2023
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.

:shipit:

@AlekSi AlekSi merged commit 8788b92 into FerretDB:main Sep 28, 2023
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@AlekSi AlekSi AlekSi approved these changes

@b1ron b1ron Awaiting requested review from b1ron

@noisersup noisersup Awaiting requested review from noisersup

Assignees

@kropidlowsky kropidlowsky

Labels
code/bug Some user-visible feature works incorrectly
Projects
Current Engineering
Archived in project
Milestone
v1.12.1
Development

Successfully merging this pull request may close these issues.

Verify that client metadata document has not been mutated
5 participants
@kropidlowsky @AlekSi @rumyantseva @chilagrow @noisersup