feat(NODE-4192): make MongoClient.connect optional #3232

nbbeeken · 2022-05-04T18:07:46Z

Description

What is changing?

MongoClient.connect and client.connect are now optional! You can immediately proceed to running operations and the client will connect itself. Connect at first operation was chosen because it let's users still catch every command event that occurs as opposed to spinning up monitoring and "connect"-ing logic at construction time.

Is there new documentation needed for these changes?

Yes! We should update our examples to omit calls to connect. We can also call out that there is no plan to remove connect, there are valid use cases where an initial method call to "test the waters" of your current environment are desirable.

What is the motivation for this change?

Other drivers do not have a connect method so this aligns with us with our cohorts. Unified testing depends on the first operation being the one to create the first connection in a pool, kicking off monitoring events etc. Beyond that, it's convenient when working in a repl or scripting 😃

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>
Changes are covered by tests
New TODOs have a related JIRA ticket

baileympearson · 2022-05-10T17:44:45Z

test/integration/node-specific/mongo_client.test.ts

-      const client = configuration.newClient('mongodb://unknownhost:36363/ddddd', {
-        serverSelectionTimeoutMS: 10
-      });
+  it('should fail to connect due to unknown host in connection string', async function () {


Our team testing doc established the pattern of not using should in it blocks (https://docs.google.com/document/d/1V1PSxBWGf95bzKRPEJVGEnqnI5yDEvgU3GRQtHjJFoM/edit). Can we remove should from any new tests you added?

baileympearson · 2022-05-10T17:54:03Z

test/integration/node-specific/mongo_client.test.ts

+
+  describe('#connect()', () => {
+    it(
+      'should create topology and send ping when auth is enabled',


We also established using (preferably) one assertion per it block and a context block to group them together. This also applies to the other tests you added in this file

context('when auth is enabled', function() { it('creates the topology', async function() { ...} ); it('sends the ping command', async function() { ... }); });

I think that we can try to strive for that but I need an async context to do a lot of what my test is responsible for, so this would mean a lot of code dupe. I feel like the assertions I have are few enough that it is clear what the correct behavior is. Do you think it is okay as is?

test/integration/node-specific/mongo_client.test.ts

baileympearson · 2022-05-10T18:13:38Z

src/change_stream.ts

+        ? (this.parent as Collection).s.db.s.client
+        : null;
+
+    if (client == null) {


question(s): even though this error should never be thrown, in theory _createChangeStreamCursor now throws.

Should we add test cases for this scenario?

Should we update the usages of this method to ensure that we're catching the error and handling it appropriately in callback code? Specifically, I'm looking at _processError in change_stream.ts.

We could add a test, the thing is this is unreachable because the constructor will throw if one of these three symbols isn't used. This is one of those fatal errors where something quite unexpected has occurred. The test would be making a change stream and then reaching in an changing the type? I can do that, would that make sense?

Yeah, that's a fair point. I'll leave this thread open for additional reviews for context.

src/sdam/topology.ts

baileympearson · 2022-05-10T18:24:10Z

src/sdam/topology.ts

@@ -626,7 +628,7 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {

  /** Start a logical session */
  startSession(options: ClientSessionOptions, clientOptions?: MongoOptions): ClientSession {
-    const session = new ClientSession(this, this.s.sessionPool, options, clientOptions);
+    const session = new ClientSession(this.client, this.s.sessionPool, options, clientOptions);


It appears this method is the only reason we need to store a reference to the client on the topology class. Additionally, the client has a reference to the topology already.

How would you feel about removing this method and moving the logic into client.startSession? That also prevents the circular client -> topology -> client reference.

This method isn't marked as internal but Topology is so I think we're okay to make that change?

I attempted this, it's quite complex but we could do it in a follow up. I'll update when I've made a ticket

from slack - startSession on the client starts an explicit session every time whereas this can start both an explicit session or an implicit session. So any internal consumers of Topology.startSession would have to work around the client starting the explicit session.

baileympearson · 2022-05-12T19:54:38Z

test/integration/node-specific/mongo_client.test.ts

+        const [pingOnConnect] = await commandToBeStarted;
+        expect(pingOnConnect).to.have.property('commandName', 'ping');
+        expect(client).to.have.property('topology').that.is.instanceOf(Topology);
+        await client.close();


did you mean to leave the client.close here?

What client.close? 😎

baileympearson · 2022-05-16T13:37:45Z

src/operations/execute_operation.ts

+
+    if (topology == null) {
+      if (client.s.hasBeenClosed) {
+        return callback(new MongoNotConnectedError('client was closed'));


Can we add a more descriptive error message? Something like:

"attempted to execute operation with closed client"

In the spirit of phrasing things as "must be" I went with this:
Client must be connected before running operations

baileympearson · 2022-05-16T13:41:04Z

test/integration/node-specific/mongo_client.test.ts

+    });
+
+    it(
+      'create topology and send ping when auth is enabled',


We should add tests for connect with auth is disabled, if we're explicitly testing the behavior when it is enabled

baileympearson · 2022-05-16T13:42:01Z

test/integration/node-specific/mongo_client.test.ts

+    });
+
+    it(
+      'create topology and send ping when auth is enabled',


This comment applies to a number of tests in this file. But can we fix the wording? Instead of it('create topology....' it should be it('creates the topology...

I fixed most of these up, the close ones didn't really work

baileympearson · 2022-05-16T13:42:52Z

test/integration/node-specific/mongo_client.test.ts

+    );
+
+    it(
+      'automatically connect upon first operation (find)',


Any tests that are testing the implicit connect should probably be in a separate describe block - this block is labeled explicit #connect

lmk what you think of the new organization

baileympearson · 2022-05-16T13:44:38Z

test/integration/node-specific/mongo_client.test.ts

+      }
+    );
+
+    it(


We're missing a test to successfully execute an operation after explicitly re-opening the client with connect

I added an assertion to the connect after close

baileympearson · 2022-05-16T15:24:20Z

src/mongo_client.ts

@@ -446,6 +452,14 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> {
    forceOrCallback?: boolean | Callback<void>,
    callback?: Callback<void>
  ): Promise<void> | void {
+    // There's no way to set hasBeenClosed back to false
+    Object.defineProperty(this.s, 'hasBeenClosed', {


There was discussion in driver devs about whether or not we should allow clients to automatically reconnect if the client is closed (thread). Based on the feedback, we decided no.

test/integration/node-specific/mongo_client.test.ts

durran · 2022-05-17T13:55:12Z

src/admin.ts

@@ -83,7 +83,7 @@ export class Admin {
    options = Object.assign({ dbName: 'admin' }, options);

    return executeOperation(
-      this.s.db,
+      this.s.db.s.client,


I feel like while we are here, all the times we access the internal states to get the client we could just add a getter or method for it so that if in the future we change the way the internal states are represented we only have to change the client access in a single place. So instead of this.s.db.client everywhere we may have a this.client or this.getClient(). (Same for this.s.client). Thoughts?

Chatted offline, we'll likely clean this up later for now, TS will protect against us making changes here and we can apply some sweeping change later using that hinting

durran · 2022-05-17T14:01:03Z

src/operations/execute_operation.ts

+          new MongoNotConnectedError('Client must be connected before running operations')
+        );
+      }
+      client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true;


Just a question - is this feature flag still needed for those who still explicitly calling client.connect()?

No one needs to set this, it was just for us to use in tests to make them consistent for other drivers. I'm then using it here to make sure the auto connect doesn't run two operations.

dariakp

Just suggestion for clarifying test case names

test/integration/node-specific/mongo_client.test.ts

Co-authored-by: Daria Pardue <daria.pardue@mongodb.com>

alecgibson · 2022-06-14T09:37:09Z

Is there any documentation on how to use the client "correctly" now? This change has broken some of our unit tests where we try to tear down and recreate a MongoClient between them.

Edit: specifically, this seems to affect tests with a change stream in "streaming" mode (ie using collection.watch(...).on('change', ...))

nbbeeken · 2022-06-14T16:16:53Z

Hey @alecgibson the way to use the client correctly remains the same as before, the automatic connect is a new way that is also correct but has some known limitations that we're working on getting fixed very soon in the next patch! The watch method will no longer throw that you must be connected but unfortunately the change stream is unable to create a session needed to initialize the events. So to work around this I would continue using connect as was needed before and this will be covered in the fix in #3286

nbbeeken added the wip label May 4, 2022

nbbeeken force-pushed the NODE-4192/auto-connect branch 5 times, most recently from 4e70af5 to adfdabd Compare May 9, 2022 19:26

feat(NODE-4192): make MongoClient.connect optional

ab10b7d

nbbeeken force-pushed the NODE-4192/auto-connect branch from adfdabd to ab10b7d Compare May 9, 2022 19:49

nbbeeken removed wip labels May 9, 2022

baileympearson requested changes May 10, 2022

View reviewed changes

nbbeeken added 2 commits May 10, 2022 17:45

fix: prevent auto reconnecting after close

c114b30

test: use hooks

bb88b29

dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 12, 2022

fix: use topo

1a9efa8

nbbeeken requested a review from baileympearson May 12, 2022 19:29

baileympearson reviewed May 12, 2022

View reviewed changes

fix: rm close

1e90d81

nbbeeken requested a review from baileympearson May 12, 2022 20:00

baileympearson requested changes May 16, 2022

View reviewed changes

baileympearson reviewed May 16, 2022

View reviewed changes

dariakp self-requested a review May 16, 2022 15:28

dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 16, 2022

dariakp assigned baileympearson May 16, 2022

fix: test and error msg

912799b

nbbeeken requested a review from baileympearson May 16, 2022 18:43

baileympearson previously approved these changes May 16, 2022

View reviewed changes

dariakp requested changes May 16, 2022

View reviewed changes

durran requested changes May 17, 2022

View reviewed changes

nbbeeken added 2 commits May 17, 2022 13:30

fix titles

da6392e

test: all the things

6296ade

nbbeeken dismissed baileympearson’s stale review via 6296ade May 17, 2022 19:07

dariakp requested changes May 17, 2022

View reviewed changes

Fix up titles

758103e

Co-authored-by: Daria Pardue <daria.pardue@mongodb.com>

nbbeeken requested review from dariakp and durran May 17, 2022 21:49

dariakp approved these changes May 18, 2022

View reviewed changes

nbbeeken merged commit a2359e4 into main May 18, 2022

nbbeeken deleted the NODE-4192/auto-connect branch May 18, 2022 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-4192): make MongoClient.connect optional #3232

feat(NODE-4192): make MongoClient.connect optional #3232

nbbeeken commented May 4, 2022 •

edited

baileympearson May 10, 2022

nbbeeken May 12, 2022

baileympearson May 10, 2022

nbbeeken May 12, 2022

baileympearson May 10, 2022

nbbeeken May 12, 2022

baileympearson May 12, 2022

baileympearson May 10, 2022

nbbeeken May 12, 2022

baileympearson May 12, 2022

baileympearson May 12, 2022

nbbeeken May 12, 2022

baileympearson May 16, 2022

nbbeeken May 16, 2022

baileympearson May 16, 2022

nbbeeken May 16, 2022

baileympearson May 16, 2022

nbbeeken May 16, 2022

baileympearson May 16, 2022

nbbeeken May 16, 2022

baileympearson May 16, 2022

nbbeeken May 16, 2022

baileympearson May 16, 2022

durran May 17, 2022

nbbeeken May 17, 2022

durran May 17, 2022

nbbeeken May 17, 2022

dariakp left a comment

alecgibson commented Jun 14, 2022 •

edited

nbbeeken commented Jun 14, 2022

+                    }
+                  );
+                  it(

feat(NODE-4192): make MongoClient.connect optional #3232

feat(NODE-4192): make MongoClient.connect optional #3232

Conversation

nbbeeken commented May 4, 2022 • edited

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

alecgibson commented Jun 14, 2022 • edited

nbbeeken commented Jun 14, 2022

nbbeeken commented May 4, 2022 •

edited

alecgibson commented Jun 14, 2022 •

edited