feat(NODE-7223): run checkout on connect regardless of credentials #4712
Conversation
| if (session?.snapshotEnabled && !topology.capabilities.supportsSnapshotReads) { | ||
| throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later'); | ||
| } |
There was a problem hiding this comment.
Now that server selection may not run above, we have stale "supportsSnapshotReads" information, however, this is always been true when relying upon this value before running server selection which may trigger monitoring checks to get the latest topology info.
| * @param options - Optional configuration options for the client | ||
| * | ||
| * @see docs.mongodb.org/manual/reference/connection-string/ | ||
| * @see https://www.mongodb.com/docs/manual/reference/connection-string/ |
There was a problem hiding this comment.
dunno if this matters but I get redirected when I visit the original, so figured we could skip the redirect?
| const error = await client | ||
| .db() | ||
| .collection('test') | ||
| .insertOne({ a: 1 }, { timeoutMS: 500 }) | ||
| .catch(error => error); |
There was a problem hiding this comment.
This change is a bit weird but maybe this is just a test clean up? Auto-connect has not started respecting timeoutMS with this change, rather insertOne is now paying the cost of being the first to select a server, which we simulate with sinon, generally the first serverSelection in a MongoClient's lifecycle is going to be the slowest as discovery is run.
So this is correct for the change we're making, the bit about not double server selecting, but the outcome is that auto-connecting operations are now subject to the higher cost of discovery (which can also be true for any operation at any time, but arguably less likely).
Citing our docs:
If you need predictable operation timing with
timeoutMS, callconnectexplicitly before performing operations.
I think this change makes sense and I can make it clear in the release notes here.
| 'Failed client bulkWrite operation: log messages have operationIds', | ||
| 'Successful bulkWrite operation: log messages have operationIds', | ||
| 'Successful client bulkWrite operation: log messages have operationIds' |
There was a problem hiding this comment.
Old bulkWrite we knew we didn't have operationId support but by nature of copying out Node.js specific versions of these tests did we miss adding support to client.bulkWrite too? I have not looked into what it is or does, so maybe it doesn't fit into our API well.
| }; | ||
|
|
||
| try { | ||
| const server = await this.selectServer( |
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
This introduces latency while the driver attempts to discover the topology for a different read preference than the auto-connecting operation is using. Since this selectServer uses MongoClient ReadPreference or default primary and the operation can use secondary or specific tagSets we block in auto-connect for longer than needed or end up throwing MongoServerSelectionError for an operation that may have a selectable server.
Description
Summary of Changes
skipPingOnConnectsection so that auto-connect no longer runs a duplicate server selection (since it is going to run one to service the operation anyway)Notes for Reviewers
modecapitalization was differenttagSetsinstead oftagsWhat is the motivation for this change?
Make connect's API consistent in all environments as a fail-fast mechanism for preparing a MongoClient for use within an application. In our particular case this is compass/mongosh/vscode which shares a common wrapping around client.connect that all those upstream projects expect certain invariants to be true if connect returns successfully. These include: Auth is configured correctly, hostnames are correct, SRV has been resolved, and TLS is setup correctly.
Release Highlight
Improve
MongoClient.connect()consistency across environmentsThe MongoClient connect function will now run a handshake regardless of credentials being defined. The upshot of this change is that connect is more consistent at verifying some fail-fast preconditions regardless of environment. For example, previously, if connecting to a
loadBalanced=truecluster without authentication there would not have been an error until a command was attempted.Additionally, when calling an operation method (find, insertOne, etc.) right away (a.k.a auto-connecting your client) the MongoClient will no longer run two server selections (one for connect, one for the operation) instead only the one for the operation will be performed.
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript