GODRIVER-3668 Add bypassEmptyTsReplacement option. #2219
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🧪 Performance ResultsCommit SHA: ef7d27cThe following benchmark tests for version 68e67f32b878170007586c17 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/operationcompatible changes(*FindAndModify).AdditionalCmd: added |
qingyang-hu
force-pushed
the
godriver3668
branch
from
f42d087 to
446c2e1
Compare
qingyang-hu
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds the bypassEmptyTsReplacement option to various MongoDB write operations by implementing an "addCommandFields" mechanism that allows arbitrary command fields to be added to operations.
- Adds support for bypassEmptyTsReplacement option across insert, update, replace, findOneAndUpdate, findOneAndReplace, and bulk write operations
- Implements addCommandFields functionality in internal options handling to support additional command parameters
- Adds comprehensive test coverage to verify the option is properly passed through to MongoDB commands
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| x/mongo/driver/xoptions/options.go | Adds "addCommandFields" case handling to all internal option setters |
| x/mongo/driver/operation/update.go | Adds additionalCmd field and AdditionalCmd method to Update operation |
| x/mongo/driver/operation/insert.go | Adds additionalCmd field and AdditionalCmd method to Insert operation |
| x/mongo/driver/operation/find_and_modify.go | Adds additionalCmd field and AdditionalCmd method to FindAndModify operation |
| mongo/collection.go | Integrates addCommandFields option handling into collection methods |
| mongo/client_bulk_write.go | Adds additionalCmd field to clientBulkWrite struct |
| mongo/client.go | Integrates addCommandFields option handling into Client.BulkWrite |
| mongo/bulk_write.go | Adds additionalCmd field and integrates it into bulk write operations |
| internal/integration/collection_test.go | Adds comprehensive tests for bypassEmptyTsReplacement option |
| internal/integration/client_test.go | Adds tests for client bulk write with bypassEmptyTsReplacement and fixes test assertions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
matthewdale
requested changes
Oct 8, 2025
internal/integration/client_test.go
Outdated
| }) | ||
| } | ||
| }) | ||
| mt.RunOpts("bulk write with bypassEmptyTsReplacement", mtBulkWriteOpts, func(mt *mtest.T) { |
There was a problem hiding this comment.
Optional: Subtests in subtests are difficult to read. Consider splitting this subtest out into a separate test function.
internal/integration/client_test.go
Outdated
Comment on lines
833
to
836
| t.Helper() | ||
|
|
||
| valType, data, err := bson.MarshalValue(val) | ||
| require.Nil(t, err, "MarshalValue error: %v", err) |
There was a problem hiding this comment.
Use mt and require.NoError.
Suggested change
| t.Helper() | |
| valType, data, err := bson.MarshalValue(val) | |
| require.Nil(t, err, "MarshalValue error: %v", err) | |
| mt.Helper() | |
| valType, data, err := bson.MarshalValue(val) | |
| require.NoError(mt, err, "MarshalValue error") |
internal/integration/client_test.go
Outdated
Comment on lines
847
to
850
| { | ||
| "insert one", | ||
| mongo.NewClientInsertOneModel().SetDocument(bson.D{{"x", 1}}), | ||
| }, |
There was a problem hiding this comment.
Optional: Keyed struct literals are easier to use. Consider including the key names.
internal/integration/client_test.go
Outdated
Comment on lines
870
to
874
| { | ||
| "empty", | ||
| options.ClientBulkWrite(), | ||
| bson.RawValue{}, | ||
| }, |
There was a problem hiding this comment.
Optional: Keyed struct literals are easier to use. Consider including the key names.
internal/integration/client_test.go
Outdated
| return opts | ||
| } | ||
|
|
||
| marshalValue := func(val interface{}) bson.RawValue { |
There was a problem hiding this comment.
Optional: Consider factoring marshalValue into a separate function since an identical function is also used in TestBypassEmptyTsReplacement.
mongo/client.go
Outdated
Comment on lines
958
to
962
| if additionalCmd := optionsutil.Value(bwo.Internal, "addCommandFields"); additionalCmd != nil { | ||
| if ac, ok := additionalCmd.(bson.D); ok { | ||
| op.additionalCmd = ac | ||
| } | ||
| } |
There was a problem hiding this comment.
Combine these conditions into one. This suggestion applies to all instances of this block in this PR.
Suggested change
| if additionalCmd := optionsutil.Value(bwo.Internal, "addCommandFields"); additionalCmd != nil { | |
| if ac, ok := additionalCmd.(bson.D); ok { | |
| op.additionalCmd = ac | |
| } | |
| } | |
| if ac, ok := optionsutil.Value(bwo.Internal, "addCommandFields").(bson.D); ok { | |
| op.additionalCmd = ac | |
| } |
mongo/collection.go
Outdated
Comment on lines
406
to
408
| if rawDataOpt := optionsutil.Value(args.Internal, "addCommandFields"); rawDataOpt != nil { | ||
| imOpts.Opts = append(imOpts.Opts, func(opts *options.InsertManyOptions) error { | ||
| opts.Internal = optionsutil.WithValue(opts.Internal, "addCommandFields", rawDataOpt) |
There was a problem hiding this comment.
rawDataOpt is confusingly named because we're copying the "addCommandFields" option here. Rename to something like ac or acOpt.
Suggested change
| if rawDataOpt := optionsutil.Value(args.Internal, "addCommandFields"); rawDataOpt != nil { | |
| imOpts.Opts = append(imOpts.Opts, func(opts *options.InsertManyOptions) error { | |
| opts.Internal = optionsutil.WithValue(opts.Internal, "addCommandFields", rawDataOpt) | |
| if ac := optionsutil.Value(args.Internal, "addCommandFields"); ac != nil { | |
| imOpts.Opts = append(imOpts.Opts, func(opts *options.InsertManyOptions) error { | |
| opts.Internal = optionsutil.WithValue(opts.Internal, "addCommandFields", ac) |
qingyang-hu
force-pushed
the
godriver3668
branch
from
843e33f to
ef7d27c
Compare
matthewdale
added
the
review-priority-normal
matthewdale
approved these changes
Oct 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GODRIVER-3668
GODRIVER-3582
Summary
Add bypassEmptyTsReplacement option to:
Background & Motivation