Merge branch 'master' into fixed-transaction-promise-mutation-issue

src/dialects/maria/transaction.js

@@ -1,6 +1,7 @@
 import Debug from 'debug';
 import Transaction from '../../transaction';
 import * as helpers from '../../helpers';
+import {isUndefined} from 'lodash';
 
 const debug = Debug('knex:tx');
 
@@ -23,7 +24,12 @@ export default class Transaction_Maria extends Transaction {
       })
       .tap(function() {
         if (status === 1) t._resolver(value)
-        if (status === 2) t._rejecter(value)
+        if (status === 2) {
+          if(isUndefined(value)) {
+            value = new Error(`Transaction rejected with non-error: ${value}`)
+          }
+          t._rejecter(value)
+        }
       })
     if (status === 1 || status === 2) {
       t._completed = true

src/dialects/mysql/transaction.js

@@ -3,7 +3,7 @@ import Transaction from '../../transaction';
 import inherits from 'inherits';
 import Debug from 'debug';
 import * as helpers from '../../helpers';
-import { assign } from 'lodash'
+import { assign, isUndefined } from 'lodash'
 
 const debug = Debug('knex:tx');
 
@@ -31,7 +31,12 @@ assign(Transaction_MySQL.prototype, {
       })
       .tap(function() {
         if (status === 1) t._resolver(value)
-        if (status === 2) t._rejecter(value)
+        if (status === 2) {
+          if(isUndefined(value)) {
+            value = new Error(`Transaction rejected with non-error: ${value}`)
+          }
+          t._rejecter(value)
+        }
       })
     if (status === 1 || status === 2) {
       t._completed = true

src/dialects/mysql2/transaction.js

@@ -4,7 +4,7 @@ import inherits from 'inherits';
 const debug = require('debug')('knex:tx')
 import * as helpers from '../../helpers';
 
-import { assign } from 'lodash'
+import { assign, isUndefined } from 'lodash'
 
 function Transaction_MySQL2() {
   Transaction.apply(this, arguments)
@@ -30,7 +30,12 @@ assign(Transaction_MySQL2.prototype, {
       })
       .tap(function() {
         if (status === 1) t._resolver(value)
-        if (status === 2) t._rejecter(value)
+        if (status === 2) {
+          if(isUndefined(value)) {
+            value = new Error(`Transaction rejected with non-error: ${value}`)
+          }
+          t._rejecter(value)
+        }
       })
     if (status === 1 || status === 2) {
       t._completed = true

src/dialects/oracle/transaction.js

@@ -1,6 +1,7 @@
 
 import Promise from 'bluebird';
 import Transaction from '../../transaction';
+import {isUndefined} from 'lodash';
 const debugTx = require('debug')('knex:tx')
 
 export default class Oracle_Transaction extends Transaction {
@@ -26,7 +27,13 @@ export default class Oracle_Transaction extends Transaction {
     debugTx('%s: rolling back', this.txid)
     return conn.rollbackAsync()
       .throw(err)
-      .catch(this._rejecter)
+      .catch((error) => {
+        if(isUndefined(error)) {
+          error = new Error(`Transaction rejected with non-error: ${error}`)
+        }
+
+        return this._rejecter(error);
+      });
   }
 
   acquireConnection(config) {

src/dialects/oracledb/transaction.js

@@ -1,3 +1,5 @@
+import {isUndefined} from 'lodash';
+
 const Promise = require('bluebird');
 const Transaction = require('../../transaction');
 const debugTx = require('debug')('knex:tx');
@@ -27,6 +29,9 @@ export default class Oracle_Transaction extends Transaction {
     return conn.rollbackAsync().timeout(5000).catch(Promise.TimeoutError, function(e) {
       self._rejecter(e);
     }).then(function() {
+      if(isUndefined(err)) {
+        err = new Error(`Transaction rejected with non-error: ${err}`)
+      }
       self._rejecter(err);
     });
   }

src/util/batchInsert.js

@@ -35,21 +35,23 @@ export default function batchInsert(client, tableName, batch, chunkSize = 1000)
       .then((tr) => {
         return Promise.mapSeries(chunks, (items) => tr(tableName).insert(items, returning))
           .then((result) => {
-            result = flatten(result);
+            result = flatten(result || []);
 
             if(autoTransaction) {
-              return tr.commit()
+              //TODO: -- Oracle tr.commit() does not return a 'thenable' !? Ugly hack for now.
+              return (tr.commit(result) || Promise.resolve())
                 .then(() => result);
             }
 
             return result;
           })
           .catch((error) => {
             if(autoTransaction) {
-              return tr.rollback(error);
+              return tr.rollback(error)
+                .then(() => Promise.reject(error));
             }
 
-            throw error;
+            return Promise.reject(error);
           })
       })
       .then(resolve)

test/integration/builder/inserts.js

@@ -699,13 +699,16 @@ module.exports = function(knex) {
             .then(function() {
               return knex.schema.createTable('batchInsertDuplicateKey', function (table) {
                 table.string('col');
-                table.unique('col');
+                table.primary('col');
               });
             })
             .then(function () {
               var rows = [{'col': 'a'}, {'col': "a"}];
               return knex.batchInsert('batchInsertDuplicateKey', rows, rows.length);
             })
+            .then(function() {
+              return reject(new Error('Should not reach this point'))
+            })
             .catch(function (error) {
               //Should reach this point before timeout of 10s
               expect(error.message.toLowerCase()).to.include('batchinsertduplicatekey');
@@ -726,10 +729,8 @@ module.exports = function(knex) {
 
       it('transaction.batchInsert using specified transaction', function() {
         return knex.transaction(function(tr) {
-          tr.batchInsert('BatchInsert', items, 30)
+          return tr.batchInsert('BatchInsert', items, 30)
           .returning(['Col1', 'Col2'])
-          .then(tr.commit)
-          .catch(tr.rollback);
         })
       });
 

test/integration/builder/transaction.js

@@ -397,17 +397,17 @@ module.exports = function(knex) {
       });
     }
 
-    it.skip('Rollback without an error should not reject with undefined #1966', function() {
+    it('Rollback without an error should not reject with undefined #1966', function() {
       return knex.transaction(function(tr) {
-          tr.rollback();
-        })
-        .then(function() {
-          expect(true).to.equal(false, 'Transaction should not have commited');
-        })
-        .catch(function(error) {
-          expect(error instanceof Error).to.equal(true);
-          expect(error.message).to.equal('Transaction rejected with non-error: undefined');
-        });
+        tr.rollback();
+      })
+      .then(function() {
+        expect(true).to.equal(false, 'Transaction should not have commited');
+      })
+      .catch(function(error) {
+        expect(error instanceof Error).to.equal(true);
+        expect(error.message).to.equal('Transaction rejected with non-error: undefined');
+      });
     });
 
     it('#1052 - transaction promise mutating', function() {