GODRIVER-3502: Refactor and remove builder pattern for MongoCryptOptions#2278
Conversation
build struct with values instead of using setter functions
RafaelCenzano
added
enhancement
review-priority-normal
🧪 Performance ResultsCommit SHA: 6645c1aThe following benchmark tests for version 695ea887deb414000754b2c1 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change Report./v2/x/mongo/driver/mongocrypt/optionsincompatible changes(*DataKeyOptions).SetKeyAltNames: removed |
There was a problem hiding this comment.
Pull request overview
This PR refactors the MongoCryptOptions API from a builder pattern to using struct literals directly. This simplification removes unnecessary builder methods for an internal-only API.
Key Changes:
- Removed all builder pattern methods (MongoCrypt(), SetKmsProviders, SetLocalSchemaMap, etc.) from MongoCryptOptions
- Updated NewMongoCrypt calls in client_encryption.go and client.go to use struct literal initialization
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| x/mongo/driver/mongocrypt/options/mongocrypt_options.go | Removed builder pattern methods, keeping only the struct definition |
| mongo/client_encryption.go | Replaced builder pattern with struct literal for ClientEncryption MongoCrypt initialization |
| mongo/client.go | Replaced builder pattern with struct literal for auto-encryption MongoCrypt initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qingyang-hu
left a comment
qingyang-hu
left a comment
There was a problem hiding this comment.
Can we also remove the setters in "x/mongo/driver/mongocrypt/options/mongocrpyt_context_options.go"?
matthewdale
left a comment
matthewdale
left a comment
There was a problem hiding this comment.
Looks good with one suggestion 👍
mongo/client_encryption.go
Outdated
| transformed := &mcopts.ExplicitEncryptionOptions{} | ||
| if args.KeyID != nil { | ||
| transformed.SetKeyID(*args.KeyID) | ||
| transformed.KeyID = args.KeyID | ||
| } | ||
| if args.KeyAltName != nil { | ||
| transformed.SetKeyAltName(*args.KeyAltName) | ||
| transformed.KeyAltName = args.KeyAltName | ||
| } | ||
| transformed.SetAlgorithm(args.Algorithm) | ||
| transformed.SetQueryType(args.QueryType) | ||
| transformed.Algorithm = args.Algorithm | ||
| transformed.QueryType = args.QueryType | ||
|
|
||
| if args.ContentionFactor != nil { | ||
| transformed.SetContentionFactor(*args.ContentionFactor) | ||
| transformed.ContentionFactor = args.ContentionFactor | ||
| } |
There was a problem hiding this comment.
Optional: Because we're now assigning these directly without dereferencing and using setters, we can use struct literal syntax for these fields.
E.g.
transformed := &mcopts.ExplicitEncryptionOptions{
KeyID: args.KeyID,
KeyAltName: args.KeyAltName,
Algorithm: args.Algorithm,
QueryType: args.QueryType,
ContentionFactor: args.ContentionFactor,
}There was a problem hiding this comment.
Great idea I updated the code to use this. Simplifies the code and cuts down on the number of lines, I always feel like that is the best outcome when possible.
mongo/client_encryption.go
Outdated
| if args.KeyMaterial != nil { | ||
| co.SetKeyMaterial(args.KeyMaterial) | ||
| co.KeyMaterial = args.KeyMaterial | ||
| } |
There was a problem hiding this comment.
[Nitpicking] This block can be merged into L177-L179, the initialization of co.
mongo/client_encryption.go
Outdated
| if args.Provider != nil { | ||
| co.SetProvider(*args.Provider) | ||
| co.Provider = args.Provider | ||
| } |
There was a problem hiding this comment.
[Nitpicking] This block can be merged into L468.
RafaelCenzano
deleted the
refactor/remove-uneeded-setters-godriver-3502
branch
GODRIVER-3502
Summary
Use the struct literal instead of builder pattern functions for a driver internal only API. Cuts down code on something that is not necessary
Background & Motivation
#1922 (comment)