feat(NODE-5233)!: prevent session from one client from being used on another

[nbbeeken]

Description

What is changing?

• Add a check for the same client reference
• Throw if they do not match

Is there new documentation needed for these changes?

API docs on startSession updated

What is the motivation for this change?

See highlights

Release Highlight

Driver methods throw if a session is provided from a different MongoClient

Providing a session from one MongoClient to a method on a different MongoClient has never been a supported use case and leads to undefined behavior. To prevent this mistake, the driver now throws a if session is provided to a driver helper from a different MongoClient.

// pre v6
const session = client1.startSession();
client2.db('foo').collection('bar').insertOne({ name: 'john doe' }, { session }); // no error thrown, undefined behavior

// v6+
const session = client1.startSession();
client2.db('foo').collection('bar').insertOne({ name: 'john doe' }, { session }); 
// MongoInvalidArgumentError thrown

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[baileympearson]

questions from kickoff:

Did we decide referential equality was okay?

• best way to identify client? - instanceof is brittle, we can generate a unique id and/or use a symbol property; needs to account for legacy client usage
• what other identifier is there?
• what problems are there with checking referential equality?

Do we want to throw if a session from a LegacyMongoClient is passed into a MongoClient or vice versa?

---

Session prose test 5 says the following:

Repeat the above for all methods that take a session parameter.

Did we decide not to implement this and if so, should we add more APIs to the test? I don't think we need to exhaustively test the driver, but we should probably test more than a single insertOne.

---

I see the following as tests in the kickoff but not in the PR:

• Unit test can check when sessions are and are not supported
• Check when a session is and is not from the client passed to executeOperation

I thought the plan was to unit test this in some way (specifically to unit test execute operation). Did the plan change?

[baileympearson]

Should this be a MongoInvalidArgument error? We this error should be thrown when the user provides an invalid session as an argument.

[nbbeeken]

From huddle:

• reference check good
• Add test for legacy client usage
• Add cursor and gridfs operations to the prose testing
• No unit tests