Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a7bc23
refactor(NODE-5809): remove executeOperation callback overload and ma…
nbbeeken Jan 3, 2024
5a620d6
docs: improve internal docs on execOp
nbbeeken Jan 8, 2024
45a6aa4
docs: clean up connecting bullet
nbbeeken Jan 8, 2024
8b85d6e
Merge branch 'main' into NODE-5809-rm-callbacks-final-v2-really-final
W-A-James Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions src/bulk/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
41 changes: 14 additions & 27 deletions src/operations/execute_operation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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> {
Expand Down
65 changes: 33 additions & 32 deletions src/sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
24 changes: 0 additions & 24 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
91 changes: 0 additions & 91 deletions test/unit/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
LEGACY_HELLO_COMMAND,
List,
matchesParentDomain,
maybeCallback,
MongoDBCollectionNamespace,
MongoDBNamespace,
MongoRuntimeError,
Expand Down Expand Up @@ -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 },
Expand Down