More consistent handling of nested transactions (#3393)

.travis.yml

@@ -1,6 +1,5 @@
 # .travis.yml
 language: node_js
-dist: xenial
 sudo: required
 
 cache:
@@ -18,7 +17,7 @@ matrix:
       env: TESTSCRIPT=test:nyc DB="oracledb mssql mysql mysql2 postgres sqlite3" KNEX_TEST_TIMEOUT=60000
 install:
   - npm i
-  - (echo $DB | grep oracledb) && npm install oracledb || true
+  - (echo $DB | grep oracledb) && npm install oracledb@3.1.2 || true
 
 before_script:
   - npm run db:start

lib/transaction.js

@@ -250,7 +250,25 @@ function makeTransactor(trx, connection, trxClient) {
   transactor.userParams = trx.userParams || {};
 
   transactor.transaction = function(container, options) {
-    return trxClient.transaction(container, options, trx);
+    if (!options) {
+      options = { doNotRejectOnRollback: true };
+    } else if (isUndefined(options.doNotRejectOnRollback)) {
+      options.doNotRejectOnRollback = true;
+    }
+
+    if (container) {
+      return trxClient.transaction(container, options, trx);
+    } else {
+      return new Promise((resolve, _reject) => {
+        trxClient.transaction(
+          (nestedTrx) => {
+            resolve(nestedTrx);
+          },
+          options,
+          trx
+        );
+      });
+    }
   };
   transactor.savepoint = function(container, options) {
     return transactor.transaction(container, options);

test/index.js

@@ -15,6 +15,30 @@ global.d = new Date();
 
 bluebird.longStackTraces();
 
+// '.timeout(ms, {cancel: true}) should throw error if cancellation cannot acquire connection' produced unhandled rejection and it's unclear how to avoid that
+const EXPECTED_REJECTION_COUNT = 2;
+const rejectionLog = [];
+process.on('unhandledRejection', (reason) => {
+  console.error('Unhandled rejection:', reason);
+  rejectionLog.push({
+    reason,
+  });
+});
+
+process.on('exit', (code) => {
+  if (rejectionLog.length) {
+    console.error(`Unhandled rejections: ${rejectionLog.length}`);
+    rejectionLog.forEach((rejection) => {
+      console.error(rejection);
+    });
+
+    if (rejectionLog.length > EXPECTED_REJECTION_COUNT) {
+      process.exitCode = code || 1;
+    }
+  }
+  console.log('No unhandled exceptions');
+});
+
 describe('Query Building Tests', function() {
   this.timeout(process.env.KNEX_TEST_TIMEOUT || 5000);
 

test/integration/index.js

@@ -7,7 +7,7 @@ const fs = require('fs');
 
 const Bluebird = require('bluebird');
 
-Bluebird.each(Object.keys(config), function(dialectName) {
+Object.keys(config).forEach((dialectName) => {
   return require('./suite')(logger(knex(config[dialectName])));
 });
 

test/integration/schema/index.js

@@ -1550,7 +1550,7 @@ module.exports = function(knex) {
         });
       });
     });
-    it('supports named primary keys', function() {
+    it('supports named primary keys', async () => {
       const constraintName = 'pk-test';
       const tableName = 'namedpk';
       const expectedRes = [
@@ -1559,14 +1559,15 @@ module.exports = function(knex) {
           name: tableName,
           tbl_name: tableName,
           sql:
-            'CREATE TABLE "' +
+            'CREATE TABLE `' +
             tableName +
-            '" ("test" varchar(255), "test2" varchar(255), constraint "' +
+            '` (`test` varchar(255), `test2` varchar(255), constraint `' +
             constraintName +
-            '" primary key ("test"))',
+            '` primary key (`test`))',
         },
       ];
-      return knex.transaction(function(tr) {
+
+      await knex.transaction(function(tr) {
         return tr.schema
           .dropTableIfExists(tableName)
           .then(function() {
@@ -1650,11 +1651,11 @@ module.exports = function(knex) {
                   name: tableName,
                   tbl_name: tableName,
                   sql:
-                    'CREATE TABLE "' +
+                    'CREATE TABLE `' +
                     tableName +
-                    '" ("test" varchar(255), "test2" varchar(255), constraint "' +
+                    '` (`test` varchar(255), `test2` varchar(255), constraint `' +
                     constraintName +
-                    '" primary key ("test", "test2"))',
+                    '` primary key (`test`, `test2`))',
                 },
               ];
               tr.select('type', 'name', 'tbl_name', 'sql')
@@ -1704,11 +1705,11 @@ module.exports = function(knex) {
                   name: singleUniqueName,
                   tbl_name: tableName,
                   sql:
-                    'CREATE UNIQUE INDEX "' +
+                    'CREATE UNIQUE INDEX `' +
                     singleUniqueName +
-                    '" on "' +
+                    '` on `' +
                     tableName +
-                    '" ("test")',
+                    '` (`test`)',
                 },
               ];
               tr.select('type', 'name', 'tbl_name', 'sql')
@@ -1758,22 +1759,22 @@ module.exports = function(knex) {
                   name: singleUniqueName,
                   tbl_name: tableName,
                   sql:
-                    'CREATE UNIQUE INDEX "' +
+                    'CREATE UNIQUE INDEX `' +
                     singleUniqueName +
-                    '" on "' +
+                    '` on `' +
                     tableName +
-                    '" ("test")',
+                    '` (`test`)',
                 },
                 {
                   type: 'index',
                   name: multiUniqueName,
                   tbl_name: tableName,
                   sql:
-                    'CREATE UNIQUE INDEX "' +
+                    'CREATE UNIQUE INDEX `' +
                     multiUniqueName +
-                    '" on "' +
+                    '` on `' +
                     tableName +
-                    '" ("test", "test2")',
+                    '` (`test`, `test2`)',
                 },
               ];
               tr.select('type', 'name', 'tbl_name', 'sql')
@@ -1860,17 +1861,17 @@ module.exports = function(knex) {
                   name: joinTableName,
                   tbl_name: joinTableName,
                   sql:
-                    'CREATE TABLE "' +
+                    'CREATE TABLE `' +
                     joinTableName +
-                    '" ("user" char(36), "group" char(36), constraint "' +
+                    '` (`user` char(36), `group` char(36), constraint `' +
                     userConstraint +
-                    '" foreign key("user") references "' +
+                    '` foreign key(`user`) references `' +
                     userTableName +
-                    '"("id"), constraint "' +
+                    '`(`id`), constraint `' +
                     groupConstraint +
-                    '" foreign key("group") references "' +
+                    '` foreign key(`group`) references `' +
                     groupTableName +
-                    '"("id"), primary key ("user", "group"))',
+                    '`(`id`), primary key (`user`, `group`))',
                 },
               ];
               tr.select('type', 'name', 'tbl_name', 'sql')

test/unit/knex.js

@@ -330,6 +330,22 @@ describe('knex', () => {
       });
   });
 
+  it('supports nested transaction for promise transactions', async () => {
+    const knex = Knex(sqliteConfig);
+    const trx = await knex.transaction();
+    const nestedTrx = await trx.transaction();
+    const nestedTrx2 = await nestedTrx.transaction();
+    expect(nestedTrx.name).to.equal('knex');
+    expect(nestedTrx2.name).to.equal('knex');
+  });
+
+  it('does not reject rolled back nested transactions by default', async () => {
+    const knex = Knex(sqliteConfig);
+    const trx = await knex.transaction();
+    const nestedTrx = await trx.transaction();
+    await nestedTrx.rollback();
+  });
+
   it('supports accessing execution promise from standalone transaction', async () => {
     const knex = Knex(sqliteConfig);