Merge pull request #2240 from overlookmotel/hooks-options

Options passed to hooks

Merge pull request #2240 from overlookmotel/hooks-options
Options passed to hooks
Mick Hansen
Commit 03f49f65 authored Sep 20, 2014 by Mick Hansen
Showing with 67 additions and 59 deletions
changelog.md
lib/hooks.js
lib/instance-validator.js
lib/instance.js
lib/model.js
test/dao-factory/find.test.js
test/dao-factory/findAll.test.js
test/hooks.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -5,10 +5,15 @@
 - [FEATURE] Added `scope` to hasMany association definitions, provides default values to association setters/finders [#2268](https://github.com/sequelize/sequelize/pull/2268)
 - [FEATURE] We now support transactions that automatically commit/rollback based on the result of the promise chain returned to the callback.
 - [BUG] Only try to create indexes which don't already exist. Closes [#2162](https://github.com/sequelize/sequelize/issues/2162)
+- [FEATURE] Hooks are passed options
+- [FEATURE] Hooks need not return a result - undefined return is interpreted as a resolved promise
+- [FEATURE] Added `find()` hooks
 #### Backwards compatability changes
 - The `fieldName` property, used in associations with a foreign key object `(A.hasMany(B, { foreignKey: { ... }})`, has been renamed to `name` to avoid confusion with `field`. 
 - The naming of the join table entry for N:M association getters is now singular (like includes)
+- Signature of hooks has changed to pass options to all hooks
+- Results returned by hooks are ignored - changes to results by hooks should be made by reference
 # v2.0.0-dev13
 We are working our way to the first 2.0.0 release candidate.
@@ -18,9 +23,9 @@ We are working our way to the first 2.0.0 release candidate.
 - [FEATURE] Added support for passing an `indexes` array in options to `sequelize.define`. [#1485](https://github.com/sequelize/sequelize/issues/1485). See API reference for details.
 - [FEATURE/INTERNALS] Standardized the output from `QueryInterface.showIndex`.
 - [FEATURE] Include deleted rows in find [#2083](https://github.com/sequelize/sequelize/pull/2083)
- [FEATURE] Make addSingular and addPlural for n:m assocations (fx `addUser` and `addUsers` now both accept an array or an instance.
+- [FEATURE] Make addSingular and addPlural for n:m associations (fx `addUser` and `addUsers` now both accept an array or an instance.
 - [BUG] Hid `dottie.transform` on raw queries behind a flag (`nest`) [#2064](https://github.com/sequelize/sequelize/pull/2064)
- [BUG] Fixed problems with transcation parameter being removed / not passed on in associations [#1789](https://github.com/sequelize/sequelize/issues/1789) and [#1968](https://github.com/sequelize/sequelize/issues/1968)
+- [BUG] Fixed problems with transaction parameter being removed / not passed on in associations [#1789](https://github.com/sequelize/sequelize/issues/1789) and [#1968](https://github.com/sequelize/sequelize/issues/1968)
 - [BUG] Fix problem with minConnections. [#2048](https://github.com/sequelize/sequelize/issues/2048)
 - [BUG] Fix default scope being overwritten [#2087](https://github.com/sequelize/sequelize/issues/2087)
 - [BUG] Fixed updatedAt timestamp not being set in bulk create when validate = true. [#1962](https://github.com/sequelize/sequelize/issues/1962)

--- a/lib/hooks.js
+++ b/lib/hooks.js
--- a/lib/instance-validator.js
+++ b/lib/instance-validator.js
@@ -165,14 +165,14 @@ InstanceValidator.prototype.validate = function() {
 */
 InstanceValidator.prototype.hookValidate = function() {
  var self = this;
-  return self.modelInstance.Model.runHooks('beforeValidate', self.modelInstance).then(function() {
+  return self.modelInstance.Model.runHooks('beforeValidate', self.modelInstance, self.options).then(function() {
    return self.validate().then(function(error) {
      if (error) {
        throw error;
      }
    });
  }).then(function() {
-    return self.modelInstance.Model.runHooks('afterValidate', self.modelInstance);
+    return self.modelInstance.Model.runHooks('afterValidate', self.modelInstance, self.options);
  }).return(self.modelInstance);
 };

--- a/lib/instance.js
+++ b/lib/instance.js
@@ -495,7 +495,10 @@ module.exports = (function() {
    return Promise.try(function() {
      // Validate
      if (options.hooks) {
-        return self.hookValidate({skip: _.difference(Object.keys(self.rawAttributes), options.fields)});
+        options.skip = _.difference(Object.keys(self.rawAttributes), options.fields);
+        return self.hookValidate(options).then(function() {
+          delete options.skip;
+        });
      }
    }).then(function() {
      options.fields.forEach(function(field) {
@@ -584,7 +587,7 @@ module.exports = (function() {
      return Promise.try(function() {
        // Run before hook
        if (options.hooks) {
-          return self.Model.runHooks('before' + hook, self).then(function() {
+          return self.Model.runHooks('before' + hook, self, options).then(function() {
            // dataValues might have changed inside the hook, rebuild the values hash
            values = {};
@@ -634,7 +637,7 @@ module.exports = (function() {
        }).tap(function(result) {
          // Run after hook
          if (options.hooks) {
-            return self.Model.runHooks('after' + hook, result);
+            return self.Model.runHooks('after' + hook, result, options);
          }
        }).then(function(result) {
          return result;
@@ -683,8 +686,8 @@ module.exports = (function() {
    return new InstanceValidator(this, options).validate();
  };
-  Instance.prototype.hookValidate = function(object) {
+  Instance.prototype.hookValidate = function(options) {
-    var validator = new InstanceValidator(this, object);
+    var validator = new InstanceValidator(this, options);
    return validator.hookValidate();
  };
@@ -732,7 +735,7 @@ module.exports = (function() {
    return Promise.try(function() {
      // Run before hook
      if (options.hooks) {
-        return self.Model.runHooks('beforeDestroy', self);
+        return self.Model.runHooks('beforeDestroy', self, options);
      }
    }).then(function() {
      var identifier;
@@ -747,7 +750,7 @@ module.exports = (function() {
    }).tap(function(result) {
      // Run after hook
      if (options.hooks) {
-        return self.Model.runHooks('afterDestroy', self);
+        return self.Model.runHooks('afterDestroy', self, options);
      }
    }).then(function(result) {
      return result;

--- a/lib/model.js
+++ b/lib/model.js
--- a/test/dao-factory/find.test.js
+++ b/test/dao-factory/find.test.js
@@ -332,19 +332,19 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        describe('generic', function() {
          it('throws an error about unexpected input if include contains a non-object', function(done) {
            var self = this
-            expect(function() {
+            self.Worker.find({ include: [ 1 ] }).catch(function(err) {
-              self.Worker.find({ include: [ 1 ] })
+              expect(err.message).to.equal('Include unexpected. Element has to be either a Model, an Association or an object.');
-            }).to.throw(Error)
              done()
            })
+          })
          it('throws an error if included DaoFactory is not associated', function(done) {
            var self = this
-            expect(function() {
+            self.Worker.find({ include: [ self.Task ] }).catch(function(err) {
-              self.Worker.find({ include: [ self.Task ] })
+              expect(err.message).to.equal('Task is not associated to Worker!');
-            }).to.throw(Error, 'Task is not associated to Worker!')
              done()
            })
+          })
          it('returns the associated worker via task.worker', function(done) {
            var self = this
@@ -518,11 +518,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Task.find({ include: [ self.Worker ] }).catch(function(err) {
-            self.Task.find({ include: [ self.Worker ] })
+            expect(err.message).to.equal('Worker is not associated to Task!');
-          }).to.throw(Error, 'Worker is not associated to Task!')
            done()
          })
+        })
        it('returns the associated task via worker.task', function(done) {
          this.Worker.find({
@@ -577,11 +577,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      describe('hasOne with alias', function() {
        it('throws an error if included DaoFactory is not referenced by alias', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.find({ include: [ self.Task ] }).catch(function(err) {
-            self.Worker.find({ include: [ self.Task ] })
+            expect(err.message).to.equal('Task is not associated to Worker!');
-          }).to.throw(Error, 'Task is not associated to Worker!')
            done()
          })
+        })
        describe('alias', function() {
          beforeEach(function(done) {
@@ -596,11 +596,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
          it('throws an error if alias is not associated', function(done) {
            var self = this
-            expect(function() {
+            self.Worker.find({ include: [ { daoFactory: self.Task, as: 'Work' } ] }).catch(function(err) {
-              self.Worker.find({ include: [ { daoFactory: self.Task, as: 'Work' } ] })
+              expect(err.message).to.equal('Task (Work) is not associated to Worker!');
-            }).to.throw(Error, 'Task (Work) is not associated to Worker!')
              done()
            })
+          })
          it('returns the associated task via worker.task', function(done) {
            this.Worker.find({
@@ -657,11 +657,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Task.find({ include: [ self.Worker ] }).catch(function(err) {
-            self.Task.find({ include: [ self.Worker ] })
+            expect(err.message).to.equal('Worker is not associated to Task!');
-          }).to.throw(Error, 'Worker is not associated to Task!')
            done()
          })
+        })
        it('returns the associated tasks via worker.tasks', function(done) {
          this.Worker.find({
@@ -759,11 +759,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
      describe('hasMany with alias', function() {
        it('throws an error if included DaoFactory is not referenced by alias', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.find({ include: [ self.Task ] }).catch(function(err) {
-            self.Worker.find({ include: [ self.Task ] })
+            expect(err.message).to.equal('Task is not associated to Worker!');
-          }).to.throw(Error, 'Task is not associated to Worker!')
            done()
          })
+        })
        describe('alias', function() {
          beforeEach(function(done) {
@@ -778,11 +778,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
          it('throws an error if alias is not associated', function(done) {
            var self = this
-            expect(function() {
+            self.Worker.find({ include: [ { daoFactory: self.Task, as: 'Work' } ] }).catch(function(err) {
-              self.Worker.find({ include: [ { daoFactory: self.Task, as: 'Work' } ] })
+              expect(err.message).to.equal('Task (Work) is not associated to Worker!');
-            }).to.throw(Error, 'Task (Work) is not associated to Worker!')
              done()
            })
+          })
          it('returns the associated task via worker.task', function(done) {
            this.Worker.find({

--- a/test/dao-factory/findAll.test.js
+++ b/test/dao-factory/findAll.test.js
@@ -575,19 +575,19 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error about unexpected input if include contains a non-object', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.all({ include: [ 1 ] }).catch(function(err) {
-            self.Worker.all({ include: [ 1 ] })
+            expect(err.message).to.equal('Include unexpected. Element has to be either a Model, an Association or an object.');
-          }).to.throw(Error)
            done()
          })
+        })
        it('throws an error if included DaoFactory is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.all({ include: [ self.Task ] }).catch(function(err) {
-            self.Worker.all({ include: [ self.Task ] })
+            expect(err.message).to.equal('TaskBelongsTo is not associated to Worker!');
-          }).to.throw(Error, 'TaskBelongsTo is not associated to Worker!')
            done()
          })
+        })
        it('returns the associated worker via task.worker', function(done) {
          this.Task.all({
@@ -627,11 +627,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Task.all({ include: [ self.Worker ] }).catch(function(err) {
-            self.Task.all({ include: [ self.Worker ] })
+            expect(err.message).to.equal('Worker is not associated to TaskHasOne!');
-          }).to.throw(Error, 'Worker is not associated to TaskHasOne!')
            done()
          })
+        })
        it('returns the associated task via worker.task', function(done) {
          this.Worker.all({
@@ -672,19 +672,19 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not referenced by alias', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.all({ include: [ self.Task ] }).catch(function(err) {
-            self.Worker.all({ include: [ self.Task ] })
+            expect(err.message).to.equal('Task is not associated to Worker!');
-          }).to.throw(Error, 'Task is not associated to Worker!')
            done()
          })
+        })
        it('throws an error if alias is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.all({ include: [ { daoFactory: self.Task, as: 'Work' } ] }).catch(function(err) {
-            self.Worker.all({ include: [ { daoFactory: self.Task, as: 'Work' } ] })
+            expect(err.message).to.equal('Task (Work) is not associated to Worker!');
-          }).to.throw(Error, 'Task (Work) is not associated to Worker!')
            done()
          })
+        })
        it('returns the associated task via worker.task', function(done) {
          this.Worker.all({
@@ -735,11 +735,11 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Task.findAll({ include: [ self.Worker ] }).catch(function(err) {
-            self.Task.findAll({ include: [ self.Worker ] })
+            expect(err.message).to.equal('worker is not associated to task!');
-          }).to.throw(Error, 'worker is not associated to task!')
            done()
          })
+        })
        it('returns the associated tasks via worker.tasks', function(done) {
          this.Worker.findAll({
@@ -780,19 +780,19 @@ describe(Support.getTestDialectTeaser("DAOFactory"), function () {
        it('throws an error if included DaoFactory is not referenced by alias', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.findAll({ include: [ self.Task ] }).catch(function(err) {
-            self.Worker.findAll({ include: [ self.Task ] })
+            expect(err.message).to.equal('Task is not associated to Worker!');
-          }).to.throw(Error, 'Task is not associated to Worker!')
            done()
          })
+        })
        it('throws an error if alias is not associated', function(done) {
          var self = this
-          expect(function() {
+          self.Worker.findAll({ include: [ { daoFactory: self.Task, as: 'Work' } ] }).catch(function(err) {
-            self.Worker.findAll({ include: [ { daoFactory: self.Task, as: 'Work' } ] })
+            expect(err.message).to.equal('Task (Work) is not associated to Worker!');
-          }).to.throw(Error, 'Task (Work) is not associated to Worker!')
            done()
          })
+        })
        it('returns the associated task via worker.task', function(done) {
          this.Worker.findAll({

--- a/test/hooks.test.js
+++ b/test/hooks.test.js