From 20321af9f4c24d55921a646cf4a5d8b6d3066e99 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 25 Jul 2023 13:45:53 -0400 Subject: [PATCH 1/4] feat(NODE-4961)!: remove command result from commit and abort transaction APIs --- src/sessions.ts | 16 ++++----- .../transactions/transactions.test.ts | 35 +++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 9b4d1063526..6c7562151bd 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -413,14 +413,14 @@ export class ClientSession extends TypedEventEmitter { /** * Commits the currently active transaction in this session. */ - async commitTransaction(): Promise { + async commitTransaction(): Promise { return endTransactionAsync(this, 'commitTransaction'); } /** * Aborts the currently active transaction in this session. */ - async abortTransaction(): Promise { + async abortTransaction(): Promise { return endTransactionAsync(this, 'abortTransaction'); } @@ -634,14 +634,14 @@ const endTransactionAsync = promisify( endTransaction as ( session: ClientSession, commandName: 'abortTransaction' | 'commitTransaction', - callback: (error: Error, result: Document) => void + callback: (error: Error) => void ) => void ); function endTransaction( session: ClientSession, commandName: 'abortTransaction' | 'commitTransaction', - callback: Callback + callback: Callback ) { // handle any initial problematic cases const txnState = session.transaction.state; @@ -715,7 +715,7 @@ function endTransaction( Object.assign(command, { maxTimeMS: session.transaction.options.maxTimeMS }); } - function commandHandler(error?: Error, result?: Document) { + function commandHandler(error?: Error) { if (commandName !== 'commitTransaction') { session.transaction.transition(TxnState.TRANSACTION_ABORTED); if (session.loadBalanced) { @@ -744,7 +744,7 @@ function endTransaction( } } - callback(error, result); + callback(error); } if (session.transaction.recoveryToken) { @@ -759,7 +759,7 @@ function endTransaction( readPreference: ReadPreference.primary, bypassPinningCheck: true }), - (error, result) => { + error => { if (command.abortTransaction) { // always unpin on abort regardless of command outcome session.unpin(); @@ -787,7 +787,7 @@ function endTransaction( ); } - commandHandler(error, result); + commandHandler(error); } ); } diff --git a/test/integration/transactions/transactions.test.ts b/test/integration/transactions/transactions.test.ts index e953fecc8c8..b7e487371f2 100644 --- a/test/integration/transactions/transactions.test.ts +++ b/test/integration/transactions/transactions.test.ts @@ -10,6 +10,15 @@ import { } from '../../mongodb'; describe('Transactions', function () { + let client: MongoClient; + beforeEach(async function () { + client = this.configuration.newClient(); + }); + + afterEach(async function () { + await client.close(); + }); + describe('withTransaction', function () { let session: ClientSession; let sessionPool: ServerSessionPool; @@ -186,6 +195,32 @@ describe('Transactions', function () { }); }); + context('commitTransaction()', () => { + it( + 'returns void', + { requires: { topology: ['sharded', 'replicaset'], mongodb: '>=4.1.0' } }, + async () => + client.withSession(async session => + session.withTransaction(async () => { + expect(await session.commitTransaction()).to.be.undefined; + }) + ) + ); + }); + + context('abortTransaction()', () => { + it( + 'returns void', + { requires: { topology: ['sharded', 'replicaset'], mongodb: '>=4.1.0' } }, + async () => + client.withSession(async session => + session.withTransaction(async () => { + expect(await session.abortTransaction()).to.be.undefined; + }) + ) + ); + }); + describe('TransientTransactionError', function () { it('should have a TransientTransactionError label inside of a transaction', { metadata: { requires: { topology: 'replicaset', mongodb: '>=4.0.0' } }, From 20bf8375f3b100719a0c611e76702d35651f58f3 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 25 Jul 2023 16:01:22 -0400 Subject: [PATCH 2/4] test: fix for abort/commit void --- .../transactions/transactions.test.ts | 42 ++----------------- test/tools/unified-spec-runner/operations.ts | 15 ++----- 2 files changed, 7 insertions(+), 50 deletions(-) diff --git a/test/integration/transactions/transactions.test.ts b/test/integration/transactions/transactions.test.ts index b7e487371f2..30c456807c8 100644 --- a/test/integration/transactions/transactions.test.ts +++ b/test/integration/transactions/transactions.test.ts @@ -10,15 +10,6 @@ import { } from '../../mongodb'; describe('Transactions', function () { - let client: MongoClient; - beforeEach(async function () { - client = this.configuration.newClient(); - }); - - afterEach(async function () { - await client.close(); - }); - describe('withTransaction', function () { let session: ClientSession; let sessionPool: ServerSessionPool; @@ -113,19 +104,18 @@ describe('Transactions', function () { expect(withTransactionResult).to.be.undefined; }); - it('should return raw command when transaction is successfully committed', async () => { + it('should return result of executor 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 }); + return 'committed!'; }) .finally(async () => await session.endSession()); - expect(withTransactionResult).to.exist; - expect(withTransactionResult).to.be.an('object'); - expect(withTransactionResult).to.have.property('ok', 1); + expect(withTransactionResult).to.be.undefined; }); it('should throw when transaction is aborted due to an error', async () => { @@ -195,32 +185,6 @@ describe('Transactions', function () { }); }); - context('commitTransaction()', () => { - it( - 'returns void', - { requires: { topology: ['sharded', 'replicaset'], mongodb: '>=4.1.0' } }, - async () => - client.withSession(async session => - session.withTransaction(async () => { - expect(await session.commitTransaction()).to.be.undefined; - }) - ) - ); - }); - - context('abortTransaction()', () => { - it( - 'returns void', - { requires: { topology: ['sharded', 'replicaset'], mongodb: '>=4.1.0' } }, - async () => - client.withSession(async session => - session.withTransaction(async () => { - expect(await session.abortTransaction()).to.be.undefined; - }) - ) - ); - }); - describe('TransientTransactionError', function () { it('should have a TransientTransactionError label inside of a transaction', { metadata: { requires: { topology: 'replicaset', mongodb: '>=4.0.0' } }, diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index 6e69660779f..304b8cae06d 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -593,18 +593,11 @@ operations.set('withTransaction', async ({ entities, operation, client, testConf maxCommitTimeMS: operation.arguments!.maxCommitTimeMS }; - let errorFromOperations = null; - const result = await session.withTransaction(async () => { - errorFromOperations = await (async () => { - for (const callbackOperation of operation.arguments!.callback) { - await executeOperationAndCheck(callbackOperation, entities, client, testConfig); - } - })().catch(error => error); + await session.withTransaction(async () => { + for (const callbackOperation of operation.arguments!.callback) { + await executeOperationAndCheck(callbackOperation, entities, client, testConfig); + } }, options); - - if (result == null || errorFromOperations) { - throw errorFromOperations ?? Error('transaction not committed'); - } }); operations.set('countDocuments', async ({ entities, operation }) => { From 7965a23da4612558c042a623ecf189f45e9c55be Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 28 Jul 2023 15:51:22 -0400 Subject: [PATCH 3/4] test: assert methods return void --- .../transactions/transactions.test.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/integration/transactions/transactions.test.ts b/test/integration/transactions/transactions.test.ts index 5169fd6dce5..c7ffe2f02fb 100644 --- a/test/integration/transactions/transactions.test.ts +++ b/test/integration/transactions/transactions.test.ts @@ -229,6 +229,39 @@ describe('Transactions', function () { }); }); + context('when completing a transaction', () => { + let client: MongoClient; + beforeEach(async function () { + client = this.configuration.newClient(); + }); + + afterEach(async function () { + await client.close(); + }); + + it( + 'commitTransaction() resolve void', + { requires: { mongodb: '>=4.2.0', topology: '!single' } }, + async () => + client.withSession(async session => + session.withTransaction(async session => { + expect(await session.commitTransaction()).to.be.undefined; + }) + ) + ); + + it( + 'abortTransaction() resolve void', + { requires: { mongodb: '>=4.2.0', topology: '!single' } }, + async () => + client.withSession(async session => + session.withTransaction(async session => { + expect(await session.abortTransaction()).to.be.undefined; + }) + ) + ); + }); + describe('TransientTransactionError', function () { it('should have a TransientTransactionError label inside of a transaction', { metadata: { requires: { topology: 'replicaset', mongodb: '>=4.0.0' } }, From 0bf23ab0dffb375e236301d2ea1df29ab262adc8 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 28 Jul 2023 15:51:48 -0400 Subject: [PATCH 4/4] test: phrasing --- test/integration/transactions/transactions.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/transactions/transactions.test.ts b/test/integration/transactions/transactions.test.ts index c7ffe2f02fb..c05bead5367 100644 --- a/test/integration/transactions/transactions.test.ts +++ b/test/integration/transactions/transactions.test.ts @@ -240,7 +240,7 @@ describe('Transactions', function () { }); it( - 'commitTransaction() resolve void', + 'commitTransaction() resolves void', { requires: { mongodb: '>=4.2.0', topology: '!single' } }, async () => client.withSession(async session => @@ -251,7 +251,7 @@ describe('Transactions', function () { ); it( - 'abortTransaction() resolve void', + 'abortTransaction() resolves void', { requires: { mongodb: '>=4.2.0', topology: '!single' } }, async () => client.withSession(async session =>