GODRIVER-2696 Use an options pattern that doesn't require merging structs #1570
Conversation
API Change Report##! second, different message for obj type ./mongo/options.NameOptions struct{Revision *int32}, isNew false, part "" ./mongoincompatible changes##(Client).Database: changed from func(string, ..../mongo/options.DatabaseOptions) *Database to func(string, ...Options[./mongo/options.DatabaseArgs]) *Database compatible changesOptions: added ./mongo/optionsincompatible changes(*EncryptOptions).SetRangeOptions: changed from func(RangeOptions) *EncryptOptions to func(*RangeOptions) *EncryptOptions compatible changes(*AggregateOptions).ArgsSetters: added ./x/mongo/driver/topologyincompatible changes##ConvertToDriverAPIOptions: changed from func(*./mongo/options.ServerAPIOptions) *./x/mongo/driver.ServerAPIOptions to func(./internal/mongoutil.MongoOptions[./mongo/options.ServerAPIArgs]) *./x/mongo/driver.ServerAPIOptions compatible changesNewConfigFromArgs: added |
prestonvasquez
changed the title
mongo/util.go
Outdated
There was a problem hiding this comment.
We can use ptrutil.Ptr instead.
internal/mongoutil/mongoutil.go
Outdated
| // The intended use case is to create options from arguments. | ||
| type Options[T Args] struct { | ||
| Args *T // Arguments to set on the option type | ||
| Clbk func(*T) error // A callback for further modification |
There was a problem hiding this comment.
Optional: It's not obvious that Clbk is an abbreviation for "Callback" because it's similar to other more common abbreviations (e.g. clk, blk). Consider renaming to Callback.
|
|
||
| // Options is an interface that wraps a method to return a list of setter | ||
| // functions that can set a generic arguments type. | ||
| type Options[T mongoutil.Args] interface { |
There was a problem hiding this comment.
Is there a risk to using T any instead of T mongoutil.Args?
There was a problem hiding this comment.
Although, functionally, T any fits here as well, I think it's better to restrict the types. The mongoutil.Args type also implies the expected usage. The only issue is that we need to maintain the Args list.
There was a problem hiding this comment.
I'm concerned about the cost of maintaining that exhaustive list and also of using something defined in an internal package in the options API. The usage of options.Options in the mongo and options APIs seems pretty clear already.
There was a problem hiding this comment.
No matter what, we would need a compile-time check that options implement ArgsSetters. mongoutil.Args serves this purpose under the proposed paradigm, as well as providing type safety to the mongo.Options type.
In terms of maintaining the list, options are part of stable API so this list could only grow within the scope of 2.x.
Unfortunately, due to dependency cycles we can't define this list in the mongo package. However, it doesn't really matter: if an embedded type (WLOG consider options.ClientArgs) is removed from mongoutil.Args, then you will get a compile-time failure akin to the following:
"go.mongodb.org/mongo-driver/mongo/options".ClientArgs does not satisfy mongoutil.Args
The only way you wouldn't get this failure is if (1) the underlying options are not used [in which case removal doesn't matter], or (2) the options have been removed in all instance [a breaking change].
IMO the benefit of type-safety outweighs the need to extend this list when new option requirements arise.
There was a problem hiding this comment.
I was actually thinking about when we need to add new Options types, which would require adding a type to the mongoutil.Args interface. For example, if we add a new Collection method coll.Foo, we'd probably also add FooOptions and FooArgs, which will need to implement Options. We'd need to add FooArgs to mongoutil.Args. Could that change break any user code?
Another consideration is users who want to write functions that operate on Options. For example, if a user wanted to pass *Args types directly to collection methods (like we do in our spec tests), they might define a generic type like:
type argsAsOptions[T mongoutil.Args] struct {
args *T
}
func (o *argsAsOptions[T]) ArgsSetters() []func(*T) error {
return []func(*T) error{
func(args *T) error {
*args = *o.args
return nil
},
}
}
func main() {
coll.FindOne(
context.Background(),
bson.D{},
&argsAsOptions[options.FindOneArgs]{
args: &options.FindOneArgs{
Comment: "blah",
},
},
)
}However, a user can't reference mongoutil.Args from outside the Go Driver module, so they would have to define a type constraint that is a copy of mongoutil.Args:
type myArgs interface {
options.AggregateArgs | options.BucketArgs | options.BulkWriteArgs |
// ...
}
type argsAsOptions[T myArgs] struct {
// ...That example is inspired by a public code search for users defining ClientOptions literals instead of using the builder API (see here). Is that a use case we want to support?
Concerning type safety, is there a problematic case that using the mongoutils.Args type constraint prevents?
There was a problem hiding this comment.
I was actually thinking about when we need to add new Options types, which would require adding a type to the mongoutil.Args interface.
We do this as a compile-time check, it's essentially this:
var _ mongoutil.Args = &FindOneArgs{}Removing any arg struct (that is actually being used somewhere in the code) from mongoutil.Args will result in the following error [WLOG]:
"go.mongodb.org/mongo-driver/mongo/options".ClientArgs does not satisfy `mongoutil.Args`
Could that change break any user code?
Users don't need to be able to access or copy mongoutil.Args. Although it's unclear to me why you would want to do this, if a user is trying to genericize their own options they can create a type that implements a subset of mongoutil.Args, e.g.:
type opts interface {
options.FindOneArgs | options.InsertOneArgs | SomeRandomStruct
}
type argsAsOptions[T opts] struct {
args *T
}In general, I would expect users to define custom types without generics:
type argsAsOptions struct {
args *options.FindOneArgs
}
func (o *argsAsOptions) ArgsSetters() []func(*options.FindOneArgs) error {
return []func(*options.FindOneArgs) error{
func(args *options.FindOneArgs) error {
*args = *o.args
return nil
},
}
}
func main() {
// ...
coll.FindOne(
context.Background(),
bson.D{},
&argsAsOptions{
args: &options.FindOneArgs{
Comment: "blah",
},
},
)
}That example is inspired by a public code search for users defining ClientOptions literals instead of using the builder API (see here). Is that a use case we want to support?
I would expect users to define struct literals by appending the Opts field:
opts := options.Client().ApplyURI("mongodb://localhost:27017")
opts.Opts = append(opts.Opts, func(args *options.ClientArgs) error {
appName := "myApp"
*args = options.ClientArgs{
AppName: &appName,
}
return nil
})
client, err := mongo.Connect(opts)
if err != nil {
panic(err)
}Concerning type safety, is there a problematic case that using the mongoutils.Args type constraint prevents?
Probably not, since we define the expected options type in the function signature, preventing any external arguments being used in our code:
func (coll *Collection) InsertOne(ctx context.Context, document interface{},
opts ...TOptions[options.InsertOneArgs]) (*InsertOneResult, error) {I don't expect users to directly interact with mongo.Options, and copying mongoutil.Args should never be necessary, although it's safe to do so since that list can only be extended. As far as I understand, strongly typing the arg setter parameters won't cause issues and will behave the same as using any. If you strongly prefer using any, I'm open to making that change. It's worth noting that using any doesn't necessarily violate the Liskov Substitution Principle, as there's currently no way to create ambiguity with custom types. However, relying on any is more likely to lead to misuse in the future compared to using strong types.
There was a problem hiding this comment.
Given the examples you provided which don't require generics or copying mongoutil.Args, I agree it doesn't block any known use cases. If a user did want to define a generic method for converting an *Args into an Options, defining a type constraint that is a subset of mongoutil.Args is somewhat inconvenient, but also not blocking. Ideally, if we think that's a use case that many users are interested in, we can provide that utility in the options package in the future.
Consider this resolved.
| // Options is an interface that wraps a method to return a list of setter | ||
| // functions that can set a generic arguments type. | ||
| type Options[T mongoutil.Args] interface { | ||
| ArgsSetters() []func(*T) error |
There was a problem hiding this comment.
Is there a reason to return errors from the setters? Handling the errors adds a significant amount of code and is currently completely unused. Do we plan to use the ability to return errors in the future for something?
There was a problem hiding this comment.
The setter error is used on L605 of the client options, in the ApplyURI method.
There was a problem hiding this comment.
Ah, good point. Considering we're going to be stuck with this API for at least all of v2.x, and there's an example of it actually being used, the added flexibility of being able to use different setter validation patterns in the future seems worth the additional error handling code, even if it remains mostly unused.
Consider this resolved.
mongo/options/clientoptions.go
Outdated
| if len(c.Hosts) > 1 { | ||
| // ValidateClientArgs checks if the client arguments will create a valid | ||
| // connection. | ||
| func ValidateClientArgs(args *ClientArgs) error { |
There was a problem hiding this comment.
It seems like this is separate from ClientOptions.Validate so it can also be called in the topology package for a ClientArgs. Can we move the Validate method to a ClientArgs instead so we don't have to add this exported symbol?
Alternatively, we should move this validation to an internal package and remove ClientOptions.Validate.
| @@ -2651,7 +2642,7 @@ func TestClientSideEncryptionProse(t *testing.T) { | |||
| SetAlgorithm("RangePreview"). | |||
| SetKeyID(key1ID). | |||
| SetContentionFactor(0). | |||
| SetRangeOptions(test.rangeOpts) | |||
| SetRangeOptions(*test.rangeOpts) | |||
There was a problem hiding this comment.
options.Range() returns a *RangeOptions, but SetRangeOptions expects a RangeOptions. That creates an awkward mismatch where users will have to immediately dereference pointers to use them.
For example, the current code requires:
options.Encrypt().SetRangeOptions(*options.Range())but we should make the following work:
options.Encrypt().SetRangeOptions(options.Range())Either by changing SetRangeOptions to take a *RangeOptions or making options.Range() return a RangeOptions.
| type RangeOptions struct { | ||
| Opts []func(*RangeArgs) error | ||
| } |
There was a problem hiding this comment.
I'm concerned this adds breaking changes and some code without a clear need because there are no APIs that accept multiple RangeOptions. Are there other motivations for changing RangeOptions to the functional options builder pattern?
There was a problem hiding this comment.
If the specifications are ever extended to include an API that accepts RangeOptions as a parameter, then we would be forced to create a duplicate type that fits the functional pattern.
| } | ||
| return driverOpts | ||
| } | ||
|
|
||
| func newLogger(opts *options.LoggerOptions) (*logger.Logger, error) { | ||
| func newLogger(opts mongoutil.MongoOptions[options.LoggerArgs]) (*logger.Logger, error) { |
There was a problem hiding this comment.
Continuing to pass the Options list through the internal driver APIs makes it awkward set and read arg values and requires additional error handling in internal APIs. We should convert the the Options list to an *Args as soon as we have it and then only pass around *Args internally. We should treat the Options pattern as syntactic sugar for the public API and use it sparingly.
This suggestion applies to most non-exported or x/ package APIs.
There was a problem hiding this comment.
Much of the stable API function signatures used in the internal and x/ packages are defined with Options. Converting options to args too quickly can be equally awkward. Consider the use case with runTest here. Part of the logic for runTest is to create a mongo client:
func runTest(ctx context.Context, clientOpts *options.ClientOptions) error {
client, err := mongo.Connect(clientOpts)
if err != nil {
return fmt.Errorf("Connect error: %w", err)
}
// ...
}If we were to update the function signature to use args, then we would have to convert the args back into options:
func runTest(ctx context.Context, clientArgs *options.ClientArgs) error {
clientOpts := mongoutil.NewOptionsFromArgs(clientArgs, nil)
client, err := mongo.Connect(clientOpts)
if err != nil {
return fmt.Errorf("Connect error: %w", err)
}
// ...
}The same is true for the integration functionexecuteAdminCommandWithRetry, which passes options to RunCommand. This is the only other instance of this pattern I could find in the proposal.
mongo/client.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This error handling block will never evaluate to true and can be removed.
internal/mongoutil/mongoutil.go
Outdated
|
|
||
| // Options implements a mongo.Options object for an arbitrary arguments type. | ||
| // The intended use case is to create options from arguments. | ||
| type Options[T Args] struct { |
There was a problem hiding this comment.
Optional: It's a bit confusing that this type is called Options but does not serve the same purpose as the mongo.Options interface. Consider renaming this to something like ArgsOptions and renaming MongoOptions to Options.
Alternatively, you could un-export this type and have NewOptionsFromArgs could return a MongoOptions[T] instead of *Options[T] which would resolve the name conflict.
E.g.
type Options[T Args] interface {
ArgsSetters() []func(*T) error
}
type options[T Args] struct {
Args *T // Arguments to set on the option type
Clbk func(*T) error // A callback for further modification
}
func NewOptionsFromArgs[T Args](args *T, clbk func(*T) error) Options[T] {
return &options[T]{Args: args, Clbk: clbk}
}Returning an interface is often an anti-pattern, but I think it would be appropriate here because the return value is only intended for use as an Options.
internal/mongoutil/mongoutil.go
Outdated
|
|
||
| // NewOptionsFromArgs will construct an Options object from the provided | ||
| // arguments object. | ||
| func NewOptionsFromArgs[T Args](args *T, clbk func(*T) error) *Options[T] { |
There was a problem hiding this comment.
Optional: The clbk argument seems unused and can be removed.
mongo/options/clientoptions.go
Outdated
| //return err | ||
|
|
There was a problem hiding this comment.
| //return err |
mongo/gridfs_bucket.go
Outdated
|
|
||
| nameOpts := options.GridFSName() | ||
| nameOpts.Revision = &options.DefaultRevision | ||
| var numSkip int32 = -1 |
There was a problem hiding this comment.
Shall we initialize numSkip with &options.DefaultRevision?
There was a problem hiding this comment.
That makes sense. Looking at this method a bit closer, it's unclear why we set the Revision on the arguments. Suggest we also remove that code.
GODRIVER-2696
Summary
This PR implements the proposed solution described here in the POC PR #1480 with the intention of updating the options pattern to abstract merging.
Background & Motivation
To avoid creating merge function for each option type, the goal of 2696 is to find and implement an optional arguments pattern that doesn't require maintaining dedicated code for merging each type of option struct together when multiple options structs are provided.