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-3339 Separate Many and One for UpdateOptions and DeleteOptions. #1819

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

qingyang-hu
Copy link
Collaborator

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

GODRIVER-3339

Summary

  • Separate the “UpdateOptions” struct into “UpdateManyOptions” and “UpdateOneOptions”.
  • Separate the “DeleteOptions” struct into “DeleteManyOptions” and “DeleteOneOptions”.

Background & Motivation

For DRIVERS-2822, sort only applies to UpdateOne. The server throws an error if a sort exists in UpdateMany. Therefore, this PR proposes separating the options for UpdateOne and UpdateMany.
Although there is no immediate requirement yet, the “DeleteOptions” struct is also considered to be divided for future changes as it is shared by both “DeleteOne” and “DeleteMany” now.

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

API Change Report

./v2/mongo/options

incompatible changes

Delete: removed
DeleteOptions: removed
DeleteOptionsBuilder: removed
Update: removed
UpdateOptions: removed
UpdateOptionsBuilder: removed

compatible changes

DeleteMany: added
DeleteManyOptions: added
DeleteManyOptionsBuilder: added
DeleteOne: added
DeleteOneOptions: added
DeleteOneOptionsBuilder: added
UpdateMany: added
UpdateManyOptions: added
UpdateManyOptionsBuilder: added
UpdateOne: added
UpdateOneOptions: added
UpdateOneOptionsBuilder: added

@qingyang-hu qingyang-hu marked this pull request as ready for review September 18, 2024 13:41
@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 18, 2024
Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

We need to update the migration guide to include sections about upgrading from options.Delete() and options.Update() to the new design.

func createUpdateArguments(args bson.Raw) (*updateArguments, error) {
ua := &updateArguments{
opts: options.Update(),
func createUpdateArguments[Options options.UpdateManyOptions | options.UpdateOneOptions](
Copy link
Collaborator

@prestonvasquez prestonvasquez Sep 18, 2024

Choose a reason for hiding this comment

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

We should not try to generalize this function. Instead, we should maintain createUpdateOneArguments and createUpdateManyArguments. Using the combination of reflection and generics here is difficult to read.

Comment on lines +148 to +150
// A set of filters specifying to which array elements an update should apply. This option is only valid for MongoDB
// versions >= 3.6. For previous server versions, the driver will return an error if this option is used. The
// default value is nil, which means the update will apply to all array elements.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't expect users to interact with the ...Options structs directly in most cases. We should move the struct field comments to the corresponding setter methods. Leave a comment on each struct field that references the corresponding setter method, like

See UpdateManyOptionsBuilder.SetArrayFilters for documentation.

This recommendation applies for the new DeleteManyOptions also. Note that we will update the pattern for the rest of the options types with GODRIVER-3327.

if doc.multi {
updateDoc = bsoncore.AppendBooleanElement(updateDoc, "multi", doc.multi)
}
if doc.sort != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

doc.sort is currently never set. Will that be used in the follow-up GODRIVER-3285 PRs?

Comment on lines +86 to +90
type updateOneArguments struct {
filter bson.Raw
update interface{}
opts *options.UpdateOneOptionsBuilder
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can avoid creating a new type here by separating filter and update from the options builder, allowing you to reuse updateArguments in both functions.

E.g. updated function signatures:

func createUpdateOneArguments(args bson.Raw) (*updateArguments, *options.UpdateOneOptionsBuilder, error)
func createUpdateManyArguments(args bson.Raw) (*updateArguments, *options.UpdateManyOptionsBuilder, error)

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