Merge pull request #3588 from sequelize/refactor-changed

Refactor changed

Merge pull request #3588 from sequelize/refactor-changed
Refactor changed
Jan Aagaard Meier
Commit 4b379702 authored Apr 26, 2015 by Jan Aagaard Meier
Showing with 152 additions and 58 deletions
changelog.md
lib/instance.js
test/integration/instance.test.js
test/integration/instance/values.test.js
test/unit/instance/changed.test.js
test/unit/instance/set.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -5,6 +5,7 @@
 - [FEATURE] [JSONB](https://github.com/sequelize/sequelize/issues/3471)
 - [FEATURE] All querys can be logged individually by inserting `logging: fn` in the query option.
 - [FEATURE] Partial index support for Postgres with `index.where`
+- [REFACTOR] `.changed()` now works proactively by setting a flag on `set` instead of matching reactively. Note that objects and arrays will not be checked for equality on set and will always result in a change if they are `set`.
 - [DEPRECATED] The query-chainer is deprecated and will be removed in version 2.2. Please use promises instead.
 - [REMOVED] Events are no longer supported.

--- a/lib/instance.js
+++ b/lib/instance.js
@@ -5,8 +5,10 @@ var Utils = require('./utils')
  , BelongsToMany = require('./associations/belongs-to-many')
  , InstanceValidator = require('./instance-validator')
  , QueryTypes = require('./query-types')
+  , Dottie = require('dottie')
  , Promise = require('./promise')
  , _ = require('lodash')
+  , primitives = ['string', 'number', 'boolean']
  , defaultsOptions = { raw: true }
  , deprecatedSeen = {}
  , deprecated = function(message) {
@@ -39,6 +41,7 @@ module.exports = (function() {
  var Instance = function(values, options) {
    this.dataValues = {};
    this._previousDataValues = {};
+    this._changed = {};
    this.__options = this.Model.options;
    this.options = options || {};
    this.hasPrimaryKeys = this.Model.options.hasPrimaryKeys;
@@ -165,6 +168,11 @@ module.exports = (function() {
   * @param {any} value
   */
  Instance.prototype.setDataValue = function(key, value) {
+    var originalValue = this._previousDataValues[key];
+    if (primitives.indexOf(typeof value) === -1 || value !== originalValue) {
+      this.changed(key, true);
+    }
    this.dataValues[key] = value;
  };
@@ -244,10 +252,13 @@ module.exports = (function() {
   * If set is called with an object, it will loop over the object, and call set recursively for each key, value pair. If you set raw to true, the underlying dataValues will either be
   * set directly to the object passed, or used to extend dataValues, if dataValues already contain values.
   *
-   * When set is called, the previous value of the field is stored, so that you can later see which fields changed (see `changed`).
+   * When set is called, the previous value of the field is stored and sets a changed flag(see `changed`).
   *
   * Set can also be used to build instances for associations, if you have values for those.
-   * TODO(mick): should probably write something here about how includes in set works - perhaps also even some tests?
+   * When using set with associations you need to make sure the property key matches the alias of the association
+   * while also making sure that the proper include options have been set (from .build() or .find())
+   *
+   * If called with a dot.seperated key on a JSON/JSONB attribute it will set the value nested and flag the entire object as changed.
   *
   * @see {Model#find} for more information about includes
   * @param {String|Object} key
@@ -255,7 +266,6 @@ module.exports = (function() {
   * @param {Object} [options]
   * @param {Boolean} [options.raw=false] If set to true, field and virtual setters will be ignored
   * @param {Boolean} [options.reset=false] Clear all previously set data values
-   * @param {Object}  [options.include]
   * @alias setAttributes
   */
  Instance.prototype.set = function(key, value, options) {
@@ -333,6 +343,10 @@ module.exports = (function() {
          if (!options.raw) {
            // If attribute is not in model definition, return
            if (!this._isAttribute(key)) {
+              if (key.indexOf('.') > -1 && this.Model._isJsonAttribute(key.split('.')[0])) {
+                Dottie.set(this.dataValues, key, value);
+                this.changed(key, true);
+              }
              return;
            }
@@ -369,8 +383,9 @@ module.exports = (function() {
            }
          }
-          if (!options.raw && originalValue !== value) {
+          if (!options.raw && (primitives.indexOf(typeof value) === -1 || value !== originalValue)) {
            this._previousDataValues[key] = originalValue;
+            this.changed(key, true);
          }
          this.dataValues[key] = value;
        }
@@ -394,13 +409,15 @@ module.exports = (function() {
   * @param {String} [key]
   * @return {Boolean|Array}
   */
-  Instance.prototype.changed = function(key) {
+  Instance.prototype.changed = function(key, value) {
    if (key) {
-      if (this.Model._isDateAttribute(key) && this._previousDataValues[key] && this.dataValues[key]) {
+      if (value !== undefined) {
-        return this._previousDataValues[key].valueOf() !== this.dataValues[key].valueOf();
+        this._changed[key] = value;
+        return this;
      }
-      return this._previousDataValues[key] !== this.dataValues[key];
+      return this._changed[key] || false;
    }
    var changed = Object.keys(this.dataValues).filter(function(key) {
      return this.changed(key);
    }.bind(this));
@@ -674,6 +691,7 @@ module.exports = (function() {
          .then(function(result) {
            options.fields.forEach(function (field) {
              result._previousDataValues[field] = result.dataValues[field];
+              self.changed(field, false);
            });
            self.isNewRecord = false;
            return result;
@@ -805,29 +823,26 @@ module.exports = (function() {
      force: false
    }, options || {});
-    var self = this;
+    return Promise.bind(this).then(function() {
-    // This semi awkward syntax where we can't return the chain directly but have to return the last .then() call is to allow sql proxying
-    return Promise.try(function() {
      // Run before hook
      if (options.hooks) {
-        return self.Model.runHooks('beforeDestroy', self, options);
+        return this.Model.runHooks('beforeDestroy', this, options);
      }
    }).then(function() {
      var where;
-      if (self.Model._timestampAttributes.deletedAt && options.force === false) {
+      if (this.Model._timestampAttributes.deletedAt && options.force === false) {
-        self.dataValues[self.Model._timestampAttributes.deletedAt] = new Date();
+        this.setDataValue(this.Model._timestampAttributes.deletedAt, new Date());
-        return self.save(_.extend(_.clone(options), {hooks : false}));
+        return this.save(_.extend(_.clone(options), {hooks : false}));
      } else {
        where = {};
-        where[self.Model.rawAttributes[self.Model.primaryKeyAttribute].field] = self.get(self.Model.primaryKeyAttribute, {raw: true});
+        where[this.Model.rawAttributes[this.Model.primaryKeyAttribute].field] = this.get(this.Model.primaryKeyAttribute, {raw: true});
-        return self.QueryInterface.delete(self, self.Model.getTableName(options), where, _.defaults(options, { type: QueryTypes.DELETE,limit: null}));
+        return this.QueryInterface.delete(this, this.Model.getTableName(options), where, _.defaults(options, { type: QueryTypes.DELETE,limit: null}));
      }
    }).tap(function() {
      // Run after hook
      if (options.hooks) {
-        return self.Model.runHooks('afterDestroy', self, options);
+        return this.Model.runHooks('afterDestroy', this, options);
      }
    }).then(function(result) {
      return result;
@@ -850,20 +865,18 @@ module.exports = (function() {
      force: false
    }, options || {});
-    var self = this;
+    return Promise.bind(this).then(function() {
-    return Promise.try(function() {
      // Run before hook
      if (options.hooks) {
-        return self.Model.runHooks('beforeRestore', self, options);
+        return this.Model.runHooks('beforeRestore', this, options);
      }
    }).then(function() {
-      self.dataValues[self.Model._timestampAttributes.deletedAt] = null;
+      this.setDataValue(this.Model._timestampAttributes.deletedAt, null); 
-      return self.save(_.extend(_.clone(options), {hooks : false, omitNull : false}));
+      return this.save(_.extend(_.clone(options), {hooks : false, omitNull : false}));
    }).tap(function() {
      // Run after hook
      if (options.hooks) {
-        return self.Model.runHooks('afterRestore', self, options);
+        return this.Model.runHooks('afterRestore', this, options);
      }
    });
  };

--- a/test/integration/instance.test.js
+++ b/test/integration/instance.test.js
@@ -141,21 +141,6 @@ describe(Support.getTestDialectTeaser('Instance'), function() {
      });
    });
-    it('returns false for non-changed date attribute', function() {
-      return this.User.create({ aDate: new Date(2013, 6, 31, 14, 25, 21) }).then(function(user) {
-        user.aDate = '2013-07-31 14:25:21';
-        expect(user.isDirty).to.be.false;
-      });
-    });
-    // In my opinion this is bad logic, null is different from an empty string
-    it.skip('returns false for two empty attributes', function() {
-      return this.User.create({ username: null }).then(function(user) {
-        user.username = '';
-        expect(user.isDirty).to.be.false;
-      });
-    });
    it('returns true for bulk changed attribute', function() {
      return this.User.create({ username: 'user' }).then(function(user) {
        user.setAttributes({
@@ -1805,8 +1790,7 @@ describe(Support.getTestDialectTeaser('Instance'), function() {
      }).then(function() {
        return ParanoidUser.find({where: {secretValue: '42'}});
      }).then(function(user) {
-        return user.destroy()
+        return user.destroy().then(function() {
-        .then(function() {
          return user.restore();
        });
      }).then(function() {

--- a/test/integration/instance/values.test.js
+++ b/test/integration/instance/values.test.js
@@ -427,21 +427,6 @@ describe(Support.getTestDialectTeaser('DAO'), function() {
        });
      });
-      it('setting the same value twice should not impact the result', function() {
-        var User = this.sequelize.define('User', {
-          name: {type: DataTypes.STRING}
-        });
-        var user = User.build({
-          name: 'Jan Meier'
-        });
-        user.set('name', 'Mick Hansen');
-        user.set('name', 'Mick Hansen');
-        expect(user.changed('name')).to.be.true;
-        expect(user.changed()).to.be.ok;
-        expect(user.isDirty).to.be.true;
-        expect(user.previous('name')).to.equal('Jan Meier');
-      });
      it('should be available to a afterUpdate hook', function () {
        var User = this.sequelize.define('User', {
          name: {type: DataTypes.STRING}

--- a/test/unit/instance/changed.test.js
+++ b/test/unit/instance/changed.test.js
+'use strict';
+/* jshint -W030 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support   = require(__dirname + '/../support')
+  , DataTypes = require(__dirname + '/../../../lib/data-types')
+  , current   = Support.sequelize;
+describe(Support.getTestDialectTeaser('Instance'), function() {
+  describe('changed', function () {
+    beforeEach(function () {
+      this.User = current.define('User', {
+        name: DataTypes.STRING,
+        birthdate: DataTypes.DATE,
+        meta: DataTypes.JSON
+      });
+    });
+    it('should return true for changed primitive', function () {
+      var user = this.User.build({
+        name: 'a'
+      }, {
+        isNewRecord: false,
+        raw: true
+      });
+      user.set('name', 'b');
+      expect(user.changed('name')).to.equal(true);
+    });
+    it('should return falsy for unchanged primitive', function () {
+      var user = this.User.build({
+        name: 'a'
+      }, {
+        isNewRecord: false,
+        raw: true
+      });
+      user.set('name', 'a');
+      expect(user.changed('name')).to.equal(false);
+    });
+    it('should return true for multiple changed values', function () {
+      var user = this.User.build({
+        name: 'a',
+        birthdate: new Date(new Date() - 10)
+      }, {
+        isNewRecord: false,
+        raw: true
+      });
+      user.set('name', 'b');
+      user.set('birthdate', new Date());
+      expect(user.changed('name')).to.equal(true);
+      expect(user.changed('birthdate')).to.equal(true);
+    });
+    it('should return true for changed JSON with same object', function () {
+      var user = this.User.build({
+        meta: {
+          city: 'Copenhagen'
+        }
+      }, {
+        isNewRecord: false,
+        raw: true
+      });
+      var meta = user.get('meta');
+      meta.city = 'Stockholm';
+      user.set('meta', meta);
+      expect(user.changed('meta')).to.equal(true);
+    });
+  });
+});
\ No newline at end of file
--- a/test/unit/instance/set.test.js
+++ b/test/unit/instance/set.test.js
+'use strict';
+/* jshint -W030 */
+var chai = require('chai')
+  , expect = chai.expect
+  , Support   = require(__dirname + '/../support')
+  , DataTypes = require(__dirname + '/../../../lib/data-types')
+  , current   = Support.sequelize;
+describe(Support.getTestDialectTeaser('Instance'), function() {
+  describe('set', function () {
+    it('sets nested keys in JSON objects', function () {
+      var User = current.define('User', {
+        meta: DataTypes.JSONB
+      });
+      var user = User.build({
+        meta: {
+          location: 'Stockhollm'
+        }
+      }, {
+        isNewRecord: false,
+        raw: true
+      });
+      var meta = user.get('meta');
+      user.set('meta.location', 'Copenhagen');
+      expect(user.dataValues['meta.location']).not.to.be.ok;
+      expect(user.get('meta').location).to.equal('Copenhagen');
+      expect(user.get('meta') === meta).to.equal(true);
+    });
+  });
+});
\ No newline at end of file