refactor(NODE-5809): remove executeOperation callback overload and maybeCallback (#3959)

Author: nbbeeken

src/bulk/common.ts

@@ -579,23 +579,18 @@ function executeCommands(
   }
 
   try {
-    if (isInsertBatch(batch)) {
-      executeOperation(
-        bulkOperation.s.collection.client,
-        new InsertOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
-        resultHandler
-      );
-    } else if (isUpdateBatch(batch)) {
-      executeOperation(
-        bulkOperation.s.collection.client,
-        new UpdateOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
-        resultHandler
-      );
-    } else if (isDeleteBatch(batch)) {
-      executeOperation(
-        bulkOperation.s.collection.client,
-        new DeleteOperation(bulkOperation.s.namespace, batch.operations, finalOptions),
-        resultHandler
+    const operation = isInsertBatch(batch)
+      ? new InsertOperation(bulkOperation.s.namespace, batch.operations, finalOptions)
+      : isUpdateBatch(batch)
+      ? new UpdateOperation(bulkOperation.s.namespace, batch.operations, finalOptions)
+      : isDeleteBatch(batch)
+      ? new DeleteOperation(bulkOperation.s.namespace, batch.operations, finalOptions)
+      : null;
+
+    if (operation != null) {
+      executeOperation(bulkOperation.s.collection.client, operation).then(
+        result => resultHandler(undefined, result),
+        error => resultHandler(error)
       );
     }
   } catch (err) {

src/operations/execute_operation.ts

@@ -25,7 +25,7 @@ import {
 } from '../sdam/server_selection';
 import type { Topology } from '../sdam/topology';
 import type { ClientSession } from '../sessions';
-import { type Callback, maybeCallback, supportsRetryableWrites } from '../utils';
+import { supportsRetryableWrites } from '../utils';
 import { AbstractOperation, Aspect } from './operation';
 
 const MMAPv1_RETRY_WRITES_ERROR_CODE = MONGODB_ERROR_CODES.IllegalOperation;
@@ -51,36 +51,23 @@ export interface ExecutionResult {
  * @internal
  *
  * @remarks
- * This method reduces large amounts of duplication in the entire codebase by providing
- * a single point for determining whether callbacks or promises should be used. Additionally
- * it allows for a single point of entry to provide features such as implicit sessions, which
+ * Allows for a single point of entry to provide features such as implicit sessions, which
  * are required by the Driver Sessions specification in the event that a ClientSession is
- * not provided
+ * not provided.
  *
- * @param topology - The topology to execute this operation on
+ * The expectation is that this function:
+ * - Connects the MongoClient if it has not already been connected
+ * - Creates a session if none is provided and cleans up the session it creates
+ * - Selects a server based on readPreference or various factors
+ * - Retries an operation if it fails for certain errors, see {@link retryOperation}
+ *
+ * @typeParam T - The operation's type
+ * @typeParam TResult - The type of the operation's result, calculated from T
+ *
+ * @param client - The MongoClient to execute this operation with
  * @param operation - The operation to execute
- * @param callback - The command result callback
  */
-export function executeOperation<
-  T extends AbstractOperation<TResult>,
-  TResult = ResultTypeFromOperation<T>
->(client: MongoClient, operation: T): Promise<TResult>;
-export function executeOperation<
-  T extends AbstractOperation<TResult>,
-  TResult = ResultTypeFromOperation<T>
->(client: MongoClient, operation: T, callback: Callback<TResult>): void;
-export function executeOperation<
-  T extends AbstractOperation<TResult>,
-  TResult = ResultTypeFromOperation<T>
->(client: MongoClient, operation: T, callback?: Callback<TResult>): Promise<TResult> | void;
-export function executeOperation<
-  T extends AbstractOperation<TResult>,
-  TResult = ResultTypeFromOperation<T>
->(client: MongoClient, operation: T, callback?: Callback<TResult>): Promise<TResult> | void {
-  return maybeCallback(() => executeOperationAsync(client, operation), callback);
-}
-
-async function executeOperationAsync<
+export async function executeOperation<
   T extends AbstractOperation<TResult>,
   TResult = ResultTypeFromOperation<T>
 >(client: MongoClient, operation: T): Promise<TResult> {

src/sessions.ts

@@ -754,45 +754,46 @@ function endTransaction(
     command.recoveryToken = session.transaction.recoveryToken;
   }
 
+  const handleFirstCommandAttempt = (error?: Error) => {
+    if (command.abortTransaction) {
+      // always unpin on abort regardless of command outcome
+      session.unpin();
+    }
+
+    if (error instanceof MongoError && isRetryableWriteError(error)) {
+      // SPEC-1185: apply majority write concern when retrying commitTransaction
+      if (command.commitTransaction) {
+        // per txns spec, must unpin session in this case
+        session.unpin({ force: true });
+
+        command.writeConcern = Object.assign({ wtimeout: 10000 }, command.writeConcern, {
+          w: 'majority'
+        });
+      }
+
+      executeOperation(
+        session.client,
+        new RunAdminCommandOperation(command, {
+          session,
+          readPreference: ReadPreference.primary,
+          bypassPinningCheck: true
+        })
+      ).then(() => commandHandler(), commandHandler);
+      return;
+    }
+
+    commandHandler(error);
+  };
+
   // send the command
   executeOperation(
     session.client,
     new RunAdminCommandOperation(command, {
       session,
       readPreference: ReadPreference.primary,
       bypassPinningCheck: true
-    }),
-    error => {
-      if (command.abortTransaction) {
-        // always unpin on abort regardless of command outcome
-        session.unpin();
-      }
-
-      if (error instanceof MongoError && isRetryableWriteError(error)) {
-        // SPEC-1185: apply majority write concern when retrying commitTransaction
-        if (command.commitTransaction) {
-          // per txns spec, must unpin session in this case
-          session.unpin({ force: true });
-
-          command.writeConcern = Object.assign({ wtimeout: 10000 }, command.writeConcern, {
-            w: 'majority'
-          });
-        }
-
-        return executeOperation(
-          session.client,
-          new RunAdminCommandOperation(command, {
-            session,
-            readPreference: ReadPreference.primary,
-            bypassPinningCheck: true
-          }),
-          commandHandler
-        );
-      }
-
-      commandHandler(error);
-    }
-  );
+    })
+  ).then(() => handleFirstCommandAttempt(), handleFirstCommandAttempt);
 }
 
 /** @public */

src/utils.ts

@@ -330,30 +330,6 @@ export function* makeCounter(seed = 0): Generator<number> {
   }
 }
 
-/**
- * Helper for handling legacy callback support.
- */
-export function maybeCallback<T>(promiseFn: () => Promise<T>, callback: null): Promise<T>;
-export function maybeCallback<T>(
-  promiseFn: () => Promise<T>,
-  callback?: Callback<T>
-): Promise<T> | void;
-export function maybeCallback<T>(
-  promiseFn: () => Promise<T>,
-  callback?: Callback<T> | null
-): Promise<T> | void {
-  const promise = promiseFn();
-  if (callback == null) {
-    return promise;
-  }
-
-  promise.then(
-    result => callback(undefined, result),
-    error => callback(error)
-  );
-  return;
-}
-
 /**
  * Synchronously Generate a UUIDv4
  * @internal

test/unit/utils.test.ts

@@ -14,7 +14,6 @@ import {
   LEGACY_HELLO_COMMAND,
   List,
   matchesParentDomain,
-  maybeCallback,
   MongoDBCollectionNamespace,
   MongoDBNamespace,
   MongoRuntimeError,
@@ -853,96 +852,6 @@ 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('compareObjectId()', () => {
     const table = [
       { oid1: null, oid2: null, result: 0 },