GODRIVER-2696 POC Update options structs #1480
Conversation
qingyang-hu
force-pushed
the
godriver2696POC
branch
from
e3feaac
to
3276282
Compare
API Change Report./mongoincompatible changes##(Client).ListDatabaseNames: changed from func(context.Context, interface{}, ..../mongo/options.ListDatabasesOptions) ([]string, error) to func(context.Context, interface{}, ...Options[./mongo/options.ListDatabasesArgs]) ([]string, error) compatible changesArgs: added ./mongo/optionsincompatible changesChangeStreamOptions.BatchSize: removed compatible changes(*ChangeStreamOptions).ArgsSetters: added |
mongo/options/findoptions.go
Outdated
| type FindOptions struct { | ||
| // FindArgs represents arguments that can be used to configure a Find operation. | ||
| type FindArgs struct { | ||
| FindOneArgs |
There was a problem hiding this comment.
I think it would be a mistake to make FindOneArgs a subset of FindArgs. It may never be a problem, but this pattern commits us to supporting all FindOneArgs in FindArgs. And so we might end up with arguments that can be passed to Find that don't do anything.
I think it's fine to retain the existing logic of creating a FindArgs from FindOneArgs, i.e.
for _, opt := range opts {
if opt == nil {
continue
}
findOpts = append(findOpts, &options.FindOptions{
AllowPartialResults: opt.AllowPartialResults,
BatchSize: opt.BatchSize,
Collation: opt.Collation,
Comment: opt.Comment,
CursorType: opt.CursorType,
Hint: opt.Hint,
Max: opt.Max,
MaxAwaitTime: opt.MaxAwaitTime,
MaxTime: opt.MaxTime,
Min: opt.Min,
NoCursorTimeout: opt.NoCursorTimeout,
OplogReplay: opt.OplogReplay,
Projection: opt.Projection,
ReturnKey: opt.ReturnKey,
ShowRecordID: opt.ShowRecordID,
Skip: opt.Skip,
Snapshot: opt.Snapshot,
Sort: opt.Sort,
})
}
mongo/collection.go
Outdated
| fo := &options.FindArgs{} | ||
| for _, opt := range opts { | ||
| for _, optFn := range opt.Opts { | ||
| if err := optFn(fo); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
CC: @matthewdale
We can avoid these type-specific merge requirements using generics. I propose that we add a new type to the options package that unify option-bearing structs:
// Getter is an interface that wraps the GetOptions method to return a
// list of setters that can set data on the functional parameter of type T.
type Getter[T Optioners] interface {
Get() []func(*T) error
}Then implement all of the option-bearing structs as "Optioners":
// Optioners is a running list of optional data than can be merged using the
// OptionsGetter.
type Optioners interface {
FindArgs
}Then we can create a generic merge method:
// Merge will functionally merge a slice of OptionGetters in a "last-one-wins"
// algorithm.
func Merge[T Optioners](opts []Getter[T]) (*T, error) {
t := new(T)
for _, opt := range opts {
for _, setOpt := range opt.Get() {
if err := setOpt(t); err != nil {
return t, err
}
}
}
return t, nil
}Then add the Get method to the options:
// Get will return the Opts list from FindOptions.
func (fo FindOptions) Get() []func(*FindArgs) error {
return fo.Opts
}And the find method changes to this:
func (coll *Collection) Find(ctx context.Context, filter interface{}, opts ...options.Getter[options.FindArgs]) (cur *Cursor, err error) {
fo, err := options.Merge[options.FindArgs](opts)
if err != nil {
return nil, err
}
}I've create a POC for this pattern using the latest commit on this PR here.
It's notable that the addition of the Get and Optioner types, and the updating of the Find method signature will not alter the use case of Find and is still backwards compatible upto the v2 proposal.
There was a problem hiding this comment.
Additionally, we can seal the Getter by not exporting get and keeping the logic internal.
There was a problem hiding this comment.
That API proposal looks great! I see @qingyang-hu has integrated it into this PoC. I'm not currently convinced that sealing the Getter interface (called Options in this PR) is necessary.
mongo/options/options.go
Outdated
| } | ||
|
|
||
| // Merge will functionally merge a slice of Options in a "last-one-wins" manner. | ||
| func Merge[T Args](args *T, opts ...Options[T]) error { |
There was a problem hiding this comment.
Merge should be moved to an internal package.
There was a problem hiding this comment.
IMO, it's acceptable to expose Merge if we expose Args and Options as well. On the other hand, moving Merge into internal may cause an import cycle when we separate it from Args and Options.
mongo/options/options.go
Outdated
| type Args interface { | ||
| FindArgs | FindOneArgs | InsertOneArgs | ||
| } | ||
|
|
||
| // 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 Args] interface { | ||
| ArgsSetters() []func(*T) error |
There was a problem hiding this comment.
It's conventional to define interfaces where they're used instead of where they're implemented. Should this be in the mongo package instead?
mongo/collection_test.go
Outdated
| assert.Equal(t, test.want, got) | ||
| var err error | ||
| got := &options.FindArgs{} | ||
| err = options.Merge(got, []options.Options[options.FindArgs]{newFindOptionsFromFindOneOptions(test.opts...)}...) |
There was a problem hiding this comment.
We can simplify the syntax here by providing an explicit type for options.Merge.
| err = options.Merge(got, []options.Options[options.FindArgs]{newFindOptionsFromFindOneOptions(test.opts...)}...) | |
| err = options.Merge[options.FindArgs](got, newFindOptionsFromFindOneOptions(test.opts...)) |
mongo/collection.go
Outdated
| for _, opt := range opts { | ||
| if opt == nil { | ||
| continue | ||
| func newFindOptionsFromFindOneOptions(opts ...options.Options[options.FindOneArgs]) *options.FindOptions { |
There was a problem hiding this comment.
We can significantly simplify the logic here if we pass around options.FindOneArgs and options.FindArgs instead of passing around arrays of options. Generally, the variadic interfaces for options are syntactic sugar for the public interfaces, but we should avoid using them for internal code because they add complexity.
E.g. updated function signature for newFindOptionsFromFindOneOptions:
func newFindArgsFromFindOneArgs(args *options.FindOneArgs) *options.FindArgsThen we would create an internal, shared find function that has the same signature as Find but takes FindArgs instead of ...FindOptions:
func (coll *Collection) find(
ctx context.Context,
filter interface{},
args *options.FindArgs,
) (cur *Cursor, err error) {| @@ -1059,7 +1059,7 @@ func executeInsertOne(ctx context.Context, operation *operation) (*operationResu | |||
| return nil, newMissingArgumentError("documents") | |||
| } | |||
|
|
|||
| res, err := coll.InsertOne(ctx, document, opts) | |||
| res, err := coll.InsertOne(ctx, document, []options.Options[options.InsertOneArgs]{opts}...) | |||
There was a problem hiding this comment.
I believe the extra type conversion is unnecessary.
| res, err := coll.InsertOne(ctx, document, []options.Options[options.InsertOneArgs]{opts}...) | |
| res, err := coll.InsertOne(ctx, document, opts) |
qingyang-hu
force-pushed
the
godriver2696POC
branch
2 times, most recently
from
f8dd429
to
5971f06
Compare
qingyang-hu
force-pushed
the
godriver2696POC
branch
from
5971f06
to
dbbc305
Compare
| type InsertOneOptions struct { | ||
| Opts []func(*InsertOneArgs) error | ||
| } |
There was a problem hiding this comment.
@matthewdale @qingyang-hu I just realized that one breaking change for the current design is that a user will no longer be able to decode bytes into an options type:
package main
import (
"encoding/json"
"go.mongodb.org/mongo-driver/mongo/options"
)
func main() {
bytes := []byte(`{"comment": "hi"}`)
ioo := options.InsertOneOptions{}
if err := json.Unmarshal(bytes, &ioo); err != nil { // Results in nothing
panic(err)
}
// ...
}If we want to keep the current suggested pattern, we could create a convenience API to construct options from arguments:
func InsertOneFromArgs(args *InsertOneArgs) *InsertOneOptions {
opts := &InsertOneOptions{}
if args == nil {
return opts
}
if args.BypassDocumentValidation != nil {
opts.SetBypassDocumentValidation(*args.BypassDocumentValidation)
}
if args.Comment != nil {
opts.SetComment(args.Comment)
}
return opts
}In this way, you can decode into InsertOneArgs, then construct the options from that data. Admittedly, this seems a little backwards since ultimately the Go Driver will convert the options back into args 😕
There might be at least one legitimate use case for this in the Go Driver, specifically in the executeListSearchIndexes in the unified spec runner . Though this pattern might actually be an antipattern when compared with what is typically done with options, such as executeAggregate.
Should we continue moving forward with the existing pattern?
There was a problem hiding this comment.
I think we can create types in the unified test runner package(s) that implement mongo.Options for the specific operation type, but can decode the args from JSON.
E.g.
type testInsertOneOpts struct {
data []byte
}
func (opts *testInsertOneOpts) ArgsSetters() []func(*options.InsertOneArgs) error {
return []func(*options.InsertOneArgs) error{
func(args *options.InsertOneArgs) error {
return json.Unmarshal(opts.data, args)
},
}
}Then we can pass that to the operation function:
coll.InsertOne(
context.Background(),
testDoc,
&testInsertOneOpts{data: testArgsBytes})
| @@ -400,27 +400,19 @@ func (coll *Collection) insert(ctx context.Context, documents []interface{}, | |||
| // | |||
| // For more information about the command, see https://www.mongodb.com/docs/manual/reference/command/insert/. | |||
| func (coll *Collection) InsertOne(ctx context.Context, document interface{}, | |||
| opts ...*options.InsertOneOptions) (*InsertOneResult, error) { | |||
| opts ...Options[options.InsertOneArgs]) (*InsertOneResult, error) { | |||
There was a problem hiding this comment.
@matthewdale @qingyang-hu Are we okay with the fact that this pattern is not-breaking for one instance of opts, but is a breaking change for a slice of opts?
// this IS NOT a breaking change
coll.InsertOne(ctx, bson.D{{"x", 1}}, options.InsertOne())// This IS a breaking change
iopts := []*options.InsertOneOptions{
options.InsertOne(),
options.InsertOne(),
}
coll.InsertOne(ctx, bson.D{{"x", 1}}, iopts...)To resolve the breaking change, you have to update the slice type to use mongo.Options:
iopts := []*mongo.Options[options.InsertOneArgs]{
options.InsertOne(),
options.InsertOne(),
}
coll.InsertOne(ctx, bson.D{{"x", 1}}, iopts...)There was a problem hiding this comment.
Here are the use cases I know of where that will be a breaking change:
- Any package that wraps the
CollectionAPIs and passes through options. - Interfaces that describe the
CollectionAPI.
See a Sourcegraph search for all known public code that would be impacted here. There are matches in about 140 repos.
The change is straightforward and seems like it can mostly be accomplished with a find & replace, so it doesn't seem like a major breaking change.
|
Closing in favor of PR #1570 |
GODRIVER-2696
Summary
Background & Motivation