DRIVERS-2822 Add sort option to updateOne
#1644
Conversation
qingyang-hu
requested review from
jmikola and
blink1073
and removed request for
a team
| object: collection0 | ||
| arguments: | ||
| filter: { _id: { $gt: 1 } } | ||
| sort: { _id: -1 } |
There was a problem hiding this comment.
This test is a bit odd for drivers like pymongo that don't share options between update_one and update_many. Could we add a comment here that explains that this test should be skipped for such drivers? I would not want implementors to accidentally add a non-functional sort option just to pass this test.
There was a problem hiding this comment.
I'd be OK with removing this test entirely since it's not testing client-side behavior at all.
We should be able to trust that the server is testing that multi: true and sort conflict.
There was a problem hiding this comment.
I will remove this test if the Go driver is the only one who shares the UpdateOptions for both update_one and update_many. I will also separate UpdateOneOptions and UpdateManyOptions because the Go driver will introduce a new options interface anyway in v2.0.
There was a problem hiding this comment.
Not a huge fan of duplicating UpdateOneOptions and UpdateManyOptions. I think it was enough to have UpdateOptions with the note that sort does not work with UpdateMany but I'll defer to others review.
There was a problem hiding this comment.
In the interest of pragmatism, I'm completely OK with keeping the singular UpdateOptions and leaving a note about sort not applying to updateMany.
@qingyang-hu: Also, I think this was previously overlooked in one of my previous comments, but shouldn't this PR entail a change to Write Models in the bulk write spec? That has a separate model classes for UpdateOne and ReplaceOne, so sort can just go where (no concern for a common options interface as we have in the CRUD spec).
Note that the bulk write spec also has separate tests (client-bulkWrite- prefix), so I don't think the existing bulkWrite-*yml tests in this PR are sufficient.
|
Removing myself, I'll defer to Jeremy + the bulk write spec authors. |
|
@qingyang-hu: Please ping me for another review once the Bulk Write spec/tests are updated. |
|
@jmikola, can you please take a look again? I've added the sort to the client-level bulk write. |
blink1073
requested review from
ShaneHarvey
and removed request for
ShaneHarvey
|
@jmikola, can you please review the updates? |
| - { _id: 3, x: 33 } | ||
|
|
||
| _yamlAnchors: | ||
| namespace: &namespace "crud-tests.coll0" |
There was a problem hiding this comment.
Thanks for using the convention from Top-level Fields here.
Summary
Add a sort option to
updateOne()so if it matches more than one candidate document the first one matched by the sort order will be updated.Background & Motivation
The new syntax allows a sort option to be supplied to an update command. The upstream server changes are at https://jira.mongodb.org/browse/SERVER-78140.
Some example commands can be found at: mongodb/mongo@e443421#diff-c9ff788fbdeb3594c45d85c449a%5B%E2%80%A6%5D21f5ef32ede8f9c858e6e3072e0c8R27
The corresponding mongosh docs:
https://www.mongodb.com/docs/upcoming/reference/method/db.collection.updateOne/
Though the command docs have not been updated yet.
Please complete the following before merging:
clusters, and serverless).