From 2e647d46a2903fc1260854baacae6bbc612f8fb7 Mon Sep 17 00:00:00 2001 From: David Luecke Date: Fri, 14 Jul 2017 08:14:16 -0700 Subject: [PATCH] Allows error hooks to swallow error by setting the result (#621) --- lib/hooks.js | 11 ++++++- test/hooks/after.test.js | 5 ++- test/hooks/error.test.js | 55 +++++++++++++++++++++++--------- test/hooks/hooks.test.js | 69 ++++++++++++++++++++++++---------------- 4 files changed, 94 insertions(+), 46 deletions(-) diff --git a/lib/hooks.js b/lib/hooks.js index 7aa6d46298..57576f5e58 100644 --- a/lib/hooks.js +++ b/lib/hooks.js @@ -92,13 +92,22 @@ const hookMixin = exports.hookMixin = function hookMixin (service) { const errorHookObject = Object.assign({}, error.hook || hookObject, { type: 'error', + result: null, original: error.hook, error }); return processHooks .call(service, hookChain, errorHookObject) - .then(hook => Promise.reject(hook.error)); + .then(hook => { + if (errorHookObject.params.__returnHook) { + return Promise.reject(hook); + } else if (hook.result) { + return Promise.resolve(hook.result); + } + + return Promise.reject(hook.error); + }); }); }; }); diff --git a/test/hooks/after.test.js b/test/hooks/after.test.js index 684a40f5af..5c68af8271 100644 --- a/test/hooks/after.test.js +++ b/test/hooks/after.test.js @@ -185,7 +185,7 @@ describe('`after` hooks', () => { }); }); - it('does not run after hook when there is an error', done => { + it('does not run after hook when there is an error', () => { const dummyService = { remove () { return Promise.reject(new Error('Error removing item')); @@ -203,10 +203,9 @@ describe('`after` hooks', () => { } }); - service.remove(1, {}).catch(error => { + return service.remove(1, {}).catch(error => { assert.ok(error, 'Got error'); assert.equal(error.message, 'Error removing item', 'Got error message from service'); - done(); }); }); diff --git a/test/hooks/error.test.js b/test/hooks/error.test.js index 754e912fe0..3b2535503a 100644 --- a/test/hooks/error.test.js +++ b/test/hooks/error.test.js @@ -13,7 +13,7 @@ describe('`error` hooks', () => { afterEach(() => service.__hooks.error.get.pop()); - it('basic error hook', done => { + it('basic error hook', () => { service.hooks({ error: { get (hook) { @@ -26,9 +26,9 @@ describe('`error` hooks', () => { } }); - service.get('test').then(() => { - done(new Error('Should never get here')); - }).catch(() => done()); + return service.get('test').then(() => { + throw new Error('Should never get here'); + }).catch(() => true); }); it('can change the error', () => { @@ -118,6 +118,23 @@ describe('`error` hooks', () => { assert.ok(error.third); }); }); + + it('setting `hook.result` will return result', () => { + const data = { + message: 'It worked' + }; + + service.hooks({ + error: { + get (hook) { + hook.result = data; + } + } + }); + + return service.get(10) + .then(result => assert.deepEqual(result, data)); + }); }); describe('error in hooks', () => { @@ -137,7 +154,7 @@ describe('`error` hooks', () => { service = app.service('dummy'); }); - it('error in before hook', done => { + it('in before hook', () => { service.hooks({ before () { throw new Error(errorMessage); @@ -149,14 +166,17 @@ describe('`error` hooks', () => { ); assert.equal(hook.id, 'dishes'); assert.equal(hook.error.message, errorMessage); - done(); } }); - service.get('dishes').then(done); + return service.get('dishes') + .then(() => { + throw new Error('Should never get here'); + }) + .catch(error => assert.equal(error.message, errorMessage)); }); - it('error in after hook', done => { + it('in after hook', () => { service.hooks({ after () { throw new Error(errorMessage); @@ -167,19 +187,22 @@ describe('`error` hooks', () => { 'Original hook still set' ); assert.equal(hook.id, 'dishes'); - assert.deepEqual(hook.result, { + assert.deepEqual(hook.original.result, { id: 'dishes', text: 'You have to do dishes' }); assert.equal(hook.error.message, errorMessage); - done(); } }); - service.get('dishes').then(done); + return service.get('dishes') + .then(() => { + throw new Error('Should never get here'); + }) + .catch(error => assert.equal(error.message, errorMessage)); }); - it('uses the current hook object if thrown in a hook and sets hook.original', done => { + it('uses the current hook object if thrown in a hook and sets hook.original', () => { service.hooks({ after (hook) { hook.modified = true; @@ -190,12 +213,14 @@ describe('`error` hooks', () => { error (hook) { assert.ok(hook.modified); assert.equal(hook.original.type, 'after'); - - done(); } }); - service.get('laundry').then(done); + return service.get('laundry') + .then(() => { + throw new Error('Should never get here'); + }) + .catch(error => assert.equal(error.message, errorMessage)); }); }); }); diff --git a/test/hooks/hooks.test.js b/test/hooks/hooks.test.js index 316d2d79d4..d9fe2ff05d 100644 --- a/test/hooks/hooks.test.js +++ b/test/hooks/hooks.test.js @@ -45,7 +45,7 @@ describe('hooks basics', () => { }); }); - it('has hook.app, hook.service and hook.path', done => { + it('has hook.app, hook.service and hook.path', () => { const app = feathers().use('/dummy', { get (id) { return Promise.resolve({ id }); @@ -56,19 +56,14 @@ describe('hooks basics', () => { service.hooks({ before (hook) { - try { - assert.equal(this, service); - assert.equal(hook.service, service); - assert.equal(hook.app, app); - assert.equal(hook.path, 'dummy'); - done(); - } catch (e) { - done(e); - } + assert.equal(this, service); + assert.equal(hook.service, service); + assert.equal(hook.app, app); + assert.equal(hook.path, 'dummy'); } }); - service.get('test'); + return service.get('test'); }); it('does not error when result is null', () => { @@ -153,23 +148,43 @@ describe('hooks basics', () => { }); }); - it('allows to return the hook object', () => { - const app = feathers().use('/dummy', { - get (id, params) { - return Promise.resolve({ id, params }); - } + describe('returns the hook object with __returnHook', () => { + it('on normal method call', () => { + const app = feathers().use('/dummy', { + get (id, params) { + return Promise.resolve({ id, params }); + } + }); + const params = { + __returnHook: true + }; + + return app.service('dummy').get(10, params).then(context => { + assert.equal(context.service, app.service('dummy')); + assert.equal(context.type, 'after'); + assert.equal(context.path, 'dummy'); + assert.deepEqual(context.result, { + id: 10, + params + }); + }); }); - const params = { - __returnHook: true - }; - - return app.service('dummy').get(10, params).then(context => { - assert.equal(context.service, app.service('dummy')); - assert.equal(context.type, 'after'); - assert.equal(context.path, 'dummy'); - assert.deepEqual(context.result, { - id: 10, - params + + it('on error', () => { + const app = feathers().use('/dummy', { + get () { + return Promise.reject(new Error('Something went wrong')); + } + }); + const params = { + __returnHook: true + }; + + return app.service('dummy').get(10, params).catch(context => { + assert.equal(context.service, app.service('dummy')); + assert.equal(context.type, 'error'); + assert.equal(context.path, 'dummy'); + assert.equal(context.error.message, 'Something went wrong'); }); }); });