Fix #2702, before/after Save hook (#6155)

* before/after save hook * before/after save and hook proxies * restore docs for hook * real proxies for hooks * [ci skip] update docs

Fix #2702, before/after Save hook (#6155)
* before/after save hook * before/after save and hook proxies * restore docs for hook * real proxies for hooks * [ci skip] update docs
Sushant · Jan Aagaard Meier
Commit 835d5bfd authored Jun 23, 2016 by Sushant Committed by Jan Aagaard Meier Jun 23, 2016
Showing with 240 additions and 27 deletions
changelog.md
docs/docs/hooks.md
lib/hooks.js
lib/model.js
test/integration/hooks/create.test.js
test/integration/hooks/hooks.test.js
test/integration/hooks/updateAttributes.test.js
--- a/changelog.md
+++ b/changelog.md
@@ -12,6 +12,8 @@
 - [ADDED] Intensive connection logging [#851](https://github.com/sequelize/sequelize/issues/851)
 - [FIXED] Only `belongsTo` uses `as` to construct foreign key - revert of [#5957](https://github.com/sequelize/sequelize/pull/5957) introduced in 4.0.0-0
 - [CHANGED] `Sequelize.Promise` is now an independent copy of `bluebird` library [#5974](https://github.com/sequelize/sequelize/issues/5974)
+- [ADDED] before/after Save hook [#2702](https://github.com/sequelize/sequelize/issues/2702)
+- [ADDED] Remove hooks by reference [#6155](https://github.com/sequelize/sequelize/issues/6155)
 ## BC breaks:
 - Range type bounds now default to [postgres default](https://www.postgresql.org/docs/9.5/static/rangetypes.html#RANGETYPES-CONSTRUCT) `[)` (inclusive, exclusive), previously was `()` (exclusive, exclusive)

--- a/docs/docs/hooks.md
+++ b/docs/docs/hooks.md
@@ -21,6 +21,7 @@ For a full list of hooks, see [Hooks API](/api/hooks).
  beforeCreate(instance, options, fn)
  beforeDestroy(instance, options, fn)
  beforeUpdate(instance, options, fn)
+  beforeSave(instance, options, fn)
 (-)
  create
  destroy
@@ -29,6 +30,7 @@ For a full list of hooks, see [Hooks API](/api/hooks).
  afterCreate(instance, options, fn)
  afterDestroy(instance, options, fn)
  afterUpdate(instance, options, fn)
+  afterSave(instance, options, fn)
 (6)
  afterBulkCreate(instances, options, fn)
  afterBulkDestroy(options, fn)
@@ -241,8 +243,8 @@ If you use `Model.bulkCreate(...)` with the `updatesOnDuplicate` option, changes
 ```
 // Bulk updating existing users with updatesOnDuplicate option
-Users.bulkCreate([{ id: 1, isMemeber: true}, 
+Users.bulkCreate([{ id: 1, isMemeber: true},
-                 { id: 2, isMember: false}], 
+                 { id: 2, isMember: false}],
                 { updatesOnDuplicate: ['isMember']})
 User.beforeBulkCreate(function (users, options) {

--- a/lib/hooks.js
+++ b/lib/hooks.js
@@ -10,7 +10,6 @@ const Utils = require('./utils')
 * 1. By specifying them as options in `sequelize.define`
 * 2. By calling `hook()` with a string and your hook handler function
 * 3. By calling the function with the same name as the hook you want
 * ```js
 * // Method 1
 * sequelize.define(name, { attributes }, {
@@ -37,7 +36,7 @@ const Utils = require('./utils')
 * @name Hooks
 */
-var hookTypes = {
+const hookTypes = {
  beforeValidate: {params: 2},
  afterValidate: {params: 2},
  validationFailed: {params: 3},
@@ -49,6 +48,8 @@ var hookTypes = {
  afterRestore: {params: 2},
  beforeUpdate: {params: 2},
  afterUpdate: {params: 2},
+  beforeSave: {params: 2, proxies: ['beforeUpdate', 'beforeCreate']},
+  afterSave: {params: 2, proxies: ['afterUpdate', 'afterCreate']},
  beforeBulkCreate: {params: 2},
  afterBulkCreate: {params: 2},
  beforeBulkDestroy: {params: 1},
@@ -73,7 +74,7 @@ var hookTypes = {
  afterBulkSync: {params: 1}
 };
-var hookAliases = {
+const hookAliases = {
  beforeDelete: 'beforeDestroy',
  afterDelete: 'afterDestroy',
  beforeBulkDelete: 'beforeBulkDestroy',
@@ -81,7 +82,16 @@ var hookAliases = {
  beforeConnection: 'beforeConnect'
 };
-var Hooks = {
+/**
+ * get array of current hook and its proxied hooks combined
+ */
+const getProxiedHooks = (hookType) => (
+  hookTypes[hookType].proxies
+    ? hookTypes[hookType].proxies.concat(hookType)
+    : [hookType]
+);
+const Hooks = {
  replaceHookAliases: function(hooks) {
    var realHookName;
@@ -119,8 +129,9 @@ var Hooks = {
    if (hookTypes[hookType] && hookTypes[hookType].sync) {
      hooks.forEach(function(hook) {
        if (typeof hook === 'object') hook = hook.fn;
-        debug(`running hook ${hookType}`); // log sync hooks
-        return hook.apply(self, fnArgs);
+        debug(`running hook(sync) ${hookType}`); // log sync hooks
+        hook.apply(self, fnArgs);
      });
      return;
    }
@@ -134,9 +145,11 @@ var Hooks = {
      if (hookType && hook.length > hookTypes[hookType].params) {
        hook = Promise.promisify(hook, self);
      }
      debug(`running hook ${hookType}`); // log async hook
      return hook.apply(self, fnArgs);
-    }).return();
+    })
+    .return();
    return promise;
  },
@@ -163,8 +176,14 @@ var Hooks = {
    debug(`adding hook ${hookType}`);
    hookType = hookAliases[hookType] || hookType;
-    this.options.hooks[hookType] = this.options.hooks[hookType] || [];
+    // check for proxies, add them too
-    this.options.hooks[hookType].push(!!name ? {name: name, fn: fn} : fn);
+    hookType = getProxiedHooks(hookType);
+    Utils._.each(hookType, (type) => {
+      this.options.hooks[type] = this.options.hooks[type] || [];
+      this.options.hooks[type].push(!!name ? {name: name, fn: fn} : fn);
+    });
    return this;
  },
@@ -172,23 +191,29 @@ var Hooks = {
   * Remove hook from the model
   *
   * @param {String} hookType
-   * @param {String} name
+   * @param {String|Function} name
   */
  removeHook: function(hookType, name) {
    hookType = hookAliases[hookType] || hookType;
+    let isReference = typeof name === 'function' ? true : false;
    if (!this.hasHook(hookType)) {
      return this;
    }
    Utils.debug(`removing hook ${hookType}`);
-    this.options.hooks[hookType] = this.options.hooks[hookType].filter(function (hook) {
-      // don't remove unnamed hooks
-      if (typeof hook === 'function') {
-        return true;
-      }
-      return typeof hook === 'object' && hook.name !== name;
+    // check for proxies, add them too
+    hookType = getProxiedHooks(hookType);
+    Utils._.each(hookType, (type) => {
+      this.options.hooks[type] = this.options.hooks[type].filter(function (hook) {
+        if (isReference && typeof hook === 'function') {
+          return hook !== name; // check if same method
+        } else {
+          return typeof hook === 'object' && hook.name !== name;
+        }
+      });
    });
    return this;
@@ -244,6 +269,20 @@ Hooks.hasHooks = Hooks.hasHook;
 * @name afterCreate
 */
+ /**
+  * A hook that is run before creating or updating a single instance, It proxies `beforeCreate` and `beforeUpdate`
+  * @param {String}   name
+  * @param {Function} fn   A callback function that is called with attributes, options
+  * @name beforeSave
+  */
+ /**
+  * A hook that is run after creating or updating a single instance, It proxies `afterCreate` and `afterUpdate`
+  * @param {String}   name
+  * @param {Function} fn   A callback function that is called with attributes, options
+  * @name afterSave
+  */
 /**
 * A hook that is run before destroying a single instance
 * @param {String}   name

--- a/lib/model.js
+++ b/lib/model.js
@@ -1292,7 +1292,7 @@ class Model {
    // forgot to pass options.where ?
    if (!options.where) {
      let commonKeys =  _.intersection(_.keys(options), _.keys(this.rawAttributes));
-      // jshint -W030 
+      // jshint -W030
      commonKeys.length && Utils.warn(`Model attributes (${commonKeys.join(',')}) found in finder method options but options.where object is empty. Did you forget to use options.where?`);
    }
@@ -3178,7 +3178,8 @@ class Model {
          ignoreChanged = _.without(ignoreChanged, updatedAtAttr);
        }
-        return this.constructor.runHooks('before' + hook, this, options).then(() => {
+        return this.constructor.runHooks('before' + hook, this, options)
+        .then(() => {
          if (options.defaultFields && !this.isNewRecord) {
            afterHookValues = _.pick(this.dataValues, _.difference(this.changed(), ignoreChanged));

--- a/test/integration/hooks/create.test.js
+++ b/test/integration/hooks/create.test.js
@@ -27,14 +27,20 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
    describe('on success', function() {
      it('should run hooks', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , afterHook = sinon.spy()
+          , beforeSave = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeCreate(beforeHook);
        this.User.afterCreate(afterHook);
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return this.User.create({username: 'Toni', mood: 'happy'}).then(function() {
          expect(beforeHook).to.have.been.calledOnce;
          expect(afterHook).to.have.been.calledOnce;
+          expect(beforeSave).to.have.been.calledOnce;
+          expect(afterSave).to.have.been.calledOnce;
        });
      });
    });
@@ -42,33 +48,46 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
    describe('on error', function() {
      it('should return an error from before', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , beforeSave = sinon.spy()
+          , afterHook = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeCreate(function(user, options) {
          beforeHook();
          throw new Error('Whoops!');
        });
        this.User.afterCreate(afterHook);
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return expect(this.User.create({username: 'Toni', mood: 'happy'})).to.be.rejected.then(function(err) {
          expect(beforeHook).to.have.been.calledOnce;
          expect(afterHook).not.to.have.been.called;
+          expect(beforeSave).not.to.have.been.called;
+          expect(afterSave).not.to.have.been.called;
        });
      });
      it('should return an error from after', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , beforeSave = sinon.spy()
+          , afterHook = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeCreate(beforeHook);
        this.User.afterCreate(function(user, options) {
          afterHook();
          throw new Error('Whoops!');
        });
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return expect(this.User.create({username: 'Toni', mood: 'happy'})).to.be.rejected.then(function(err) {
          expect(beforeHook).to.have.been.calledOnce;
          expect(afterHook).to.have.been.calledOnce;
+          expect(beforeSave).to.have.been.calledOnce;
+          expect(afterSave).not.to.have.been.called;
        });
      });
    });
@@ -134,7 +153,7 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
        });
      });
-      it('beforeCreate', function(){
+      it('beforeCreate', function() {
        var hookCalled = 0;
        this.User.beforeCreate(function(user, options) {
@@ -149,6 +168,42 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
        });
      });
+      it('beforeSave', function() {
+        var hookCalled = 0;
+        this.User.beforeSave(function(user, options) {
+          user.mood = 'happy';
+          hookCalled++;
+        });
+        return this.User.create({username: 'akira'}).then(function(user) {
+          expect(user.mood).to.equal('happy');
+          expect(user.username).to.equal('akira');
+          expect(hookCalled).to.equal(1);
+        });
+      });
+      it('beforeSave with beforeCreate', function() {
+        var hookCalled = 0;
+        this.User.beforeCreate(function(user, options) {
+          user.mood = 'sad';
+          hookCalled++;
+        });
+        this.User.beforeSave(function(user, options) {
+          user.mood = 'happy';
+          hookCalled++;
+        });
+        return this.User.create({username: 'akira'}).then(function(user) {
+          expect(user.mood).to.equal('happy');
+          expect(user.username).to.equal('akira');
+          expect(hookCalled).to.equal(2);
+        });
+      });
    });
  });

--- a/test/integration/hooks/hooks.test.js
+++ b/test/integration/hooks/hooks.test.js
@@ -413,4 +413,60 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
    });
  });
+  describe('#removal', function() {
+    it('should be able to remove by name', function() {
+      var sasukeHook = sinon.spy()
+        , narutoHook = sinon.spy();
+      this.User.hook('beforeCreate', 'sasuke', sasukeHook);
+      this.User.hook('beforeCreate', 'naruto', narutoHook);
+      return this.User.create({ username: 'makunouchi'}).then(() => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledOnce;
+        this.User.removeHook('beforeCreate', 'sasuke');
+        return this.User.create({ username: 'sendo'});
+      }).then(() => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledTwice;
+      });
+    });
+    it('should be able to remove by reference', function() {
+      var sasukeHook = sinon.spy()
+        , narutoHook = sinon.spy();
+      this.User.hook('beforeCreate', sasukeHook);
+      this.User.hook('beforeCreate', narutoHook);
+      return this.User.create({ username: 'makunouchi'}).then(() => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledOnce;
+        this.User.removeHook('beforeCreate', sasukeHook);
+        return this.User.create({ username: 'sendo'});
+      }).then(() => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledTwice;
+      });
+    });
+    it('should be able to remove proxies', function() {
+      var sasukeHook = sinon.spy()
+        , narutoHook = sinon.spy();
+      this.User.hook('beforeSave', sasukeHook);
+      this.User.hook('beforeSave', narutoHook);
+      return this.User.create({ username: 'makunouchi'}).then((user) => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledOnce;
+        this.User.removeHook('beforeSave', sasukeHook);
+        return user.updateAttributes({ username: 'sendo'});
+      }).then(() => {
+        expect(sasukeHook).to.have.been.calledOnce;
+        expect(narutoHook).to.have.been.calledTwice;
+      });
+    });
+  });
 });
--- a/test/integration/hooks/updateAttributes.test.js
+++ b/test/integration/hooks/updateAttributes.test.js
@@ -26,15 +26,21 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
    describe('on success', function() {
      it('should run hooks', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , afterHook = sinon.spy()
+          , beforeSave = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeUpdate(beforeHook);
        this.User.afterUpdate(afterHook);
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return this.User.create({username: 'Toni', mood: 'happy'}).then(function(user) {
          return user.updateAttributes({username: 'Chong'}).then(function(user) {
            expect(beforeHook).to.have.been.calledOnce;
            expect(afterHook).to.have.been.calledOnce;
+            expect(beforeSave).to.have.been.calledTwice;
+            expect(afterSave).to.have.been.calledTwice;
            expect(user.username).to.equal('Chong');
          });
        });
@@ -44,36 +50,48 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
    describe('on error', function() {
      it('should return an error from before', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , afterHook = sinon.spy()
+          , beforeSave = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeUpdate(function(user, options) {
          beforeHook();
          throw new Error('Whoops!');
        });
        this.User.afterUpdate(afterHook);
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return this.User.create({username: 'Toni', mood: 'happy'}).then(function(user) {
          return expect(user.updateAttributes({username: 'Chong'})).to.be.rejected.then(function() {
            expect(beforeHook).to.have.been.calledOnce;
+            expect(beforeSave).to.have.been.calledOnce;
            expect(afterHook).not.to.have.been.called;
+            expect(afterSave).to.have.been.calledOnce;
          });
        });
      });
      it('should return an error from after', function() {
        var beforeHook = sinon.spy()
-          , afterHook = sinon.spy();
+          , afterHook = sinon.spy()
+          , beforeSave = sinon.spy()
+          , afterSave = sinon.spy();
        this.User.beforeUpdate(beforeHook);
        this.User.afterUpdate(function(user, options) {
          afterHook();
          throw new Error('Whoops!');
        });
+        this.User.beforeSave(beforeSave);
+        this.User.afterSave(afterSave);
        return this.User.create({username: 'Toni', mood: 'happy'}).then(function(user) {
          return expect(user.updateAttributes({username: 'Chong'})).to.be.rejected.then(function() {
            expect(beforeHook).to.have.been.calledOnce;
            expect(afterHook).to.have.been.calledOnce;
+            expect(beforeSave).to.have.been.calledTwice;
+            expect(afterSave).to.have.been.calledOnce;
          });
        });
      });
@@ -107,6 +125,46 @@ describe(Support.getTestDialectTeaser('Hooks'), function() {
          expect(user.mood).to.equal('sad');
        });
      });
+      it('beforeSave', function() {
+        var hookCalled = 0;
+        this.User.beforeSave(function(user, options) {
+          user.mood = 'happy';
+          hookCalled++;
+        });
+        return this.User.create({username: 'fireninja', mood: 'nuetral'}).then(function(user) {
+          return user.updateAttributes({username: 'spider', mood: 'sad'});
+        }).then(function(user) {
+          expect(user.username).to.equal('spider');
+          expect(user.mood).to.equal('happy');
+          expect(hookCalled).to.equal(2);
+        });
+      });
+      it('beforeSave with beforeUpdate', function() {
+        var hookCalled = 0;
+        this.User.beforeUpdate(function(user, options) {
+          user.mood = 'sad';
+          hookCalled++;
+        });
+        this.User.beforeSave(function(user, options) {
+          user.mood = 'happy';
+          hookCalled++;
+        });
+        return this.User.create({username: 'akira'}).then(function(user) {
+          return user.updateAttributes({username: 'spider', mood: 'sad'});
+        }).then(function(user) {
+          expect(user.mood).to.equal('happy');
+          expect(user.username).to.equal('spider');
+          expect(hookCalled).to.equal(3);
+        });
+      });
    });
  });