Deprecate .commit(), .rollback() or .close() managed transactions (#890)

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.

Co-authored-by: Robsdedude <[email protected]>

Author: bigmontz

packages/core/src/index.ts

@@ -70,6 +70,7 @@ import ConnectionProvider from './connection-provider'
 import Connection from './connection'
 import Transaction from './transaction'
 import TransactionPromise from './transaction-promise'
+import ManagedTransaction from './transaction-managed'
 import Session, { TransactionConfig } from './session'
 import Driver, * as driver from './driver'
 import auth from './auth'
@@ -136,6 +137,7 @@ const forExport = {
   Result,
   Transaction,
   TransactionPromise,
+  ManagedTransaction,
   Session,
   Driver,
   Connection,
@@ -194,6 +196,7 @@ export {
   Connection,
   Transaction,
   TransactionPromise,
+  ManagedTransaction,
   Session,
   Driver,
   types,

packages/core/src/session.ts

@@ -32,9 +32,10 @@ import { Query, SessionMode } from './types'
 import Connection from './connection'
 import { NumberOrInteger } from './graph-types'
 import TransactionPromise from './transaction-promise'
+import ManagedTransaction from './transaction-managed'
 
 type ConnectionConsumer = (connection: Connection | void) => any | undefined
-type TransactionWork<T> = (tx: Transaction) => Promise<T> | T
+type TransactionWork<T> = (tx: ManagedTransaction) => Promise<T> | T
 
 interface TransactionConfig {
   timeout?: NumberOrInteger
@@ -336,7 +337,7 @@ class Session {
    * delay of 1 second and maximum retry time of 30 seconds. Maximum retry time is configurable via driver config's
    * `maxTransactionRetryTime` property in milliseconds.
    *
-   * @param {function(tx: Transaction): Promise} transactionWork - Callback that executes operations against
+   * @param {function(tx: ManagedTransaction): Promise} transactionWork - Callback that executes operations against
    * a given {@link Transaction}.
    * @param {TransactionConfig} [transactionConfig] - Configuration for all transactions started to execute the unit of work.
    * @return {Promise} Resolved promise as returned by the given function or rejected promise when given
@@ -358,7 +359,7 @@ class Session {
    * delay of 1 second and maximum retry time of 30 seconds. Maximum retry time is configurable via driver config's
    * `maxTransactionRetryTime` property in milliseconds.
    *
-   * @param {function(tx: Transaction): Promise} transactionWork - Callback that executes operations against
+   * @param {function(tx: ManagedTransaction): Promise} transactionWork - Callback that executes operations against
    * a given {@link Transaction}.
    * @param {TransactionConfig} [transactionConfig] - Configuration for all transactions started to execute the unit of work.
    * @return {Promise} Resolved promise as returned by the given function or rejected promise when given

packages/core/src/transaction-managed.ts

@@ -0,0 +1,70 @@
+/**
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import Transaction from './transaction'
+
+/**
+ * Represents a transaction that is managed by the transaction executor.
+ * 
+ * @public
+ */
+class ManagedTransaction extends Transaction {
+  
+  /**
+   * Commits the transaction and returns the result.
+   *
+   * After committing the transaction can no longer be used.
+   *
+   * @deprecated Commit should be done by returning from the transaction work.
+   *
+   * @returns {Promise<void>} An empty promise if committed successfully or error if any error happened during commit.
+   */
+  commit(): Promise<void> {
+    return super.commit()
+  }
+
+  /**
+   * Rollbacks the transaction.
+   *
+   * After rolling back, the transaction can no longer be used.
+   *
+   * @deprecated Rollback should be done by throwing or returning a rejected promise from the transaction work.
+   *
+   * @returns {Promise<void>} An empty promise if rolled back successfully or error if any error happened during
+   * rollback.
+   */
+  rollback(): Promise<void> {
+    return super.rollback()
+  }
+
+  /**
+   * Closes the transaction
+   *
+   * This method will roll back the transaction if it is not already committed or rolled back.
+   *
+   * @deprecated Closure should not be done in transaction work. See {@link ManagedTransaction#commit} and {@link ManagedTransaction#rollback}
+   *
+   * @returns {Promise<void>} An empty promise if closed successfully or error if any error happened during
+   */
+  close(): Promise<void> {
+    return super.close()
+  }
+}
+
+export default ManagedTransaction

packages/core/src/transaction-promise.ts

@@ -170,6 +170,7 @@ class TransactionPromise extends Transaction implements Promise<Transaction>{
 
   /**
    * @access private
+   * @returns {void}
    */
   private _onBeginError(error: Error): void {
     this._beginError = error;
@@ -180,6 +181,7 @@ class TransactionPromise extends Transaction implements Promise<Transaction>{
 
   /**
    * @access private
+   * @returns {void}
    */
   private _onBeginMetadata(metadata: any): void {
     this._beginMetadata = metadata || {};

packages/core/test/transaction.test.ts

@@ -21,10 +21,12 @@ import { ConnectionProvider, newError, Transaction, TransactionPromise } from ".
 import { Bookmarks } from "../src/internal/bookmarks";
 import { ConnectionHolder } from "../src/internal/connection-holder";
 import { TxConfig } from "../src/internal/tx-config";
+import ManagedTransaction from "../src/transaction-managed";
 import FakeConnection from "./utils/connection.fake";
 
 
 testTx('Transaction', newRegularTransaction)
+testTx('ManagedTransaction', newManagedTransaction)
 
 testTx('TransactionPromise', newTransactionPromise, () => {
   describe('Promise', () => {
@@ -498,6 +500,39 @@ function newRegularTransaction({
   return transaction
 }
 
+function newManagedTransaction({
+  connection,
+  fetchSize = 1000,
+  highRecordWatermark = 700,
+  lowRecordWatermark = 300
+}: {
+  connection: FakeConnection
+  fetchSize?: number
+  highRecordWatermark?: number,
+  lowRecordWatermark?: number
+}): ManagedTransaction {
+  const connectionProvider = new ConnectionProvider()
+  connectionProvider.acquireConnection = () => Promise.resolve(connection)
+  connectionProvider.close = () => Promise.resolve()
+
+  const connectionHolder = new ConnectionHolder({ connectionProvider })
+  connectionHolder.initializeConnection()
+
+  const transaction = new ManagedTransaction({
+    connectionHolder,
+    onClose: () => { },
+    onBookmarks: (_: Bookmarks) => { },
+    onConnection: () => { },
+    reactive: false,
+    fetchSize,
+    impersonatedUser: "",
+    highRecordWatermark,
+    lowRecordWatermark
+  })
+
+  return transaction
+}
+
 function newFakeConnection(): FakeConnection {
   return new FakeConnection()
 }

packages/neo4j-driver-lite/src/index.ts

@@ -63,6 +63,7 @@ import {
   Session,
   Transaction,
   TransactionPromise,
+  ManagedTransaction,
   ServerInfo,
   Connection,
   driver as coreDriver,
@@ -427,6 +428,7 @@ const forExport = {
   Session,
   Transaction,
   TransactionPromise,
+  ManagedTransaction,
   Point,
   Duration,
   LocalTime,
@@ -476,6 +478,7 @@ export {
   Session,
   Transaction,
   TransactionPromise,
+  ManagedTransaction,
   Point,
   Duration,
   LocalTime,

packages/neo4j-driver/src/session-rx.js

@@ -21,6 +21,7 @@ import { flatMap, catchError, concat } from 'rxjs/operators'
 import RxResult from './result-rx'
 import { Session, internal } from 'neo4j-driver-core'
 import RxTransaction from './transaction-rx'
+import RxManagedTransaction from './transaction-managed-rx'
 import RxRetryLogic from './internal/retry-logic-rx'
 
 const {
@@ -84,7 +85,7 @@ export default class RxSession {
    * Executes the provided unit of work in a {@link READ} reactive transaction which is created with the provided
    * transaction configuration.
    * @public
-   * @param {function(txc: RxTransaction): Observable} work - A unit of work to be executed.
+   * @param {function(txc: RxManagedTransaction): Observable} work - A unit of work to be executed.
    * @param {TransactionConfig} transactionConfig - Configuration for the enclosing transaction created by the driver.
    * @returns {Observable} - A reactive stream returned by the unit of work.
    */
@@ -96,7 +97,7 @@ export default class RxSession {
    * Executes the provided unit of work in a {@link WRITE} reactive transaction which is created with the provided
    * transaction configuration.
    * @public
-   * @param {function(txc: RxTransaction): Observable} work - A unit of work to be executed.
+   * @param {function(txc: RxManagedTransaction): Observable} work - A unit of work to be executed.
    * @param {TransactionConfig} transactionConfig - Configuration for the enclosing transaction created by the driver.
    * @returns {Observable} - A reactive stream returned by the unit of work.
    */

packages/neo4j-driver/src/transaction-managed-rx.js

@@ -0,0 +1,62 @@
+/**
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [http://neo4j.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import RxTransaction from './transaction-rx'
+
+/**
+ * Represents a RX transaction that is managed by the transaction executor.
+ *
+ * @public
+ */
+class RxManagedTransaction extends RxTransaction {
+  /**
+   *  Commits the transaction.
+   *
+   * @public
+   * @deprecated Commit should be done by returning from the transaction work.
+   * @returns {Observable} - An empty observable
+   */
+  commit () {
+    return super.commit()
+  }
+
+  /**
+   *  Rolls back the transaction.
+   *
+   * @public
+   * @deprecated Rollback should be done by throwing or returning a rejected promise from the transaction work.
+   * @returns {Observable} - An empty observable
+   */
+  rollback () {
+    return super.rollback()
+  }
+
+  /**
+   * Closes the transaction
+   *
+   * This method will roll back the transaction if it is not already committed or rolled back.
+   * @deprecated Closure should not be done in transaction work. See {@link RxManagedTransaction#commit} and {@link RxManagedTransaction#rollback}
+   * @returns {Observable} - An empty observable
+   */
+  close () {
+    return super.close()
+  }
+}
+
+export default RxManagedTransaction