-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(NODE-4192): make MongoClient.connect optional #3232
Conversation
4e70af5
to
adfdabd
Compare
adfdabd
to
ab10b7d
Compare
const client = configuration.newClient('mongodb://unknownhost:36363/ddddd', { | ||
serverSelectionTimeoutMS: 10 | ||
}); | ||
it('should fail to connect due to unknown host in connection string', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
|
||
describe('#connect()', () => { | ||
it( | ||
'should create topology and send ping when auth is enabled', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() { ... });
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
? (this.parent as Collection).s.db.s.client | ||
: null; | ||
|
||
if (client == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a fair point. I'll leave this thread open for additional reviews for context.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const [pingOnConnect] = await commandToBeStarted; | ||
expect(pingOnConnect).to.have.property('commandName', 'ping'); | ||
expect(client).to.have.property('topology').that.is.instanceOf(Topology); | ||
await client.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to leave the client.close
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What client.close? 😎
src/operations/execute_operation.ts
Outdated
|
||
if (topology == null) { | ||
if (client.s.hasBeenClosed) { | ||
return callback(new MongoNotConnectedError('client was closed')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a more descriptive error message? Something like:
"attempted to execute operation with closed client"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of phrasing things as "must be" I went with this:
Client must be connected before running operations
}); | ||
|
||
it( | ||
'create topology and send ping when auth is enabled', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add tests for connect
with auth is disabled, if we're explicitly testing the behavior when it is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
}); | ||
|
||
it( | ||
'create topology and send ping when auth is enabled', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed most of these up, the close ones didn't really work
); | ||
|
||
it( | ||
'automatically connect upon first operation (find)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests that are testing the implicit connect should probably be in a separate describe block - this block is labeled explicit #connect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmk what you think of the new organization
} | ||
); | ||
|
||
it( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing a test to successfully execute an operation after explicitly re-opening the client with connect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an assertion to the connect after close
@@ -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 a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -83,7 +83,7 @@ export class Admin { | |||
options = Object.assign({ dbName: 'admin' }, options); | |||
|
|||
return executeOperation( | |||
this.s.db, | |||
this.s.db.s.client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
new MongoNotConnectedError('Client must be connected before running operations') | ||
); | ||
} | ||
client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question - is this feature flag still needed for those who still explicitly calling client.connect()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just suggestion for clarifying test case names
Co-authored-by: Daria Pardue <[email protected]>
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 Edit: specifically, this seems to affect tests with a change stream in "streaming" mode (ie using |
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 |
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
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>