Do not reject promise on transaction rollback (#3235)

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Master (Unreleased)
 
+### Bug fixes:
+
+- Do not reject promise on transaction rollback (only for new, non-callback, style of transactions for now to avoid breaking old code) #3235
+
 ### New features:
 
 - Use extension from knexfile for generating migrations unless overriden #3282

src/dialects/mssql/transaction.js

@@ -33,6 +33,10 @@ module.exports = class Transaction_MSSQL extends Transaction {
       () => {
         let err = error;
         if (isUndefined(error)) {
+          if (this.doNotRejectOnRollback) {
+            this._resolver();
+            return;
+          }
           err = new Error(`Transaction rejected with non-error: ${error}`);
         }
         this._rejecter(err);

src/dialects/mysql/transaction.js

@@ -30,6 +30,14 @@ assign(Transaction_MySQL.prototype, {
         if (status === 1) t._resolver(value);
         if (status === 2) {
           if (isUndefined(value)) {
+            if (
+              sql &&
+              sql.toUpperCase() === 'ROLLBACK' &&
+              t.doNotRejectOnRollback
+            ) {
+              t._resolver();
+              return;
+            }
             value = new Error(`Transaction rejected with non-error: ${value}`);
           }
           t._rejecter(value);

src/dialects/mysql2/transaction.js

@@ -29,6 +29,14 @@ assign(Transaction_MySQL2.prototype, {
         if (status === 1) t._resolver(value);
         if (status === 2) {
           if (isUndefined(value)) {
+            if (
+              sql &&
+              sql.toUpperCase() === 'ROLLBACK' &&
+              t.doNotRejectOnRollback
+            ) {
+              t._resolver();
+              return;
+            }
             value = new Error(`Transaction rejected with non-error: ${value}`);
           }
           t._rejecter(value);

src/dialects/oracle/transaction.js

@@ -29,6 +29,10 @@ module.exports = class Oracle_Transaction extends Transaction {
       .throw(err)
       .catch((error) => {
         if (isUndefined(error)) {
+          if (this.doNotRejectOnRollback) {
+            this._resolver();
+            return;
+          }
           error = new Error(`Transaction rejected with non-error: ${error}`);
         }
 

src/dialects/oracledb/transaction.js

@@ -34,6 +34,10 @@ module.exports = class Oracle_Transaction extends Transaction {
       })
       .then(function() {
         if (isUndefined(err)) {
+          if (self.doNotRejectOnRollback) {
+            self._resolver();
+            return;
+          }
           err = new Error(`Transaction rejected with non-error: ${err}`);
         }
         self._rejecter(err);

src/transaction.js

@@ -8,7 +8,7 @@ const makeKnex = require('./util/make-knex');
 
 const debug = Debug('knex:tx');
 
-const { uniqueId, isUndefined } = require('lodash');
+const { uniqueId, isUndefined, get } = require('lodash');
 
 // Acts as a facade for a Promise, keeping the internal state
 // and managing any child transactions.
@@ -20,12 +20,26 @@ class Transaction extends EventEmitter {
 
     // If there is no container provided, assume user wants to get instance of transaction and use it directly
     if (!container) {
+      // Default behaviour for new style of transactions is not to reject on rollback
+      if (!config || isUndefined(config.doNotRejectOnRollback)) {
+        this.doNotRejectOnRollback = true;
+      } else {
+        this.doNotRejectOnRollback = config.doNotRejectOnRollback;
+      }
+
       this.initPromise = new Promise((resolve, reject) => {
         this.initRejectFn = reject;
         container = (transactor) => {
           resolve(transactor);
         };
       });
+    } else {
+      // Default behaviour for old style of transactions is to reject on rollback
+      if (!config || isUndefined(config.doNotRejectOnRollback)) {
+        this.doNotRejectOnRollback = false;
+      } else {
+        this.doNotRejectOnRollback = config.doNotRejectOnRollback;
+      }
     }
 
     this.client = client;
@@ -173,6 +187,14 @@ class Transaction extends EventEmitter {
         }
         if (status === 2) {
           if (isUndefined(value)) {
+            if (
+              get(res, 'context.sql', '').toUpperCase() === 'ROLLBACK' &&
+              this.doNotRejectOnRollback
+            ) {
+              this._resolver();
+              return;
+            }
+
             value = new Error(`Transaction rejected with non-error: ${value}`);
           }
           this._rejecter(value);

test/integration/builder/transaction.js

@@ -375,6 +375,15 @@ module.exports = function(knex) {
         });
     });
 
+    it('does not reject promise when rolling back a transaction', async () => {
+      const trxProvider = knex.transactionProvider();
+      const trxPromise = trxProvider();
+
+      await trxPromise.then((trx) => {
+        return trx.rollback();
+      });
+    });
+
     it('should allow for nested transactions', function() {
       if (/redshift/i.test(knex.client.driverName)) {
         return Promise.resolve();

test/unit/knex.js

@@ -362,7 +362,7 @@ describe('knex', () => {
     expect(result).to.be.undefined;
   });
 
-  it('rejects execution promise if there was a rollback', async () => {
+  it('resolves execution promise if there was a manual rollback and transaction is set not to reject', async () => {
     const knex = Knex(sqliteConfig);
 
     const trx = await knex.transaction();
@@ -373,6 +373,23 @@ describe('knex', () => {
     expect(rows[0].result).to.equal(1);
     await trx.rollback();
 
+    const result = await executionPromise;
+    expect(result).to.be.undefined;
+  });
+
+  it('rejects execution promise if there was a manual rollback and transaction is set to reject', async () => {
+    const knex = Knex(sqliteConfig);
+
+    const trx = await knex.transaction(undefined, {
+      doNotRejectOnRollback: false,
+    });
+    const executionPromise = trx.executionPromise;
+
+    expect(trx.client.transacting).to.equal(true);
+    const rows = await knex.transacting(trx).select(knex.raw('1 as result'));
+    expect(rows[0].result).to.equal(1);
+    await trx.rollback();
+
     let errorWasThrown;
     try {
       await executionPromise;
@@ -382,10 +399,19 @@ describe('knex', () => {
         'Transaction rejected with non-error: undefined'
       );
     }
-
     expect(errorWasThrown).to.be.true;
   });
 
+  it('does not reject promise when rolling back a transaction', async () => {
+    const knex = Knex(sqliteConfig);
+    const trxProvider = knex.transactionProvider();
+    const trxPromise = trxProvider();
+
+    await trxPromise.then((trx) => {
+      return trx.rollback();
+    });
+  });
+
   it('creating transaction copy with user params should throw an error', () => {
     if (!sqliteConfig) {
       return;
@@ -409,6 +435,7 @@ describe('knex', () => {
     function ClientFoobar(config) {
       SqliteClient.call(this, config);
     }
+
     inherits(ClientFoobar, SqliteClient);
 
     ClientFoobar.prototype._driver = () => {