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.

Sign up for GitHub

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

Merged
merged 9 commits into from
May 18, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ab10b7d
feat(NODE-4192): make MongoClient.connect optional
nbbeeken May 9, 2022
c114b30
fix: prevent auto reconnecting after close
nbbeeken May 10, 2022
bb88b29
test: use hooks
nbbeeken May 11, 2022
1a9efa8
fix: use topo
nbbeeken May 12, 2022
1e90d81
fix: rm close
nbbeeken May 12, 2022
912799b
fix: test and error msg
nbbeeken May 16, 2022
da6392e
fix titles
nbbeeken May 17, 2022
6296ade
test: all the things
nbbeeken May 17, 2022
758103e
Fix up titles
nbbeeken May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/admin.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading