[GODRIVER-2696] Allow only 0 or 1 options structs in all APIs that accept variadic options args Created: 16/Dec/22  Updated: 27/Nov/23

Status: Investigating
Project: Go Driver
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0.0

Type: Improvement Priority: Unknown
Reporter: Matt Dale Assignee: Qingyang Hu
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on GODRIVER-2689 Simplify the "readpref" API Backlog
depends on GODRIVER-2685 Simplify "writeconcern" API Closed
depends on GODRIVER-2686 Simplify "readconcern" API Closed
depends on GODRIVER-2749 Deprecate all "Merge*Options" functions Closed
is depended on by GODRIVER-2499 Use a nested struct for default trans... Blocked
is depended on by GODRIVER-2699 Accept exactly one "ClientOptions" in... Blocked
Gantt Dependency
has to be done after GODRIVER-2617 Remove or un-export all currently dep... Closed
Related
is related to GODRIVER-2804 Merging ChangeStreamOptions can squas... Closed
Epic Link: Go Driver 2.0: Driver
Backwards Compatibility: Major Change

 Description   

Currently we maintain many Merge*Options functions that merge options structs together. These are used to merge possibly many options structs passed to functions that accept a variadic argument of 0 or more options structs. While that function signature has the benefit of allowing users to pass or not pass options, maintaining the Merge*Options functions adds a significant maintenance burden and has resulted in numerous bugs. Very few users pass multiple options structs (maybe no users), so there's no major need to support passing more than 1 options struct. Instead, return an error if 2 or more options structs are passed and remove all Merge*Options functions.

Definition of done:

  • For all functions that accept a variadic argument for function options structs, accept either 0 or 1 structs. If the user passes 2 or more structs, return an InvalidArgumentError.
  • Remove all Merge*Options options struct merging functions in all packages.


 Comments   
Comment by Matt Dale [ 18/Oct/23 ]

I've refined my above "functional options" proposal significantly. Here's the updated proposal:

The operation function signature would basically stay the same:

func (*Collection) FindOne(
	ctx context.Context,
	filter any,
	opts ...*options.FindOneOptions,
) *SingleResult

The options type would be updated to something like like:

type FindOneOptions struct {
    Fns []func(*FindOneArgs) error
}

and we would add a new “...Args” struct that is basically the same as the old “...Options” struct:

type FindOneArgs struct {
    Sort any
}

The code to merge the option functions together to get the “...Args” looks like:

args := &options.FindOneArgs{}
for _, opt := range opts {
   for _, optFn := range opt.Opts {
       if err := optFn(args); err != nil {
           return err
       }
   }
}

See a full working example here. Unfortunately, there isn’t an obvious way to improve the above proposal with generics. It still requires adding 1 new type per options type, so about +50 new types in the options package.

Comment by Matt Dale [ 10/Oct/23 ]

Considering the above usage metrics, it may not make sense to add "Opt" functions for all of the frequently used methods. Instead, we could consider just maintaining the "options struct merging" logic for the frequently used methods and dropping it for all others methods.

Functional Options + Chainable Setters

Another option is to use a "functional options" pattern, but layer the existing "chainable setters" pattern on top of that to make migration easier. For example, an existing call

coll.Find(ctx, bson.D{}, options.Find().SetComment("comment").SetHint("hint"))

might become

coll.Find(ctx, bson.D{}, options.Find().SetComment("comment").SetHint("hint").Opts...)

The function signature might look like

func (*Collection) Find(ctx context.Context, filter any, opts ...options.FindOptionsFn)

See a full working example here.

Comment by Matt Dale [ 10/Oct/23 ]

Here are counts of how often each method is used per repository for the 3rd and 6th most frequent repository users (i.e. how many calls to a particular method there are in a repository). I picked the 3rd and 6th most frequent to give an idea of a high-usage and typical-usage repository.

For example, to search for BulkWrite use a query like:

context:global lang:Go  -repo:^github\.com/mongodb/mongo-go-driver$ repo:has.content(go.mongodb.org/mongo-driver) -file:(^|/)vendor/ .BulkWrite(c

See an example Sourcegraph search here.

Method #3 Count per Repository #6 Count per Repository
Database 479 303
Collection 489 340
Find 211 114
FindOne 137 103
FindOneAndUpdate 28 12
FindOneAndReplace 12 3
FindOneAndDelete 11 7
InsertOne 137 93
InsertMany 24 17
UpdateOne 93 55
UpdateMany 24 19
DeleteOne 59 16
DeleteMany 42 26
ReplaceOne 10 8
Aggregate 55 32
WithTransaction 29 12
StartTransaction 114 18
UpdateByID 14 11
BulkWrite 48 6

Takeaways:

  • Database and Collection are called extremely frequently, more than any other method queried by far.
  • UpdateByID is actually called more often than expected, although only about 20% as often as UpdateOne.
  • There's not an obvious pattern for excluding many of the above methods that still covers most of the oft-called methods, so if we want to do a mix of option #2 and #3, we're stuck adding about 18 new methods as described in my previous comment.
Comment by Matt Dale [ 09/Oct/23 ]

I think generally option #2 is the most maintainable. However, I think we could considerably improve the user experience by doing a mix of option #2 and #3. For example, the default API pattern would be have a non-variadic options struct as the last parameter (requiring users to pass nil for default options). Some APIs that are called a lot would have two versions: Operation and OperationOpt (naming TBD).

For example, we might create Opt methods for the following APIs:

func (c *Client) Database(name string)
func (c *Client) DatabaseOpt(name string, opts *options.DatabaseOptions)
 
func (db *Database) Collection(name string)
func (db *Database) CollectionOpt(name string, opts *options.CollectionOptions)
 
func (coll *Collection) Find(ctx context.Context, filter interface{})
func (coll *Collection) FindOpt(ctx context.Context, filter interface{}, opts *options.FindOptions)
 
func (coll *Collection) FindOne(ctx context.Context, filter interface{})
func (coll *Collection) FindOneOpt(ctx context.Context, filter interface{}, opts *options.FindOneOptions)
 
func (coll *Collection) FindOneAndDelete(ctx context.Context, filter interface{})
func (coll *Collection) FindOneAndDeleteOpt(ctx context.Context, filter interface{}, opts *options.FindOneAndDeleteOptions)
 
func (coll *Collection) FindOneAndReplace(ctx context.Context, filter interface{}, replacement interface{})
func (coll *Collection) FindOneAndReplaceOpt(ctx context.Context, filter interface{}, replacement interface{}, opts *options.FindOneAndReplaceOptions)
 
func (coll *Collection) FindOneAndUpdate(ctx context.Context, filter interface{}, update interface{})
func (coll *Collection) FindOneAndUpdateOpt(ctx context.Context, filter interface{}, update interface{}, opts *options.FindOneAndUpdateOptions)
 
func (coll *Collection) InsertOne(ctx context.Context, document interface{})
func (coll *Collection) InsertOneOpt(ctx context.Context, document interface{}, opts *options.InsertOneOptions)
 
func (coll *Collection) InsertMany(ctx context.Context, documents []interface{})
func (coll *Collection) InsertManyOpt(ctx context.Context, documents []interface{}, opts *options.InsertManyOptions)
 
func (coll *Collection) DeleteOne(ctx context.Context, filter interface{})
func (coll *Collection) DeleteOneOpt(ctx context.Context, filter interface{}, opts *options.DeleteOptions)
 
func (coll *Collection) DeleteMany(ctx context.Context, filter interface{})
func (coll *Collection) DeleteManyOpt(ctx context.Context, filter interface{}, opts *options.DeleteOptions)
 
func (coll *Collection) UpdateOne(ctx context.Context, filter interface{}, update interface{})
func (coll *Collection) UpdateOneOpt(ctx context.Context, filter interface{}, update interface{}, opts *options.UpdateOptions)
 
func (coll *Collection) UpdateMany(ctx context.Context, filter interface{}, update interface{})
func (coll *Collection) UpdateManyOpt(ctx context.Context, filter interface{}, update interface{}, opts *options.UpdateOptions)
 
func (coll *Collection) ReplaceOne(ctx context.Context, filter interface{}, replacement interface{})
func (coll *Collection) ReplaceOneOpt(ctx context.Context, filter interface{}, replacement interface{}, opts *options.ReplaceOptions)
 
func (coll *Collection) Aggregate(ctx context.Context, pipeline interface{})
func (coll *Collection) AggregateOpt(ctx context.Context, pipeline interface{}, opts *options.AggregateOptions)
 
func (db *Database) Aggregate(ctx context.Context, pipeline interface{})
func (db *Database) AggregateOpt(ctx context.Context, pipeline interface{}, opts *options.AggregateOptions)
 
func (s *sessionImpl) WithTransaction(ctx context.Context, fn func(ctx SessionContext) (interface{}, error))
func (s *sessionImpl) WithTransactionOpt(ctx context.Context, fn func(ctx SessionContext) (interface{}, error), opts *options.TransactionOptions)
 
func (s *sessionImpl) StartTransaction()
func (s *sessionImpl) StartTransactionOpt(opts *options.TransactionOptions)

That's 18 new methods. We could probably pare that down based on some Sourcegraph research to get an idea of what the actual most common CRUD methods are. All other methods that currently accept a variadic options arg would be updated to accept a single options arg and would require users to pass nil for default options.

We'd also want to update the function signature of UseSessionWithOptions to match whatever naming pattern we end up with. So change

func (c *Client) UseSessionWithOptions(ctx context.Context, opts *options.SessionOptions, fn func(SessionContext) error)

to

func (c *Client) UseSessionOpt(ctx context.Context, fn func(SessionContext) error, opts *options.SessionOptions)

Comment by Preston Vasquez [ 09/Oct/23 ]

I think unless we want to change our options API to a functional pattern I agree that #2 is probably the best solution.

Another Alt 1: Functional Options in the style of googleapi.CallOption

The Google API does what we currently do with options, which is that the last property wins: https://github.com/googleapis/google-api-go-client/blob/main/internal/gensupport/params.go#L48 . However, they use CallOption as their option type, which is essentially functional options. I'm not sure we want to go this route, since it will undo the concept of structural options in the Go Driver.

collection.InsertOne(ctx, bson.D{{"x", 1}}, options.Comment("Comment"), options.BypassDocumentValidation(true))

// what would be in the options pacakge:
type CallOption interface {
	Get() (key string, value any)
}
 
func BatchSize(i32 int32) CallOption { return batchSize(i32) }
 
type batchSize int32
 
func (bs batchSize) Get() (string, any) { return "batchSize", int32(bs) }
 
// How it's used
type Collection struct{}
 
func (coll *Collection) InsertOne(opts ...CallOption) {
	for _, co := range opts {
		switch bs, val := co.Get(); bs {
		case "batchSize":
			fmt.Println("batch size is: ", val.(int32))
		}
	}
}
 
func main() {
	(&Collection{}).InsertOne(BatchSize(10))
}

playground: https://go.dev/play/

Another Alt 2: Builder Method

If we want to avoid having to add more arguments to function signatures, we could use more of a builder pattern:

collection.InsertOneOptions(options.InsertOne{}).InsertOne(...)

This is essentially how the drive API behaves, e.g.:

s.svc.Files.List().Q(fmt.Sprintf("name='%s'", orig)).Do()

Comment by Qingyang Hu [ 06/Oct/23 ]

We talked about several solutions for the Options structs, using InsertOne(...) and InsertOneOptions as an example:

  1. Return an error when the length of the variadic argument exceeds as this ticket describes.
    Its variadic argument is not intuitive as it strongly implies the method accepts multiple InsertOneOptions arguments. However, users can only realize the misuse after the runtime error.
  2. Explicitly accept exact one *InsertOneOptions in InsertOne(...).
    Users have to pass an additional argument even if they do not need to specify it.
  3. Have two flavors simultaneously, InsertOne(...) vs. InsertOneWithOption(...).
    A more reasonable choice is to use an InsertOneOptions value than its pointer for InsertOneWithOption(...). Otherwise, InsertOne(...) is merely a convenient function for InsertOneWithOption(...), and this solution is identical to solution 2. However, switching from pointer to value impacts a wide range of the current code base.
  4. Use a struct containing the *InsertOneOptions, for instance:

    Type InsertOneStruct struct {
        document interface{}
        opts          *options.InsertOneOptions
    }
    

    This results in an awkward method signature: InsertOne(context.Context, InsertOneStruct), or maybe, InsertOneStruct.Excute(context.Context)?

  5. Use a series of

    func WithProperty(v T) func(*InsertOneOptions) {
        return func(opts *InsertOneOptions) { /* Set property in opts by v. */ }
    }
    

    like what we have been using in x/mongo/driver/topology/connection_options.go.
    This solution requires a method for each property. On the other hand, users cannot easily access the properties if they need to read it.

So overall, I prefer solution 2 because it impacts relatively less yet does what we want.
matt.dale@mongodb.com, preston.vasquez@mongodb.com, your idea?

Generated at Thu Feb 08 08:39:10 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.