From ab10b7d6eb9a036c46d7ba13d42368988542cb28 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 9 May 2022 15:26:42 -0400 Subject: [PATCH 1/9] feat(NODE-4192): make MongoClient.connect optional --- src/admin.ts | 14 +- src/bulk/common.ts | 8 +- src/change_stream.ts | 26 +- src/cmap/wire_protocol/shared.ts | 6 +- src/collection.ts | 61 ++-- src/cursor/abstract_cursor.ts | 29 +- src/cursor/aggregation_cursor.ts | 12 +- src/cursor/find_cursor.ts | 14 +- src/db.ts | 32 +- src/mongo_client.ts | 6 +- src/operations/connect.ts | 1 + src/operations/execute_operation.ts | 45 ++- src/operations/indexes.ts | 12 +- src/operations/list_collections.ts | 6 +- src/sdam/topology.ts | 6 +- src/sessions.ts | 29 +- src/utils.ts | 2 +- .../connection.test.js | 53 +--- ...go_client.test.js => mongo_client.test.ts} | 290 +++++++++++++----- test/unit/cursor/aggregation_cursor.test.js | 43 ++- test/unit/cursor/find_cursor.test.js | 59 ++-- test/unit/sessions.test.js | 2 +- 22 files changed, 445 insertions(+), 311 deletions(-) rename test/integration/node-specific/{mongo_client.test.js => mongo_client.test.ts} (51%) diff --git a/src/admin.ts b/src/admin.ts index 78d9df17cf2..303af58a718 100644 --- a/src/admin.ts +++ b/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, new RunCommandOperation(this.s.db, command, options), callback ); @@ -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 ); @@ -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 ); @@ -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 ); @@ -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 + ); } /** diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 9a050f918a2..41b69da657a 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -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 ); @@ -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); } /** diff --git a/src/change_stream.ts b/src/change_stream.ts index d48f4ccddc3..4c0b8c91dbb 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -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) { + // 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( - getTopology(this.parent), + client, this.namespace, pipeline, cursorOptions @@ -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; @@ -907,7 +923,7 @@ export class ChangeStreamCursor< } clone(): AbstractCursor { - return new ChangeStreamCursor(this.topology, this.namespace, this.pipeline, { + return new ChangeStreamCursor(this.client, this.namespace, this.pipeline, { ...this.cursorOptions }); } @@ -920,7 +936,7 @@ export class ChangeStreamCursor< }); executeOperation>( - session, + session.client, aggregateOperation, (err, response) => { if (err || response == null) { diff --git a/src/cmap/wire_protocol/shared.ts b/src/cmap/wire_protocol/shared.ts index ee4a35c0d4d..bc13ff6d856 100644 --- a/src/cmap/wire_protocol/shared.ts +++ b/src/cmap/wire_protocol/shared.ts @@ -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; } diff --git a/src/collection.ts b/src/collection.ts index b749b8e76ba..8bb2fb0b8a2 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -93,7 +93,6 @@ import { checkCollectionName, DEFAULT_PK_FACTORY, emitWarningOnce, - getTopology, MongoDBNamespace, normalizeHintField, resolveOptions @@ -296,7 +295,7 @@ export class Collection { } return executeOperation( - this, + this.s.db.s.client, new InsertOneOperation( this as TODO_NODE_3286, doc, @@ -338,7 +337,7 @@ export class Collection { options = options ? Object.assign({}, options) : { ordered: true }; return executeOperation( - this, + this.s.db.s.client, new InsertManyOperation( this as TODO_NODE_3286, docs, @@ -406,7 +405,7 @@ export class Collection { } return executeOperation( - this, + this.s.db.s.client, new BulkWriteOperation( this as TODO_NODE_3286, operations as TODO_NODE_3286, @@ -453,7 +452,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new UpdateOneOperation( this as TODO_NODE_3286, filter, @@ -501,7 +500,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new ReplaceOneOperation( this as TODO_NODE_3286, filter, @@ -549,7 +548,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new UpdateManyOperation( this as TODO_NODE_3286, filter, @@ -583,7 +582,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new DeleteOneOperation(this as TODO_NODE_3286, filter, resolveOptions(this, options)), callback ); @@ -623,7 +622,7 @@ export class Collection { } return executeOperation( - this, + this.s.db.s.client, new DeleteManyOperation(this as TODO_NODE_3286, filter, resolveOptions(this, options)), callback ); @@ -652,7 +651,7 @@ export class Collection { // Intentionally, we do not inherit options from parent for this operation. return executeOperation( - this, + this.s.db.s.client, new RenameOperation(this as TODO_NODE_3286, newName, { ...options, readPreference: ReadPreference.PRIMARY @@ -679,7 +678,7 @@ export class Collection { options = options ?? {}; return executeOperation( - this, + this.s.db.s.client, new DropCollectionOperation(this.s.db, this.collectionName, options), callback ); @@ -759,7 +758,7 @@ export class Collection { } return new FindCursor>( - getTopology(this), + this.s.db.s.client, this.s.namespace, filter, resolveOptions(this as TODO_NODE_3286, options) @@ -783,7 +782,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new OptionsOperation(this as TODO_NODE_3286, resolveOptions(this, options)), callback ); @@ -806,7 +805,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new IsCappedOperation(this as TODO_NODE_3286, resolveOptions(this, options)), callback ); @@ -857,7 +856,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new CreateIndexOperation( this as TODO_NODE_3286, this.collectionName, @@ -918,7 +917,7 @@ export class Collection { if (typeof options.maxTimeMS !== 'number') delete options.maxTimeMS; return executeOperation( - this, + this.s.db.s.client, new CreateIndexesOperation( this as TODO_NODE_3286, this.collectionName, @@ -952,7 +951,7 @@ export class Collection { options.readPreference = ReadPreference.primary; return executeOperation( - this, + this.s.db.s.client, new DropIndexOperation(this as TODO_NODE_3286, indexName, options), callback ); @@ -975,7 +974,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new DropIndexesOperation(this as TODO_NODE_3286, resolveOptions(this, options)), callback ); @@ -1013,7 +1012,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new IndexExistsOperation(this as TODO_NODE_3286, indexes, resolveOptions(this, options)), callback ); @@ -1036,7 +1035,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new IndexInformationOperation(this.s.db, this.collectionName, resolveOptions(this, options)), callback ); @@ -1058,7 +1057,7 @@ export class Collection { ): Promise | void { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new EstimatedDocumentCountOperation(this as TODO_NODE_3286, resolveOptions(this, options)), callback ); @@ -1118,7 +1117,7 @@ export class Collection { filter ??= {}; return executeOperation( - this, + this.s.db.s.client, new CountDocumentsOperation( this as TODO_NODE_3286, filter as Document, @@ -1193,7 +1192,7 @@ export class Collection { filter ??= {}; return executeOperation( - this, + this.s.db.s.client, new DistinctOperation( this as TODO_NODE_3286, key as TODO_NODE_3286, @@ -1221,7 +1220,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new IndexesOperation(this as TODO_NODE_3286, resolveOptions(this, options)), callback ); @@ -1245,7 +1244,7 @@ export class Collection { options = options ?? {}; return executeOperation( - this, + this.s.db.s.client, new CollStatsOperation(this as TODO_NODE_3286, options), callback ); @@ -1277,7 +1276,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new FindOneAndDeleteOperation( this as TODO_NODE_3286, filter, @@ -1324,7 +1323,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new FindOneAndReplaceOperation( this as TODO_NODE_3286, filter, @@ -1372,7 +1371,7 @@ export class Collection { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.db.s.client, new FindOneAndUpdateOperation( this as TODO_NODE_3286, filter, @@ -1408,7 +1407,7 @@ export class Collection { } return new AggregationCursor( - getTopology(this), + this.s.db.s.client, this.s.namespace, pipeline, resolveOptions(this, options) @@ -1526,7 +1525,7 @@ export class Collection { } return executeOperation( - this, + this.s.db.s.client, new MapReduceOperation( this as TODO_NODE_3286, map, @@ -1667,7 +1666,7 @@ export class Collection { filter ??= {}; return executeOperation( - this, + this.s.db.s.client, new CountOperation( MongoDBNamespace.fromString(this.namespace), filter, diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index 8fff8c8dd9b..d9069e933e7 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -10,13 +10,13 @@ import { MongoRuntimeError, MongoTailableCursorError } from '../error'; +import type { MongoClient } from '../mongo_client'; import { TODO_NODE_3286, TypedEventEmitter } from '../mongo_types'; import { executeOperation, ExecutionResult } from '../operations/execute_operation'; import { GetMoreOperation } from '../operations/get_more'; import { ReadConcern, ReadConcernLike } from '../read_concern'; import { ReadPreference, ReadPreferenceLike } from '../read_preference'; import type { Server } from '../sdam/server'; -import type { Topology } from '../sdam/topology'; import { ClientSession, maybeClearPinnedConnection } from '../sessions'; import { Callback, maybePromise, MongoDBNamespace, ns } from '../utils'; @@ -29,7 +29,7 @@ const kServer = Symbol('server'); /** @internal */ const kNamespace = Symbol('namespace'); /** @internal */ -const kTopology = Symbol('topology'); +const kClient = Symbol('client'); /** @internal */ const kSession = Symbol('session'); /** @internal */ @@ -126,7 +126,7 @@ export abstract class AbstractCursor< /** @internal */ [kDocuments]: TSchema[]; /** @internal */ - [kTopology]: Topology; + [kClient]: MongoClient; /** @internal */ [kTransform]?: (doc: TSchema) => any; /** @internal */ @@ -143,13 +143,16 @@ export abstract class AbstractCursor< /** @internal */ constructor( - topology: Topology, + client: MongoClient, namespace: MongoDBNamespace, options: AbstractCursorOptions = {} ) { super(); - this[kTopology] = topology; + if (!client.s.isMongoClient) { + throw new MongoRuntimeError('Cursor must be constructed with MongoClient'); + } + this[kClient] = client; this[kNamespace] = namespace; this[kDocuments] = []; // TODO: https://github.com/microsoft/TypeScript/issues/36230 this[kInitialized] = false; @@ -192,8 +195,8 @@ export abstract class AbstractCursor< } /** @internal */ - get topology(): Topology { - return this[kTopology]; + get client(): MongoClient { + return this[kClient]; } /** @internal */ @@ -236,7 +239,7 @@ export abstract class AbstractCursor< } get loadBalanced(): boolean { - return this[kTopology].loadBalanced; + return !!this[kClient].topology?.loadBalanced; } /** Returns current buffered documents length */ @@ -630,7 +633,7 @@ export abstract class AbstractCursor< batchSize }); - executeOperation(this, getMoreOperation, callback); + executeOperation(this[kClient], getMoreOperation, callback); } /** @@ -642,13 +645,13 @@ export abstract class AbstractCursor< */ [kInit](callback: Callback): void { if (this[kSession] == null) { - if (this[kTopology].shouldCheckForSessionSupport()) { - return this[kTopology].selectServer(ReadPreference.primaryPreferred, {}, err => { + if (this[kClient].topology?.shouldCheckForSessionSupport()) { + return this[kClient].topology?.selectServer(ReadPreference.primaryPreferred, {}, err => { if (err) return callback(err); return this[kInit](callback); }); - } else if (this[kTopology].hasSessionSupport()) { - this[kSession] = this[kTopology].startSession({ owner: this, explicit: false }); + } else if (this[kClient].topology?.hasSessionSupport()) { + this[kSession] = this[kClient].startSession({ owner: this, explicit: false }); } } diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index 39d57f073fe..350696ee4ea 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -1,8 +1,8 @@ import type { Document } from '../bson'; import type { ExplainVerbosityLike } from '../explain'; +import type { MongoClient } from '../mongo_client'; import { AggregateOperation, AggregateOptions } from '../operations/aggregate'; import { executeOperation, ExecutionResult } from '../operations/execute_operation'; -import type { Topology } from '../sdam/topology'; import type { ClientSession } from '../sessions'; import type { Sort } from '../sort'; import type { Callback, MongoDBNamespace } from '../utils'; @@ -33,12 +33,12 @@ export class AggregationCursor extends AbstractCursor { /** @internal */ constructor( - topology: Topology, + client: MongoClient, namespace: MongoDBNamespace, pipeline: Document[] = [], options: AggregateOptions = {} ) { - super(topology, namespace, options); + super(client, namespace, options); this[kPipeline] = pipeline; this[kOptions] = options; @@ -51,7 +51,7 @@ export class AggregationCursor extends AbstractCursor { clone(): AggregationCursor { const clonedOptions = mergeOptions({}, this[kOptions]); delete clonedOptions.session; - return new AggregationCursor(this.topology, this.namespace, this[kPipeline], { + return new AggregationCursor(this.client, this.namespace, this[kPipeline], { ...clonedOptions }); } @@ -68,7 +68,7 @@ export class AggregationCursor extends AbstractCursor { session }); - executeOperation(this, aggregateOperation, (err, response) => { + executeOperation(this.client, aggregateOperation, (err, response) => { if (err || response == null) return callback(err); // TODO: NODE-2882 @@ -88,7 +88,7 @@ export class AggregationCursor extends AbstractCursor { if (verbosity == null) verbosity = true; return executeOperation( - this, + this.client, new AggregateOperation(this.namespace, this[kPipeline], { ...this[kOptions], // NOTE: order matters here, we may need to refine this ...this.cursorOptions, diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index ae483ca2158..8034bfd7c10 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -1,12 +1,12 @@ import type { Document } from '../bson'; import { MongoInvalidArgumentError, MongoTailableCursorError } from '../error'; import type { ExplainVerbosityLike } from '../explain'; +import type { MongoClient } from '../mongo_client'; import type { CollationOptions } from '../operations/command'; import { CountOperation, CountOptions } from '../operations/count'; import { executeOperation, ExecutionResult } from '../operations/execute_operation'; import { FindOperation, FindOptions } from '../operations/find'; import type { Hint } from '../operations/operation'; -import type { Topology } from '../sdam/topology'; import type { ClientSession } from '../sessions'; import { formatSort, Sort, SortDirection } from '../sort'; import { Callback, emitWarningOnce, mergeOptions, MongoDBNamespace } from '../utils'; @@ -40,12 +40,12 @@ export class FindCursor extends AbstractCursor { /** @internal */ constructor( - topology: Topology, + client: MongoClient, namespace: MongoDBNamespace, filter: Document | undefined, options: FindOptions = {} ) { - super(topology, namespace, options); + super(client, namespace, options); this[kFilter] = filter || {}; this[kBuiltOptions] = options; @@ -58,7 +58,7 @@ export class FindCursor extends AbstractCursor { clone(): FindCursor { const clonedOptions = mergeOptions({}, this[kBuiltOptions]); delete clonedOptions.session; - return new FindCursor(this.topology, this.namespace, this[kFilter], { + return new FindCursor(this.client, this.namespace, this[kFilter], { ...clonedOptions }); } @@ -75,7 +75,7 @@ export class FindCursor extends AbstractCursor { session }); - executeOperation(this, findOperation, (err, response) => { + executeOperation(this.client, findOperation, (err, response) => { if (err || response == null) return callback(err); // TODO: We only need this for legacy queries that do not support `limit`, maybe @@ -143,7 +143,7 @@ export class FindCursor extends AbstractCursor { options = options ?? {}; return executeOperation( - this, + this.client, new CountOperation(this.namespace, this[kFilter], { ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this ...this.cursorOptions, @@ -165,7 +165,7 @@ export class FindCursor extends AbstractCursor { if (verbosity == null) verbosity = true; return executeOperation( - this, + this.client, new FindOperation(undefined, this.namespace, this[kFilter], { ...this[kBuiltOptions], // NOTE: order matters here, we may need to refine this ...this.cursorOptions, diff --git a/src/db.ts b/src/db.ts index 257dfd20391..3f6a1f0f142 100644 --- a/src/db.ts +++ b/src/db.ts @@ -258,7 +258,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new CreateCollectionOperation(this, name, resolveOptions(this, options)) as TODO_NODE_3286, callback ) as TODO_NODE_3286; @@ -286,7 +286,11 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); // Intentionally, we do not inherit options from parent for this operation. - return executeOperation(this, new RunCommandOperation(this, command, options ?? {}), callback); + return executeOperation( + this.s.client, + new RunCommandOperation(this, command, options ?? {}), + callback + ); } /** @@ -310,7 +314,7 @@ export class Db { } return new AggregationCursor( - getTopology(this), + this.s.client, this.s.namespace, pipeline, resolveOptions(this, options) @@ -355,7 +359,7 @@ export class Db { ): Promise | void { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new DbStatsOperation(this, resolveOptions(this, options)), callback ); @@ -434,7 +438,7 @@ export class Db { options.new_collection = true; return executeOperation( - this, + this.s.client, new RenameOperation( this.collection(fromCollection) as TODO_NODE_3286, toCollection, @@ -463,7 +467,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new DropCollectionOperation(this, name, resolveOptions(this, options)), callback ); @@ -486,7 +490,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new DropDatabaseOperation(this, resolveOptions(this, options)), callback ); @@ -509,7 +513,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new CollectionsOperation(this, resolveOptions(this, options)), callback ); @@ -545,7 +549,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new CreateIndexOperation(this, name, indexSpec, resolveOptions(this, options)), callback ); @@ -591,7 +595,7 @@ export class Db { } return executeOperation( - this, + this.s.client, new AddUserOperation(this, username, password, resolveOptions(this, options)), callback ); @@ -616,7 +620,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new RemoveUserOperation(this, username, resolveOptions(this, options)), callback ); @@ -648,7 +652,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new SetProfilingLevelOperation(this, level, resolveOptions(this, options)), callback ); @@ -671,7 +675,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new ProfilingLevelOperation(this, resolveOptions(this, options)), callback ); @@ -700,7 +704,7 @@ export class Db { if (typeof options === 'function') (callback = options), (options = {}); return executeOperation( - this, + this.s.client, new IndexInformationOperation(this, name, resolveOptions(this, options)), callback ); diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 90edf2a1a40..6adfd387a09 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -268,11 +268,12 @@ export interface MongoClientPrivate { sessions: Set; bsonOptions: BSONSerializeOptions; namespace: MongoDBNamespace; - readonly options?: MongoOptions; + readonly options: MongoOptions; readonly readConcern?: ReadConcern; readonly writeConcern?: WriteConcern; readonly readPreference: ReadPreference; readonly logger: Logger; + readonly isMongoClient: true; } /** @public */ @@ -366,6 +367,9 @@ export class MongoClient extends TypedEventEmitter { }, get logger() { return client[kOptions].logger; + }, + get isMongoClient(): true { + return true; } }; } diff --git a/src/operations/connect.ts b/src/operations/connect.ts index f2685426df4..a71e95867d5 100644 --- a/src/operations/connect.ts +++ b/src/operations/connect.ts @@ -62,6 +62,7 @@ function createTopology( // Events can be emitted before initialization is complete so we have to // save the reference to the topology on the client ASAP if the event handlers need to access it mongoClient.topology = topology; + topology.client = mongoClient; topology.once(Topology.OPEN, () => mongoClient.emit('open', mongoClient)); diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 22b69996c09..7849db7ff8d 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -12,6 +12,7 @@ import { MongoTransactionError, MongoUnexpectedServerResponseError } from '../error'; +import type { MongoClient } from '../mongo_client'; import { ReadPreference } from '../read_preference'; import type { Server } from '../sdam/server'; import { @@ -21,13 +22,7 @@ import { } from '../sdam/server_selection'; import type { Topology } from '../sdam/topology'; import type { ClientSession } from '../sessions'; -import { - Callback, - getTopology, - maybePromise, - supportsRetryableWrites, - TopologyProvider -} from '../utils'; +import { Callback, maybePromise, supportsRetryableWrites } from '../utils'; import { AbstractOperation, Aspect } from './operation'; const MMAPv1_RETRY_WRITES_ERROR_CODE = MONGODB_ERROR_CODES.IllegalOperation; @@ -66,45 +61,43 @@ export interface ExecutionResult { export function executeOperation< T extends AbstractOperation, TResult = ResultTypeFromOperation ->(topologyProvider: TopologyProvider, operation: T): Promise; +>(client: MongoClient, operation: T): Promise; export function executeOperation< T extends AbstractOperation, TResult = ResultTypeFromOperation ->(topologyProvider: TopologyProvider, operation: T, callback: Callback): void; +>(client: MongoClient, operation: T, callback: Callback): void; export function executeOperation< T extends AbstractOperation, TResult = ResultTypeFromOperation ->( - topologyProvider: TopologyProvider, - operation: T, - callback?: Callback -): Promise | void; +>(client: MongoClient, operation: T, callback?: Callback): Promise | void; export function executeOperation< T extends AbstractOperation, TResult = ResultTypeFromOperation ->( - topologyProvider: TopologyProvider, - operation: T, - callback?: Callback -): Promise | void { +>(client: MongoClient, operation: T, callback?: Callback): Promise | void { if (!(operation instanceof AbstractOperation)) { // TODO(NODE-3483): Extend MongoRuntimeError throw new MongoRuntimeError('This method requires a valid operation instance'); } return maybePromise(callback, callback => { - let topology: Topology; - try { - // TODO(NODE-4151): Use skipPingOnConnect and call connect here to make client.connect optional - topology = getTopology(topologyProvider); - } catch (error) { - return callback(error); + const topology = client.topology; + + if (topology == null) { + client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true; + return client.connect(error => { + delete client.s.options[Symbol.for('@@mdb.skipPingOnConnect')]; + if (error) { + return callback(error); + } + return executeOperation(client, operation, callback); + }); } + if (topology.shouldCheckForSessionSupport()) { return topology.selectServer(ReadPreference.primaryPreferred, {}, err => { if (err) return callback(err); - executeOperation(topologyProvider, operation, callback); + executeOperation(client, operation, callback); }); } diff --git a/src/operations/indexes.ts b/src/operations/indexes.ts index d7a27adfe01..af27bbcb139 100644 --- a/src/operations/indexes.ts +++ b/src/operations/indexes.ts @@ -7,13 +7,7 @@ import type { OneOrMore } from '../mongo_types'; import { ReadPreference } from '../read_preference'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { - Callback, - getTopology, - maxWireVersion, - MongoDBNamespace, - parseIndexOptions -} from '../utils'; +import { Callback, maxWireVersion, MongoDBNamespace, parseIndexOptions } from '../utils'; import { CollationOptions, CommandOperation, @@ -424,7 +418,7 @@ export class ListIndexesCursor extends AbstractCursor { options?: ListIndexesOptions; constructor(collection: Collection, options?: ListIndexesOptions) { - super(getTopology(collection), collection.s.namespace, options); + super(collection.s.db.s.client, collection.s.namespace, options); this.parent = collection; this.options = options; } @@ -444,7 +438,7 @@ export class ListIndexesCursor extends AbstractCursor { session }); - executeOperation(this.parent, operation, (err, response) => { + executeOperation(this.parent.s.db.s.client, operation, (err, response) => { if (err || response == null) return callback(err); // TODO: NODE-2882 diff --git a/src/operations/list_collections.ts b/src/operations/list_collections.ts index ccc515baabe..507b2c0b408 100644 --- a/src/operations/list_collections.ts +++ b/src/operations/list_collections.ts @@ -3,7 +3,7 @@ import { AbstractCursor } from '../cursor/abstract_cursor'; import type { Db } from '../db'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { Callback, getTopology, maxWireVersion } from '../utils'; +import { Callback, maxWireVersion } from '../utils'; import { CommandOperation, CommandOperationOptions } from './command'; import { executeOperation, ExecutionResult } from './execute_operation'; import { Aspect, defineAspects } from './operation'; @@ -97,7 +97,7 @@ export class ListCollectionsCursor< options?: ListCollectionsOptions; constructor(db: Db, filter: Document, options?: ListCollectionsOptions) { - super(getTopology(db), db.s.namespace, options); + super(db.s.client, db.s.namespace, options); this.parent = db; this.filter = filter; this.options = options; @@ -118,7 +118,7 @@ export class ListCollectionsCursor< session }); - executeOperation(this.parent, operation, (err, response) => { + executeOperation(this.parent.s.client, operation, (err, response) => { if (err || response == null) return callback(err); // TODO: NODE-2882 diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 45e0f7f9b8d..edda138bd4b 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -27,7 +27,7 @@ import { MongoServerSelectionError, MongoTopologyClosedError } from '../error'; -import type { MongoOptions, ServerApi } from '../mongo_client'; +import type { MongoClient, MongoOptions, ServerApi } from '../mongo_client'; import { TypedEventEmitter } from '../mongo_types'; import { ReadPreference, ReadPreferenceLike } from '../read_preference'; import { @@ -203,6 +203,8 @@ export class Topology extends TypedEventEmitter { /** @internal */ _type?: string; + client!: MongoClient; + /** @event */ static readonly SERVER_OPENING = SERVER_OPENING; /** @event */ @@ -626,7 +628,7 @@ export class Topology extends TypedEventEmitter { /** 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); session.once('ended', () => { this.s.sessions.delete(session); }); diff --git a/src/sessions.ts b/src/sessions.ts index 3fe4620a33d..00460425e9e 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -19,7 +19,7 @@ import { MongoTransactionError, MongoWriteConcernError } from './error'; -import type { MongoOptions } from './mongo_client'; +import type { MongoClient, MongoOptions } from './mongo_client'; import { TypedEventEmitter } from './mongo_types'; import { executeOperation } from './operations/execute_operation'; import { RunAdminCommandOperation } from './operations/run_command'; @@ -97,7 +97,7 @@ export interface EndSessionOptions { */ export class ClientSession extends TypedEventEmitter { /** @internal */ - topology: Topology; + client: MongoClient; /** @internal */ sessionPool: ServerSessionPool; hasEnded: boolean; @@ -124,22 +124,22 @@ export class ClientSession extends TypedEventEmitter { /** * Create a client session. * @internal - * @param topology - The current client's topology (Internal Class) + * @param client - The current client * @param sessionPool - The server session pool (Internal Class) * @param options - Optional settings * @param clientOptions - Optional settings provided when creating a MongoClient */ constructor( - topology: Topology, + client: MongoClient, sessionPool: ServerSessionPool, options: ClientSessionOptions, clientOptions?: MongoOptions ) { super(); - if (topology == null) { + if (client == null) { // TODO(NODE-3483) - throw new MongoRuntimeError('ClientSession requires a topology'); + throw new MongoRuntimeError('ClientSession requires a MongoClient'); } if (sessionPool == null || !(sessionPool instanceof ServerSessionPool)) { @@ -158,7 +158,7 @@ export class ClientSession extends TypedEventEmitter { } } - this.topology = topology; + this.client = client; this.sessionPool = sessionPool; this.hasEnded = false; this.clientOptions = clientOptions; @@ -205,7 +205,7 @@ export class ClientSession extends TypedEventEmitter { } get loadBalanced(): boolean { - return this.topology.description.type === TopologyType.LoadBalanced; + return this.client.topology?.description.type === TopologyType.LoadBalanced; } /** @internal */ @@ -394,9 +394,9 @@ export class ClientSession extends TypedEventEmitter { this.unpin(); } - const topologyMaxWireVersion = maxWireVersion(this.topology); + const topologyMaxWireVersion = maxWireVersion(this.client.topology); if ( - isSharded(this.topology) && + isSharded(this.client.topology) && topologyMaxWireVersion != null && topologyMaxWireVersion < minWireVersionForShardedTransactions ) { @@ -518,10 +518,11 @@ export function maybeClearPinnedConnection( return; } + const topology = session.client.topology; // NOTE: the spec talks about what to do on a network error only, but the tests seem to // to validate that we don't unpin on _all_ errors? - if (conn) { - const servers = Array.from(session.topology.s.servers.values()); + if (conn && topology != null) { + const servers = Array.from(topology.s.servers.values()); const loadBalancer = servers[0]; if (options?.error == null || options?.force) { @@ -760,7 +761,7 @@ function endTransaction( // send the command executeOperation( - session, + session.client, new RunAdminCommandOperation(undefined, command, { session, readPreference: ReadPreference.primary, @@ -784,7 +785,7 @@ function endTransaction( } return executeOperation( - session, + session.client, new RunAdminCommandOperation(undefined, command, { session, readPreference: ReadPreference.primary, diff --git a/src/utils.ts b/src/utils.ts index 6210e94dc77..9268b135620 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -348,7 +348,7 @@ export type TopologyProvider = */ export function getTopology(provider: TopologyProvider): Topology { // MongoClient or ClientSession or AbstractCursor - if (`topology` in provider && provider.topology) { + if ('topology' in provider && provider.topology) { return provider.topology; } else if ('s' in provider && 'client' in provider.s && provider.s.client.topology) { return provider.s.client.topology; diff --git a/test/integration/connection-monitoring-and-pooling/connection.test.js b/test/integration/connection-monitoring-and-pooling/connection.test.js index cfefbe987d0..81653e06cfc 100644 --- a/test/integration/connection-monitoring-and-pooling/connection.test.js +++ b/test/integration/connection-monitoring-and-pooling/connection.test.js @@ -1,10 +1,6 @@ 'use strict'; -const { - ServerHeartbeatStartedEvent, - MongoClient, - MongoNotConnectedError -} = require('../../../src'); +const { ServerHeartbeatStartedEvent, MongoClient } = require('../../../src'); const { Connection } = require('../../../src/cmap/connection'); const { connect } = require('../../../src/cmap/connect'); const { expect } = require('chai'); @@ -455,52 +451,5 @@ describe('Connection', function () { }); }) ); - - it('throws when attempting an operation if the client is not connected', function (done) { - const client = this.configuration.newClient(); - const collection = client.db('shouldCorrectlyFailOnRetry').collection('test'); - collection.insertOne({ a: 2 }, err => { - expect(err).to.be.instanceof(MongoNotConnectedError); - done(); - }); - }); - - it('throws when attempting an operation if the client is not connected (promises)', async function () { - const client = this.configuration.newClient(); - const collection = client.db('shouldCorrectlyFailOnRetry').collection('test'); - - const err = await collection.insertOne({ a: 2 }).catch(err => err); - expect(err).to.be.instanceof(MongoNotConnectedError); - }); - - it( - 'should correctly fail on retry when client has been closed', - withClient(function (client, done) { - const collection = client.db('shouldCorrectlyFailOnRetry').collection('test'); - collection.insertOne({ a: 1 }, (err, result) => { - expect(err).to.not.exist; - expect(result).to.exist; - - client.close(true, function (err) { - expect(err).to.not.exist; - - collection.insertOne({ a: 2 }, err => { - expect(err).to.be.instanceof(MongoNotConnectedError); - done(); - }); - }); - }); - }) - ); - - it('should correctly fail on retry when client has been closed (promises)', async function () { - const client = await this.configuration.newClient().connect(); - const collection = client.db('shouldCorrectlyFailOnRetry').collection('test'); - await collection.insertOne({ a: 1 }); - await client.close(true); - - const err = await collection.insertOne({ a: 2 }).catch(err => err); - expect(err).to.be.instanceof(MongoNotConnectedError); - }); }); }); diff --git a/test/integration/node-specific/mongo_client.test.js b/test/integration/node-specific/mongo_client.test.ts similarity index 51% rename from test/integration/node-specific/mongo_client.test.js rename to test/integration/node-specific/mongo_client.test.ts index 5f9fd0d3911..d8730495ada 100644 --- a/test/integration/node-specific/mongo_client.test.js +++ b/test/integration/node-specific/mongo_client.test.ts @@ -1,28 +1,21 @@ -'use strict'; -const { expect } = require('chai'); - -const sinon = require('sinon'); - -const { setupDatabase, assert: test } = require('../shared'); -const { format: f } = require('util'); - -const { MongoClient, ReadPreference } = require('../../../src'); -const { Db } = require('../../../src/db'); -const { Connection } = require('../../../src/cmap/connection'); -const { getTopology, isHello } = require('../../../src/utils'); - -describe('MongoClient integration', function () { +import { expect } from 'chai'; +import { once } from 'events'; +import * as sinon from 'sinon'; + +import { MongoClient, MongoServerSelectionError, ReadPreference } from '../../../src'; +import { Connection } from '../../../src/cmap/connection'; +import { Db } from '../../../src/db'; +import { Topology } from '../../../src/sdam/topology'; +import { getTopology, isHello } from '../../../src/utils'; +import { setupDatabase } from '../shared'; + +describe('class MongoClient', function () { before(function () { return setupDatabase(this.configuration); }); - it('Should correctly pass through extra db options', { - metadata: { - requires: { - topology: ['single'] - } - }, - + it('should correctly pass through extra db options', { + metadata: { requires: { topology: ['single'] } }, test: function (done) { const configuration = this.configuration; const client = configuration.newClient( @@ -46,17 +39,24 @@ describe('MongoClient integration', function () { const db = client.db(configuration.db); - test.equal(1, db.writeConcern.w); - test.equal(1000, db.writeConcern.wtimeout); - test.equal(true, db.writeConcern.fsync); - test.equal(true, db.writeConcern.j); - - test.equal('nearest', db.s.readPreference.mode); - test.deepEqual([{ loc: 'ny' }], db.s.readPreference.tags); - - test.equal(true, db.s.options.forceServerObjectId); - test.equal(1, db.s.pkFactory.createPk()); - test.equal(true, db.bsonOptions.serializeFunctions); + expect(db).to.have.property('writeConcern'); + expect(db.writeConcern).to.have.property('w', 1); + expect(db.writeConcern).to.have.property('wtimeout', 1000); + expect(db.writeConcern).to.have.property('fsync', true); + expect(db.writeConcern).to.have.property('j', true); + + expect(db).to.have.property('s'); + expect(db.s).to.have.property('readPreference'); + expect(db.s.readPreference).to.have.property('mode', 'nearest'); + expect(db.s.readPreference) + .to.have.property('tags') + .that.deep.equals([{ loc: 'ny' }]); + + expect(db.s).to.have.nested.property('options.forceServerObjectId'); + expect(db.s.options).to.have.property('forceServerObjectId', true); + expect(db.s).to.have.nested.property('pkFactory.createPk').that.is.a('function'); + expect(db.s.pkFactory.createPk()).to.equal(1); + expect(db).to.have.nested.property('bsonOptions.serializeFunctions'); client.close(done); }); @@ -86,7 +86,7 @@ describe('MongoClient integration', function () { }); client.connect(function (err) { - test.ok(err != null); + expect(err).to.exist; done(); }); @@ -97,7 +97,7 @@ describe('MongoClient integration', function () { metadata: { requires: { topology: ['single'], os: '!win32' } }, test: function (done) { - var configuration = this.configuration; + const configuration = this.configuration; const client = configuration.newClient('mongodb://%2Ftmp%2Fmongodb-27017.sock/test'); client.connect(function (err) { expect(err).to.not.exist; @@ -106,24 +106,14 @@ describe('MongoClient integration', function () { } }); - it('should fail dure to garbage connection string', { - metadata: { - requires: { - topology: ['single'] - } - }, - - test: function (done) { - const configuration = this.configuration; - const client = configuration.newClient('mongodb://unknownhost:36363/ddddd', { - serverSelectionTimeoutMS: 10 - }); + it('should fail to connect due to unknown host in connection string', async function () { + const configuration = this.configuration; + const client = configuration.newClient('mongodb://iLoveJavascript:36363/ddddd', { + serverSelectionTimeoutMS: 10 + }); - client.connect(function (err) { - test.ok(err != null); - done(); - }); - } + const error = await client.connect().catch(error => error); + expect(error).to.be.instanceOf(MongoServerSelectionError); }); it('Should correctly pass through appname', { @@ -142,7 +132,9 @@ describe('MongoClient integration', function () { client.connect(function (err, client) { expect(err).to.not.exist; - test.equal('hello world', client.topology.clientMetadata.application.name); + expect(client) + .to.have.nested.property('topology.clientMetadata.application.name') + .to.equal('hello world'); client.close(done); }); @@ -163,7 +155,9 @@ describe('MongoClient integration', function () { const client = configuration.newClient(url, { appname: 'hello world' }); client.connect(err => { expect(err).to.not.exist; - test.equal('hello world', client.topology.clientMetadata.application.name); + expect(client) + .to.have.nested.property('topology.clientMetadata.application.name') + .to.equal('hello world'); client.close(done); }); @@ -198,12 +192,7 @@ describe('MongoClient integration', function () { } }); - ////////////////////////////////////////////////////////////////////////////////////////// - // - // new MongoClient connection tests - // - ////////////////////////////////////////////////////////////////////////////////////////// - it('Should open a new MongoClient connection', { + it('should open a new MongoClient connection', { metadata: { requires: { topology: ['single'] @@ -221,7 +210,7 @@ describe('MongoClient integration', function () { .collection('new_mongo_client_collection') .insertOne({ a: 1 }, function (err, r) { expect(err).to.not.exist; - test.ok(r); + expect(r).to.be.an('object'); mongoclient.close(done); }); @@ -229,19 +218,16 @@ describe('MongoClient integration', function () { } }); - it('Should correctly connect with MongoClient `connect` using Promise', function () { + it('should correctly connect with MongoClient `connect` using Promise', function () { const configuration = this.configuration; let url = configuration.url(); - url = - url.indexOf('?') !== -1 - ? f('%s&%s', url, 'maxPoolSize=100') - : f('%s?%s', url, 'maxPoolSize=100'); + url = url.indexOf('?') !== -1 ? `${url}&maxPoolSize=100` : `${url}?maxPoolSize=100`; const client = configuration.newClient(url); return client.connect().then(() => client.close()); }); - it('Should open a new MongoClient connection using promise', { + it('should open a new MongoClient connection using promise', { metadata: { requires: { topology: ['single'] @@ -257,7 +243,7 @@ describe('MongoClient integration', function () { .collection('new_mongo_client_collection') .insertOne({ a: 1 }) .then(function (r) { - test.ok(r); + expect(r).to.exist; mongoclient.close(done); }); @@ -277,7 +263,9 @@ describe('MongoClient integration', function () { }) .catch(_err => (err = _err)) .then(() => client.close()) - .catch(() => {}) + .catch(() => { + // ignore + }) .then(() => { if (err) { throw err; @@ -295,11 +283,12 @@ describe('MongoClient integration', function () { metadata: { requires: { topology: 'single' } }, test: function (done) { - var configuration = this.configuration; + const configuration = this.configuration; MongoClient.connect( configuration.url(), { maxPoolSize: 4, + // @ts-expect-error: unexpected option test notlegal: {}, validateOptions: true }, @@ -318,12 +307,13 @@ describe('MongoClient integration', function () { metadata: { requires: { topology: 'single' } }, test() { - MongoClient.connect(this.configuration.url(), { + const options = { maxPoolSize: 4, notlegal: {}, validateOptions: true - }) - .then(() => expect().fail()) + }; + MongoClient.connect(this.configuration.url(), options) + .then(() => expect.fail()) .catch(err => { expect(err) .property('message') @@ -341,11 +331,12 @@ describe('MongoClient integration', function () { }); client.connect(err => { expect(err).to.not.exist; - const stub = sinon.stub(Connection.prototype, 'command').callsFake(function () { - const args = Array.prototype.slice.call(arguments); + const stub = sinon.stub(Connection.prototype, 'command').callsFake(function (...args) { const ns = args[0]; const command = args[1]; const options = args[2] || {}; + + // @ts-expect-error: exhaustAllowed is a protocol option if (ns.toString() === 'admin.$cmd' && isHello(command) && options.exhaustAllowed) { expect(options).property('socketTimeoutMS').to.equal(0); stub.restore(); @@ -366,11 +357,12 @@ describe('MongoClient integration', function () { }); client.connect(err => { expect(err).to.not.exist; - const stub = sinon.stub(Connection.prototype, 'command').callsFake(function () { - const args = Array.prototype.slice.call(arguments); + const stub = sinon.stub(Connection.prototype, 'command').callsFake(function (...args) { const ns = args[0]; const command = args[1]; const options = args[2] || {}; + + // @ts-expect-error: exhaustAllowed is a protocol option if (ns.toString() === 'admin.$cmd' && isHello(command) && options.exhaustAllowed) { expect(options).property('socketTimeoutMS').to.equal(510); stub.restore(); @@ -381,4 +373,150 @@ describe('MongoClient integration', function () { }); } }); + + describe('#connect()', () => { + it( + 'should create topology and send ping when auth is enabled', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + const commandToBeStarted = once(client, 'commandStarted'); + + await client.connect(); + + const [pingOnConnect] = await commandToBeStarted; + + expect(pingOnConnect).to.have.property('commandName', 'ping'); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); + + it( + 'should permit operations to be run after connect is called', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + const pingCommandToBeStarted = once(client, 'commandStarted'); + await client.connect(); + const [pingOnConnect] = await pingCommandToBeStarted; + + const findCommandToBeStarted = once(client, 'commandStarted'); + await client.db('test').collection('test').findOne(); + const [findCommandStarted] = await findCommandToBeStarted; + + expect(pingOnConnect).to.have.property('commandName', 'ping'); + expect(findCommandStarted).to.have.property('commandName', 'find'); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); + }); + + it( + 'should automatically connect upon first operation (find)', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + const findCommandToBeStarted = once(client, 'commandStarted'); + await client.db().collection('test').findOne(); + const [findCommandStarted] = await findCommandToBeStarted; + + expect(findCommandStarted).to.have.property('commandName', 'find'); + expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + + // Assertion is redundant but it shows that no initial ping is run + expect(findCommandStarted.commandName).to.not.equal('ping'); + + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); + + it( + 'should automatically connect upon first operation (insertOne)', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + const insertOneCommandToBeStarted = once(client, 'commandStarted'); + await client.db().collection('test').insertOne({ a: 1 }); + const [insertCommandStarted] = await insertOneCommandToBeStarted; + + expect(insertCommandStarted).to.have.property('commandName', 'insert'); + expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + + // Assertion is redundant but it shows that no initial ping is run + expect(insertCommandStarted.commandName).to.not.equal('ping'); + + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); + + it( + 'should pass connection errors to the user through the first operation', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient( + 'mongodb://iLoveJavascript?serverSelectionTimeoutMS=100', + { monitorCommands: true } + ); + + const result = await client + .db('test') + .collection('test') + .findOne() + .catch(error => error); + + expect(result).to.be.instanceOf(MongoServerSelectionError); + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); + + it( + 'should permit client to be reconnected if closed', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + await client.db('test').collection('test').findOne(); + + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + + expect(client).to.have.property('topology', undefined); + + await client.db('test').collection('test').findOne(); + + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + } + ); }); diff --git a/test/unit/cursor/aggregation_cursor.test.js b/test/unit/cursor/aggregation_cursor.test.js index 65a654dc097..226b7963c60 100644 --- a/test/unit/cursor/aggregation_cursor.test.js +++ b/test/unit/cursor/aggregation_cursor.test.js @@ -2,10 +2,11 @@ const { expect } = require('chai'); const mock = require('../../tools/mongodb-mock/index'); -const { Topology } = require('../../../src/sdam/topology'); const { Long } = require('bson'); const { MongoDBNamespace, isHello } = require('../../../src/utils'); const { AggregationCursor } = require('../../../src/cursor/aggregation_cursor'); +const { MongoClient } = require('../../../src/mongo_client'); +const { default: ConnectionString } = require('mongodb-connection-string-url'); const test = {}; describe('Aggregation Cursor', function () { @@ -37,17 +38,19 @@ describe('Aggregation Cursor', function () { }); it('sets the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress()); + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString() + ); const cursor = new AggregationCursor( - topology, + client, MongoDBNamespace.fromString('test.test'), [], {} ); - topology.connect(function () { + client.connect(function () { cursor.next(function () { expect(cursor.session).to.exist; - topology.close(done); + client.close(done); }); }); }); @@ -73,19 +76,22 @@ describe('Aggregation Cursor', function () { }); it('does not set the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { - serverSelectionTimeoutMS: 1000 - }); + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { + serverSelectionTimeoutMS: 1000 + } + ); const cursor = new AggregationCursor( - topology, + client, MongoDBNamespace.fromString('test.test'), [], {} ); - topology.connect(function () { + client.connect(function () { cursor.next(function () { expect(cursor.session).to.not.exist; - topology.close(done); + client.close(done); }); }); }); @@ -117,19 +123,22 @@ describe('Aggregation Cursor', function () { }); it('sets the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { - serverSelectionTimeoutMS: 1000 - }); + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { + serverSelectionTimeoutMS: 1000 + } + ); const cursor = new AggregationCursor( - topology, + client, MongoDBNamespace.fromString('test.test'), [], {} ); - topology.connect(function () { + client.connect(function () { cursor.next(function () { expect(cursor.session).to.exist; - topology.close(done); + client.close(done); }); }); }); diff --git a/test/unit/cursor/find_cursor.test.js b/test/unit/cursor/find_cursor.test.js index 4e4bb8d54bf..bd8515424f8 100644 --- a/test/unit/cursor/find_cursor.test.js +++ b/test/unit/cursor/find_cursor.test.js @@ -1,12 +1,12 @@ 'use strict'; -const expect = require('chai').expect; -const { MongoError } = require('../../../src/error'); +const { expect } = require('chai'); const mock = require('../../tools/mongodb-mock/index'); -const { Topology } = require('../../../src/sdam/topology'); const { Long } = require('bson'); const { MongoDBNamespace, isHello } = require('../../../src/utils'); const { FindCursor } = require('../../../src/cursor/find_cursor'); +const { MongoClient, MongoServerError } = require('../../../src'); +const { default: ConnectionString } = require('mongodb-connection-string-url'); const test = {}; describe('Find Cursor', function () { @@ -38,12 +38,17 @@ describe('Find Cursor', function () { }); it('sets the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress()); - const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); - topology.connect(function () { + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { + serverSelectionTimeoutMS: 1000 + } + ); + const cursor = new FindCursor(client, MongoDBNamespace.fromString('test.test'), {}, {}); + client.connect(function () { cursor.next(function () { expect(cursor.session).to.exist; - topology.close(done); + client.close(done); }); }); }); @@ -69,14 +74,17 @@ describe('Find Cursor', function () { }); it('does not set the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { - serverSelectionTimeoutMS: 1000 - }); - const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); - topology.connect(function () { + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { + serverSelectionTimeoutMS: 1000 + } + ); + const cursor = new FindCursor(client, MongoDBNamespace.fromString('test.test'), {}, {}); + client.connect(function () { cursor.next(function () { expect(cursor.session).to.not.exist; - topology.close(done); + client.close(done); }); }); }); @@ -108,14 +116,17 @@ describe('Find Cursor', function () { }); it('sets the session on the cursor', function (done) { - const topology = new Topology(test.server.hostAddress(), { - serverSelectionTimeoutMS: 1000 - }); - const cursor = new FindCursor(topology, MongoDBNamespace.fromString('test.test'), {}, {}); - topology.connect(function () { + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { + serverSelectionTimeoutMS: 1000 + } + ); + const cursor = new FindCursor(client, MongoDBNamespace.fromString('test.test'), {}, {}); + client.connect(function () { cursor.next(function () { expect(cursor.session).to.exist; - topology.close(done); + client.close(done); }); }); }); @@ -135,7 +146,10 @@ describe('Find Cursor', function () { errmsg: 'Cursor not found (namespace: "liveearth.entityEvents", id: 2018648316188432590).' }; - const client = new Topology(test.server.hostAddress()); + const client = new MongoClient( + new ConnectionString(`mongodb://${test.server.hostAddress()}`).toString(), + { serverSelectionTimeoutMS: 1000 } + ); test.server.setMessageHandler(request => { const doc = request.document; @@ -162,19 +176,18 @@ describe('Find Cursor', function () { }); client.on('error', done); - client.once('connect', () => { + client.connect(() => { const cursor = new FindCursor(client, MongoDBNamespace.fromString('test.test'), {}, {}); // Execute next cursor.next(function (err) { expect(err).to.exist; - expect(err).to.be.instanceof(MongoError); + expect(err).to.be.instanceof(MongoServerError); expect(err.message).to.equal(errdoc.errmsg); client.close(done); }); }); - client.connect(); }); }); }); diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 84de683ab2a..0ac8bf8bee4 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -163,7 +163,7 @@ describe('Sessions - unit', function () { it('should throw errors with invalid parameters', function () { expect(() => { new ClientSession(); - }).to.throw(/ClientSession requires a topology/); + }).to.throw(/ClientSession requires a MongoClient/); expect(() => { new ClientSession({}); From c114b300606674b7bc0880554fc0469ddd9613ba Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 10 May 2022 17:45:51 -0400 Subject: [PATCH 2/9] fix: prevent auto reconnecting after close --- src/mongo_client.ts | 10 + src/operations/execute_operation.ts | 4 + .../node-specific/mongo_client.test.ts | 244 +++++++++++------- 3 files changed, 170 insertions(+), 88 deletions(-) diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 6adfd387a09..2b04f01e639 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -268,6 +268,7 @@ export interface MongoClientPrivate { sessions: Set; bsonOptions: BSONSerializeOptions; namespace: MongoDBNamespace; + hasBeenClosed: boolean; readonly options: MongoOptions; readonly readConcern?: ReadConcern; readonly writeConcern?: WriteConcern; @@ -352,6 +353,7 @@ export class MongoClient extends TypedEventEmitter { sessions: new Set(), bsonOptions: resolveBSONOptions(this[kOptions]), namespace: ns('admin'), + hasBeenClosed: false, get options() { return client[kOptions]; @@ -450,6 +452,14 @@ export class MongoClient extends TypedEventEmitter { forceOrCallback?: boolean | Callback, callback?: Callback ): Promise | void { + // There's no way to set hasBeenClosed back to false + Object.defineProperty(this.s, 'hasBeenClosed', { + value: true, + enumerable: true, + configurable: false, + writable: false + }); + if (typeof forceOrCallback === 'function') { callback = forceOrCallback; } diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 7849db7ff8d..0ef74d3643f 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -7,6 +7,7 @@ import { MongoError, MongoExpiredSessionError, MongoNetworkError, + MongoNotConnectedError, MongoRuntimeError, MongoServerError, MongoTransactionError, @@ -83,6 +84,9 @@ export function executeOperation< const topology = client.topology; if (topology == null) { + if (client.s.hasBeenClosed) { + return callback(new MongoNotConnectedError('client was closed')); + } client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true; return client.connect(error => { delete client.s.options[Symbol.for('@@mdb.skipPingOnConnect')]; diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index d8730495ada..c54ec53bebb 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -2,7 +2,12 @@ import { expect } from 'chai'; import { once } from 'events'; import * as sinon from 'sinon'; -import { MongoClient, MongoServerSelectionError, ReadPreference } from '../../../src'; +import { + MongoClient, + MongoNotConnectedError, + MongoServerSelectionError, + ReadPreference +} from '../../../src'; import { Connection } from '../../../src/cmap/connection'; import { Db } from '../../../src/db'; import { Topology } from '../../../src/sdam/topology'; @@ -374,30 +379,25 @@ describe('class MongoClient', function () { } }); - describe('#connect()', () => { + context('#connect()', () => { it( - 'should create topology and send ping when auth is enabled', + 'create topology and send ping when auth is enabled', { requires: { auth: 'enabled' } }, async function () { const client = this.configuration.newClient(this.configuration.url(), { monitorCommands: true }); - const commandToBeStarted = once(client, 'commandStarted'); - await client.connect(); - const [pingOnConnect] = await commandToBeStarted; - expect(pingOnConnect).to.have.property('commandName', 'ping'); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - await client.close(); } ); it( - 'should permit operations to be run after connect is called', + 'permit operations to be run after connect is called', { requires: { auth: 'enabled' } }, async function () { const client = this.configuration.newClient(this.configuration.url(), { @@ -419,104 +419,172 @@ describe('class MongoClient', function () { await client.close(); } ); - }); - it( - 'should automatically connect upon first operation (find)', - { requires: { auth: 'enabled' } }, - async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); + it( + 'automatically connect upon first operation (find)', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); - const findCommandToBeStarted = once(client, 'commandStarted'); - await client.db().collection('test').findOne(); - const [findCommandStarted] = await findCommandToBeStarted; + const findCommandToBeStarted = once(client, 'commandStarted'); + await client.db().collection('test').findOne(); + const [findCommandStarted] = await findCommandToBeStarted; - expect(findCommandStarted).to.have.property('commandName', 'find'); - expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); - expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + expect(findCommandStarted).to.have.property('commandName', 'find'); + expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); - // Assertion is redundant but it shows that no initial ping is run - expect(findCommandStarted.commandName).to.not.equal('ping'); + // Assertion is redundant but it shows that no initial ping is run + expect(findCommandStarted.commandName).to.not.equal('ping'); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); - await client.close(); - } - ); - - it( - 'should automatically connect upon first operation (insertOne)', - { requires: { auth: 'enabled' } }, - async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); + await client.close(); + } + ); - const insertOneCommandToBeStarted = once(client, 'commandStarted'); - await client.db().collection('test').insertOne({ a: 1 }); - const [insertCommandStarted] = await insertOneCommandToBeStarted; + it( + 'automatically connect upon first operation (insertOne)', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); - expect(insertCommandStarted).to.have.property('commandName', 'insert'); - expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); - expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + const insertOneCommandToBeStarted = once(client, 'commandStarted'); + await client.db().collection('test').insertOne({ a: 1 }); + const [insertCommandStarted] = await insertOneCommandToBeStarted; - // Assertion is redundant but it shows that no initial ping is run - expect(insertCommandStarted.commandName).to.not.equal('ping'); + expect(insertCommandStarted).to.have.property('commandName', 'insert'); + expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); + expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect')); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); + // Assertion is redundant but it shows that no initial ping is run + expect(insertCommandStarted.commandName).to.not.equal('ping'); - await client.close(); - } - ); - - it( - 'should pass connection errors to the user through the first operation', - { requires: { auth: 'enabled' } }, - async function () { - const client = this.configuration.newClient( - 'mongodb://iLoveJavascript?serverSelectionTimeoutMS=100', - { monitorCommands: true } - ); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); - const result = await client - .db('test') - .collection('test') - .findOne() - .catch(error => error); + await client.close(); + } + ); - expect(result).to.be.instanceOf(MongoServerSelectionError); - expect(client).to.be.instanceOf(MongoClient); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); + it( + 'pass connection errors to the user through the first operation', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient( + 'mongodb://iLoveJavascript?serverSelectionTimeoutMS=100', + { monitorCommands: true } + ); + + const result = await client + .db('test') + .collection('test') + .findOne() + .catch(error => error); + + expect(result).to.be.instanceOf(MongoServerSelectionError); + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); - await client.close(); - } - ); - - it( - 'should permit client to be reconnected if closed', - { requires: { auth: 'enabled' } }, - async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); + await client.close(); + } + ); - await client.db('test').collection('test').findOne(); + it( + 'client.close will not permit operations to auto reconnect', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); - expect(client).to.be.instanceOf(MongoClient); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); + await client.db('test').collection('test').findOne(); - await client.close(); + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); - expect(client).to.have.property('topology', undefined); + await client.close(); - await client.db('test').collection('test').findOne(); + expect(client).to.have.property('topology', undefined); - expect(client).to.be.instanceOf(MongoClient); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); + const result = await client + .db('test') + .collection('test') + .findOne() + .catch(error => error); - await client.close(); - } - ); + expect(client).to.have.property('topology', undefined); + expect(result).to.be.instanceOf(MongoNotConnectedError); + + await client.close(); + } + ); + + it( + 'client.close will not permit operations to auto reconnect permanently', + { requires: { auth: 'enabled' } }, + async function () { + const client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + + expect(client.s).to.have.property('hasBeenClosed', false); + + await client.db('test').collection('test').findOne(); + + expect(client).to.be.instanceOf(MongoClient); + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + + await client.close(); + + expect(client).to.have.property('topology', undefined); + + const firstResult = await client + .db('test') + .collection('test') + .findOne() + .catch(error => error); + + expect(client).to.have.property('topology', undefined); + expect(firstResult).to.be.instanceOf(MongoNotConnectedError); + + const postCloseHasBeenClosedDescriptor = { + value: true, + enumerable: true, + configurable: false, + writable: false + }; + + expect(client.s).to.have.ownPropertyDescriptor( + 'hasBeenClosed', + postCloseHasBeenClosedDescriptor + ); + + await client.connect(); // explicitly connect again + expect(client.s).to.have.ownPropertyDescriptor( + 'hasBeenClosed', + postCloseHasBeenClosedDescriptor + ); + + await client.close(); + + const secondResult = await client + .db('test') + .collection('test') + .findOne() + .catch(error => error); + + expect(client).to.have.property('topology', undefined); + expect(secondResult).to.be.instanceOf(MongoNotConnectedError); + + expect(client.s).to.have.ownPropertyDescriptor( + 'hasBeenClosed', + postCloseHasBeenClosedDescriptor + ); + } + ); + }); }); From bb88b29dfedb31e539bb1de1c2fc0db356343f3d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 11 May 2022 14:19:25 -0400 Subject: [PATCH 3/9] test: use hooks --- .../node-specific/mongo_client.test.ts | 44 +++++-------------- 1 file changed, 12 insertions(+), 32 deletions(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index c54ec53bebb..5df58c6fc35 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -379,14 +379,22 @@ describe('class MongoClient', function () { } }); - context('#connect()', () => { + context('explict #connect()', () => { + let client: MongoClient; + beforeEach(function () { + client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + }); + + afterEach(async function () { + await client.close(); + }); + it( 'create topology and send ping when auth is enabled', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); const commandToBeStarted = once(client, 'commandStarted'); await client.connect(); const [pingOnConnect] = await commandToBeStarted; @@ -400,10 +408,6 @@ describe('class MongoClient', function () { 'permit operations to be run after connect is called', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); - const pingCommandToBeStarted = once(client, 'commandStarted'); await client.connect(); const [pingOnConnect] = await pingCommandToBeStarted; @@ -415,8 +419,6 @@ describe('class MongoClient', function () { expect(pingOnConnect).to.have.property('commandName', 'ping'); expect(findCommandStarted).to.have.property('commandName', 'find'); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); } ); @@ -424,10 +426,6 @@ describe('class MongoClient', function () { 'automatically connect upon first operation (find)', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); - const findCommandToBeStarted = once(client, 'commandStarted'); await client.db().collection('test').findOne(); const [findCommandStarted] = await findCommandToBeStarted; @@ -440,8 +438,6 @@ describe('class MongoClient', function () { expect(findCommandStarted.commandName).to.not.equal('ping'); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); } ); @@ -449,10 +445,6 @@ describe('class MongoClient', function () { 'automatically connect upon first operation (insertOne)', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); - const insertOneCommandToBeStarted = once(client, 'commandStarted'); await client.db().collection('test').insertOne({ a: 1 }); const [insertCommandStarted] = await insertOneCommandToBeStarted; @@ -465,8 +457,6 @@ describe('class MongoClient', function () { expect(insertCommandStarted.commandName).to.not.equal('ping'); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); } ); @@ -488,8 +478,6 @@ describe('class MongoClient', function () { expect(result).to.be.instanceOf(MongoServerSelectionError); expect(client).to.be.instanceOf(MongoClient); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); } ); @@ -497,10 +485,6 @@ describe('class MongoClient', function () { 'client.close will not permit operations to auto reconnect', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); - await client.db('test').collection('test').findOne(); expect(client).to.be.instanceOf(MongoClient); @@ -527,10 +511,6 @@ describe('class MongoClient', function () { 'client.close will not permit operations to auto reconnect permanently', { requires: { auth: 'enabled' } }, async function () { - const client = this.configuration.newClient(this.configuration.url(), { - monitorCommands: true - }); - expect(client.s).to.have.property('hasBeenClosed', false); await client.db('test').collection('test').findOne(); From 1a9efa85756d187f6d28a74f0ef3eb3b52e9c812 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 12 May 2022 15:28:33 -0400 Subject: [PATCH 4/9] fix: use topo --- src/cursor/abstract_cursor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts index d9069e933e7..9fca0de2c47 100644 --- a/src/cursor/abstract_cursor.ts +++ b/src/cursor/abstract_cursor.ts @@ -651,7 +651,7 @@ export abstract class AbstractCursor< return this[kInit](callback); }); } else if (this[kClient].topology?.hasSessionSupport()) { - this[kSession] = this[kClient].startSession({ owner: this, explicit: false }); + this[kSession] = this[kClient].topology?.startSession({ owner: this, explicit: false }); } } From 1e90d81b39f863e9f1dafefc29f277985c72a4c6 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 12 May 2022 15:57:26 -0400 Subject: [PATCH 5/9] fix: rm close --- test/integration/node-specific/mongo_client.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 5df58c6fc35..13e26af09f1 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -400,7 +400,6 @@ describe('class MongoClient', function () { const [pingOnConnect] = await commandToBeStarted; expect(pingOnConnect).to.have.property('commandName', 'ping'); expect(client).to.have.property('topology').that.is.instanceOf(Topology); - await client.close(); } ); From 912799bd5f61cb45fd0d2d6e13681a7c588ae14e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 16 May 2022 14:40:40 -0400 Subject: [PATCH 6/9] fix: test and error msg --- src/operations/execute_operation.ts | 4 +- .../node-specific/mongo_client.test.ts | 46 +++++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 0ef74d3643f..17674e54874 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -85,7 +85,9 @@ export function executeOperation< if (topology == null) { if (client.s.hasBeenClosed) { - return callback(new MongoNotConnectedError('client was closed')); + return callback( + new MongoNotConnectedError('Client must be connected before running operations') + ); } client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true; return client.connect(error => { diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 13e26af09f1..ebb879221b2 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -12,6 +12,7 @@ import { Connection } from '../../../src/cmap/connection'; import { Db } from '../../../src/db'; import { Topology } from '../../../src/sdam/topology'; import { getTopology, isHello } from '../../../src/utils'; +import { runLater } from '../../tools/utils'; import { setupDatabase } from '../shared'; describe('class MongoClient', function () { @@ -392,7 +393,7 @@ describe('class MongoClient', function () { }); it( - 'create topology and send ping when auth is enabled', + 'creates topology and send ping when auth is enabled', { requires: { auth: 'enabled' } }, async function () { const commandToBeStarted = once(client, 'commandStarted'); @@ -404,7 +405,24 @@ describe('class MongoClient', function () { ); it( - 'permit operations to be run after connect is called', + 'does not send ping when authentication is disabled', + { requires: { auth: 'disabled' } }, + async function () { + const commandToBeStarted = once(client, 'commandStarted'); + await client.connect(); + const delayedFind = runLater(async () => { + await client.db().collection('test').findOne(); + }, 300); + const [findOneOperation] = await commandToBeStarted; + // Proves that the first command started event that is emitted is a find and not a ping + expect(findOneOperation).to.have.property('commandName', 'find'); + await delayedFind; + expect(client).to.have.property('topology').that.is.instanceOf(Topology); + } + ); + + it( + 'permits operations to be run after connect is called', { requires: { auth: 'enabled' } }, async function () { const pingCommandToBeStarted = once(client, 'commandStarted'); @@ -420,9 +438,22 @@ describe('class MongoClient', function () { expect(client).to.have.property('topology').that.is.instanceOf(Topology); } ); + }); + + context('implicit #connect()', () => { + let client: MongoClient; + beforeEach(function () { + client = this.configuration.newClient(this.configuration.url(), { + monitorCommands: true + }); + }); + + afterEach(async function () { + await client.close(); + }); it( - 'automatically connect upon first operation (find)', + 'automatically connects upon first operation (find)', { requires: { auth: 'enabled' } }, async function () { const findCommandToBeStarted = once(client, 'commandStarted'); @@ -441,7 +472,7 @@ describe('class MongoClient', function () { ); it( - 'automatically connect upon first operation (insertOne)', + 'automatically connects upon first operation (insertOne)', { requires: { auth: 'enabled' } }, async function () { const insertOneCommandToBeStarted = once(client, 'commandStarted'); @@ -460,7 +491,7 @@ describe('class MongoClient', function () { ); it( - 'pass connection errors to the user through the first operation', + 'passes connection errors to the user through the first operation', { requires: { auth: 'enabled' } }, async function () { const client = this.configuration.newClient( @@ -542,7 +573,12 @@ describe('class MongoClient', function () { postCloseHasBeenClosedDescriptor ); + const pingCommandToBeStarted = once(client, 'commandStarted'); await client.connect(); // explicitly connect again + const [pingOnConnect] = await pingCommandToBeStarted; + + expect(pingOnConnect).to.have.property('commandName', 'ping'); + expect(client.s).to.have.ownPropertyDescriptor( 'hasBeenClosed', postCloseHasBeenClosedDescriptor From da6392e19a21b3d8783c84de0c71dadb3a48b577 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 17 May 2022 13:30:22 -0400 Subject: [PATCH 7/9] fix titles --- test/integration/node-specific/mongo_client.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index ebb879221b2..db5dd18fa8f 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -512,12 +512,11 @@ describe('class MongoClient', function () { ); it( - 'client.close will not permit operations to auto reconnect', + 'does not permit auto reconnect after client.close', { requires: { auth: 'enabled' } }, async function () { await client.db('test').collection('test').findOne(); - expect(client).to.be.instanceOf(MongoClient); expect(client).to.have.property('topology').that.is.instanceOf(Topology); await client.close(); @@ -538,7 +537,7 @@ describe('class MongoClient', function () { ); it( - 'client.close will not permit operations to auto reconnect permanently', + 'does not auto reconnect after client.close', { requires: { auth: 'enabled' } }, async function () { expect(client.s).to.have.property('hasBeenClosed', false); From 6296ade787c99090d43a24dae8eb4c13e05e2161 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 17 May 2022 15:07:26 -0400 Subject: [PATCH 8/9] test: all the things --- global.d.ts | 6 +- .../node-specific/mongo_client.test.ts | 185 ++++++++++-------- 2 files changed, 109 insertions(+), 82 deletions(-) diff --git a/global.d.ts b/global.d.ts index 8f7b44b6187..fefd1762f3a 100644 --- a/global.d.ts +++ b/global.d.ts @@ -65,7 +65,7 @@ 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!'; @@ -73,13 +73,13 @@ declare global { * * 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 to run'; diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index db5dd18fa8f..52b7f73153f 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -510,95 +510,122 @@ describe('class MongoClient', function () { expect(client).to.have.property('topology').that.is.instanceOf(Topology); } ); + }); - it( - 'does not permit auto reconnect after client.close', - { requires: { auth: 'enabled' } }, - async function () { - await client.db('test').collection('test').findOne(); - - expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); - - expect(client).to.have.property('topology', undefined); - - const result = await client - .db('test') - .collection('test') - .findOne() - .catch(error => error); - - expect(client).to.have.property('topology', undefined); - expect(result).to.be.instanceOf(MongoNotConnectedError); - - await client.close(); - } - ); - - it( - 'does not auto reconnect after client.close', - { requires: { auth: 'enabled' } }, - async function () { - expect(client.s).to.have.property('hasBeenClosed', false); - - await client.db('test').collection('test').findOne(); - - expect(client).to.be.instanceOf(MongoClient); - expect(client).to.have.property('topology').that.is.instanceOf(Topology); - - await client.close(); - - expect(client).to.have.property('topology', undefined); - - const firstResult = await client - .db('test') - .collection('test') - .findOne() - .catch(error => error); + context('#close()', () => { + let client: MongoClient; + let db: Db; + + const RD_ONLY_HAS_BEEN_CLOSED = { + value: true, + enumerable: true, + configurable: false, + writable: false + }; + + const INIT_HAS_BEEN_CLOSED = { + value: false, + enumerable: true, + configurable: true, + writable: true + }; - expect(client).to.have.property('topology', undefined); - expect(firstResult).to.be.instanceOf(MongoNotConnectedError); + beforeEach(function () { + client = this.configuration.newClient(); + db = client.db(); + }); - const postCloseHasBeenClosedDescriptor = { - value: true, - enumerable: true, - configurable: false, - writable: false - }; + afterEach(async function () { + await client.close(); + db = null; + }); - expect(client.s).to.have.ownPropertyDescriptor( - 'hasBeenClosed', - postCloseHasBeenClosedDescriptor - ); + it('prevents automatic connection on a closed non-connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const error = await db.command({ ping: 1 }).catch(error => error); + expect(error).to.be.instanceOf(MongoNotConnectedError); + }); - const pingCommandToBeStarted = once(client, 'commandStarted'); - await client.connect(); // explicitly connect again - const [pingOnConnect] = await pingCommandToBeStarted; + it('allows explicit connection on a closed non-connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + await client.connect(); + const result = await db.command({ ping: 1 }).catch(error => error); + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + }); - expect(pingOnConnect).to.have.property('commandName', 'ping'); + it('prevents automatic reconnect on a closed previously connected client', async () => { + await client.connect(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const error = await db.command({ ping: 1 }).catch(error => error); + expect(error).to.be.instanceOf(MongoNotConnectedError); + }); - expect(client.s).to.have.ownPropertyDescriptor( - 'hasBeenClosed', - postCloseHasBeenClosedDescriptor - ); + it('allows explicit reconnect on a previously closed but reconnected client', async () => { + await client.connect(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + await client.connect(); + const result = await db.command({ ping: 1 }).catch(error => error); + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + }); - await client.close(); + it('prevents auto reconnect on closed non-connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + const result = await db.command({ ping: 1 }).catch(error => error); // auto connect + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const error = await db.command({ ping: 1 }).catch(error => error); + expect(error).to.be.instanceOf(MongoNotConnectedError); + }); - const secondResult = await client - .db('test') - .collection('test') - .findOne() - .catch(error => error); + it('allows explicit reconnect on closed non-connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + const result = await db.command({ ping: 1 }).catch(error => error); // auto connect + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + await client.close(); + await client.connect(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const result2 = await db.command({ ping: 1 }).catch(error => error); + expect(result2).to.not.be.instanceOf(MongoNotConnectedError); + expect(result2).to.have.property('ok', 1); + }); - expect(client).to.have.property('topology', undefined); - expect(secondResult).to.be.instanceOf(MongoNotConnectedError); + it('prevents auto reconnect on closed explicitly connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.connect(); + const result = await db.command({ ping: 1 }).catch(error => error); + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + await client.close(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const error = await db.command({ ping: 1 }).catch(error => error); + expect(error).to.be.instanceOf(MongoNotConnectedError); + }); - expect(client.s).to.have.ownPropertyDescriptor( - 'hasBeenClosed', - postCloseHasBeenClosedDescriptor - ); - } - ); + it('allows explicit reconnect on closed previously connected client', async () => { + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); + await client.connect(); + const result = await db.command({ ping: 1 }).catch(error => error); + expect(result).to.not.be.instanceOf(MongoNotConnectedError); + expect(result).to.have.property('ok', 1); + await client.close(); + await client.connect(); + expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', RD_ONLY_HAS_BEEN_CLOSED); + const result2 = await db.command({ ping: 1 }).catch(error => error); + expect(result2).to.not.be.instanceOf(MongoNotConnectedError); + expect(result2).to.have.property('ok', 1); + }); }); }); From 758103e55f4d06701e960c3a26221322331093c4 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 17 May 2022 17:44:32 -0400 Subject: [PATCH 9/9] Fix up titles Co-authored-by: Daria Pardue --- test/integration/node-specific/mongo_client.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 52b7f73153f..4a6659cbde8 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -558,7 +558,7 @@ describe('class MongoClient', function () { expect(result).to.have.property('ok', 1); }); - it('prevents automatic reconnect on a closed previously connected client', async () => { + it('prevents automatic reconnect on a closed previously explicitly connected client', async () => { await client.connect(); expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); await client.close(); @@ -567,7 +567,7 @@ describe('class MongoClient', function () { expect(error).to.be.instanceOf(MongoNotConnectedError); }); - it('allows explicit reconnect on a previously closed but reconnected client', async () => { + it('allows explicit reconnect on a closed previously explicitly connected client', async () => { await client.connect(); expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); await client.close(); @@ -578,7 +578,7 @@ describe('class MongoClient', function () { expect(result).to.have.property('ok', 1); }); - it('prevents auto reconnect on closed non-connected client', async () => { + it('prevents auto reconnect on closed previously implicitly connected client', async () => { expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); const result = await db.command({ ping: 1 }).catch(error => error); // auto connect expect(result).to.not.be.instanceOf(MongoNotConnectedError); @@ -589,7 +589,7 @@ describe('class MongoClient', function () { expect(error).to.be.instanceOf(MongoNotConnectedError); }); - it('allows explicit reconnect on closed non-connected client', async () => { + it('allows explicit reconnect on closed previously implicitly connected client', async () => { expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); const result = await db.command({ ping: 1 }).catch(error => error); // auto connect expect(result).to.not.be.instanceOf(MongoNotConnectedError); @@ -602,7 +602,7 @@ describe('class MongoClient', function () { expect(result2).to.have.property('ok', 1); }); - it('prevents auto reconnect on closed explicitly connected client', async () => { + it('prevents auto reconnect on closed explicitly connected client after an operation', async () => { expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); await client.connect(); const result = await db.command({ ping: 1 }).catch(error => error); @@ -614,7 +614,7 @@ describe('class MongoClient', function () { expect(error).to.be.instanceOf(MongoNotConnectedError); }); - it('allows explicit reconnect on closed previously connected client', async () => { + it('allows explicit reconnect on closed explicitly connected client after an operation', async () => { expect(client.s).to.have.ownPropertyDescriptor('hasBeenClosed', INIT_HAS_BEEN_CLOSED); await client.connect(); const result = await db.command({ ping: 1 }).catch(error => error);