Fix for self-eviction hooks (#307)

* don't use _.has for hooks * check if hooks are functions
esatterwhite · Oct 18, 2016 · e00468b · e00468b
1 parent 4f809f7
commit e00468b
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 8 deletions.
diff --git a/lib/self-evict.js b/lib/self-evict.js
@@ -135,6 +135,20 @@ SelfEvict.prototype.registerHooks = function registerHooks(hooks) {
         });
     }
 
+    if (hooks.preEvict && !_.isFunction(hooks.preEvict)) {
+        throw errors.InvalidOptionError({
+            option: 'preEvict',
+            reason: 'it is not a function'
+        });
+    }
+
+    if (hooks.postEvict && !_.isFunction(hooks.postEvict)) {
+        throw errors.InvalidOptionError({
+            option: 'postEvict',
+            reason: 'it is not a function'
+        });
+    }
+
     this.hooks[name] = hooks;
 };
 
@@ -294,7 +308,7 @@ SelfEvict.prototype._runHooks = function _runHooks(type, done) {
     var ringpop = self.ringpop;
 
     var names = _.filter(_.keys(this.hooks), function filter(name) {
-        return _.has(self.hooks[name], type);
+        return self.hooks[name][type];
     });
 
     if (names.length === 0) {

diff --git a/test/unit/self-evict-test.js b/test/unit/self-evict-test.js
@@ -67,6 +67,16 @@ test('register hooks', function t(assert) {
             type: 'ringpop.method-required',
             argument: 'hooks',
             method: 'preEvict and/or postEvict'
+        }],
+        [{name: 'test', preEvict: 'non-function'}, {
+            type: 'ringpop.invalid-option',
+            option: 'preEvict',
+            reason: 'it is not a function'
+        }],
+        [{name: 'test', postEvict: 'non-function'}, {
+            type: 'ringpop.invalid-option',
+            option: 'postEvict',
+            reason: 'it is not a function'
         }]
     ];
 
@@ -173,6 +183,7 @@ testRingpop({async: true}, 'self evict sequence invokes hooks', function t(deps,
         }
     });
 
+    var exampleHook;
     var ExampleHook = function ExampleHook(name){
         this.name = name;
 
@@ -183,17 +194,24 @@ testRingpop({async: true}, 'self evict sequence invokes hooks', function t(deps,
             assert.equal(this, self, 'context is correct');
             cb();
         };
+    };
+    ExampleHook.prototype.preEvict = function preEvict(cb){
+        assert.equal(selfEvict.currentPhase().phase, SelfEvict.PhaseNames.PreEvict);
+        assert.pass('exampleHook.preEvict called');
+        assert.equal(this, exampleHook, 'context is correct');
+        cb();
+    };
 
-        this.postEvict = function(cb){
-            assert.equal(selfEvict.currentPhase().phase, SelfEvict.PhaseNames.PostEvict);
-            assert.pass('exampleHook.postEvict called');
-            assert.equal(this, self, 'context is correct');
+    ExampleHook.prototype.postEvict = function postEvict(cb){
+        assert.equal(selfEvict.currentPhase().phase, SelfEvict.PhaseNames.PostEvict);
+        assert.pass('exampleHook.postEvict called');
+        assert.equal(this, exampleHook, 'context is correct');
 
-            cb();
-        };
+        cb();
     };
 
-    selfEvict.registerHooks(new ExampleHook('InstanceExample'));
+    exampleHook = new ExampleHook('InstanceExample');
+    selfEvict.registerHooks(exampleHook);
 
     selfEvict.initiate(cleanup);
 });