GODRIVER-3663 Expose atClusterTime parameter in snapshot sessions #2271
Conversation
prestonvasquez
added
enhancement
review-priority-normal
🧪 Performance ResultsCommit SHA: 956bb6fThe following benchmark tests for version 6945cc77c7a4cf0007e1148f 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/mongoincompatible changes##(*Session).ClientSession: changed from func() *./v2/x/mongo/driver/session.Client to func() ./v2/x/mongo/driver/session.Client ./v2/mongo/optionscompatible changes(*SessionOptionsBuilder).SetSnapshotTime: added ./v2/x/mongo/driver/sessionincompatible changes##Client.SnapshotTime: changed from *./v2/bson.Timestamp to ./v2/bson.Timestamp compatible changesClient.SnapshotTimeSet: added |
prestonvasquez
changed the title
| }() | ||
|
|
||
| // Expect this call to panic. | ||
| sess.ClientSession().SetServer() |
There was a problem hiding this comment.
This isn't possible now.
There was a problem hiding this comment.
Pull request overview
This PR exposes the atClusterTime parameter for snapshot sessions by adding a SnapshotTime option that allows users to set or access snapshot read timestamps. The change addresses GODRIVER-3663 and includes test infrastructure improvements.
Key changes:
- Added
SnapshotTimefield to session options and client session struct, making it immutable once set - Changed
Session.ClientSession()to return a copy instead of a pointer to prevent mutation - Enhanced test utilities with
Setup()/Teardown()methods and addedEmptyassertion functions
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| x/mongo/driver/session/options.go | Added SnapshotTime field to ClientOptions |
| x/mongo/driver/session/client_session.go | Moved SnapshotTime from pointer to value type with immutability flag |
| x/mongo/driver/operation.go | Updated snapshot time check to use SnapshotTimeSet flag |
| mongo/session.go | Changed ClientSession() to return copy instead of pointer |
| mongo/options/sessionoptions.go | Added SetSnapshotTime() builder method |
| mongo/client.go | Wired up SnapshotTime option in StartSession and cleaned up formatting |
| internal/spectest/skip.go | Removed skipped tests for GODRIVER-3663 |
| internal/require/require.go | Added Empty() assertion function |
| internal/integration/unified_spec_test.go | Updated to use session copy and improved session validation |
| internal/integration/unified/testrunner_operation.go | Updated function signature for session copy |
| internal/integration/unified/session_options.go | Added snapshotTimeID field for entity lookup |
| internal/integration/unified/session_operation_execution.go | Implemented getSnapshotTime operation |
| internal/integration/unified/operation.go | Added getSnapshotTime case handler |
| internal/integration/unified/matches.go | Changed variable declarations to use short form |
| internal/integration/unified/entity.go | Enhanced addBSONEntity to handle any type and added snapshot time resolution |
| internal/integration/sessions_test.go | Added prose tests for snapshot time behavior |
| internal/integration/mtest/mongotest.go | Refactored Setup/Teardown into standalone methods |
| internal/integration/mongointernal_test.go | Removed test that relied on pointer mutation |
| internal/assert/assertions.go | Added Empty() assertion and removed extra blank lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
prestonvasquez
force-pushed
the
enhancement/godriver-3663-expose-atClusterTime
branch
from
a1ffbe6 to
3bbb4d6
Compare
Introduce Setup/Teardown helpers to centralize mtest client/collection initialization and cleanup. RunOpts now calls Setup/Teardown automatically, reducing duplicated setup logic and making manual New() usage clearer.
Add Empty assertions to the internal assert/require packages. This allows tests to assert "unset" for value types without relying on pointer-only checks and keeps assertions consistent with existing zero-value semantics.
Change Session.ClientSession to return a copy of the underlying driver session to prevent mutation of internal session state. Update internal test helpers for value semantics, add explicit session/pinning guards for testRunner ops, and remove a mongointernal panic test that no longer applies.
Expose snapshotTime on SessionOptions and plumb it through StartSession into the driver session. Driver sessions track snapshotTime as an immutable value (SnapshotTimeSet), validate snapshotTime cannot be provided when snapshot=false, and include atClusterTime in readConcern when set.
Extend the unified spec runner to round-trip snapshotTime values through entities. Parse sessionOptions.snapshotTime as an entity reference during entity creation and add a getSnapshotTime operation to store a session's snapshotTime as a BSON entity.
Remove GODRIVER-3663 skip list entries for snapshotTime snapshot session tests now that the driver and unified runner support them.
Add prose tests covering snapshotTime session option behavior. Validate StartSession rejects snapshotTime when snapshot=false, and verify Session.ClientSession() returns a defensive copy by demonstrating snapshotTime cannot be mutated through the returned value. Prose 22 is skipped because the Go driver does not expose a snapshotTime getter that errors for non-snapshot sessions.
prestonvasquez
force-pushed
the
enhancement/godriver-3663-expose-atClusterTime
branch
from
3bbb4d6 to
a4b00c5
Compare
| t.T.Run(name, func(wrapped *testing.T) { | ||
| sub := newT(wrapped, t.baseOpts, opts) | ||
| sub.Setup() | ||
| defer sub.Teardown() |
There was a problem hiding this comment.
Instead of deferring this call, use T.Cleanup so we never have to explicitly call it. Ideally call it in Setup. Also, you can unexport Teardown since it's only called internally.
E.g.
func (t *T) Setup() {
// ...
t.T.Cleanup(t.teardown())
}There was a problem hiding this comment.
Teardown and Setup were added in this PR so that I could use mtest with one-off non-subtest tests:
func TestSessionsProse_21_SettingSnapshotTimeWithoutSnapshot(t *testing.T) {
mt := mtest.New(t, mtOpts)
mt.Setup()
defer mt.Teardown()
}There was a problem hiding this comment.
Are there any cases where you call mt.Setup() and not call mt.Teardown() after the test? If not, setting mt.Teardown() as a test cleanup task means you don't have to remember to call it, preventing a possible programming error.
| mt := mtest.New(t, mtOpts) | ||
|
|
||
| mt.Setup() |
There was a problem hiding this comment.
Optional: Consider combining these two calls into a new function to make it easier to use, like mtest.Setup. We can reconcile the two entry points in GODRIVER-3656.
E.g.
func TestBlah(...) {
mt := mtest.Setup(t, mtOpts)
// ...
}package mtest
func Setup(...) *T {
mt := mtest.New(...)
mt.setup()
// ...
return mt
}There was a problem hiding this comment.
Suggest deferring to GODRIVER-3721 / GODRIVER-3656, which should end up removing Setup entirely.
There was a problem hiding this comment.
Sounds good. Consider this resolved.
| func (s *Session) ClientSession() session.Client { | ||
| if s.clientSession == nil { | ||
| return session.Client{} | ||
| } | ||
|
|
||
| return *s.clientSession // Return a copy to prevent mutation. |
There was a problem hiding this comment.
This change doesn't make a deep copy, so many fields can still be mutated. I'm also concerned that this may break some internal use case that requires modifying session.Client values.
Do you have a concrete example of what we're trying to avoid with this change? Are there examples of external users doing that?
Edit: I see the description links to this comment as an example of dangerous behavior, but that seems like behavior we're facilitating by adding SetSnapshotTime. What does it have to do with returning a copy from Session.ClientSession?
There was a problem hiding this comment.
This change doesn't make a deep copy
The intent is to prevent mutation of the live session’s top-level state through this deprecated accessor. A deep copy seems unnecessary at the moment.
I'm also concerned that this may break some internal use case that requires modifying session.Client values.
IIRC the only reason we export this accessor is for testing. Letting callers mutate the underlying configuration bypasses invariants and can create bugs that are very difficult to reason about.
We should create a follow-up ticket to use a build tag to hide ClientSession from non-testing environments.
Edit: I see the description links to this comment as an example of dangerous behavior, but that seems like behavior we're facilitating by adding SetSnapshotTime. What does it have to do with returning a copy from Session.ClientSession?
Snapshot sessions cannot be used in transactions, so this conflict scenario doesn't apply to snapshotTime on snapshot sessions. However, if Session.ClientSession returns a pointer then users could set snapshotTime with snapshot=false which could lead to the conflict scenario.
There was a problem hiding this comment.
Makes sense. In that case, I believe we can remove the ClientSession method completely and use reflection to access it for testing, similar to how we extract a Topology from a Client.
E.g.
func getClientSessionFromSession(sess *mongo.Session) *session.Client {
elem := reflect.ValueOf(sess).Elem()
field := elem.FieldByName("clientSession")
field = reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem()
return field.Interface().(*session.Client)
}I also did a 3rd party code search for uses of ClientSession and found no uses.
2 participants
GODRIVER-3663
Summary
The snapshot session specification states that drivers should store the atClusterTime value in a private field, but there are many instances where an application may want to set/access the value.
Add
T.Setup()andT.Teardown()tomtestto allow using mtest on one-off tests without subtests.Add assert.Empty and require.Empty to our vendored testify code.
This change is required to prevent users from mutating the client session, which can lead to dangerous behavior. This function is deprecated with the warning "It may be changed or removed in any release." Regardless, we should consider the existing definition as a bug.
Background & Motivation
Users of a driver that would like to take advantage of snapshot reads.