diff --git a/index.js b/index.js index 4358aeb..89f606e 100644 --- a/index.js +++ b/index.js @@ -360,26 +360,7 @@ Router.prototype.handle = function handle (req, res, callback) { */ Router.prototype.use = function use (handler) { - let offset = 0 - let path = '/' - - // default path to '/' - // disambiguate router.use([handler]) - if (typeof handler !== 'function') { - let arg = handler - - while (Array.isArray(arg) && arg.length !== 0) { - arg = arg[0] - } - - // first arg is the path - if (typeof arg !== 'function') { - offset = 1 - path = handler - } - } - - const callbacks = flatten.call(slice.call(arguments, offset), Infinity) + const [path, callbacks] = normalizeMiddlewareArg(arguments) if (callbacks.length === 0) { throw new TypeError('argument handler is required') @@ -409,6 +390,75 @@ Router.prototype.use = function use (handler) { return this } +/** + * Use the given error handler, with optional path, defaulting to "/". + * + * `.error` (like `.all`) will run for any http METHOD, but it will not add + * handlers for those methods so OPTIONS requests will not consider `.error` + * functions even if they could respond. This is the same behavior as `.use`. + * + * You can use multiple `.error` calls to add multiple handlers. They will run + * as long as `next(err)` is called. If you want to stop processing, omit the + * call. + * + * function logErrorToConsole(err, req, res, next) { + * console.error(err) + * next(err) + * } + * + * function logErrorToFile(err, req, res, next) { + * fs.appendFile('errors.txt', err.toString(), function() { + * next(err) + * }) + * } + * + * // Note that the arity of the error handler can be 3 when passed to + * // `router.error` (it must be 4 when passed to to `router.use`). + * function sendErrorResponse(err, req, res) { + * res.statusCode = 500 + * res.setHeader('Content-Type', 'text/plain') + * res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message) + * } + * + * router + * .error(logErrorToConsole) + * .error(logErrorToFile) + * .error(sendErrorResponse) + * + * @public + */ + +Router.prototype.error = function error (handler) { + const [path, callbacks] = normalizeMiddlewareArg(arguments) + + if (callbacks.length === 0) { + throw new TypeError('argument handler is required') + } + + // `router.error` can be thought of as a special case of `router.use` which + // ensures the arity of the handler is 4 (to indicate to `use` that the + // handler is an error handler). Once the arity is set to 4, we can fall back + // to `use` for the rest of the logic. + this.use(path, callbacks.map(fn => { + if (typeof fn !== 'function') { + throw new TypeError('argument handler must be a function') + } + + const errorHandler = function (err, req, res, next) { + return fn(err, req, res, next) + } + + // preserve the original function name for debug logging + Object.defineProperty(errorHandler, 'name', { + value: fn.name + }) + + return errorHandler + })) + + return this +} + /** * Create a new Route for the given path. * @@ -746,3 +796,36 @@ function wrap (old, fn) { fn.apply(this, args) } } + +/** + * Normalize variadic/polymorphic arguments to middleware creation functions + * (e.g. `router.use`, `router.error`). + * + * @private + */ + +function normalizeMiddlewareArg (originalArgs) { + let offset = 0 + let path = '/' + const handler = originalArgs[0] + + // default path to '/' + // disambiguate router.use([handler]) + if (typeof handler !== 'function') { + let arg = handler + + while (Array.isArray(arg) && arg.length !== 0) { + arg = arg[0] + } + + // first arg is the path + if (typeof arg !== 'function') { + offset = 1 + path = handler + } + } + + const callbacks = flatten.call(slice.call(originalArgs, offset), Infinity) + + return [path, callbacks] +} diff --git a/test/router.js b/test/router.js index b440e40..dddba7d 100644 --- a/test/router.js +++ b/test/router.js @@ -1396,6 +1396,677 @@ describe('Router', function () { }) }) + describe('.error(...fn)', function () { + it('should reject missing functions', function () { + const router = new Router() + assert.throws(router.error.bind(router), /argument handler is required/) + }) + + it('should reject empty array', function () { + const router = new Router() + assert.throws(router.error.bind(router, []), /argument handler is required/) + }) + + it('should reject non-functions', function () { + const router = new Router() + assert.throws(router.error.bind(router, '/', 'hello'), /argument handler must be a function/) + assert.throws(router.error.bind(router, '/', 5), /argument handler must be a function/) + assert.throws(router.error.bind(router, '/', null), /argument handler must be a function/) + assert.throws(router.error.bind(router, '/', new Date()), /argument handler must be a function/) + }) + + it('should accept functions with arity 3', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error(function arity3Handler (err, req, res) { + res.statusCode = 500 + res.setHeader('Content-Type', 'text/plain') + res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message) + }) + + request(server) + .get('/foo') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should accept functions with arity 4', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error(function arity4Handler (err, req, res, next) { + res.statusCode = 500 + res.setHeader('Content-Type', 'text/plain') + res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message) + }) + + request(server) + .get('/foo') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should be chainable', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error(function (err, req, res, next) { + res.setHeader('x-err-1', err.message) + next(err) + }) + router.error(function (err, req, res, next) { + res.setHeader('x-err-2', err.message) + next(err) + }) + assert.equal(router.error(ouchErrorHandler), router) + + request(server) + .get('/foo') + .expect('x-err-1', 'boom!') + .expect('x-err-2', 'boom!') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should invoke error handler for all requests', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error(ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .put('/') + .expect(500, 'ouch on PUT /: boom!', cb) + }, + function (cb) { + request(server) + .post('/') + .expect(500, 'ouch on POST /: boom!', cb) + }, + function (cb) { + rawrequest(server) + .options('*') + .expect(500, 'ouch on OPTIONS *: boom!', cb) + } + ], done) + }) + + it('should not invoke for blank URLs', function (done) { + const router = new Router() + const server = createServer(function hander (req, res, next) { + req.url = '' + router(req, res, next) + }) + + router.use(boomError) + router.error(ouchErrorHandler) + + request(server) + .get('/') + .expect(404, done) + }) + + it('should accept multiple arguments', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error(function (err, req, res, next) { + res.setHeader('x-err-1', err.message) + next(err) + }, function (err, req, res, next) { + res.setHeader('x-err-2', err.message) + next(err) + }, ouchErrorHandler) + + request(server) + .get('/foo') + .expect('x-err-1', 'boom!') + .expect('x-err-2', 'boom!') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should accept single array of error handlers', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error([ + function (err, req, res, next) { + res.setHeader('x-err-1', err.message) + next(err) + }, + function (err, req, res, next) { + res.setHeader('x-err-2', err.message) + next(err) + }, + ouchErrorHandler + ]) + + request(server) + .get('/foo') + .expect('x-err-1', 'boom!') + .expect('x-err-2', 'boom!') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should accept nested arrays of error handlers', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(boomError) + router.error( + [ + [ + function (err, req, res, next) { + res.setHeader('x-err-1', err.message) + next(err) + }, + function (err, req, res, next) { + res.setHeader('x-err-2', err.message) + next(err) + } + ], + function (err, req, res, next) { + res.setHeader('x-err-3', err.message) + next(err) + } + ], + ouchErrorHandler + ) + + request(server) + .get('/foo') + .expect('x-err-1', 'boom!') + .expect('x-err-2', 'boom!') + .expect('x-err-3', 'boom!') + .expect(500, 'ouch on GET /foo: boom!', done) + }) + + it('should not invoke singular error handler', function (done) { + const router = new Router() + const server = createServer(router) + + router.error(ouchErrorHandler) + + request(server) + .get('/') + .expect(404, done) + }) + + it('should not stack overflow with a large sync stack', function (done) { + this.timeout(5000) // long-running test + + const router = new Router() + const server = createServer(router) + + for (let i = 0; i < 6000; i++) { + router.error(function (err, req, res, next) { next(err) }) + } + + router.use(boomError) + router.error(ouchErrorHandler) + + request(server) + .get('/') + .expect(500, 'ouch on GET /: boom!', done) + }) + + describe('error handling', function () { + it('should invoke error function after next(err)', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(function handle (req, res, next) { + next(new Error('boom!')) + }) + + router.error(ouchErrorHandler) + + request(server) + .get('/') + .expect(500, 'ouch on GET /: boom!', done) + }) + + it('should invoke error function after throw err', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(function handle (req, res, next) { + throw new Error('boom!') + }) + + router.error(ouchErrorHandler) + + request(server) + .get('/') + .expect(500, 'ouch on GET /: boom!', done) + }) + + it('should not invoke error functions above function', function (done) { + const router = new Router() + const server = createServer(router) + + // this error handler returns 200 + router.error(sawError) + + router.use(boomError) + + request(server) + .get('/') + .expect(500, done) + }) + }) + }) + + describe('.error(path, ...fn)', function () { + it('should be chainable', function () { + const router = new Router() + assert.equal(router.error('/', ouchErrorHandler), router) + }) + + it('should invoke when req.url starts with path', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .post('/foo') + .expect(500, 'ouch on POST /: boom!', cb) + }, + function (cb) { + request(server) + .post('/foo/bar') + .expect(500, 'ouch on POST /bar: boom!', cb) + } + ], done) + }) + + it('should match if path has trailing slash', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/foo/', boomError) + router.error('/foo/', ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .post('/foo') + .expect(500, 'ouch on POST /: boom!', cb) + }, + function (cb) { + request(server) + .post('/foo/bar') + .expect(500, 'ouch on POST /bar: boom!', cb) + } + ], done) + }) + + it('should support array of paths', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(['/foo/', '/bar'], boomError) + router.error(['/foo/', '/bar'], ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/bar') + .expect(500, 'ouch on GET /: boom!', cb) + } + ], done) + }) + + it('should support regexp path', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(/^\/[a-z]oo$/, boomError) + router.error(/^\/[a-z]oo$/, ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/fooo') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/zoo/bear') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/get/zoo') + .expect(404, cb) + } + ], done) + }) + + it('should support regexp path with params', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(/^\/([a-z]oo)$/, boomError) + router.use(/^\/([a-z]oo)\/(?bear)$/, boomError) + + // these error handlers are trigged by the above `use` calls but silence + // the error since they call next() with no params + router.error(/^\/([a-z]oo)$/, function (_err, req, res, next) { + createHitHandle(req.params[0])(req, res, next) + }) + router.error(/^\/([a-z]oo)\/(?bear)$/, function (_err, req, res, next) { + createHitHandle(req.params[0] + req.params.animal)(req, res, next) + }) + + // so we throw another error to trigger the final error handler + router.use(/^\/([a-z]oo)$/, boomError) + router.use(/^\/([a-z]oo)\/(?bear)$/, boomError) + router.error(/^\/([a-z]oo)$/, ouchErrorHandler) + router.use(/^\/([a-z]oo)\/(?bear)$/, ouchErrorHandler) + + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/foo') + .expect(shouldHitHandle('foo')) + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/zoo') + .expect(shouldHitHandle('zoo')) + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/fooo') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/zoo/bear') + .expect(shouldHitHandle('zoobear')) + .expect(500, cb) + }, + function (cb) { + request(server) + .get('/get/zoo') + .expect(404, cb) + } + ], done) + }) + + it('should ensure regexp matches path prefix', function (done) { + const router = new Router() + const server = createServer(router) + + router.use(/\/api.*/, boomError) + router.error(/\/api.*/, function (_err, req, res, next) { + createHitHandle(1)(req, res, next) + }) + + router.use(/api/, boomError) + router.error(/api/, function (_err, req, res, next) { + createHitHandle(2)(req, res, next) + }) + + router.use(/\/test/, boomError) + router.error(/\/test/, function (_err, req, res, next) { + createHitHandle(3)(req, res, next) + }) + + router.use(boomError) + router.error(ouchErrorHandler) + + request(server) + .get('/test/api/1234') + .expect(shouldNotHitHandle(1)) + .expect(shouldNotHitHandle(2)) + .expect(shouldHitHandle(3)) + .expect(500, 'ouch on GET /test/api/1234: boom!', done) + }) + + it('should support parameterized path', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/:thing', boomError) + router.error('/:thing', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/bar') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/foo/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + } + ], done) + }) + + it('should accept multiple arguments', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', function (err, req, res, next) { + res.setHeader('x-err-1', err.message) + next(err) + }, function (err, req, res, next) { + res.setHeader('x-err-2', err.message) + next(err) + }, ouchErrorHandler) + + request(server) + .get('/foo') + .expect('x-err-1', 'boom!') + .expect('x-err-2', 'boom!') + .expect(500, 'ouch on GET /: boom!', done) + }) + + describe('with "caseSensitive" option', function () { + it('should not match paths case-sensitively by default', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + }, + function (cb) { + request(server) + .get('/FOO/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + }, + function (cb) { + request(server) + .get('/FOO/BAR') + .expect(500, 'ouch on GET /BAR: boom!', cb) + } + ], done) + }) + + it('should not match paths case-sensitively when false', function (done) { + const router = new Router({ caseSensitive: false }) + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + }, + function (cb) { + request(server) + .get('/FOO/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + }, + function (cb) { + request(server) + .get('/FOO/BAR') + .expect(500, 'ouch on GET /BAR: boom!', cb) + } + ], done) + }) + + it('should match paths case-sensitively when true', function (done) { + const router = new Router({ caseSensitive: true }) + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo/bar') + .expect(500, 'ouch on GET /bar: boom!', cb) + }, + function (cb) { + request(server) + .get('/FOO/bar') + .expect(404, cb) + }, + function (cb) { + request(server) + .get('/FOO/BAR') + .expect(404, cb) + } + ], done) + }) + }) + + describe('with "strict" option', function () { + it('should accept optional trailing slashes by default', function (done) { + const router = new Router() + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/foo/') + .expect(500, 'ouch on GET /: boom!', cb) + } + ], done) + }) + + it('should accept optional trailing slashes when false', function (done) { + const router = new Router({ strict: false }) + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/foo/') + .expect(500, 'ouch on GET /: boom!', cb) + } + ], done) + }) + + it('should accept optional trailing slashes when true', function (done) { + const router = new Router({ strict: true }) + const server = createServer(router) + + router.use('/foo', boomError) + router.error('/foo', ouchErrorHandler) + series([ + function (cb) { + request(server) + .get('/foo') + .expect(500, 'ouch on GET /: boom!', cb) + }, + function (cb) { + request(server) + .get('/foo/') + .expect(500, 'ouch on GET /: boom!', cb) + } + ], done) + }) + }) + }) + describe('request rewriting', function () { it('should support altering req.method', function (done) { const router = new Router() @@ -1451,6 +2122,16 @@ function helloWorld (req, res) { res.end('hello, world') } +function boomError (req, res) { + throw new Error('boom!') +} + +function ouchErrorHandler (err, req, res, next) { + res.statusCode = 500 + res.setHeader('Content-Type', 'text/plain') + res.end('ouch on ' + req.method + ' ' + req.url + ': ' + err.message) +} + function setsaw (num) { const name = 'x-saw-' + String(num) return function saw (req, res, next) {