Run hooks during validation by default (#7182)

* run hooks during validation by default

* Revert "run hooks during validation by default"

This reverts commit da66b72bce4bc274f10745ec98cad86ccb1def20.

* hooks by default

* write validation sequence more cleanly and migrate instance validator integration tests to unit tests

* remove only

* ignore w30

* change log

* fix jshint error and use const

* jshint fix

* only validationFailed  hook when validation fails

* fix changelog

* fix changelog

* add integration tests back

* fix indentation

* and fix spacing

* final code review changes

changelog.md

@@ -61,8 +61,10 @@
 - [FIXED] `removeAttribute('id')` results in `undefined: null` data value [#7318](https://github.com/sequelize/sequelize/issues/7318)
 - [FIXED] `bulkCreate` now runs in O(N) time instead of O(N^2) time. [#4247](https://github.com/sequelize/sequelize/issues/4247)
 - [FIXED] Passing parameters to model getters [#7404](https://github.com/sequelize/sequelize/issues/7404)
+- [FIXED] Model.validate runs validation hooks by default [#7182](https://github.com/sequelize/sequelize/pull/7182)
 
 ## BC breaks:
+- Model.validate instance method now runs validation hooks by default. Previously you needed to pass { hooks: true }. You can override this behavior by passing { hooks: false }
 - `DATEONLY` now returns string in `YYYY-MM-DD` format rather than `Date` type
 - With `BelongsToMany` relationships `add/set/create` setters now set `through` attributes by passing them as `options.through` (previously second argument was used as `through` attributes, now its considered `options` with `through` being a sub option)
 - `options.order` Now only excepts values with type of array or Sequelize method.  Support for string values (ie `{order: 'name DESC'}`) has been deprecated.

lib/instance-validator.js

@@ -27,7 +27,8 @@ class InstanceValidator {
 
     // assign defined and default options
     this.options = Utils._.defaults(options, {
-      skip: []
+      skip: [],
+      hooks: true
     });
 
     this.modelInstance = modelInstance;
@@ -79,7 +80,7 @@ class InstanceValidator {
   }
 
   /**
-   * Invoke the Validation sequence:
+   * Invoke the Validation sequence and run validation hooks if defined
    *   - Before Validation Model Hooks
    *   - Validation
    *   - On validation success: After Validation Model Hooks
@@ -89,20 +90,30 @@ class InstanceValidator {
    * @private
    */
   validate() {
-    if (this.options.hooks) {
-      return this.modelInstance.constructor.runHooks('beforeValidate', this.modelInstance, this.options)
+    return this.options.hooks ? this._validateAndRunHooks() : this._validate();
+  }
+
+  /**
+   * Invoke the Validation sequence and run hooks
+   *   - Before Validation Model Hooks
+   *   - Validation
+   *   - On validation success: After Validation Model Hooks
+   *   - On validation failure: Validation Failed Model Hooks
+   *
+   * @return {Promise}
+   * @private
+   */
+  _validateAndRunHooks() {
+    const runHooks = this.modelInstance.constructor.runHooks.bind(this.modelInstance.constructor);
+    return runHooks('beforeValidate', this.modelInstance, this.options)
       .then(() =>
-          this._validate().catch(error =>
-            this.modelInstance.constructor.runHooks('validationFailed', this.modelInstance, this.options, error).then(newError => {
-              throw newError || error;
-            })
-          )
+        this._validate()
+        .catch(error => runHooks('validationFailed', this.modelInstance, this.options, error)
+          .then(newError => { throw newError || error; }))
       )
-        .then(() => this.modelInstance.constructor.runHooks('afterValidate', this.modelInstance, this.options))
+      .then(() => runHooks('afterValidate', this.modelInstance, this.options))
       .return(this.modelInstance);
   }
-    return this._validate();
-  }
 
   /**
    * Will run all the built-in validators.

test/integration/hooks/validate.test.js

 'use strict';
 
 /* jshint -W030 */
-var chai = require('chai')
-  , expect = chai.expect
-  , Support = require(__dirname + '/../support')
-  , DataTypes = require(__dirname + '/../../../lib/data-types')
-  , sinon = require('sinon');
+const chai = require('chai');
+const sinon = require('sinon');
+const expect = chai.expect;
+const Support = require(__dirname + '/../support');
+const DataTypes = require(__dirname + '/../../../lib/data-types');
 
 describe(Support.getTestDialectTeaser('Hooks'), function() {
   beforeEach(function() {
@@ -150,5 +150,4 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
       });
     });
   });
-
 });

test/unit/instance-validator.test.js

+'use strict';
+
+/* jshint -W030 */
+const chai = require('chai');
+const expect = chai.expect;
+const Support = require(__dirname + '/support');
+const InstanceValidator = require('../../lib/instance-validator');
+const sinon = require('sinon');
+var Promise = Support.sequelize.Promise;
+const SequelizeValidationError = require('../../lib/errors').ValidationError;
+
+describe(Support.getTestDialectTeaser('InstanceValidator'), () => {
+  beforeEach(() => {
+    this.User = Support.sequelize.define('user');
+  });
+
+  it('configures itself to run hooks by default', () => {
+    const instanceValidator = new InstanceValidator();
+    expect(instanceValidator.options.hooks).to.equal(true);
+  });
+
+  describe('validate', () => {
+    it('runs the validation sequence and hooks when the hooks option is true', () => {
+      const instanceValidator = new InstanceValidator(this.User.build(), {hooks: true});
+      const _validate = sinon.spy(instanceValidator, '_validate');
+      const _validateAndRunHooks = sinon.spy(instanceValidator, '_validateAndRunHooks');
+
+      instanceValidator.validate();
+
+      expect(_validateAndRunHooks).to.have.been.calledOnce;
+      expect(_validate).to.not.have.been.called;
+    });
+
+    it('runs the validation sequence but skips hooks if the hooks option is false', () => {
+      const instanceValidator = new InstanceValidator(this.User.build(), {hooks: false});
+      const _validate = sinon.spy(instanceValidator, '_validate');
+      const _validateAndRunHooks = sinon.spy(instanceValidator, '_validateAndRunHooks');
+
+      instanceValidator.validate();
+
+      expect(_validate).to.have.been.calledOnce;
+      expect(_validateAndRunHooks).to.not.have.been.called;
+    });
+  });
+
+  describe('_validateAndRunHooks', () => {
+    beforeEach(() => {
+      this.successfulInstanceValidator = new InstanceValidator(this.User.build());
+      sinon.stub(this.successfulInstanceValidator, '_validate').returns(Promise.resolve());
+    });
+
+    it('should run beforeValidate and afterValidate hooks when _validate is successful', () => {
+      const beforeValidate = sinon.spy();
+      const afterValidate = sinon.spy();
+      this.User.beforeValidate(beforeValidate);
+      this.User.afterValidate(afterValidate);
+
+      return expect(this.successfulInstanceValidator._validateAndRunHooks()).to.be.fulfilled.then(() => {
+        expect(beforeValidate).to.have.been.calledOnce;
+        expect(afterValidate).to.have.been.calledOnce;
+      });
+    });
+
+    it('should run beforeValidate hook but not afterValidate hook when _validate is unsuccessful', () => {
+      const failingInstanceValidator = new InstanceValidator(this.User.build());
+      sinon.stub(failingInstanceValidator, '_validate').returns(Promise.reject());
+      const beforeValidate = sinon.spy();
+      const afterValidate = sinon.spy();
+      this.User.beforeValidate(beforeValidate);
+      this.User.afterValidate(afterValidate);
+
+      return expect(failingInstanceValidator._validateAndRunHooks()).to.be.rejected.then(() => {
+        expect(beforeValidate).to.have.been.calledOnce;
+        expect(afterValidate).to.not.have.been.called;
+      });
+    });
+
+    it('should emit an error from after hook when afterValidate fails', () => {
+      this.User.afterValidate(() => {
+        throw new Error('after validation error');
+      });
+
+      return expect(this.successfulInstanceValidator._validateAndRunHooks()).to.be.rejectedWith('after validation error');
+    });
+
+    describe('validatedFailed hook', () => {
+      it('should call validationFailed hook when validation fails', () => {
+        const failingInstanceValidator = new InstanceValidator(this.User.build());
+        sinon.stub(failingInstanceValidator, '_validate').returns(Promise.reject());
+        const validationFailedHook = sinon.spy();
+        this.User.validationFailed(validationFailedHook);
+
+        return expect(failingInstanceValidator._validateAndRunHooks()).to.be.rejected.then((err) => {
+          expect(validationFailedHook).to.have.been.calledOnce;
+        });
+      });
+
+      it('should not replace the validation error in validationFailed hook by default', () => {
+        const failingInstanceValidator = new InstanceValidator(this.User.build());
+        sinon.stub(failingInstanceValidator, '_validate').returns(Promise.reject(new SequelizeValidationError()));
+        const validationFailedHook = sinon.stub().returns(Promise.resolve());
+        this.User.validationFailed(validationFailedHook);
+
+        return expect(failingInstanceValidator._validateAndRunHooks()).to.be.rejected.then((err) => {
+          expect(err.name).to.equal('SequelizeValidationError');
+        });
+      });
+
+      it('should replace the validation error if validationFailed hook creates a new error', () => {
+        const failingInstanceValidator = new InstanceValidator(this.User.build());
+        sinon.stub(failingInstanceValidator, '_validate').returns(Promise.reject(new SequelizeValidationError()));
+        const validationFailedHook = sinon.stub().throws(new Error('validation failed hook error'));
+        this.User.validationFailed(validationFailedHook);
+
+        return expect(failingInstanceValidator._validateAndRunHooks()).to.be.rejected.then((err) => {
+          expect(err.message).to.equal('validation failed hook error');
+        });
+      });
+    });
+  });
+});