Skip to content
New issue

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

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

fix(NODE-4108): improve return type for withTransaction() #3236

Merged
merged 1 commit into from
May 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ declare global {
}

namespace Mocha {
interface SuiteFunction {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a request for changes just wanting to understand the rationale for including these 2 definitions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our metadata UI also allows specifying filters at the describe level, only in the 3 argument form of title, metadata, function So this just prevents type issues when using a metadata object in describe or context. I just don't think we've used metadata at the suite level much before or we haven't needed it in TS test yet. But the transactions tests will always need to filter up to 4.2 maybe 4.4 so it makes sense to specify it higher up.

(title: string, metadata: MongoDBMetadataUI, fn: (this: Suite) => void): Mocha.Suite;
}

interface PendingSuiteFunction {
(title: string, metadata: MongoDBMetadataUI, fn: (this: Suite) => void): Mocha.Suite;
}

interface TestFunction {
(title: string, metadata: MongoDBMetadataUI, fn: Mocha.Func): Mocha.Test;
(title: string, metadata: MongoDBMetadataUI, fn: Mocha.AsyncFunc): Mocha.Test;
Expand Down
22 changes: 16 additions & 6 deletions src/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,20 +457,30 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
}

/**
* Runs a provided lambda within a transaction, retrying either the commit operation
* Runs a provided callback within a transaction, retrying either the commitTransaction operation
* or entire transaction as needed (and when the error permits) to better ensure that
* the transaction can complete successfully.
*
* IMPORTANT: This method requires the user to return a Promise, all lambdas that do not
* return a Promise will result in undefined behavior.
* **IMPORTANT:** This method requires the user to return a Promise, and `await` all operations.
* Any callbacks that do not return a Promise will result in undefined behavior.
*
* @param fn - A lambda to run within a transaction
* @param options - Optional settings for the transaction
* @remarks
* This function:
* - Will return the command response from the final commitTransaction if every operation is successful (can be used as a truthy object)
* - Will return `undefined` if the transaction is explicitly aborted with `await session.abortTransaction()`
* - Will throw if one of the operations throws or `throw` statement is used inside the `withTransaction` callback
*
* Checkout a descriptive example here:
* @see https://www.mongodb.com/developer/quickstart/node-transactions/
*
* @param fn - callback to run within a transaction
* @param options - optional settings for the transaction
* @returns A raw command response or undefined
*/
withTransaction<T = void>(
fn: WithTransactionCallback<T>,
options?: TransactionOptions
): ReturnType<typeof fn> {
): Promise<Document | undefined> {
const startTime = now();
return attemptTransaction(this, startTime, fn, options);
}
Expand Down
2 changes: 1 addition & 1 deletion src/transactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export interface TransactionOptions extends CommandOperationOptions {
writeConcern?: WriteConcern;
/** A default read preference for commands in this transaction */
readPreference?: ReadPreference;

/** Specifies the maximum amount of time to allow a commit action on a transaction to run in milliseconds */
maxCommitTimeMS?: number;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,25 @@
'use strict';
import { expect } from 'chai';

const { expect } = require('chai');
const { Topology } = require('../../../src/sdam/topology');
const { ClientSession } = require('../../../src/sessions');
const { MongoNetworkError } = require('../../../src/error');
import { Collection, MongoClient, ServerSessionPool } from '../../../src';
import { MongoNetworkError } from '../../../src/error';
import { ClientSession } from '../../../src/sessions';

describe('Transactions', function () {
describe('withTransaction', function () {
let session, sessionPool;
beforeEach(() => {
const topology = new Topology('localhost:27017');
let session: ClientSession;
let sessionPool: ServerSessionPool;
let client: MongoClient;

beforeEach(async function () {
client = this.configuration.newClient();
const topology = (await client.connect()).topology;
sessionPool = topology.s.sessionPool;
session = new ClientSession(topology, sessionPool);
session = new ClientSession(topology, sessionPool, {});
});

afterEach(() => {
afterEach(async () => {
sessionPool.endAllPooledSessions();
await client.close();
});

it('should provide a useful error if a Promise is not returned', {
Expand All @@ -27,6 +31,7 @@ describe('Transactions', function () {
return false;
}

// @ts-expect-error: Testing that a non promise returning function is handled correctly
expect(() => session.withTransaction(fnThatDoesntReturnPromise)).to.throw(
/must return a Promise/
);
Expand All @@ -37,8 +42,7 @@ describe('Transactions', function () {

it('should return readable error if promise rejected with no reason', {
metadata: {
requires: { topology: ['replicaset', 'sharded'], mongodb: '>=4.0.2' },
serverless: 'forbid'
requires: { topology: ['replicaset', 'sharded'], mongodb: '>=4.2.0', serverless: 'forbid' }
},
test: function (done) {
function fnThatReturnsBadPromise() {
Expand All @@ -54,6 +58,73 @@ describe('Transactions', function () {
});
}
});

describe(
'return value semantics',
{ requires: { mongodb: '>=4.2.0', topology: '!single' } },
() => {
let client: MongoClient;
let collection: Collection<{ a: number }>;

beforeEach(async function () {
client = this.configuration.newClient();
await client.connect();
collection = await client
.db('withTransactionReturnType')
.createCollection('withTransactionReturnType');
});

afterEach(async function () {
await collection.drop();
await client.close();
});

it('should return undefined when transaction is aborted explicitly', async () => {
const session = client.startSession();

const withTransactionResult = await session
.withTransaction(async session => {
await collection.insertOne({ a: 1 }, { session });
await collection.findOne({ a: 1 }, { session });
await session.abortTransaction();
})
.finally(async () => await session.endSession());

expect(withTransactionResult).to.be.undefined;
});

it('should return raw command when transaction is successfully committed', async () => {
const session = client.startSession();

const withTransactionResult = await session
.withTransaction(async session => {
await collection.insertOne({ a: 1 }, { session });
await collection.findOne({ a: 1 }, { session });
})
.finally(async () => await session.endSession());

expect(withTransactionResult).to.exist;
expect(withTransactionResult).to.be.an('object');
expect(withTransactionResult).to.have.property('ok', 1);
});

it('should throw when transaction is aborted due to an error', async () => {
const session = client.startSession();

const withTransactionResult = await session
.withTransaction(async session => {
await collection.insertOne({ a: 1 }, { session });
await collection.findOne({ a: 1 }, { session });
throw new Error("I don't wanna transact anymore!");
})
.catch(error => error)
.finally(async () => await session.endSession());

expect(withTransactionResult).to.be.instanceOf(Error);
expect(withTransactionResult.message).to.equal("I don't wanna transact anymore!");
});
}
);
});

describe('startTransaction', function () {
Expand Down Expand Up @@ -137,7 +208,9 @@ describe('Transactions', function () {

coll.insertOne({ b: 2 }, { session }, err => {
expect(err).to.exist.and.to.be.an.instanceof(MongoNetworkError);
expect(err.hasErrorLabel('TransientTransactionError')).to.be.true;
if (err instanceof MongoNetworkError) {
expect(err.hasErrorLabel('TransientTransactionError')).to.be.true;
}

session.abortTransaction(() => session.endSession(() => client.close(done)));
});
Expand Down