diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 9cd81370cf6..21e19f15d12 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -22,7 +22,6 @@ import { executeOperation } from '../operations/execute_operation'; import { InsertOperation } from '../operations/insert'; import { AbstractOperation, Hint } from '../operations/operation'; import { makeUpdateStatement, UpdateOperation, UpdateStatement } from '../operations/update'; -import { PromiseProvider } from '../promise_provider'; import type { Server } from '../sdam/server'; import type { Topology } from '../sdam/topology'; import type { ClientSession } from '../sessions'; @@ -31,6 +30,7 @@ import { Callback, getTopology, hasAtomicOperators, + maybeCallback, MongoDBNamespace, resolveOptions } from '../utils'; @@ -1270,11 +1270,19 @@ export abstract class BulkOperationBase { options?: BulkWriteOptions | Callback, callback?: Callback ): Promise | void { - if (typeof options === 'function') (callback = options), (options = {}); - options = options ?? {}; + callback = + typeof callback === 'function' + ? callback + : typeof options === 'function' + ? options + : undefined; + options = options != null && typeof options !== 'function' ? options : {}; if (this.s.executed) { - return handleEarlyError(new MongoBatchReExecutionError(), callback); + // eslint-disable-next-line @typescript-eslint/require-await + return maybeCallback(async () => { + throw new MongoBatchReExecutionError(); + }, callback); } const writeConcern = WriteConcern.fromOptions(options); @@ -1292,10 +1300,10 @@ export abstract class BulkOperationBase { } // If we have no operations in the bulk raise an error if (this.s.batches.length === 0) { - const emptyBatchError = new MongoInvalidArgumentError( - 'Invalid BulkOperation, Batch cannot be empty' - ); - return handleEarlyError(emptyBatchError, callback); + // eslint-disable-next-line @typescript-eslint/require-await + return maybeCallback(async () => { + throw new MongoInvalidArgumentError('Invalid BulkOperation, Batch cannot be empty'); + }, callback); } this.s.executed = true; @@ -1351,20 +1359,6 @@ Object.defineProperty(BulkOperationBase.prototype, 'length', { } }); -/** helper function to assist with promiseOrCallback behavior */ -function handleEarlyError( - err?: AnyError, - callback?: Callback -): Promise | void { - if (typeof callback === 'function') { - callback(err); - return; - } - - const PromiseConstructor = PromiseProvider.get() ?? Promise; - return PromiseConstructor.reject(err); -} - function shouldForceServerObjectId(bulkOperation: BulkOperationBase): boolean { if (typeof bulkOperation.s.options.forceServerObjectId === 'boolean') { return bulkOperation.s.options.forceServerObjectId; diff --git a/src/change_stream.ts b/src/change_stream.ts index e51f9adeb80..5887e3f338f 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -20,7 +20,7 @@ import type { AggregateOptions } from './operations/aggregate'; import type { CollationOptions, OperationParent } from './operations/command'; import type { ReadPreference } from './read_preference'; import type { ServerSessionId } from './sessions'; -import { Callback, filterOptions, getTopology, maybePromise, MongoDBNamespace } from './utils'; +import { Callback, filterOptions, getTopology, maybeCallback, MongoDBNamespace } from './utils'; /** @internal */ const kCursorStream = Symbol('cursorStream'); @@ -649,29 +649,25 @@ export class ChangeStream< hasNext(callback: Callback): void; hasNext(callback?: Callback): Promise | void { this._setIsIterator(); - // TOOD(NODE-4319): Add eslint rule preventing accidental variable shadowing - // Shadowing is intentional here. We want to override the `callback` variable - // from the outer scope so that the inner scope doesn't accidentally call the wrong callback. - return maybePromise(callback, callback => { - (async () => { + return maybeCallback(async () => { + try { + const hasNext = await this.cursor.hasNext(); + return hasNext; + } catch (error) { try { + await this._processErrorIteratorMode(error); const hasNext = await this.cursor.hasNext(); return hasNext; } catch (error) { try { - await this._processErrorIteratorMode(error); - const hasNext = await this.cursor.hasNext(); - return hasNext; - } catch (error) { - await this.close().catch(err => err); - throw error; + await this.close(); + } catch { + // We are not concerned with errors from close() } + throw error; } - })().then( - hasNext => callback(undefined, hasNext), - error => callback(error) - ); - }); + } + }, callback); } /** Get the next available document from the Change Stream. */ @@ -680,31 +676,27 @@ export class ChangeStream< next(callback: Callback): void; next(callback?: Callback): Promise | void { this._setIsIterator(); - // TOOD(NODE-4319): Add eslint rule preventing accidental variable shadowing - // Shadowing is intentional here. We want to override the `callback` variable - // from the outer scope so that the inner scope doesn't accidentally call the wrong callback. - return maybePromise(callback, callback => { - (async () => { + return maybeCallback(async () => { + try { + const change = await this.cursor.next(); + const processedChange = this._processChange(change ?? null); + return processedChange; + } catch (error) { try { + await this._processErrorIteratorMode(error); const change = await this.cursor.next(); const processedChange = this._processChange(change ?? null); return processedChange; } catch (error) { try { - await this._processErrorIteratorMode(error); - const change = await this.cursor.next(); - const processedChange = this._processChange(change ?? null); - return processedChange; - } catch (error) { - await this.close().catch(err => err); - throw error; + await this.close(); + } catch { + // We are not concerned with errors from close() } + throw error; } - })().then( - change => callback(undefined, change), - error => callback(error) - ); - }); + } + }, callback); } /** @@ -715,29 +707,25 @@ export class ChangeStream< tryNext(callback: Callback): void; tryNext(callback?: Callback): Promise | void { this._setIsIterator(); - // TOOD(NODE-4319): Add eslint rule preventing accidental variable shadowing - // Shadowing is intentional here. We want to override the `callback` variable - // from the outer scope so that the inner scope doesn't accidentally call the wrong callback. - return maybePromise(callback, callback => { - (async () => { + return maybeCallback(async () => { + try { + const change = await this.cursor.tryNext(); + return change ?? null; + } catch (error) { try { + await this._processErrorIteratorMode(error); const change = await this.cursor.tryNext(); return change ?? null; } catch (error) { try { - await this._processErrorIteratorMode(error); - const change = await this.cursor.tryNext(); - return change ?? null; - } catch (error) { - await this.close().catch(err => err); - throw error; + await this.close(); + } catch { + // We are not concerned with errors from close() } + throw error; } - })().then( - change => callback(undefined, change), - error => callback(error) - ); - }); + } + }, callback); } /** Is the cursor closed */ @@ -752,13 +740,14 @@ export class ChangeStream< close(callback?: Callback): Promise | void { this[kClosed] = true; - return maybePromise(callback, cb => { + return maybeCallback(async () => { const cursor = this.cursor; - return cursor.close(err => { + try { + await cursor.close(); + } finally { this._endStream(); - return cb(err); - }); - }); + } + }, callback); } /** diff --git a/src/gridfs/index.ts b/src/gridfs/index.ts index eb96cebca47..874762b0132 100644 --- a/src/gridfs/index.ts +++ b/src/gridfs/index.ts @@ -7,7 +7,7 @@ import type { Logger } from '../logger'; import { Filter, TypedEventEmitter } from '../mongo_types'; import type { ReadPreference } from '../read_preference'; import type { Sort } from '../sort'; -import { Callback, maybePromise } from '../utils'; +import { Callback, maybeCallback } from '../utils'; import { WriteConcern, WriteConcernOptions } from '../write_concern'; import type { FindOptions } from './../operations/find'; import { @@ -144,28 +144,18 @@ export class GridFSBucket extends TypedEventEmitter { /** @deprecated Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance */ delete(id: ObjectId, callback: Callback): void; delete(id: ObjectId, callback?: Callback): Promise | void { - return maybePromise(callback, callback => { - return this.s._filesCollection.deleteOne({ _id: id }, (error, res) => { - if (error) { - return callback(error); - } + return maybeCallback(async () => { + const { deletedCount } = await this.s._filesCollection.deleteOne({ _id: id }); - return this.s._chunksCollection.deleteMany({ files_id: id }, error => { - if (error) { - return callback(error); - } + // Delete orphaned chunks before returning FileNotFound + await this.s._chunksCollection.deleteMany({ files_id: id }); - // Delete orphaned chunks before returning FileNotFound - if (!res?.deletedCount) { - // TODO(NODE-3483): Replace with more appropriate error - // Consider creating new error MongoGridFSFileNotFoundError - return callback(new MongoRuntimeError(`File not found for id ${id}`)); - } - - return callback(); - }); - }); - }); + if (deletedCount === 0) { + // TODO(NODE-3483): Replace with more appropriate error + // Consider creating new error MongoGridFSFileNotFoundError + throw new MongoRuntimeError(`File not found for id ${id}`); + } + }, callback); } /** Convenience wrapper around find on the files collection */ @@ -215,21 +205,14 @@ export class GridFSBucket extends TypedEventEmitter { /** @deprecated Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance */ rename(id: ObjectId, filename: string, callback: Callback): void; rename(id: ObjectId, filename: string, callback?: Callback): Promise | void { - return maybePromise(callback, callback => { + return maybeCallback(async () => { const filter = { _id: id }; const update = { $set: { filename } }; - return this.s._filesCollection.updateOne(filter, update, (error?, res?) => { - if (error) { - return callback(error); - } - - if (!res?.matchedCount) { - return callback(new MongoRuntimeError(`File with id ${id} not found`)); - } - - return callback(); - }); - }); + const { matchedCount } = await this.s._filesCollection.updateOne(filter, update); + if (matchedCount === 0) { + throw new MongoRuntimeError(`File with id ${id} not found`); + } + }, callback); } /** Removes this bucket's files collection, followed by its chunks collection. */ @@ -237,20 +220,10 @@ export class GridFSBucket extends TypedEventEmitter { /** @deprecated Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance */ drop(callback: Callback): void; drop(callback?: Callback): Promise | void { - return maybePromise(callback, callback => { - return this.s._filesCollection.drop(error => { - if (error) { - return callback(error); - } - return this.s._chunksCollection.drop(error => { - if (error) { - return callback(error); - } - - return callback(); - }); - }); - }); + return maybeCallback(async () => { + await this.s._filesCollection.drop(); + await this.s._chunksCollection.drop(); + }, callback); } /** Get the Db scoped logger. */ diff --git a/src/gridfs/upload.ts b/src/gridfs/upload.ts index 68d4ad224a9..7ccc3a17be6 100644 --- a/src/gridfs/upload.ts +++ b/src/gridfs/upload.ts @@ -4,7 +4,7 @@ import type { Document } from '../bson'; import { ObjectId } from '../bson'; import type { Collection } from '../collection'; import { AnyError, MongoAPIError, MONGODB_ERROR_CODES, MongoError } from '../error'; -import { Callback, maybePromise } from '../utils'; +import { Callback, maybeCallback } from '../utils'; import type { WriteConcernOptions } from '../write_concern'; import { WriteConcern } from './../write_concern'; import type { GridFSFile } from './download'; @@ -149,20 +149,20 @@ export class GridFSBucketWriteStream extends Writable implements NodeJS.Writable /** @deprecated Callbacks are deprecated and will be removed in the next major version. See [mongodb-legacy](https://github.com/mongodb-js/nodejs-mongodb-legacy) for migration assistance */ abort(callback: Callback): void; abort(callback?: Callback): Promise | void { - return maybePromise(callback, callback => { + return maybeCallback(async () => { if (this.state.streamEnd) { // TODO(NODE-3485): Replace with MongoGridFSStreamClosed - return callback(new MongoAPIError('Cannot abort a stream that has already completed')); + throw new MongoAPIError('Cannot abort a stream that has already completed'); } if (this.state.aborted) { // TODO(NODE-3485): Replace with MongoGridFSStreamClosed - return callback(new MongoAPIError('Cannot call abort() on a stream twice')); + throw new MongoAPIError('Cannot call abort() on a stream twice'); } this.state.aborted = true; - this.chunks.deleteMany({ files_id: this.id }, error => callback(error)); - }); + await this.chunks.deleteMany({ files_id: this.id }); + }, callback); } /** diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 321b8a63fb5..979f2a21253 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -17,7 +17,6 @@ import { MongoInvalidArgumentError } from './error'; import type { Logger, LoggerLevel } from './logger'; import { TypedEventEmitter } from './mongo_types'; import { connect } from './operations/connect'; -import { PromiseProvider } from './promise_provider'; import type { ReadConcern, ReadConcernLevel, ReadConcernLike } from './read_concern'; import { ReadPreference, ReadPreferenceMode } from './read_preference'; import type { TagSet } from './sdam/server_description'; @@ -29,6 +28,7 @@ import { Callback, ClientMetadata, HostAddress, + maybeCallback, maybePromise, MongoDBNamespace, ns, @@ -580,26 +580,18 @@ export class MongoClient extends TypedEventEmitter { options?: MongoClientOptions | Callback, callback?: Callback ): Promise | void { - if (typeof options === 'function') (callback = options), (options = {}); - options = options ?? {}; - - try { - // Create client - const mongoClient = new MongoClient(url, options); - // Execute the connect method - if (callback) { - return mongoClient.connect(callback); - } else { - return mongoClient.connect(); - } - } catch (error) { - if (callback) { - return callback(error); - } else { - const PromiseConstructor = PromiseProvider.get() ?? Promise; - return PromiseConstructor.reject(error); - } - } + callback = + typeof callback === 'function' + ? callback + : typeof options === 'function' + ? options + : undefined; + + return maybeCallback(async () => { + options = typeof options !== 'function' ? options : undefined; + const client = new this(url, options); + return await client.connect(); + }, callback); } /** Starts a new session on the server */ @@ -649,16 +641,18 @@ export class MongoClient extends TypedEventEmitter { } const session = this.startSession(options); - const PromiseConstructor = PromiseProvider.get() ?? Promise; - - return PromiseConstructor.resolve() - .then(() => withSessionCallback(session)) - .then(() => { - // Do not return the result of callback - }) - .finally(() => { - session.endSession().catch(() => null); - }); + + return maybeCallback(async () => { + try { + await withSessionCallback(session); + } finally { + try { + await session.endSession(); + } catch { + // We are not concerned with errors from endSession() + } + } + }, null); } /** diff --git a/src/utils.ts b/src/utils.ts index f871df54a7c..b2321d3a8c3 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -435,6 +435,38 @@ export function* makeCounter(seed = 0): Generator { } } +/** + * Helper for handling legacy callback support. + */ +export function maybeCallback(promiseFn: () => Promise, callback: null): Promise; +export function maybeCallback( + promiseFn: () => Promise, + callback?: Callback +): Promise | void; +export function maybeCallback( + promiseFn: () => Promise, + callback?: Callback | null +): Promise | void { + const PromiseConstructor = PromiseProvider.get(); + + const promise = promiseFn(); + if (callback == null) { + if (PromiseConstructor == null) { + return promise; + } else { + return new PromiseConstructor((resolve, reject) => { + promise.then(resolve, reject); + }); + } + } + + promise.then( + result => callback(undefined, result), + error => callback(error) + ); + return; +} + /** * Helper function for either accepting a callback, or returning a promise * @internal diff --git a/test/integration/sessions/sessions.spec.prose.test.ts b/test/integration/sessions/sessions.spec.prose.test.ts index 870c017c2e6..d55b0dea2b8 100644 --- a/test/integration/sessions/sessions.spec.prose.test.ts +++ b/test/integration/sessions/sessions.spec.prose.test.ts @@ -46,6 +46,6 @@ describe('ServerSession', () => { expect(events).to.have.lengthOf(operations.length); // This is a guarantee in node, unless you are performing a transaction (which is not being done in this test) - expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); + expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex')))).to.have.lengthOf(1); }); }); diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index db4e589bc62..2dd0a687309 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -1,12 +1,15 @@ +import { Promise as BluebirdPromise } from 'bluebird'; import { expect } from 'chai'; import { LEGACY_HELLO_COMMAND } from '../../src/constants'; import { MongoRuntimeError } from '../../src/error'; +import { Promise as PromiseProvider } from '../../src/index'; import { BufferPool, eachAsync, HostAddress, isHello, + maybeCallback, MongoDBNamespace, shuffle } from '../../src/utils'; @@ -350,4 +353,132 @@ describe('driver utils', function () { }); }); }); + + describe('maybeCallback()', () => { + it('should accept two arguments', () => { + expect(maybeCallback).to.have.lengthOf(2); + }); + + describe('when handling an error case', () => { + it('should pass the error to the callback provided', done => { + const superPromiseRejection = Promise.reject(new Error('fail')); + const result = maybeCallback( + () => superPromiseRejection, + (error, result) => { + try { + expect(result).to.not.exist; + expect(error).to.be.instanceOf(Error); + return done(); + } catch (assertionError) { + return done(assertionError); + } + } + ); + expect(result).to.be.undefined; + }); + + it('should return the rejected promise to the caller when no callback is provided', async () => { + const superPromiseRejection = Promise.reject(new Error('fail')); + const returnedPromise = maybeCallback(() => superPromiseRejection, undefined); + expect(returnedPromise).to.equal(superPromiseRejection); + // @ts-expect-error: There is no overload to change the return type not be nullish, + // and we do not want to add one in fear of making it too easy to neglect adding the callback argument + const thrownError = await returnedPromise.catch(error => error); + expect(thrownError).to.be.instanceOf(Error); + }); + + it('should not modify a rejection error promise', async () => { + class MyError extends Error {} + const driverError = Object.freeze(new MyError()); + const rejection = Promise.reject(driverError); + // @ts-expect-error: There is no overload to change the return type not be nullish, + // and we do not want to add one in fear of making it too easy to neglect adding the callback argument + const thrownError = await maybeCallback(() => rejection, undefined).catch(error => error); + expect(thrownError).to.be.equal(driverError); + }); + + it('should not modify a rejection error when passed to callback', done => { + class MyError extends Error {} + const driverError = Object.freeze(new MyError()); + const rejection = Promise.reject(driverError); + maybeCallback( + () => rejection, + error => { + try { + expect(error).to.exist; + expect(error).to.equal(driverError); + done(); + } catch (assertionError) { + done(assertionError); + } + } + ); + }); + }); + + describe('when handling a success case', () => { + it('should pass the result and undefined error to the callback provided', done => { + const superPromiseSuccess = Promise.resolve(2); + + const result = maybeCallback( + () => superPromiseSuccess, + (error, result) => { + try { + expect(error).to.be.undefined; + expect(result).to.equal(2); + done(); + } catch (assertionError) { + done(assertionError); + } + } + ); + expect(result).to.be.undefined; + }); + + it('should return the resolved promise to the caller when no callback is provided', async () => { + const superPromiseSuccess = Promise.resolve(2); + const result = maybeCallback(() => superPromiseSuccess); + expect(result).to.equal(superPromiseSuccess); + expect(await result).to.equal(2); + }); + }); + + describe('when a custom promise constructor is set', () => { + beforeEach(() => { + PromiseProvider.set(BluebirdPromise); + }); + + afterEach(() => { + PromiseProvider.set(null); + }); + + it('should return the custom promise if no callback is provided', async () => { + const superPromiseSuccess = Promise.resolve(2); + const result = maybeCallback(() => superPromiseSuccess); + expect(result).to.not.equal(superPromiseSuccess); + expect(result).to.be.instanceOf(BluebirdPromise); + }); + + it('should return a rejected custom promise instance if promiseFn rejects', async () => { + const superPromiseFailure = Promise.reject(new Error('ah!')); + const result = maybeCallback(() => superPromiseFailure); + expect(result).to.not.equal(superPromiseFailure); + expect(result).to.be.instanceOf(BluebirdPromise); + // @ts-expect-error: There is no overload to change the return type not be nullish, + // and we do not want to add one in fear of making it too easy to neglect adding the callback argument + expect(await result.catch(e => e)).to.have.property('message', 'ah!'); + }); + + it('should return void even if a custom promise is set and a callback is provided', async () => { + const superPromiseSuccess = Promise.resolve(2); + const result = maybeCallback( + () => superPromiseSuccess, + () => { + // ignore + } + ); + expect(result).to.be.undefined; + }); + }); + }); });