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

GODRIVER-3285 Allow update to supply sort option. #1797

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented Sep 9, 2024

GODRIVER-3285

Summary

Allow v2 update to supply sort option.
Sort option for the client bulk write will be added after #1884.

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Sep 9, 2024
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Sep 9, 2024

API Change Report

./v2/mongo

compatible changes

(*ReplaceOneModel).SetSort: added
(*UpdateOneModel).SetSort: added
ReplaceOneModel.Sort: added
UpdateOneModel.Sort: added

./v2/mongo/options

compatible changes

(*ReplaceOptionsBuilder).SetSort: added
(*UpdateOneOptionsBuilder).SetSort: added
ReplaceOptions.Sort: added
UpdateOneOptions.Sort: added

@qingyang-hu qingyang-hu force-pushed the godriver3285v2 branch 4 times, most recently from fab1a09 to 8533d0a Compare September 9, 2024 22:36
@qingyang-hu qingyang-hu added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Sep 10, 2024
@qingyang-hu qingyang-hu changed the title GODRIVER-3285 [v2]Allow update to supply sort option. GODRIVER-3285 Allow update to supply sort option. Nov 5, 2024
@qingyang-hu qingyang-hu marked this pull request as ready for review November 5, 2024 22:59
// matched by the sort order will be replaced. This option is only valid for MongoDB versions >= 8.0. The driver will
// return an error if the sort parameter is a multi-key map. The default value is nil.
func (rom *ReplaceOneModel) SetSort(sort interface{}) *ReplaceOneModel {
rom.Sort = &sort
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is taking the address of sort intentional? If so, what is the purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fat-fingers... 😅

Comment on lines 1294 to 1299
case "sort":
sort, err := createSort(val)
if err != nil {
return nil, fmt.Errorf("error creating sort: %w", err)
}
opts.SetSort(sort)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional createSort logic seems unnecessary. Can we do something similar to the "comment" case and just pass val to SetSort?

E.g.

opts.SetSort(val)

Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@qingyang-hu qingyang-hu merged commit 9dd4bcd into mongodb:master Dec 17, 2024
31 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-medium Medium Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants