fix(model): make .changed() deep aware (#10851)

Simon Schick · Sushant
Commit 29c9be37 authored Oct 19, 2019 by Simon Schick Committed by Sushant Oct 19, 2019
Showing with 51 additions and 36 deletions
lib/model.js
test/unit/instance/changed.test.js
--- a/lib/model.js
+++ b/lib/model.js
@@ -104,7 +104,7 @@ class Model {
    this.dataValues = {};
    this._previousDataValues = {};
-    this._changed = {};
+    this._changed = new Set();
    this._modelOptions = this.constructor.options;
    this._options = options || {};
@@ -160,12 +160,10 @@ class Model {
        delete defaults[this.constructor._timestampAttributes.deletedAt];
      }
-      if (Object.keys(defaults).length) {
+      for (key in defaults) {
-        for (key in defaults) {
+        if (values[key] === undefined) {
-          if (values[key] === undefined) {
+          this.set(key, Utils.toDefaultValue(defaults[key], this.sequelize.options.dialect), { raw: true });
-            this.set(key, Utils.toDefaultValue(defaults[key], this.sequelize.options.dialect), { raw: true });
+          delete values[key];
-            delete values[key];
-          }
        }
      }
    }
@@ -2450,8 +2448,9 @@ class Model {
    const hasPrimary = this.primaryKeyField in values || this.primaryKeyAttribute in values;
    const instance = this.build(values);
+    const changed = Array.from(instance._changed);
    if (!options.fields) {
-      options.fields = Object.keys(instance._changed);
+      options.fields = changed;
    }
    return Promise.try(() => {
@@ -2460,7 +2459,7 @@ class Model {
      }
    }).then(() => {
      // Map field names
-      const updatedDataValues = _.pick(instance.dataValues, Object.keys(instance._changed));
+      const updatedDataValues = _.pick(instance.dataValues, changed);
      const insertValues = Utils.mapValueFieldNames(instance.dataValues, Object.keys(instance.rawAttributes), this);
      const updateValues = Utils.mapValueFieldNames(updatedDataValues, options.fields, this);
      const now = Utils.now(this.sequelize.options.dialect);
@@ -3477,7 +3476,7 @@ class Model {
  setDataValue(key, value) {
    const originalValue = this._previousDataValues[key];
-    if (!Utils.isPrimitive(value) && value !== null || value !== originalValue) {
+    if (!_.isEqual(value, originalValue)) {
      this.changed(key, true);
    }
@@ -3654,7 +3653,7 @@ class Model {
      // custom setter should have changed value, get that changed value
      // TODO: v5 make setters return new value instead of changing internal store
      const newValue = this.dataValues[key];
-      if (!Utils.isPrimitive(newValue) && newValue !== null || newValue !== originalValue) {
+      if (!_.isEqual(newValue, originalValue)) {
        this._previousDataValues[key] = originalValue;
        this.changed(key, true);
      }
@@ -3707,7 +3706,7 @@ class Model {
          // Check for data type type comparators
          !(value instanceof Utils.SequelizeMethod) && this.constructor._dataTypeChanges[key] && this.constructor._dataTypeChanges[key].call(this, value, originalValue, options) ||
          // Check default
-          !this.constructor._dataTypeChanges[key] && (!Utils.isPrimitive(value) && value !== null || value !== originalValue)
+          !this.constructor._dataTypeChanges[key] && !_.isEqual(value, originalValue)
        )
      ) {
        this._previousDataValues[key] = originalValue;
@@ -3731,23 +3730,42 @@ class Model {
   *
   * If changed is called without an argument and no keys have changed, it will return `false`.
   *
+   * Please note that this function will return `false` when a property from a nested (for example JSON) property
+   * was edited manually, you must call `changed('key', true)` manually in these cases.
+   * Writing an entirely new object (eg. deep cloned) will be detected.
+   *
+   * @example
+   * ```
+   * const mdl = await MyModel.findOne();
+   * mdl.myJsonField.a = 1;
+   * console.log(mdl.changed()) => false
+   * mdl.save(); // this will not save anything
+   * mdl.changed('myJsonField', true);
+   * console.log(mdl.changed()) => ['myJsonField']
+   * mdl.save(); // will save
+   * ```
+   *
   * @param {string} [key] key to check or change status of
   * @param {any} [value] value to set
   *
   * @returns {boolean|Array}
   */
  changed(key, value) {
-    if (key) {
+    if (key === undefined) {
-      if (value !== undefined) {
+      if (this._changed.size > 0) {
-        this._changed[key] = value;
+        return Array.from(this._changed);
-        return this;
      }
-      return this._changed[key] || false;
+      return false;
    }
+    if (value === true) {
-    const changed = Object.keys(this.dataValues).filter(key => this.changed(key));
+      this._changed.add(key);
+      return this;
-    return changed.length ? changed : false;
+    }
+    if (value === false) {
+      this._changed.delete(key);
+      return this;
+    }
+    return this._changed.has(key);
  }
  /**

--- a/test/unit/instance/changed.test.js
+++ b/test/unit/instance/changed.test.js
@@ -78,21 +78,18 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
      expect(user.changed('birthday')).to.equal(false);
    });
-    it('should return true for changed JSON with same object', function() {
+    it('should not detect changes when equal', function() {
-      const user = this.User.build({
+      for (const value of [null, 1, 'asdf', new Date(), [], {}, Buffer.from('')]) {
-        meta: {
+        const t = new this.User({
-          city: 'Copenhagen'
+          json: value
-        }
+        }, {
-      }, {
+          isNewRecord: false,
-        isNewRecord: false,
+          raw: true
-        raw: true
+        });
-      });
+        t.json = value;
+        expect(t.changed('json')).to.be.false;
-      const meta = user.get('meta');
+        expect(t.changed()).to.be.false;
-      meta.city = 'Stockholm';
+      }
-      user.set('meta', meta);
-      expect(user.changed('meta')).to.equal(true);
    });
    it('should return true for JSON dot.separated key with changed values', function() {