fix(instance): update() using undefined/fields will now also validate values changed in hooks

changelog.md

@@ -4,13 +4,13 @@
 - [FEATURE] add `sequelize.escape(value)` convenience method
 - [FEATURE] add `sequelize.escape(value)` convenience method
 - [BUG] Fixes crash with `findAll({include: [Model], order: sequelize.literal()})`
 - [BUG] Fixes crash with `findAll({include: [Model], order: sequelize.literal()})`
 - [FEATURE] Now possible to pass `createdAt` and `updatedAt` values to `Model.create`/`Model.bulkCreate` when using silent: true (when importing datasets with existing timestamps)
 - [FEATURE] Now possible to pass `createdAt` and `updatedAt` values to `Model.create`/`Model.bulkCreate` when using silent: true (when importing datasets with existing timestamps)
-- [FEATURE] `instance.update()` using default fields will now automatically also save values provided via `beforeUpdate` hooks
+- [FEATURE] `instance.update()` using default fields will now automatically also save and validate values provided via `beforeUpdate` hooks
 - [BUG] Fixed bad SQL when updating a JSON attribute with a different `field`
 - [BUG] Fixed bad SQL when updating a JSON attribute with a different `field`
 - [BUG] Fixed issue with creating and updating values of a `DataTypes.ARRAY(DataTypes.JSON)` attribute
 - [BUG] Fixed issue with creating and updating values of a `DataTypes.ARRAY(DataTypes.JSON)` attribute
 - [BUG] `Model.bulkCreate([{}], {returning: true})` will now correctly result in instances with primary key values.
 - [BUG] `Model.bulkCreate([{}], {returning: true})` will now correctly result in instances with primary key values.
 
 
 #### Backwards compatability changes
 #### Backwards compatability changes
-- `instance.update()` using default fields will now automatically also save values provided via `beforeUpdate` hooks
+- `instance.update()` using default fields will now automatically also save and validate values provided via `beforeUpdate` hooks
 
 
 
 
 # 2.0.0-rc6
 # 2.0.0-rc6

lib/instance.js

@@ -584,9 +584,10 @@ module.exports = (function() {
       return Promise.bind(this).then(function() {
       return Promise.bind(this).then(function() {
         // Run before hook
         // Run before hook
         if (options.hooks) {
         if (options.hooks) {
-          var hookFields
-            , ignoreChanged = _.difference(this.changed(), options.fields) // In case of update where it's only supposed to update the passed values and the hook values
-            , fieldsHadUpdatedAtAttr = updatedAtAttr && options.fields.indexOf(updatedAtAttr) !== -1; 
+          var beforeHookValues = _.pick(this.dataValues, options.fields)
+            , afterHookValues
+            , hookChanged
+            , ignoreChanged = _.difference(this.changed(), options.fields); // In case of update where it's only supposed to update the passed values and the hook values
 
 
           if (updatedAtAttr && options.fields.indexOf(updatedAtAttr) !== -1) {
           if (updatedAtAttr && options.fields.indexOf(updatedAtAttr) !== -1) {
             ignoreChanged = _.without(ignoreChanged, updatedAtAttr);
             ignoreChanged = _.without(ignoreChanged, updatedAtAttr);
@@ -594,14 +595,16 @@ module.exports = (function() {
 
 
           return this.Model.runHooks('before' + hook, this, options).bind(this).then(function() {
           return this.Model.runHooks('before' + hook, this, options).bind(this).then(function() {
             if (options.defaultFields && !this.isNewRecord) {
             if (options.defaultFields && !this.isNewRecord) {
+              afterHookValues = _.pick(this.dataValues, _.difference(this.changed(), ignoreChanged));
 
 
-              //options.fields = _.without.apply(_, [_.intersection(this.changed(), Object.keys(this.Model.attributes))].concat(ignoreChanged));
-              options.fields = _.difference(_.intersection(this.changed(), Object.keys(this.Model.attributes)), ignoreChanged);
-              if (fieldsHadUpdatedAtAttr && options.fields.indexOf(updatedAtAttr) === -1) {
-                options.fields.push(updatedAtAttr);
-              }
+              hookChanged = [];
+              Object.keys(afterHookValues).forEach(function (key) {
+                if (afterHookValues[key] !== beforeHookValues[key]) {
+                  hookChanged.push(key);
+                }
+              });
 
 
-              hookFields = _.difference(options.fields, options.defaultFields);
+              options.fields = _.unique(options.fields.concat(hookChanged));
             }
             }
 
 
             // dataValues might have changed inside the hook, rebuild the values hash
             // dataValues might have changed inside the hook, rebuild the values hash
@@ -613,13 +616,25 @@ module.exports = (function() {
               }
               }
 
 
               // Field name mapping
               // Field name mapping
-              if (self.Model.rawAttributes[attr] && self.Model.rawAttributes[attr].field && self.Model.rawAttributes[attr].field !== attr) {
-                values[self.Model.rawAttributes[attr].field] = values[attr];
+              if (this.Model.rawAttributes[attr] && this.Model.rawAttributes[attr].field && this.Model.rawAttributes[attr].field !== attr) {
+                values[this.Model.rawAttributes[attr].field] = values[attr];
                 delete values[attr];
                 delete values[attr];
               }
               }
             }, this);
             }, this);
 
 
             args[2] = values;
             args[2] = values;
+
+            if (hookChanged) {
+              return Promise.bind(this).then(function() {
+                // Validate again
+                if (options.validate) {
+                  options.skip = _.difference(Object.keys(this.Model.rawAttributes), hookChanged);
+                  return (options.hooks ? this.hookValidate(options) : this.validate(options)).then(function() {
+                    delete options.skip;
+                  });
+                }
+              });
+            }
           });
           });
         }
         }
       }).then(function() {
       }).then(function() {

test/instance/update.test.js

@@ -111,33 +111,68 @@ describe(Support.getTestDialectTeaser('Instance'), function() {
       });
       });
     });
     });
 
 
-    it('should update attributes added in hooks when default fields are used', function () {
-      var User = this.sequelize.define('User' + config.rand(), {
-        name: DataTypes.STRING,
-        bio: DataTypes.TEXT,
-        email: DataTypes.STRING
-      });
+    describe('hooks', function () {
+      it('should update attributes added in hooks when default fields are used', function () {
+        var User = this.sequelize.define('User' + config.rand(), {
+          name: DataTypes.STRING,
+          bio: DataTypes.TEXT,
+          email: DataTypes.STRING
+        });
+
+        User.beforeUpdate(function(instance, options) {
+          instance.set('email', 'B');
+        });
 
 
-      User.beforeUpdate(function(instance, options) {
-        instance.set('email', 'B');
+        return User.sync({force: true}).then(function() {
+          return User.create({
+            name: 'A',
+            bio: 'A',
+            email: 'A'
+          }).then(function (user) {
+            return user.update({
+              name: 'B',
+              bio: 'B'
+            });
+          }).then(function () {
+            return User.findOne({});
+          }).then(function (user) {
+            expect(user.get('name')).to.equal('B');
+            expect(user.get('bio')).to.equal('B');
+            expect(user.get('email')).to.equal('B');
+          });
+        });
       });
       });
 
 
-      return User.sync({force: true}).then(function() {
-        return User.create({
-          name: 'A',
-          bio: 'A',
-          email: 'A'
-        }).then(function (user) {
-          return user.update({
-            name: 'B',
-            bio: 'B'
+      it('should validate attributes added in hooks when default fields are used', function () {
+        var User = this.sequelize.define('User' + config.rand(), {
+          name: DataTypes.STRING,
+          bio: DataTypes.TEXT,
+          email: {
+            type: DataTypes.STRING,
+            validate: {
+              isEmail: true
+            }
+          }
+        });
+
+        User.beforeUpdate(function(instance, options) {
+          instance.set('email', 'B');
+        });
+
+        return User.sync({force: true}).then(function () {
+          return User.create({
+            name: 'A',
+            bio: 'A',
+            email: 'valid.email@gmail.com'
+          }).then(function (user) {
+            return expect(user.update({
+              name: 'B'
+            })).to.be.rejectedWith(Sequelize.ValidationError);
+          }).then(function () {
+            return User.findOne({}).then(function (user) {
+              expect(user.get('email')).to.equal('valid.email@gmail.com');
+            });
           });
           });
-        }).then(function () {
-          return User.findOne({});
-        }).then(function (user) {
-          expect(user.get('name')).to.equal('B');
-          expect(user.get('bio')).to.equal('B');
-          expect(user.get('email')).to.equal('B');
         });
         });
       });
       });
     });
     });