fix(increment): broken queries (#11852)

lib/dialects/abstract/query-generator.js

@@ -407,20 +407,21 @@ class QueryGenerator {
   /**
    * Returns an update query using arithmetic operator
    *
-   * @param {string} operator      String with the arithmetic operator (e.g. '+' or '-')
-   * @param {string} tableName     Name of the table
-   * @param {Object} attrValueHash A hash with attribute-value-pairs
-   * @param {Object} where         A hash with conditions (e.g. {name: 'foo'}) OR an ID as integer
+   * @param {string} operator                    String with the arithmetic operator (e.g. '+' or '-')
+   * @param {string} tableName                   Name of the table
+   * @param {Object} where                       A plain-object with conditions (e.g. {name: 'foo'}) OR an ID as integer
+   * @param {Object} incrementAmountsByField     A plain-object with attribute-value-pairs
+   * @param {Object} extraAttributesToBeUpdated  A plain-object with attribute-value-pairs
    * @param {Object} options
-   * @param {Object} attributes
+   * 
+   * @private
    */
-  arithmeticQuery(operator, tableName, attrValueHash, where, options, attributes) {
+  arithmeticQuery(operator, tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options) {
     options = options || {};
     _.defaults(options, { returning: true });
 
-    attrValueHash = Utils.removeNullValuesFromHash(attrValueHash, this.options.omitNull);
+    extraAttributesToBeUpdated = Utils.removeNullValuesFromHash(extraAttributesToBeUpdated, this.options.omitNull);
 
-    const values = [];
     let outputFragment = '';
     let returningFragment = '';
 
@@ -431,18 +432,21 @@ class QueryGenerator {
       returningFragment = returnValues.returningFragment;
     }
 
-    for (const key in attrValueHash) {
-      const value = attrValueHash[key];
-      values.push(`${this.quoteIdentifier(key)}=${this.quoteIdentifier(key)}${operator} ${this.escape(value)}`);
+    const updateSetSqlFragments = [];
+    for (const field in incrementAmountsByField) {
+      const incrementAmount = incrementAmountsByField[field];
+      const quotedField = this.quoteIdentifier(field);
+      const escapedAmount = this.escape(incrementAmount);
+      updateSetSqlFragments.push(`${quotedField}=${quotedField}${operator} ${escapedAmount}`);
     }
-
-    attributes = attributes || {};
-    for (const key in attributes) {
-      const value = attributes[key];
-      values.push(`${this.quoteIdentifier(key)}=${this.escape(value)}`);
+    for (const field in extraAttributesToBeUpdated) {
+      const newValue = extraAttributesToBeUpdated[field];
+      const quotedField = this.quoteIdentifier(field);
+      const escapedValue = this.escape(newValue);
+      updateSetSqlFragments.push(`${quotedField}=${escapedValue}`);
     }
 
-    return `UPDATE ${this.quoteTable(tableName)} SET ${values.join(',')}${outputFragment} ${this.whereQuery(where)}${returningFragment}`.trim();
+    return `UPDATE ${this.quoteTable(tableName)} SET ${updateSetSqlFragments.join(',')}${outputFragment} ${this.whereQuery(where)}${returningFragment}`.trim();
   }
 
   /*

lib/model.js

@@ -3334,55 +3334,69 @@ class Model {
    */
   static increment(fields, options) {
     options = options || {};
+    if (typeof fields === 'string') fields = [fields];
+    if (Array.isArray(fields)) {
+      fields = fields.map(f => {
+        if (this.rawAttributes[f] && this.rawAttributes[f].field && this.rawAttributes[f].field !== f) {
+          return this.rawAttributes[f].field;
+        }
+        return f;
+      });
+    }
 
     this._injectScope(options);
     this._optionsMustContainWhere(options);
 
-    const updatedAtAttr = this._timestampAttributes.updatedAt;
-    const versionAttr = this._versionAttribute;
-    const updatedAtAttribute = this.rawAttributes[updatedAtAttr];
     options = Utils.defaults({}, options, {
       by: 1,
-      attributes: {},
       where: {},
       increment: true
     });
+    const isSubtraction = !options.increment;
 
     Utils.mapOptionFieldNames(options, this);
 
     const where = Object.assign({}, options.where);
-    let values = {};
 
-    if (typeof fields === 'string') {
-      values[fields] = options.by;
-    } else if (Array.isArray(fields)) {
-      fields.forEach(field => {
-        values[field] = options.by;
-      });
-    } else { // Assume fields is key-value pairs
-      values = fields;
+    // A plain object whose keys are the fields to be incremented and whose values are
+    // the amounts to be incremented by.
+    let incrementAmountsByField = {};
+    if (Array.isArray(fields)) {
+      incrementAmountsByField = {};
+      for (const field of fields) {
+        incrementAmountsByField[field] = options.by;
+      }
+    } else {
+      // If the `fields` argument is not an array, then we assume it already has the
+      // form necessary to be placed directly in the `incrementAmountsByField` variable.
+      incrementAmountsByField = fields;
     }
 
-    if (!options.silent && updatedAtAttr && !values[updatedAtAttr]) {
-      options.attributes[updatedAtAttribute.field || updatedAtAttr] = this._getDefaultTimestamp(updatedAtAttr) || Utils.now(this.sequelize.options.dialect);
-    }
-    if (versionAttr) {
-      values[versionAttr] = options.increment ? 1 : -1;
+    // If optimistic locking is enabled, we can take advantage that this is an
+    // increment/decrement operation and send it here as well. We put `-1` for
+    // decrementing because it will be subtracted, getting `-(-1)` which is `+1`
+    if (this._versionAttribute) {
+      incrementAmountsByField[this._versionAttribute] = isSubtraction ? -1 : 1;
     }
 
-    for (const attr of Object.keys(values)) {
-      // Field name mapping
-      if (this.rawAttributes[attr] && this.rawAttributes[attr].field && this.rawAttributes[attr].field !== attr) {
-        values[this.rawAttributes[attr].field] = values[attr];
-        delete values[attr];
-      }
+    const extraAttributesToBeUpdated = {};
+
+    const updatedAtAttr = this._timestampAttributes.updatedAt;
+    if (!options.silent && updatedAtAttr && !incrementAmountsByField[updatedAtAttr]) {
+      const attrName = this.rawAttributes[updatedAtAttr].field || updatedAtAttr;
+      extraAttributesToBeUpdated[attrName] = this._getDefaultTimestamp(updatedAtAttr) || Utils.now(this.sequelize.options.dialect);
     }
 
+    const tableName = this.getTableName(options);
     let promise;
-    if (!options.increment) {
-      promise = this.QueryInterface.decrement(this, this.getTableName(options), values, where, options);
+    if (isSubtraction) {
+      promise = this.QueryInterface.decrement(
+        this, tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options
+      );
     } else {
-      promise = this.QueryInterface.increment(this, this.getTableName(options), values, where, options);
+      promise = this.QueryInterface.increment(
+        this, tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options
+      );
     }
 
     return promise.then(affectedRows => {

lib/query-interface.js

@@ -1133,10 +1133,10 @@ class QueryInterface {
     );
   }
 
-  increment(model, tableName, values, identifier, options) {
+  increment(model, tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options) {
     options = Utils.cloneDeep(options);
 
-    const sql = this.QueryGenerator.arithmeticQuery('+', tableName, values, identifier, options, options.attributes);
+    const sql = this.QueryGenerator.arithmeticQuery('+', tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options);
 
     options.type = QueryTypes.UPDATE;
     options.model = model;
@@ -1144,10 +1144,10 @@ class QueryInterface {
     return this.sequelize.query(sql, options);
   }
 
-  decrement(model, tableName, values, identifier, options) {
+  decrement(model, tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options) {
     options = Utils.cloneDeep(options);
 
-    const sql = this.QueryGenerator.arithmeticQuery('-', tableName, values, identifier, options, options.attributes);
+    const sql = this.QueryGenerator.arithmeticQuery('-', tableName, where, incrementAmountsByField, extraAttributesToBeUpdated, options);
 
     options.type = QueryTypes.UPDATE;
     options.model = model;

test/integration/model/increment.test.js

@@ -53,7 +53,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
     describe(method, () => {
       before(function() {
         this.assert = (increment, decrement) => {
-          return method === 'increment'  ? increment : decrement;
+          return method === 'increment' ? increment : decrement;
         };
       });
 
@@ -231,6 +231,41 @@ describe(Support.getTestDialectTeaser('Model'), () => {
           expect(notJeff.aNumber).to.equal(this.assert(3, 3));
         });
       });
+
+      it('should not care for attributes in the instance scope', function() {
+        this.User.addScope('test', {
+          attributes: ['foo', 'bar']
+        });
+        return this.User.scope('test').create({ id: 5, aNumber: 5 })
+          .then(createdUser => createdUser[method]('aNumber', { by: 2 }))
+          .then(() => this.User.findByPk(5))
+          .then(user => {
+            expect(user.aNumber).to.equal(this.assert(7, 3));
+          });
+      });
+      it('should not care for exclude-attributes in the instance scope', function() {
+        this.User.addScope('test', {
+          attributes: { exclude: ['foo', 'bar'] }
+        });
+        return this.User.scope('test').create({ id: 5, aNumber: 5 })
+          .then(createdUser => createdUser[method]('aNumber', { by: 2 }))
+          .then(() => this.User.findByPk(5))
+          .then(user => {
+            expect(user.aNumber).to.equal(this.assert(7, 3));
+          });
+      });
+      it('should not care for include-attributes in the instance scope', function() {
+        this.User.addScope('test', {
+          attributes: { include: ['foo', 'bar'] }
+        });
+        return this.User.scope('test').create({ id: 5, aNumber: 5 })
+          .then(createdUser => createdUser[method]('aNumber', { by: 2 }))
+          .then(() => this.User.findByPk(5))
+          .then(user => {
+            expect(user.aNumber).to.equal(this.assert(7, 3));
+          });
+      });
+
     });
   });
 });

test/unit/dialects/mariadb/query-generator.test.js

@@ -82,27 +82,27 @@ if (dialect === 'mariadb') {
       arithmeticQuery: [
         {
           title: 'Should use the plus operator',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\''
         },
         {
           title: 'Should use the plus operator with where clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['+', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\' WHERE `bar` = \'biz\''
         },
         {
           title: 'Should use the minus operator',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\''
         },
         {
           title: 'Should use the minus operator with negative value',
-          arguments: ['-', 'myTable', { foo: -1 }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- -1'
         },
         {
           title: 'Should use the minus operator with where clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['-', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\' WHERE `bar` = \'biz\''
         }
       ],

test/unit/dialects/mssql/query-generator.test.js

@@ -260,37 +260,37 @@ if (current.dialect.name === 'mssql') {
       [
         {
           title: 'Should use the plus operator',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]+ N\'bar\' OUTPUT INSERTED.*'
         },
         {
           title: 'Should use the plus operator with where clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['+', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]+ N\'bar\' OUTPUT INSERTED.* WHERE [bar] = N\'biz\''
         },
         {
           title: 'Should use the plus operator without returning clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, { returning: false }],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, { returning: false }],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]+ N\'bar\''
         },
         {
           title: 'Should use the minus operator',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]- N\'bar\' OUTPUT INSERTED.*'
         },
         {
           title: 'Should use the minus operator with negative value',
-          arguments: ['-', 'myTable', { foo: -1 }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]- -1 OUTPUT INSERTED.*'
         },
         {
           title: 'Should use the minus operator with where clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['-', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]- N\'bar\' OUTPUT INSERTED.* WHERE [bar] = N\'biz\''
         },
         {
           title: 'Should use the minus operator without returning clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, { returning: false }],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, { returning: false }],
           expectation: 'UPDATE [myTable] SET [foo]=[foo]- N\'bar\''
         }
       ].forEach(test => {

test/unit/dialects/mysql/query-generator.test.js

@@ -39,27 +39,27 @@ if (dialect === 'mysql') {
       arithmeticQuery: [
         {
           title: 'Should use the plus operator',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\''
         },
         {
           title: 'Should use the plus operator with where clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['+', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\' WHERE `bar` = \'biz\''
         },
         {
           title: 'Should use the minus operator',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\''
         },
         {
           title: 'Should use the minus operator with negative value',
-          arguments: ['-', 'myTable', { foo: -1 }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- -1'
         },
         {
           title: 'Should use the minus operator with where clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['-', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\' WHERE `bar` = \'biz\''
         }
       ],

test/unit/dialects/postgres/query-generator.test.js

@@ -55,37 +55,37 @@ if (dialect.startsWith('postgres')) {
       arithmeticQuery: [
         {
           title: 'Should use the plus operator',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE "myTable" SET "foo"="foo"+ \'bar\'  RETURNING *'
         },
         {
           title: 'Should use the plus operator with where clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['+', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE "myTable" SET "foo"="foo"+ \'bar\' WHERE "bar" = \'biz\' RETURNING *'
         },
         {
           title: 'Should use the plus operator without returning clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}, { returning: false }],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, { returning: false }],
           expectation: 'UPDATE "myTable" SET "foo"="foo"+ \'bar\''
         },
         {
           title: 'Should use the minus operator',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE "myTable" SET "foo"="foo"- \'bar\'  RETURNING *'
         },
         {
           title: 'Should use the minus operator with negative value',
-          arguments: ['-', 'myTable', { foo: -1 }, {}, {}],
+          arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
           expectation: 'UPDATE "myTable" SET "foo"="foo"- -1  RETURNING *'
         },
         {
           title: 'Should use the minus operator with where clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, { bar: 'biz' }, {}],
+          arguments: ['-', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE "myTable" SET "foo"="foo"- \'bar\' WHERE "bar" = \'biz\' RETURNING *'
         },
         {
           title: 'Should use the minus operator without returning clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, {}, { returning: false }],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, { returning: false }],
           expectation: 'UPDATE "myTable" SET "foo"="foo"- \'bar\''
         }
       ],

test/unit/dialects/sqlite/query-generator.test.js

@@ -23,27 +23,27 @@ if (dialect === 'sqlite') {
       arithmeticQuery: [
         {
           title: 'Should use the plus operator',
-          arguments: ['+', 'myTable', { foo: 'bar' }, {}],
+          arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\''
         },
         {
           title: 'Should use the plus operator with where clause',
-          arguments: ['+', 'myTable', { foo: 'bar' }, { bar: 'biz' }],
+          arguments: ['+', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`+ \'bar\' WHERE `bar` = \'biz\''
         },
         {
           title: 'Should use the minus operator',
-          arguments: ['-', 'myTable', { foo: 'bar' }],
+          arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\''
         },
         {
           title: 'Should use the minus operator with negative value',
-          arguments: ['-', 'myTable', { foo: -1 }],
+          arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- -1'
         },
         {
           title: 'Should use the minus operator with where clause',
-          arguments: ['-', 'myTable', { foo: 'bar' }, { bar: 'biz' }],
+          arguments: ['-', 'myTable', { bar: 'biz' }, { foo: 'bar' }, {}, {}],
           expectation: 'UPDATE `myTable` SET `foo`=`foo`- \'bar\' WHERE `bar` = \'biz\''
         }
       ],