Auto commit disabled by default (#5960)

* auto commit disabled by default

* test for autocommit disable, prevent sqlite to send fake query

* auto commit test refactor

* refactor autocommit test to check sequence of queries

* enabled es6 in jshint

.jshintrc

@@ -16,6 +16,7 @@
   "unused": "vars",
   "nonbsp": true,
   "maxdepth": 8,
+  "esversion": 6,
   "quotmark": true, // deprecated
   "-W041": false,
 

changelog.md

@@ -18,6 +18,7 @@
 - [REWRITE] Rewrite model and instance to a single class - instance instanceof Model [#5924](https://github.com/sequelize/sequelize/issues/5924)
 - [REMOVED] Counter cache plugin
 - [FIXED] All associations now prefer aliases to construct foreign key [#5267](https://github.com/sequelize/sequelize/issues/5267)
+- [REMOVED] Default transaction auto commit [#5094](https://github.com/sequelize/sequelize/issues/5094)
 
 ## BC breaks:
 - `hookValidate` removed in favor of `validate` with `hooks: true | false`. `validate` returns a promise which is rejected if validation fails
@@ -33,6 +34,7 @@
 - Instances (database rows) are now instances of the model, instead of being a separate class. This means you can replace User.build() with new User() and sequelize.define with User extends Sequelize.Model. See #5924
 - The counter cache plugin, and consequently the `counterCache` option for associations has been removed. The plugin is seeking a new maintainer - You can find the code [here](https://github.com/sequelize/sequelize/blob/aace1250dfa8cd81a4edfd2086c9058b513f6ee0/lib/plugins/counter-cache.js)
 - All associations type will prefer `as` when constructing the `foreignKey` name. You can override this by `foreignKey` option.
+- Removed default `AUTO COMMIT` for transaction. Its only sent if explicitly set by user or required by dialects (like `mysql`)
 
 # 3.23.2
 - [FIXED] Type validation now works with non-strings due to updated validator@5.0.0 [#5861](https://github.com/sequelize/sequelize/pull/5861)

lib/dialects/mysql/index.js

@@ -35,7 +35,10 @@ MysqlDialect.prototype.supports = _.merge(_.cloneDeep(Abstract.prototype.support
   updateOnDuplicate: true,
   indexViaAlter: true,
   NUMERIC: true,
-  GEOMETRY: true
+  GEOMETRY: true,
+  transactionOptions: {
+    autocommit: true
+  }
 });
 
 ConnectionManager.prototype.defaultVersion = '5.6.0';

lib/dialects/sqlite/index.js

@@ -26,7 +26,8 @@ SqliteDialect.prototype.supports = _.merge(_.cloneDeep(Abstract.prototype.suppor
     using: false
   },
   transactionOptions: {
-    type: true
+    type: true,
+    autocommit: false
   },
   joinTableDependent: false,
   groupedLimit: false,

lib/dialects/sqlite/query-generator.js

@@ -353,7 +353,8 @@ var QueryGenerator = {
   },
 
   setAutocommitQuery: function() {
-    return '-- SQLite does not support SET autocommit.';
+    // SQLite does not support SET autocommit
+    return null;
   },
 
   setIsolationLevelQuery: function(value) {

lib/sequelize.js

@@ -1249,7 +1249,7 @@ Sequelize.where = Sequelize.condition = Sequelize.prototype.condition = Sequeliz
  * @see {Transaction}
 
  * @param {Object}   [options={}]
- * @param {Boolean}  [options.autocommit=true]
+ * @param {Boolean}  [options.autocommit]
  * @param {String}   [options.type='DEFERRED'] See `Sequelize.Transaction.TYPES` for possible options. Sqlite only.
  * @param {String}   [options.isolationLevel] See `Sequelize.Transaction.ISOLATION_LEVELS` for possible options
  * @param {Function} [options.logging=false] A function that gets executed while running the query to log the sql.

lib/transaction.js

@@ -12,7 +12,7 @@ var Utils = require('./utils')
  *
  * @param {Sequelize} sequelize A configured sequelize Instance
  * @param {Object} options An object with options
- * @param {Boolean} options.autocommit=true Sets the autocommit property of the transaction.
+ * @param {Boolean} options.autocommit Sets the autocommit property of the transaction.
  * @param {String} options.type=true Sets the type of the transaction.
  * @param {String} options.isolationLevel=true Sets the isolation level of the transaction.
  * @param {String} options.deferrable Sets the constraints to be deferred or immediately checked.
@@ -20,8 +20,12 @@ var Utils = require('./utils')
 var Transaction = module.exports = function(sequelize, options) {
   this.sequelize = sequelize;
   this.savepoints = [];
+
+  // get dialect specific transaction options
+  var transactionOptions = sequelize.dialect.supports.transactionOptions || {};
+
   this.options = Utils._.extend({
-    autocommit: true,
+    autocommit: transactionOptions.autocommit || null,
     type: sequelize.options.transactionType,
     isolationLevel: sequelize.options.isolationLevel
   }, options || {});

test/unit/model/find-create-find.test.js

@@ -66,4 +66,4 @@ describe(Support.getTestDialectTeaser('Model'), function() {
       });
     });
   });
-});
\ No newline at end of file
+});

test/unit/transaction.test.js

+'use strict';
+
+/* jshint -W030 */
+var chai = require('chai')
+, expect = chai.expect
+, sinon = require('sinon')
+, Support = require(__dirname + '/support')
+, Sequelize = Support.Sequelize
+, dialect = Support.getTestDialect()
+, current = Support.sequelize
+, Promise = Sequelize.Promise;
+
+describe('Transaction', function() {
+
+  before(function () {
+    this.stub = sinon.stub(current, 'query').returns(Promise.resolve({}));
+
+    this.stubConnection = sinon.stub(current.connectionManager, 'getConnection')
+    .returns(Promise.resolve({ uuid: 'ssfdjd-434fd-43dfg23-2d', close : function() { }}));
+  });
+
+  beforeEach(function () {
+    this.stub.reset();
+    this.stubConnection.reset();
+  });
+
+  after(function () {
+    this.stub.restore();
+    this.stubConnection.restore();
+  });
+
+  it('should run auto commit query only when needed', function() {
+    var expectations = {
+      all: [
+        'START TRANSACTION;'
+      ],
+      mysql: [
+        'START TRANSACTION;',
+        'SET autocommit = 1;'
+      ],
+      sqlite: [
+        'BEGIN DEFERRED TRANSACTION;'
+      ],
+      mssql: [
+        'BEGIN TRANSACTION;'
+      ]
+    };
+    return current.transaction(() => {
+      expect(this.stub.args.map(arg => arg[0])).to.deep.equal(expectations[dialect] || expectations.all);
+      return Promise.resolve();
+    });
+  });
+});