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

Support multi update's argument #790

Merged
merged 16 commits into from
Aug 1, 2022

Conversation

fcoury
Copy link
Contributor

@fcoury fcoury commented Jun 26, 2022

Closes #486

This PR accepts the additional optional flag for multiple updates straight on the update command:

https://www.mongodb.com/docs/manual/reference/method/db.collection.update/#std-label-update-multi

Since there is no high level API to call update (only UpdateOne or UpdateMany), I added a test that uses the lower level RunCommand API, providing the expected update command payload document.

My strategy on this particular test was to add two documents with a same field (x: 1) and then update using multi: false UpdateOne first, and checking that only the first document was updated; and then update using multi: true UpdateMany making sure both documents were modified.

Checklist

  • Tests are added for new functionality or fixed bugs.
  • task all passes.

See CONTRIBUTING.md for more details.

@ferretdb-bot
Copy link
Member

ferretdb-bot commented Jun 26, 2022

CLA assistant check
All committers have signed the CLA.

@AlekSi AlekSi linked an issue Jun 27, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #790 (6c51fe8) into main (3eec3c2) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #790      +/-   ##
==========================================
- Coverage   62.51%   62.45%   -0.07%     
==========================================
  Files         231      231              
  Lines       10798    10810      +12     
==========================================
+ Hits         6750     6751       +1     
- Misses       3273     3284      +11     
  Partials      775      775              
Impacted Files Coverage Δ
internal/handlers/tigris/msg_update.go 0.00% <0.00%> (ø)
internal/handlers/pg/msg_update.go 50.87% <100.00%> (+1.78%) ⬆️
internal/handlers/pg/pgdb/collections.go 54.86% <0.00%> (-5.31%) ⬇️
internal/util/version/version.go 55.00% <0.00%> (-5.00%) ⬇️
internal/handlers/pg/pgdb/databases.go 60.00% <0.00%> (+5.33%) ⬆️
Flag Coverage Δ
integration 57.25% <50.00%> (-0.04%) ⬇️
mongodb 18.56% <0.00%> (-0.07%) ⬇️
pg 61.87% <100.00%> (-0.01%) ⬇️
tigris 22.61% <0.00%> (-0.06%) ⬇️
unit 24.99% <0.00%> (+0.16%) ⬆️

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

@AlekSi AlekSi added the code/feature Some user-visible feature is not implemented yet label Jun 27, 2022
@AlekSi
Copy link
Member

AlekSi commented Jun 27, 2022

Since there is no high level API to call update (only UpdateOne or UpdateMany)

I think they use update command under the hood, setting multi flag to false (or omitting it) and true

@fcoury
Copy link
Contributor Author

fcoury commented Jun 27, 2022

Since there is no high level API to call update (only UpdateOne or UpdateMany)

I think they use update command under the hood, setting multi flag to false (or omitting it) and true

Updated the test case to use UpdateOne and UpdateMany. Also added an additional test case to make sure multi falls back to false when absent.

Now the behavior is the same when testing from the go driver and mongosh:

FerretDB
image

MongoDB
image

@fcoury
Copy link
Contributor Author

fcoury commented Jun 27, 2022

Rebased from new main branch.

@fcoury
Copy link
Contributor Author

fcoury commented Jun 29, 2022

Ready for review.

@fcoury fcoury closed this Jun 29, 2022
@fcoury fcoury reopened this Jun 29, 2022
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.

May you also port those changes to tigris/msg_update.go. No tests are necessary for that handler yet

integration/update_test.go Outdated Show resolved Hide resolved
@AlekSi AlekSi requested a review from gevorgyg June 29, 2022 16:36
@AlekSi
Copy link
Member

AlekSi commented Jun 29, 2022

@GinGin3203 PTAL if you have a moment :)

integration/update_test.go Outdated Show resolved Hide resolved
integration/update_test.go Outdated Show resolved Hide resolved
integration/update_test.go Outdated Show resolved Hide resolved
internal/handlers/pg/msg_update.go Outdated Show resolved Hide resolved
@fcoury fcoury requested a review from a team as a code owner July 28, 2022 10:47
@w84thesun w84thesun requested review from AlekSi, a team and noisersup and removed request for a team July 28, 2022 13:31
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.

It doesn't look correct to me, but maybe I just don't understand something.

integration/update_test.go Show resolved Hide resolved
internal/handlers/pg/msg_update.go Show resolved Hide resolved
@AlekSi AlekSi enabled auto-merge (squash) July 29, 2022 11:59
@AlekSi AlekSi changed the title Add multi support to updates Support multi update's argument Jul 29, 2022
@AlekSi AlekSi disabled auto-merge July 29, 2022 12:00
@AlekSi AlekSi enabled auto-merge (squash) July 29, 2022 12:01
@AlekSi AlekSi added this to the v0.6.0 milestone Aug 1, 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.

LGTM

@AlekSi AlekSi merged commit 288018a into FerretDB:main Aug 1, 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.

Support multi update's argument
6 participants