Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,21 @@ declare global {
* An optional string the test author can attach to print out why a test is skipped
*
* @example
* ```
* ```ts
* it.skip('my test', () => {
* //...
* }).skipReason = 'TODO(NODE-XXXX): Feature implementation impending!';
* ```
*
* The reporter (`test/tools/reporter/mongodb_reporter.js`) will print out the skipReason
* indented directly below the test name.
* ```
* ```txt
* - my test
* - TODO(NODE-XXXX): Feature implementation impending!
* ```
*
* You can also skip a set of tests via beforeEach:
* ```
* ```ts
* beforeEach(() => {
* if ('some condition') {
* this.currentTest.skipReason = 'requires <run condition> to run';
Expand Down
14 changes: 9 additions & 5 deletions src/admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class Admin {
options = Object.assign({ dbName: 'admin' }, options);

return executeOperation(
this.s.db,
this.s.db.s.client,
Copy link
Member

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?

Copy link
Contributor Author

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 RunCommandOperation(this.s.db, command, options),
callback
);
Expand Down Expand Up @@ -207,7 +207,7 @@ export class Admin {
options = Object.assign({ dbName: 'admin' }, options);

return executeOperation(
this.s.db,
this.s.db.s.client,
new AddUserOperation(this.s.db, username, password, options),
callback
);
Expand All @@ -233,7 +233,7 @@ export class Admin {
options = Object.assign({ dbName: 'admin' }, options);

return executeOperation(
this.s.db,
this.s.db.s.client,
new RemoveUserOperation(this.s.db, username, options),
callback
);
Expand Down Expand Up @@ -263,7 +263,7 @@ export class Admin {
options = options ?? {};

return executeOperation(
this.s.db,
this.s.db.s.client,
new ValidateCollectionOperation(this, collectionName, options),
callback
);
Expand All @@ -286,7 +286,11 @@ export class Admin {
if (typeof options === 'function') (callback = options), (options = {});
options = options ?? {};

return executeOperation(this.s.db, new ListDatabasesOperation(this.s.db, options), callback);
return executeOperation(
this.s.db.s.client,
new ListDatabasesOperation(this.s.db, options),
callback
);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions src/bulk/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -655,19 +655,19 @@ function executeCommands(
try {
if (isInsertBatch(batch)) {
executeOperation(
bulkOperation.s.collection,
bulkOperation.s.collection.s.db.s.client,
new InsertOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
resultHandler
);
} else if (isUpdateBatch(batch)) {
executeOperation(
bulkOperation.s.collection,
bulkOperation.s.collection.s.db.s.client,
new UpdateOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
resultHandler
);
} else if (isDeleteBatch(batch)) {
executeOperation(
bulkOperation.s.collection,
bulkOperation.s.collection.s.db.s.client,
new DeleteOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
resultHandler
);
Expand Down Expand Up @@ -1288,7 +1288,7 @@ export abstract class BulkOperationBase {
const finalOptions = { ...this.s.options, ...options };
const operation = new BulkWriteShimOperation(this, finalOptions);

return executeOperation(this.s.collection, operation, callback);
return executeOperation(this.s.collection.s.db.s.client, operation, callback);
}

/**
Expand Down
26 changes: 21 additions & 5 deletions src/change_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,24 @@ export class ChangeStream<

const cursorOptions: ChangeStreamCursorOptions = filterOptions(options, CURSOR_OPTIONS);

const client: MongoClient | null =
this.type === CHANGE_DOMAIN_TYPES.CLUSTER
? (this.parent as MongoClient)
: this.type === CHANGE_DOMAIN_TYPES.DATABASE
? (this.parent as Db).s.client
: this.type === CHANGE_DOMAIN_TYPES.COLLECTION
? (this.parent as Collection).s.db.s.client
: null;

if (client == null) {
Copy link
Contributor

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.

  1. Should we add test cases for this scenario?
  2. 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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

// This should never happen because of the assertion in the constructor
throw new MongoRuntimeError(
`Changestream type should only be one of cluster, database, collection. Found ${this.type.toString()}`
);
}

const changeStreamCursor = new ChangeStreamCursor<TSchema, TChange>(
getTopology(this.parent),
client,
this.namespace,
pipeline,
cursorOptions
Expand Down Expand Up @@ -835,12 +851,12 @@ export class ChangeStreamCursor<
pipeline: Document[];

constructor(
topology: Topology,
client: MongoClient,
namespace: MongoDBNamespace,
pipeline: Document[] = [],
options: ChangeStreamCursorOptions = {}
) {
super(topology, namespace, options);
super(client, namespace, options);

this.pipeline = pipeline;
this.options = options;
Expand Down Expand Up @@ -907,7 +923,7 @@ export class ChangeStreamCursor<
}

clone(): AbstractCursor<TChange> {
return new ChangeStreamCursor(this.topology, this.namespace, this.pipeline, {
return new ChangeStreamCursor(this.client, this.namespace, this.pipeline, {
...this.cursorOptions
});
}
Expand All @@ -920,7 +936,7 @@ export class ChangeStreamCursor<
});

executeOperation<TODO_NODE_3286, ChangeStreamAggregateRawResult<TChange>>(
session,
session.client,
aggregateOperation,
(err, response) => {
if (err || response == null) {
Expand Down
6 changes: 5 additions & 1 deletion src/cmap/wire_protocol/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ export function applyCommonQueryOptions(
return queryOptions;
}

export function isSharded(topologyOrServer: Topology | Server | Connection): boolean {
export function isSharded(topologyOrServer?: Topology | Server | Connection): boolean {
if (topologyOrServer == null) {
return false;
}

if (topologyOrServer.description && topologyOrServer.description.type === ServerType.Mongos) {
return true;
}
Expand Down