Allow updateOne and replaceOne to supply sort option #1585
Conversation
joykim1005
requested review from
a team and
jyemin
and removed request for
a team
There was a problem hiding this comment.
Looking good overall! I noticed in the spec changes here: https://github.com/mongodb/specifications/pull/1644/files#diff-d6a7b61abed64b14a2e1eb4827556e8ed74c84bc8ae1f635ff8de67a4cfbcfba that ReplaceOneOptions might also need to be updated. Should we include that in this PR?
| @@ -40,6 +41,7 @@ public class UpdateOptions { | |||
| private String hintString; | |||
| private BsonValue comment; | |||
| private Bson variables; | |||
| private BsonDocument sort; | |||
There was a problem hiding this comment.
Let’s use the Bson type here to allow users to choose between Document and BsonDocument, similar to how it’s done in FindOneAndDeleteOptions and other cases.
| * | ||
| * @return a document describing the sort criteria | ||
| * @since 5.3 | ||
| * @mongodb.driver.manual reference/method/cursor.sort/ Sort |
There was a problem hiding this comment.
Should we remove the manual reference here and from the getSort method, given that it seems more relevant to the cursor?
There was a problem hiding this comment.
I referenced the findOptions and the other findOneAnd..Options and thought it might be helpful to have a reference! I don't have a strong opinion here though.
| * @since 5.3 | ||
| * @mongodb.driver.manual reference/method/cursor.sort/ Sort | ||
| */ | ||
| public UpdateOptions sort(@Nullable final BsonDocument sort) { |
There was a problem hiding this comment.
Should we consider adding a note in the Javadoc to clarify that sort is not applicable to updateMany? This could help avoid confusion for users.
There was a problem hiding this comment.
This is a good point, will go ahead and add it!
|
|
||
| /** | ||
| * Sets the sort criteria to apply to the query. | ||
| * |
There was a problem hiding this comment.
Could we clarify the purpose of this sort option in the Javadoc? It would be helpful to explain how it determines which document is updated when multiple matches are found.
| * | |
| * | |
| * When multiple documents match the query, the sort order specifies the document | |
| * to be updated first. | |
| * |
| @@ -31,6 +32,7 @@ public final class BulkWriteOptions { | |||
| private Boolean bypassDocumentValidation; | |||
| private BsonValue comment; | |||
| private Bson variables; | |||
| private BsonDocument sort; | |||
There was a problem hiding this comment.
Judging by the spec, BulkWriteOptions does not seem to require a sort option.
There was a problem hiding this comment.
I overlooked that part, will remove it!
I originally didn't include it because the ticket specifically dealt with |
joykim1005
changed the title
Ticket
JAVA-5722
Description
This PR adds a sort option for
updateOneandreplaceOnecommands so if it matches more than one candidate document, the first one matched by sort order will be updated. It also syncs CRUD unified spec tests to this.Testing
./gradlew checkUnifiedCRUDTest.java