Doesn't allow .commit(), .rollback() or .close() managed transactions

Allowing self-manage managed transactions could causing unexpected behaviours in the user code depends on how the different possible code paths in the transaction work.

Author: bigmontz

packages/core/src/internal/transaction-executor.ts

@@ -168,7 +168,8 @@ export class TransactionExecutor {
     transactionWork: TransactionWork<T>
   ): Promise<T> {
     try {
-      const result = transactionWork(tx)
+      const managedTx = this._toManagedTx(tx)
+      const result = transactionWork(managedTx)
       // user defined callback is supposed to return a promise, but it might not; so to protect against an
       // incorrect API usage we wrap the returned value with a resolved promise; this is effectively a
       // validation step without type checks
@@ -178,6 +179,23 @@ export class TransactionExecutor {
     }
   }
 
+  _toManagedTx(tx: Transaction): Transaction {
+    const _thown = 
+      (method: string) => 
+      () => Promise.reject(newError(`Transaction.${method}() is not supported in managed transactions.`))
+    const _isTxManagementMethod = 
+      (method: string) => ['rollback', 'commit', 'close'].includes(method)
+
+    return new Proxy(tx, {
+      get: (target, name) => {
+        if (_isTxManagementMethod(name.toString())) {
+          return _thown(name.toString())
+        } 
+        return (target as any)[name];
+      }
+    }) as Transaction;
+  }
+
   _handleTransactionWorkSuccess<T>(
     result: T,
     tx: Transaction,

packages/core/test/session.test.ts

@@ -18,6 +18,7 @@
  */
 import { ConnectionProvider, Session, Connection, TransactionPromise, Transaction } from '../src'
 import { bookmarks } from '../src/internal'
+import { newError  } from '../src/error'
 import { ACCESS_MODE_READ, FETCH_ALL } from '../src/internal/constants'
 import FakeConnection from './utils/connection.fake'
 
@@ -204,6 +205,30 @@ describe('session', () => {
     })
   }, 70000)
 
+  it.each([
+    ['commit', (tx: Transaction) => tx.commit()],
+    ['rollback', (tx: Transaction) => tx.rollback()],
+    ['close', (tx: Transaction) => tx.close()],
+  ])('should not allow call Transaction.%s() in the read transaction work', async (method, work) => {
+    const connection = mockBeginWithSuccess(newFakeConnection())
+    const session = newSessionWithConnection(connection, false, 1000)
+    const expectedError = newError(`Transaction.${method}() is not supported in managed transactions.`)
+
+    expect(session.readTransaction(work)).rejects.toThrow(expectedError)
+  })
+
+  it.each([
+    ['commit', (tx: Transaction) => tx.commit()],
+    ['rollback', (tx: Transaction) => tx.rollback()],
+    ['close', (tx: Transaction) => tx.close()],
+  ])('should not allow call Transaction.%s() in the write transaction work', async (method, work) => {
+    const connection = mockBeginWithSuccess(newFakeConnection())
+    const session = newSessionWithConnection(connection, false, 1000)
+    const expectedError = newError(`Transaction.${method}() is not supported in managed transactions.`)
+
+    expect(session.writeTransaction(work)).rejects.toThrow(expectedError)
+  })
+
   describe('.lastBookmark()', () => {
     it.each([
       [bookmarks.Bookmarks.empty()],

packages/testkit-backend/src/skipped-tests/common.js

@@ -138,10 +138,6 @@ const skippedTests = [
     'Results are always valid but don\'t return records when out of scope',
     ifStartsWith('stub.iteration.test_result_scope.TestResultScope.')
   ),
-  skip(
-    'Driver (still) allows explicit managing of managed transaction',
-    ifEquals('stub.tx_lifetime.test_tx_lifetime.TestTxLifetime.test_managed_tx_raises_tx_managed_exec')
-  ),
   skip(
     'Flaky tests, requires investigation',
     ifEndsWith('.test_should_fail_when_reading_from_unexpectedly_interrupting_readers_using_tx_function')