-
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
UpdateOneCommand implementation. #50
Conversation
Also enable array and subdoc update.
value = | ||
""" | ||
{ | ||
"status": { |
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.
This seems different from what Mongo returns as per https://www.mongodb.com/docs/manual/reference/method/db.collection.updateOne/, which says
The method returns a document that contains:
'matchedCount' containing the number of matched documents
'modifiedCount' containing the number of modified documents
'upsertedId' containing the _id for the upserted document.
A 'boolean' acknowledged as true if the operation ran with write concern or false if write concern was disabled
we can talk about this tomorrow, change in follow-up PR if needs changing.
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 agree. The distinction between matchedCount
and modifiedCount
is somewhat important as well depending on how you've implemented updatedIds
.
matchedCount
is the number of documents that matched the filter. 0 or 1 for updateOne
. And modifiedCount
is the number of documents that changed.
You may have a case where matchedCount = 1
but modifiedCount = 0
if either (1) no properties in the document changed, for example $set: { answer: 42 }
when answer is already 42, or (2) update operators didn't apply, for example $setOnInsert: { answer: 42 }
if no upsert occurred.
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 point on details wrt modifiedCount
. This needs to be considered on server side, so that update operations need to indicate if any changes were made.
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'd also advise against using updatedIds
because that might mean we need to have separate return values for updateOne()
and updateMany()
. updateMany()
may update millions of documents - sending all those ids over the network may be extremely slow.
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 Are you suggesting not to return updateIds for updateOne() also?
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.
Yes. For the sake of consistency between updateOne()
and updateMany()
.
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.
Looks good for everything else expect for return value for operation which I think might be different from what Mongo returns.
Issue #51 and also enable array and subdoc update.