[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: |
|
||||||||||||||||||||||||||||||||||||||||||||
| 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:
|
| 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:
The options type would be updated to something like like:
and we would add a new “...Args” struct that is basically the same as the old “...Options” struct:
The code to merge the option functions together to get the “...Args” looks like:
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 SettersAnother 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
might become
The function signature might look like
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:
See an example Sourcegraph search here.
Takeaways:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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:
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
to
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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.CallOptionThe 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.
playground: https://go.dev/play/ Another Alt 2: Builder MethodIf we want to avoid having to add more arguments to function signatures, we could use more of a builder pattern:
This is essentially how the drive API behaves, e.g.:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Qingyang Hu [ 06/Oct/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
We talked about several solutions for the Options structs, using InsertOne(...) and InsertOneOptions as an example:
So overall, I prefer solution 2 because it impacts relatively less yet does what we want. |