From 82702bedb02199cee7d0e7c5124583133d1efd3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 11:13:33 +0100 Subject: [PATCH 1/8] add 3 failling tests to check the middleware error handling --- tests/ConnectSequence.spec.js | 85 +++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/ConnectSequence.spec.js b/tests/ConnectSequence.spec.js index 88a5d7a..1041f22 100644 --- a/tests/ConnectSequence.spec.js +++ b/tests/ConnectSequence.spec.js @@ -10,6 +10,8 @@ var beforeEach = global.beforeEach var it = global.it var expect = chai.expect +var EXPECTED_MIDDLEWARE_ERROR_MESSAGE = 'expected middleware error' + describe('ConnectSequence', function () { var seq, req, res, next, mid @@ -567,6 +569,89 @@ describe('ConnectSequence', function () { seq.append(_mid0, _mid1, _mid2, _mid3) seq.run(req, res, next) }) + + describe('When a middleware throw an error in the next callback', function () { + var midErr, nextErr, errorHandler + var midBefore0, midBefore1 + var midAfter0 + var runSeq + + beforeEach(function () { + midBefore0 = mid0 + midBefore1 = mid1 + midErr = new Error(EXPECTED_MIDDLEWARE_ERROR_MESSAGE) + nextErr = function (req, res, next) { next(midErr) } + }) + + describe('The error handler middleware f(req, res, next, err)', function () { + it('should handle any error passed in the fourth argument', function (done) { + next = function (req, res) { + done() + } + errorHandler = function (req, res, next, err) { + expect(err).to.equal(EXPECTED_MIDDLEWARE_ERROR_MESSAGE) + next() + done() + } + midAfter0 = function (req, res, next) { + ensureReqIdsDefined(req) + req.ids.push('midAfter0') + next() + } + seq = new ConnectSequence(req, res, next) + seq.append(midBefore0, nextErr, midBefore1, errorHandler, midAfter0) + seq.run() + }) + + it('should skip the initial next middleware', function (done) { + next = function (req, res) { + throw new Error('Forbidden middleware') + } + errorHandler = function (req, res, next, err) { + // the error is handled ... + next() + done() + } + midAfter0 = function (req, res, next) { + ensureReqIdsDefined(req) + req.ids.push('midAfter0') + next() + } + seq = new ConnectSequence(req, res, next) + seq.append(midBefore0, nextErr, midBefore1, errorHandler, midAfter0) + runSeq = function () { seq.run() } + expect(runSeq).to.not.throw(Error) + }) + + it('should skip all middlewares after the middleware throwing and error', function (done) { + next = function (req, res) { + if (!req.isDone) { + req.isDone = true + done() + } + } + errorHandler = function (req, res, next, err) { + // the error is handled ... + next() + if (!req.isDone) { + req.isDone = true + done() + } + } + midAfter0 = function (req, res, next) { + ensureReqIdsDefined(req) + req.ids.push('midAfter0') + // next() + throw new Error('Forbidden middleware') + } + seq = new ConnectSequence(req, res, next) + seq.append(midBefore0, nextErr, midBefore1, errorHandler, midAfter0) + expect(function () { + seq.run() + }).to.not.throw(Error) + }) + }) + }) }) }) From 81b46bed884f24cb2f458ca90d1826711ac5bb7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:13:47 +0100 Subject: [PATCH 2/8] implements middleware error handling --- lib/ConnectSequence.js | 68 ++++++++++++++++++++++++++++------- tests/ConnectSequence.spec.js | 56 +++++++++++++++++------------ 2 files changed, 88 insertions(+), 36 deletions(-) diff --git a/lib/ConnectSequence.js b/lib/ConnectSequence.js index 8823582..ae7e551 100644 --- a/lib/ConnectSequence.js +++ b/lib/ConnectSequence.js @@ -56,14 +56,31 @@ ConnectSequence.prototype = { * @returns {undefined} */ function run () { - var that = this var midSequence = this.middlewares.reverse() - var initialNext = this.next.bind(null, this.req, this.res, this.next) + var initialNext = this.next.bind() + var req = this.req + var res = this.res var nestedCallSequence = midSequence.reduce(middlewareReducer, initialNext) nestedCallSequence.call() - function middlewareReducer (prev, current) { - return current.bind(null, that.req, that.res, prev) + function middlewareReducer (callSequence, middleware) { + return function nextHandler (err) { + if (err !== undefined) { + if (isErrorHandler(middleware)) { + console.log(getFunctionName(middleware)) + middleware(err, req, res, callSequence) + } else { + callSequence(err) + } + } else { + if (isErrorHandler(middleware)) { + callSequence() + } else { + console.log(getFunctionName(middleware)) + middleware(req, res, callSequence) + } + } + } } } @@ -156,13 +173,38 @@ function appendIf (filter, middleware) { throw new TypeError(errorMsg) } - this.append(function (req, res, next) { - if (filter(req)) { - middleware(req, res, next) - return - } else { - next() - return - } - }) + if (isErrorHandler(middleware)) { + this.append(function (err, req, res, next) { + if (filter(req)) { + middleware(err, req, res, next) + return + } else { + next() + return + } + }) + } else { + this.append(function (req, res, next) { + if (filter(req)) { + middleware(req, res, next) + return + } else { + next() + return + } + }) + } +} + +function isErrorHandler (cb) { + if (typeof cb !== 'function') { + throw new TypeError('cb must be a function') + } + var str = cb.toString() + var args = str.split('(')[1].split(')')[0].split(',') + return args.length === 4 +} + +function getFunctionName (func) { + return func.toString().split(' ')[1].split('(')[0] } diff --git a/tests/ConnectSequence.spec.js b/tests/ConnectSequence.spec.js index 1041f22..10b2757 100644 --- a/tests/ConnectSequence.spec.js +++ b/tests/ConnectSequence.spec.js @@ -441,7 +441,7 @@ describe('ConnectSequence', function () { describe('when the previous condition on `req` is `true`', function () { it('should run the given middleware', function (done) { - next = function (req, res) { + next = function () { expect(res.foo).to.equal('bar') done() } @@ -458,7 +458,7 @@ describe('ConnectSequence', function () { describe('when the previous condition on `req` is `false`', function () { it('should not run the given middleware', function (done) { - next = function (req, res) { + next = function () { expect(res.foo).to.not.equal('bar') expect(res.foo).to.be.undefined done() @@ -507,7 +507,7 @@ describe('ConnectSequence', function () { }) it('should run the initial next middleware at last', function (done) { - next = function (req, res) { + next = function () { req.ids = 'initialNext' done() } @@ -519,7 +519,7 @@ describe('ConnectSequence', function () { }) it('should run all the middlewares in the passed array of middlewares', function (done) { - next = function (req, res) { + next = function () { req.ids.push('initial') expect(req.ids).to.contain('initial') expect(req.ids).to.contain('mid0') @@ -534,7 +534,7 @@ describe('ConnectSequence', function () { }) it('should run all the middlewares in the same order than the given array', function (done) { - next = function (req, res) { + next = function () { req.ids.push('initial') expect(req.ids.join()).to.equal('mid0,mid1,mid2,mid3,initial') setTimeout(done, 20) @@ -546,7 +546,7 @@ describe('ConnectSequence', function () { it('should run each middleware as a callback of the previous', function (done) { this.slow(500) - next = function (req, res) { + next = function () { if (req && req.ids) { req.ids.push('initial') expect(req.ids.join()).to.equal('mid0,mid1,mid2,mid3,initial') @@ -580,20 +580,26 @@ describe('ConnectSequence', function () { midBefore0 = mid0 midBefore1 = mid1 midErr = new Error(EXPECTED_MIDDLEWARE_ERROR_MESSAGE) - nextErr = function (req, res, next) { next(midErr) } + nextErr = function nextErr (req, res, next) { next(midErr) } }) - describe('The error handler middleware f(req, res, next, err)', function () { + describe(', the error handler middleware f(err, req, res, next)', function () { it('should handle any error passed in the fourth argument', function (done) { - next = function (req, res) { - done() + next = function () { + if (!req.isDone) { + req.isDone = true + done() + } } - errorHandler = function (req, res, next, err) { - expect(err).to.equal(EXPECTED_MIDDLEWARE_ERROR_MESSAGE) + errorHandler = function errorHandler (err, req, res, next) { + expect(err.message).to.equal(EXPECTED_MIDDLEWARE_ERROR_MESSAGE) next() - done() + if (!req.isDone) { + req.isDone = true + done() + } } - midAfter0 = function (req, res, next) { + midAfter0 = function midAfter0 (req, res, next) { ensureReqIdsDefined(req) req.ids.push('midAfter0') next() @@ -604,15 +610,17 @@ describe('ConnectSequence', function () { }) it('should skip the initial next middleware', function (done) { - next = function (req, res) { + next = function () { throw new Error('Forbidden middleware') } - errorHandler = function (req, res, next, err) { - // the error is handled ... - next() + errorHandler = function errorHandler (err, req, res, next) { + if (err) { + // the error is handled ... + } + // do not call next, it is forbidden done() } - midAfter0 = function (req, res, next) { + midAfter0 = function midAfter0 (req, res, next) { ensureReqIdsDefined(req) req.ids.push('midAfter0') next() @@ -624,21 +632,23 @@ describe('ConnectSequence', function () { }) it('should skip all middlewares after the middleware throwing and error', function (done) { - next = function (req, res) { + next = function next () { if (!req.isDone) { req.isDone = true done() } } - errorHandler = function (req, res, next, err) { + errorHandler = function errorHandler (err, req, res, next) { // the error is handled ... - next() + if (err) { + // ... + } if (!req.isDone) { req.isDone = true done() } } - midAfter0 = function (req, res, next) { + midAfter0 = function midAfter0 (req, res, next) { ensureReqIdsDefined(req) req.ids.push('midAfter0') // next() From 7fb32c0297360ab16838f2b62f391e3cf530d688 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:14:30 +0100 Subject: [PATCH 3/8] remove developement logs --- lib/ConnectSequence.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ConnectSequence.js b/lib/ConnectSequence.js index ae7e551..ec75d7f 100644 --- a/lib/ConnectSequence.js +++ b/lib/ConnectSequence.js @@ -67,7 +67,6 @@ function run () { return function nextHandler (err) { if (err !== undefined) { if (isErrorHandler(middleware)) { - console.log(getFunctionName(middleware)) middleware(err, req, res, callSequence) } else { callSequence(err) @@ -76,7 +75,6 @@ function run () { if (isErrorHandler(middleware)) { callSequence() } else { - console.log(getFunctionName(middleware)) middleware(req, res, callSequence) } } From b73f245bcc22af3b6c5d726702bcb248d521727f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:16:03 +0100 Subject: [PATCH 4/8] remove unused function --- lib/ConnectSequence.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/ConnectSequence.js b/lib/ConnectSequence.js index ec75d7f..e99c7b3 100644 --- a/lib/ConnectSequence.js +++ b/lib/ConnectSequence.js @@ -202,7 +202,3 @@ function isErrorHandler (cb) { var args = str.split('(')[1].split(')')[0].split(',') return args.length === 4 } - -function getFunctionName (func) { - return func.toString().split(' ')[1].split('(')[0] -} From 347c864ce44a8eb142f7826c5f65d574b829eced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:26:23 +0100 Subject: [PATCH 5/8] remove unecessary check --- lib/ConnectSequence.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/ConnectSequence.js b/lib/ConnectSequence.js index e99c7b3..19205b9 100644 --- a/lib/ConnectSequence.js +++ b/lib/ConnectSequence.js @@ -195,9 +195,6 @@ function appendIf (filter, middleware) { } function isErrorHandler (cb) { - if (typeof cb !== 'function') { - throw new TypeError('cb must be a function') - } var str = cb.toString() var args = str.split('(')[1].split(')')[0].split(',') return args.length === 4 From d15bf68fcb543cb93e955aeff8993e09b49064af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:35:34 +0100 Subject: [PATCH 6/8] improve testing coverage for #appendIf() --- tests/ConnectSequence.spec.js | 48 ++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/tests/ConnectSequence.spec.js b/tests/ConnectSequence.spec.js index 10b2757..169b059 100644 --- a/tests/ConnectSequence.spec.js +++ b/tests/ConnectSequence.spec.js @@ -440,19 +440,43 @@ describe('ConnectSequence', function () { }) describe('when the previous condition on `req` is `true`', function () { - it('should run the given middleware', function (done) { - next = function () { - expect(res.foo).to.equal('bar') - done() - } - seq = new ConnectSequence(req, res, next) - seq.appendIf(function (req) { - return req.foo === 'bar' - }, function (req, res, next) { - res.foo = req.foo // 'bar' - next() + describe('when passing a normal middleware', function () { + it('should run the given middleware', function (done) { + next = function () { + expect(res.foo).to.equal('bar') + done() + } + seq = new ConnectSequence(req, res, next) + seq.appendIf(function (req) { + return req.foo === 'bar' + }, function (req, res, next) { + res.foo = req.foo // 'bar' + next() + }) + seq.run() + }) + }) + + describe('when passing a error handler middleware', function () { + it('should run the given error handler', function (done) { + next = function () { + expect(res.foo).to.equal('bar') + done() + } + seq = new ConnectSequence(req, res, next) + seq.append(function (req, res, next) { + next('middleware in error') + }) + seq.appendIf(function (req) { + return req.foo === 'bar' + }, function (err, req, res, next) { + if (err) { + res.foo = req.foo // 'bar' + } + next() + }) + seq.run() }) - seq.run() }) }) From d4dc68db4a06ead5126447a68987bbef1dcf6a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 16:50:49 +0100 Subject: [PATCH 7/8] improve testing coverage for #appendIf() --- tests/ConnectSequence.spec.js | 50 ++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/tests/ConnectSequence.spec.js b/tests/ConnectSequence.spec.js index 169b059..d855e12 100644 --- a/tests/ConnectSequence.spec.js +++ b/tests/ConnectSequence.spec.js @@ -481,20 +481,44 @@ describe('ConnectSequence', function () { }) describe('when the previous condition on `req` is `false`', function () { - it('should not run the given middleware', function (done) { - next = function () { - expect(res.foo).to.not.equal('bar') - expect(res.foo).to.be.undefined - done() - } - seq = new ConnectSequence(req, res, next) - seq.appendIf(function (req) { - return !req.foo - }, function (req, res, next) { - res.foo = req.foo // 'bar' - next() + describe('when passing a normal middleware', function () { + it('should not run the given middleware', function (done) { + next = function () { + expect(res.foo).to.not.equal('bar') + expect(res.foo).to.be.undefined + done() + } + seq = new ConnectSequence(req, res, next) + seq.appendIf(function (req) { + return !req.foo + }, function (req, res, next) { + res.foo = req.foo // 'bar' + next() + }) + seq.run() + }) + }) + + describe('when passing a error handler middleware', function () { + it('should not run the given error handler', function (done) { + next = function () { + expect(res.foo).to.not.equal('bar') + done() + } + seq = new ConnectSequence(req, res, next) + seq.append(function (req, res, next) { + next('middleware in error') + }) + seq.appendIf(function (req) { + return !req.foo // 'bar' + }, function (err, req, res, next) { + if (err) { + res.foo = req.foo // 'bar' + } + next() + }) + seq.run() }) - seq.run() }) }) }) From 4ffb52bcc4caa574fd76eea031e2076dfd8c6570 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Becheras?= Date: Mon, 9 Jan 2017 17:27:02 +0100 Subject: [PATCH 8/8] better testing of error handling --- lib/ConnectSequence.js | 19 ++++++++++- tests/ConnectSequence.spec.js | 59 ++++++++++++++++++++++------------- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/lib/ConnectSequence.js b/lib/ConnectSequence.js index 19205b9..3bf43bf 100644 --- a/lib/ConnectSequence.js +++ b/lib/ConnectSequence.js @@ -60,21 +60,38 @@ function run () { var initialNext = this.next.bind() var req = this.req var res = this.res - var nestedCallSequence = midSequence.reduce(middlewareReducer, initialNext) + var nestedCallSequence + + // create the call sequence + nestedCallSequence = midSequence.reduce(middlewareReducer, initialNext) + // call it nestedCallSequence.call() + /** + * Reduce the middleware sequence to a nested middleware handler sequence + * @function + * @param {Function} callSequence intermediate resulting call sequence + * @param {Function} middleware the current middleware + * @returns {Function} the new intermediate resulting call sequence + */ function middlewareReducer (callSequence, middleware) { return function nextHandler (err) { + // if the previous middleware passed an error argument if (err !== undefined) { if (isErrorHandler(middleware)) { + // call the current middleware if it is an error handler middleware middleware(err, req, res, callSequence) } else { + // else skip the current middleware and call the intermediate sequence callSequence(err) } } else { + // if no error argument is passed if (isErrorHandler(middleware)) { + // skip the current middleware if it is an errorHandler callSequence() } else { + // else call it middleware(req, res, callSequence) } } diff --git a/tests/ConnectSequence.spec.js b/tests/ConnectSequence.spec.js index d855e12..4d85aae 100644 --- a/tests/ConnectSequence.spec.js +++ b/tests/ConnectSequence.spec.js @@ -679,34 +679,49 @@ describe('ConnectSequence', function () { expect(runSeq).to.not.throw(Error) }) - it('should skip all middlewares after the middleware throwing and error', function (done) { - next = function next () { - if (!req.isDone) { - req.isDone = true + describe('when a middleware pass an error in its next middleware', function () { + it('should skip all middlewares after the middleware throwing and error', function (done) { + var midBetween = function (req, res, next) { + req.midBetween = true + } + errorHandler = function errorHandler (err, req, res, next) { + if (err) { + // the error is handled ... + } + next() + } + next = function next () { + expect(req.midBetween).to.be.undefined done() } - } - errorHandler = function errorHandler (err, req, res, next) { - // the error is handled ... - if (err) { - // ... + seq = new ConnectSequence(req, res, next) + seq.append(midBefore0, nextErr, midBetween, errorHandler) + seq.run() + }) + }) + + describe('when any middleware pass any error in its next middleware', function () { + it('should skip an error handler middleware if no error is passed', function (done) { + errorHandler = function errorHandler (err, req, res, next) { + if (err) { + // the error is handled ... + } + req.errorHandler = true + next() } - if (!req.isDone) { - req.isDone = true + midAfter0 = function midAfter0 (req, res, next) { + req.midAfter0 = true + next() + } + next = function next () { + expect(req.errorHandler).to.be.undefined + expect(req.midAfter0).to.equal(true) done() } - } - midAfter0 = function midAfter0 (req, res, next) { - ensureReqIdsDefined(req) - req.ids.push('midAfter0') - // next() - throw new Error('Forbidden middleware') - } - seq = new ConnectSequence(req, res, next) - seq.append(midBefore0, nextErr, midBefore1, errorHandler, midAfter0) - expect(function () { + seq = new ConnectSequence(req, res, next) + seq.append(midBefore0, errorHandler, midAfter0) seq.run() - }).to.not.throw(Error) + }) }) }) })