-
Notifications
You must be signed in to change notification settings - Fork 16
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
Update Many command implementation #192
Conversation
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadAndUpdateOperation.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/UpdateOperationPage.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/updater/DocumentUpdater.java
Show resolved
Hide resolved
src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it all looks good, but I am unsure if that calculation of updated
could lead to operations skipped -- I think it does?
""" | ||
{ | ||
"updateMany": { | ||
"filter" : {"_id": "doc6"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another detail worth confirming: do additional fields in the filter get added to the resulting document on upsert? For example, if this is "filter" : {"_id": "doc6", "answer": 42}
, does the resulting document have an answer
property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question: I think method "getEmptyDocument(s)" (renamed as "getNewDocument") is only inserting document id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkarpov15 During upsert the getNewDocument() method creates document populated with _id if it is part of the filter condition. So the resulting document will not have the answer property populated based on the filter condition. I will add a test case for this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maheshrajamani this is a somewhat important detail. Upserting should apply any fields from the filter that don't have a non- $eq
query operator. Below is some output from the mongodb shell that shows this behavior.
Mongoose does use this behavior for setting schema defaults on upsert.
MongoDB Enterprise > db.tests.countDocuments({})
0
MongoDB Enterprise > db.tests.updateOne({ _id: 'foo', answer: 42 }, { $set: { bar: 'baz' } }, { upsert: true })
{
"acknowledged" : true,
"matchedCount" : 0,
"modifiedCount" : 0,
"upsertedId" : "foo"
}
MongoDB Enterprise > db.tests.findOne()
{ "_id" : "foo", "answer" : 42, "bar" : "baz" }
MongoDB Enterprise >
MongoDB Enterprise > db.tests.updateOne({ _id: 'foo', answer: { $eq: 42 }, count: { $gte: 42 } }, { $set: { bar: 'baz' } }, { upsert: true })
{
"acknowledged" : true,
"matchedCount" : 0,
"modifiedCount" : 0,
"upsertedId" : "foo"
}
MongoDB Enterprise > db.tests.findOne()
{ "_id" : "foo", "answer" : 42, "bar" : "baz" }
MongoDB Enterprise >
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vkarpov15 Got it. Will add equals condition filter fields to the newly created document and will submit a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in PR #200
What this PR does:
Update many command implementation
Response change for UpdateOne and FindOneAndUpdate
Which issue(s) this PR fixes:
Fixes #145 #168 and #169
Checklist