diff --git a/Makefile b/Makefile index 2cdb59dc9..8ed5890b4 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,6 @@ NODEUNIT := ./node_modules/.bin/nodeunit MOCHA := ./node_modules/.bin/mocha NODECOVER := ./node_modules/.bin/cover DOCS_BUILD := ./tools/docsBuild.js -BENCHMARK := ./benchmark/index.js NPM := npm NODE := node PRETTIER := ./node_modules/.bin/prettier @@ -74,7 +73,7 @@ docs-build: .PHONY: benchmark benchmark: - @($(NODE) $(BENCHMARK)) + @(cd ./benchmark && $(NPM) i && $(NODE) index.js) include ./tools/mk/Makefile.deps include ./tools/mk/Makefile.targ diff --git a/benchmark/benchmarks/middleware.js b/benchmark/benchmarks/middleware.js new file mode 100644 index 000000000..d4789f247 --- /dev/null +++ b/benchmark/benchmarks/middleware.js @@ -0,0 +1,33 @@ +'use strict'; + +var restify = process.argv.includes('version=head') + ? require('../../lib') + : require('restify'); + +var server = restify.createServer(); +var path = '/'; +var port = 3000; + +module.exports = { + url: 'http://localhost:' + port + path +}; + +function handler(req, res, next) { + next(); +} + +for (var i = 0; i < 10; i++) { + server.pre(handler); +} + +for (var j = 0; j < 10; j++) { + server.use(handler); +} + +server.get(path, function get(req, res) { + res.send('hello world'); +}); + +if (!module.parent) { + server.listen(port); +} diff --git a/benchmark/benchmarks/response-json.js b/benchmark/benchmarks/response-json.js index 81e0ba90c..81af3b5df 100644 --- a/benchmark/benchmarks/response-json.js +++ b/benchmark/benchmarks/response-json.js @@ -5,9 +5,17 @@ var restify = process.argv.includes('version=head') : require('restify'); var server = restify.createServer(); +var path = '/'; +var port = 3000; -server.get('/', function onRequest(req, res) { +module.exports = { + url: 'http://localhost:' + port + path +}; + +server.get(path, function onRequest(req, res) { res.send({ hello: 'world' }); }); -server.listen(3000); +if (!module.parent) { + server.listen(port); +} diff --git a/benchmark/benchmarks/response-text.js b/benchmark/benchmarks/response-text.js index 79f421499..db0f90441 100644 --- a/benchmark/benchmarks/response-text.js +++ b/benchmark/benchmarks/response-text.js @@ -5,9 +5,17 @@ var restify = process.argv.includes('version=head') : require('restify'); var server = restify.createServer(); +var path = '/'; +var port = 3000; -server.get('/', function onRequest(req, res) { +module.exports = { + url: 'http://localhost:' + port + path +}; + +server.get(path, function onRequest(req, res) { res.send('hello world'); }); -server.listen(3000); +if (!module.parent) { + server.listen(port); +} diff --git a/benchmark/benchmarks/router-heavy.js b/benchmark/benchmarks/router-heavy.js new file mode 100644 index 000000000..7114fed1e --- /dev/null +++ b/benchmark/benchmarks/router-heavy.js @@ -0,0 +1,132 @@ +'use strict'; + +var restify = process.argv.includes('version=head') + ? require('../../lib') + : require('restify'); + +var server = restify.createServer(); +var path = '/whiskeys/scotch/islay/lagavulin/16-years/50'; +var methods = ['post', 'put', 'get', 'del', 'patch']; +var _ = require('lodash'); +var port = 3000; + +// Disabling cache: it's not fair as it aims to the worst case, when +// cache hit ratio is 0%. However, it's still better than the worst +// as it doesn't require extra time to maintain the LRU cache. +// There is no other way to simulate 100+ different endpoint +// calls with the current benchmark suite. +if (!process.argv.includes('version=head')) { + server.router.cache = { + get: function get() { + return null; + }, + set: function get() { + return null; + }, + dump: function get() { + return []; + } + }; +} + +module.exports = { + url: 'http://localhost:' + port + path +}; + +var routes = { + beers: { + ale: { + 'pale-ale': { + 'american-pale-ale': [], + 'indian-pale-ale': [] + }, + lambic: [], + stout: { + 'american-porter': [], + 'imperial-stout': [], + 'irish-stout': [] + } + }, + lager: { + 'german-lager': { + marzen: [] + }, + pilsner: { + 'german-pilsner': [] + } + } + }, + + whiskeys: { + american: { + bourbon: { + kentchuky: { + 'jim-beam': ['jim-beam', 'bookers', 'old-crow'], + 'makers-mark': ['makers-mark'], + 'woodford-reserve': ['woodford-reserve'] + }, + tennessee: { + 'jack-daniels': ['jack-daniels'] + } + }, + rye: { + 'beam-suntory': ['jim-beam-rye', 'knob-creek'] + } + }, + irish: { + 'single-malt': { + bushmills: ['bushmills'], + connemare: ['connemare'] + }, + 'single-pot': { + redbreast: ['redbreast'], + jameson: ['jameson-15-year'] + } + }, + japanese: { + nikka: ['coffeey-malt', 'blended', 'from-the-barrel'], + hibiki: ['japanese-harmony'], + yamazakura: ['blended'] + }, + scotch: { + islay: { + bruichladdich: ['25-years', 'islay-barley-2009'], + octomore: ['7.2', 'islay-barley-8.3'], + laphroaig: ['lore', '15-years', 'four-oak'], + lagavulin: ['distillers-edition', '8-years', '16-years'] + } + } + } +}; + +function handler(req, res) { + res.send('hello'); +} + +function attachRoute(parent, routeConfig) { + _.map(routeConfig, function map(route, routeKey) { + var pathChunk = _.isString(routeKey) ? routeKey : route; + var routePath = parent + '/' + pathChunk; + + methods.forEach(function forEach(method) { + server[method](routePath, handler); + }); + + if (_.isObject(route) || _.isArray(route)) { + attachRoute(routePath, route); + } + if (_.isString(route)) { + for (var i = 0; i <= 100; i++) { + methods.forEach(function forEach(method) { + server[method](routePath + '/' + i, handler); + }); + } + } + }); +} + +attachRoute('', routes); + +if (!module.parent) { + server.listen(port); +} diff --git a/benchmark/index.js b/benchmark/index.js index 0c2d7900c..f2815d877 100644 --- a/benchmark/index.js +++ b/benchmark/index.js @@ -5,7 +5,12 @@ var inquirer = require('inquirer'); var bench = require('./lib/bench'); var stableVersion = require('restify/package.json').version; -var BENCHMARKS = ['response-json', 'response-text']; +var BENCHMARKS = [ + 'response-json', + 'response-text', + 'router-heavy', + 'middleware' +]; function select(callback) { var choices = BENCHMARKS.map(function map(name) { diff --git a/benchmark/lib/autocannon.js b/benchmark/lib/autocannon.js index 769441144..f0381bf9e 100644 --- a/benchmark/lib/autocannon.js +++ b/benchmark/lib/autocannon.js @@ -22,7 +22,7 @@ function writeResult(handler, version, result) { function fire(opts, handler, version, save, cb) { opts = opts || {}; - opts.url = 'http://localhost:3000'; + opts.url = opts.url || 'http://localhost:3000'; var instance = autocannon(opts, function onResult(err, result) { if (err) { diff --git a/benchmark/lib/bench.js b/benchmark/lib/bench.js index 88f02b37f..8881e0c27 100644 --- a/benchmark/lib/bench.js +++ b/benchmark/lib/bench.js @@ -14,6 +14,7 @@ function runBenchmark(opts, handler, version, cb) { var spinner = ora('Started ' + version + '/' + handler).start(); var modulePath = path.join(__dirname, '../benchmarks', handler); + var url = require(modulePath).url; var forked = fork(modulePath, ['version=' + version]); pipeline( @@ -24,8 +25,12 @@ function runBenchmark(opts, handler, version, cb) { spinner.text = 'Warming ' + version + '/' + handler + ' for 5s'; + var fireOpts = Object.assign({}, opts, { + duration: 5, + url: url + }); autocannon.fire( - Object.assign({}, opts, { duration: 5 }), + fireOpts, handler, version, false, @@ -48,7 +53,8 @@ function runBenchmark(opts, handler, version, cb) { 's'; } - autocannon.fire(opts, handler, version, true, callback); + var fireOpts = Object.assign({}, opts, { url: url }); + autocannon.fire(fireOpts, handler, version, true, callback); } ] }, diff --git a/benchmark/package.json b/benchmark/package.json new file mode 100644 index 000000000..9d66526f7 --- /dev/null +++ b/benchmark/package.json @@ -0,0 +1,19 @@ +{ + "name": "restify-benchmark", + "homepage": "http://restifyjs.com", + "description": "Restify benchmark", + "version": "0.0.0", + "private": true, + "main": "index.js", + "engines": { + "node": ">=0.10" + }, + "dependencies": { + "restify": "*" + }, + "devDependencies": {}, + "license": "MIT", + "scripts": { + "start": "node indec" + } +} diff --git a/docs/config/plugins.yaml b/docs/config/plugins.yaml index 3fa66725f..f3b7bdfd4 100644 --- a/docs/config/plugins.yaml +++ b/docs/config/plugins.yaml @@ -29,6 +29,7 @@ toc: - requestExpiry - inflightRequestThrottle - cpuUsageThrottle + - conditionalHandler - conditionalRequest - auditLogger - metrics diff --git a/docs/guides/6to7guide.md b/docs/guides/6to7guide.md new file mode 100644 index 000000000..27eb84c1d --- /dev/null +++ b/docs/guides/6to7guide.md @@ -0,0 +1,173 @@ +--- +title: restify 6.x to 7.x migration guide +permalink: /docs/6to7/ +--- + +## Introduction + +restify `7.x` comes with a completely new router and middleware logic that +brings significant performance improvement to your application. +From `v7.0.0` restify uses the Radix Tree based +[find-my-way](https://github.com/delvedor/find-my-way) package as a router +backend. + +## Breaking Changes + +### Server returns `RequestCloseError` instead of `RequestAbortedError` + +Server returns `RequestCloseError` instead of `RequestAbortedError` in the case +of the request was terminated by the client for some reason. + +The new version of restify never returns `RequestAbortedError`. + +### Non-strict routing is gone + +Option `strictRouting` is removed `createServer({ strictRouting: false })`. +Strict routing is the new default. + +### Different `RegExp` usage in router path and wildcards + +restify's new router backend +[find-my-way](https://github.com/delvedor/find-my-way) has limited RegExp +support. + +#### Guide to define paths + +To register a **parametric** path, use the *colon* before the parameter name. +For **wildcard** use the *star*. +*Remember that static routes are always inserted before parametric and wildcard.* + +```js +// parametric +server.get('GET', '/example/:userId', (req, res, next) => {})) +server.get('GET', '/example/:userId/:secretToken', (req, res, next) => {})) + +// wildcard +server.get('GET', '/example/*', (req, res, next) => {})) +``` + +Regular expression routes are supported as well, but pay attention, RegExp are +very expensive in term of performance! + +```js +// parametric with RegExp +server.get('GET', '/example/:file(^\\d+).png', () => {})) +``` + +RegExp path chunks needs to be between parentheses. + +It's possible to define more than one parameter within the same couple of slash +("/"). Such as: + +```js +server.get('/example/near/:lat-:lng/radius/:r', (req, res, next) => {})) +``` + +*Remember in this case to use the dash ("-") as parameters separator.* + +Finally it's possible to have multiple parameters with RegExp. + +```js +server.get('/example/at/:hour(^\\d{2})h:minute(^\\d{2})m', (req, res, next) => { + // req.params => { hour: 12, minute: 15 } +})) +``` +In this case as parameter separator it's possible to use whatever character is +not matched by the regular expression. + +Having a route with multiple parameters may affect negatively the performance, +so prefer single parameter approach whenever possible, especially on routes +which are on the hot path of your application. + +Fore more info see: https://github.com/delvedor/find-my-way + +### Remove already deprecated `next.ifError` + +`next.ifError(err)` is not available anymore. + +### Disable DTrace probes by default + +DTrace probes comes with some performance impact that's fine for the sake of +observability but you may don't use it at all. + +### Change in calling `next` multiple times + +Earlier `restify` automatically prevented calling the `next()` more than once. +In the new version this behaviour is disabled by default, but you can activate +it with the `onceNext` property. + +The behaviour of the `strictNext` option is unchanged. +Which means `strictNext` enforces `onceNext` option. + +```js +var server = restify.createServer({ onceNext: true }) +server.use(function (req, req, next) { + next(); + next(); +}); +// -> fine + +var server = restify.createServer({ strictNext: true }) +server.use(function (req, req, next) { + next(); + next(); +}); +// -> throws an Error +``` + +### Router versioning and content type + +`accept-version` and `accept` based conditional routing moved to the +`conditionalHandler` plugin, see docs or example: + +```js +var server = restify.createServer() + +server.use(restify.plugins.conditionalHandler({ + contentType: 'application/json', + version: '1.0.0' + handler: function (req, res, next) { + next(); + }) +}); + +server.get('/hello/:name', restify.plugins.conditionalHandler([ + { + version: '1.0.0', + handler: function(req, res, next) { res.send('1.x') } + }, + { + version: ['1.5.0', '2.0.0'], + handler: function(req, res, next) { res.send('1.5.x, 2.x') } + }, + { + version: '3.0.0', + contentType: ['text/html', 'text/html'] + handler: function(req, res, next) { res.send('3.x, text') } + }, + { + version: '3.0.0', + contentType: 'application/json' + handler: function(req, res, next) { res.send('3.x, json') } + } +]); + +// 'accept-version': '^1.1.0' => 1.5.x, 2.x' +// 'accept-version': '3.x', accept: 'application/json' => '3.x, json' +``` + +### After event fires when both request is flushed and last handler is finished + +In 7.x `after` event fires after both request is flushed +and last handler is finished. + +### Metrics plugin latency + +In 7.x Metrics plugin's `latency` is calculated when the request is +fully flushed. Earlier it was calculated when the last handler finished. + +To address the previous use-cases, new timings were added to the metrics plugin: + + - `metrics.preLatency` pre handlers latency + - `metrics.useLatency` use handlers latency + - `metrics.routeLatency` route handlers latency diff --git a/docs/guides/dtrace.md b/docs/guides/dtrace.md index efbb5c7f0..786c06a01 100644 --- a/docs/guides/dtrace.md +++ b/docs/guides/dtrace.md @@ -5,13 +5,16 @@ permalink: /docs/dtrace/ One of the coolest features of restify is that it automatically creates DTrace probes for you whenever you add a new route/handler. +To use DTrace you need to pass `dtrace` option to the server +`restify.createServer({ dtrace: true })`. The easiest way to explain this is with an example: ```js var restify = require('restify'); var server = restify.createServer({ - name: 'helloworld' + name: 'helloworld', + dtrace: true }); server.use(restify.acceptParser(server.acceptable)); diff --git a/examples/dtrace/demo.js b/examples/dtrace/demo.js index 321dd64fb..9e4c54616 100644 --- a/examples/dtrace/demo.js +++ b/examples/dtrace/demo.js @@ -87,6 +87,7 @@ var log = new Logger({ var server = restify.createServer({ name: NAME, Logger: log, + dtrace: true, formatters: { 'application/foo': function(req, res, body) { if (body instanceof Error) { diff --git a/examples/dtrace/hello.js b/examples/dtrace/hello.js index 0c74af57b..8e48ba961 100644 --- a/examples/dtrace/hello.js +++ b/examples/dtrace/hello.js @@ -1,7 +1,8 @@ var restify = require('../../lib'); var server = restify.createServer({ - name: 'helloworld' + name: 'helloworld', + dtrace: true }); server.use(restify.plugins.acceptParser(server.acceptable)); diff --git a/examples/example.js b/examples/example.js new file mode 100644 index 000000000..aa9df86eb --- /dev/null +++ b/examples/example.js @@ -0,0 +1,33 @@ +'use strict'; + +var restify = require('../lib'); +var server = restify.createServer(); + +server.pre(function pre(req, res, next) { + console.log('pre'); + next(); +}); + +server.use(function use(req, res, next) { + console.log('use'); + next(); +}); + +server.on('after', function(req, res, route, err) { + console.log('after'); +}); + +server.get( + '/:userId', + function onRequest(req, res, next) { + console.log(req.url, '1'); + next(); + }, + function onRequest(req, res, next) { + console.log(req.url, '2'); + res.send({ hello: 'world' }); + next(); + } +); + +server.listen(3001); diff --git a/lib/chain.js b/lib/chain.js new file mode 100644 index 000000000..59314d5de --- /dev/null +++ b/lib/chain.js @@ -0,0 +1,171 @@ +'use strict'; + +var assert = require('assert-plus'); +var once = require('once'); + +module.exports = Chain; + +/** + * Create a new middleware chain + * + * @public + * @class Chain + * @param {Object} [options] - options + * @param {Boolean} [options.onceNext=false] - Prevents calling next multiple + * times + * @param {Boolean} [options.strictNext=false] - Throws error when next() is + * called more than once, enables onceNext option + * @example + * var chain = new Chain(); + * chain.add(function (req, res, next) { next(); }) + * // chain.add(function (req, res, next) { next(new Error('Foo')); }) + * // chain.add(function (req, res, next) { next(false); }) + * + * http.createServer((req, res) => { + * chain.run(req, res, function done(err) { + * res.end(err ? err.message : 'hello world'); + * }); + * }) + */ +function Chain(options) { + assert.optionalObject(options, 'options'); + options = options || {}; + assert.optionalBool(options.onceNext, 'options.onceNext'); + assert.optionalBool(options.strictNext, 'options.strictNext'); + + this.onceNext = !!options.onceNext; + this.strictNext = !!options.strictNext; + + // strictNext next enforces onceNext + if (this.strictNext) { + this.onceNext = true; + } + + this._stack = []; + this._once = this.strictNext === false ? once : once.strict; +} + +/** + * Public methods. + * @private + */ + +/** + * Get handlers of a chain instance + * + * @memberof Chain + * @instance + * @returns {Function[]} handlers + */ +Chain.prototype.getHandlers = function getHandlers() { + return this._stack; +}; + +/** + * Utilize the given middleware `handler` + * + * @public + * @memberof Chain + * @instance + * @param {Function} handler - handler + * @returns {undefined} no return value + */ +Chain.prototype.add = function add(handler) { + // _name is assigned in the server and router + handler._name = handler._name || handler.name; + + // add the middleware + this._stack.push(handler); +}; + +/** + * Returns the number of handlers + * + * @public + * @memberof Chain + * @instance + * @returns {Number} number of handlers in the stack + */ +Chain.prototype.count = function count() { + return this._stack.length; +}; + +/** + * Handle server requests, punting them down + * the middleware stack. + * + * @public + * @memberof Chain + * @instance + * @param {Request} req - request + * @param {Response} res - response + * @param {Function} done - final handler + * @returns {undefined} no return value + */ +Chain.prototype.run = function run(req, res, done) { + var self = this; + var index = 0; + + function next(err) { + // next callback + var handler = self._stack[index++]; + + // all done or request closed + if (!handler || req.closed()) { + setImmediate(done, err, req, res); + return; + } + + // call the handler + call(handler, err, req, res, self.onceNext ? self._once(next) : next); + } + + next(); + return; +}; + +/** + * Helper functions + * @private + */ + +/** + * Invoke a handler. + * + * @private + * @param {Function} handler - handler function + * @param {Error|false|*} err - error, abort when true value or false + * @param {Request} req - request + * @param {Response} res - response + * @param {Function} _next - next handler + * @returns {undefined} no return value + */ +function call(handler, err, req, res, _next) { + var arity = handler.length; + var error = err; + var hasError = err === false || Boolean(err); + + // Meassure handler timings + // _name is assigned in the server and router + req._currentHandler = handler._name; + req.startHandlerTimer(handler._name); + + function next(nextErr) { + req.endHandlerTimer(handler._name); + _next(nextErr, req, res); + } + + if (hasError && arity === 4) { + // error-handling middleware + handler(err, req, res, next); + return; + } else if (!hasError && arity < 4) { + // request-handling middleware + handler(req, res, next); + return; + } + + // continue + next(error, req, res); + return; +} diff --git a/lib/deprecationWarnings.js b/lib/deprecationWarnings.js index c10112574..71d7da67a 100644 --- a/lib/deprecationWarnings.js +++ b/lib/deprecationWarnings.js @@ -7,11 +7,11 @@ function deprecationWarnings(server) { [ 'DEPRECATION WARNING: Due to deprecation of the domain module', 'in node.js, all features in restify that depend on it have', - 'been deprecated as well. This includes', - '`handleUncaughtExceptions` and `next.ifError()`.', - 'They will continue to work in 5.x, but', - 'consider them unsupported and likely', - 'to be removed from future versions of restify.' + 'been deprecated as well.', + 'This includes `handleUncaughtExceptions` and', + '`next.ifError()`. They will continue to work in 5.x, but', + 'consider them unsupported and likely to be removed', + 'from future versions of restify.' ].join(' ') ); } diff --git a/lib/errorTypes.js b/lib/errorTypes.js index a941e6b5c..521d7fdcd 100644 --- a/lib/errorTypes.js +++ b/lib/errorTypes.js @@ -3,4 +3,4 @@ var errors = require('restify-errors'); errors.makeConstructor('RequestCloseError'); -errors.makeConstructor('RequestAbortedError'); +errors.makeConstructor('RouteMissingError'); diff --git a/lib/index.js b/lib/index.js index 255a1bc32..6b01fddca 100644 --- a/lib/index.js +++ b/lib/index.js @@ -21,11 +21,10 @@ require('./errorTypes'); * @function createServer * @param {Object} [options] - an options object * @param {String} [options.name="restify"] - Name of the server. + * @param {Boolean} [options.dtrace=false] - enable DTrace support * @param {Router} [options.router=new Router(opts)] - Router * @param {Object} [options.log=bunyan.createLogger(options.name || "restify")] * - [bunyan](https://github.com/trentm/node-bunyan) instance. - * @param {String|Array} [options.version] - Default version(s) to use in all - * routes. * @param {Array} [options.acceptable] - String)|List of content-types this * server can respond with. * @param {String} [options.url] - Once listen() is called, this will be filled @@ -41,6 +40,7 @@ require('./errorTypes'); * exceptions that occur in it's handler stack. * [bunyan](https://github.com/trentm/node-bunyan) instance. * response header, default is `restify`. Pass empty string to unset the header. + * Comes with significant negative performance impact. * @param {Object} [options.spdy] - Any options accepted by * [node-spdy](https://github.com/indutny/node-spdy). * @param {Object} [options.http2] - Any options accepted by diff --git a/lib/plugins/audit.js b/lib/plugins/audit.js index f077521ab..fbc508bc6 100644 --- a/lib/plugins/audit.js +++ b/lib/plugins/audit.js @@ -265,7 +265,7 @@ function auditLogger(opts) { var latency = res.get('Response-Time'); if (typeof latency !== 'number') { - latency = hrTimeDurationInMs(req._time, process.hrtime()); + latency = hrTimeDurationInMs(req._timeStart, req._timeFlushed); } var obj = { diff --git a/lib/plugins/authorization.js b/lib/plugins/authorization.js index 92a595268..1d9120205 100644 --- a/lib/plugins/authorization.js +++ b/lib/plugins/authorization.js @@ -116,9 +116,7 @@ function authorizationParser(options) { var pieces = req.headers.authorization.split(' ', 2); if (!pieces || pieces.length !== 2) { - var e = new InvalidHeaderError( - 'BasicAuth content ' + 'is invalid.' - ); + var e = new InvalidHeaderError('BasicAuth content is invalid.'); return next(e); } diff --git a/lib/plugins/conditionalHandler.js b/lib/plugins/conditionalHandler.js new file mode 100644 index 000000000..555d62b1d --- /dev/null +++ b/lib/plugins/conditionalHandler.js @@ -0,0 +1,187 @@ +'use strict'; + +var errors = require('restify-errors'); +var _ = require('lodash'); +var assert = require('assert-plus'); +var semver = require('semver'); +var Negotiator = require('negotiator'); +var Chain = require('../chain'); + +///--- Globals + +var InvalidVersionError = errors.InvalidVersionError; +var UnsupportedMediaTypeError = errors.UnsupportedMediaTypeError; +var DEF_CT = 'application/octet-stream'; + +///--- Exports + +/** + * Runs first handler that matches to the condition + * + * @public + * @function conditionalHandler + * @param {Object|Object[]} candidates - candidates + * @param {Function|Function[]} candidates.handler - handler(s) + * @param {String|String[]} [candidates.version] - '1.1.0', ['1.1.0', '1.2.0'] + * @param {String} [candidates.contentType] - accepted content type, '*\/json' + * @returns {Function} Handler + * @throws {InvalidVersionError} + * @throws {UnsupportedMediaTypeError} + * @example + * server.use(restify.plugins.conditionalHandler({ + * contentType: 'application/json', + * version: '1.0.0', + * handler: function (req, res, next) { + * next(); + * }) + * }); + * + * server.get('/hello/:name', restify.plugins.conditionalHandler([ + * { + * version: '1.0.0', + * handler: function(req, res, next) { res.send('1.x'); } + * }, + * { + * version: ['1.5.0', '2.0.0'], + * handler: function(req, res, next) { res.send('1.5.x, 2.x'); } + * }, + * { + * version: '3.0.0', + * contentType: ['text/html', 'text/html'] + * handler: function(req, res, next) { res.send('3.x, text'); } + * }, + * { + * version: '3.0.0', + * contentType: 'application/json' + * handler: function(req, res, next) { res.send('3.x, json'); } + * }, + * // Array of handlers + * { + * version: '4.0.0', + * handler: [ + * function(req, res, next) { next(); }, + * function(req, res, next) { next(); }, + * function(req, res, next) { res.send('4.x') } + * ] + * }, + * ]); + * // 'accept-version': '^1.1.0' => 1.5.x, 2.x' + * // 'accept-version': '3.x', accept: 'application/json' => '3.x, json' + */ +function conditionalHandler(candidates) { + var isVersioned = false; + var isContentTyped = false; + + if (!_.isArray(candidates)) { + candidates = [candidates]; + } + + // Assert + assert.arrayOfObject(candidates, 'candidates'); + candidates = candidates.map(function map(candidate) { + // Array of handlers, convert to chain + if (_.isArray(candidate.handler)) { + var chain = new Chain(); + candidate.handler.forEach(function forEach(_handler) { + assert.func(_handler); + chain.add(_handler); + }); + candidate.handler = chain.run.bind(chain); + } + + assert.func(candidate.handler); + + if (_.isString(candidate.version)) { + candidate.version = [candidate.version]; + } + if (_.isString(candidate.contentType)) { + candidate.contentType = [candidate.contentType]; + } + + assert.optionalArrayOfString(candidate.version); + assert.optionalArrayOfString(candidate.contentType); + + isVersioned = isVersioned || !!candidate.version; + isContentTyped = isContentTyped || !!candidate.contentType; + + return candidate; + }); + + /** + * Conditional Handler + * + * @private + * @param {Request} req - request + * @param {Response} res - response + * @param {Function} next - next + * @returns {undefined} no return value + */ + return function _conditionalHandlerFactory(req, res, next) { + var contentType = req.headers.accept || DEF_CT; + var reqCandidates = candidates; + + // Content Type + if (isContentTyped) { + var contentTypes = contentType.split(/\s*,\s*/); + reqCandidates = candidates.filter(function filter(candidate) { + var neg = new Negotiator({ + headers: { + accept: candidate.contentType.join(', ') + } + }); + var tmp = neg.preferredMediaType(contentTypes); + return tmp && tmp.length; + }); + + if (!reqCandidates.length) { + next(new UnsupportedMediaTypeError(contentType)); + return; + } + } + + // Accept Version + if (isVersioned) { + var reqVersion = req.version(); + var maxVersion; + var maxVersionIndex; + + reqCandidates.forEach(function forEach(candidate, idx) { + var version = semver.maxSatisfying( + candidate.version, + reqVersion + ); + + if (version) { + if (!maxVersion || semver.gt(version, maxVersion)) { + maxVersion = version; + maxVersionIndex = idx; + } + } + }); + + // No version find + if (_.isUndefined(maxVersionIndex)) { + next( + new InvalidVersionError( + '%s is not supported by %s %s', + req.version() || '?', + req.method, + req.path() + ) + ); + return; + } + + // Add api-version response header + res.header('api-version', maxVersion); + + reqCandidates[maxVersionIndex].handler(req, res, next); + return; + } + + // When not versioned + reqCandidates[0].handler(req, res, next); + }; +} + +module.exports = conditionalHandler; diff --git a/lib/plugins/fullResponse.js b/lib/plugins/fullResponse.js index 0edd1d1d5..21b8c5c3c 100644 --- a/lib/plugins/fullResponse.js +++ b/lib/plugins/fullResponse.js @@ -43,9 +43,11 @@ function setHeaders(req, res) { } if (!res.getHeader('Response-Time')) { + // we cannot use req._timeFlushed here as + // the response is not flushed yet res.setHeader( 'Response-Time', - hrTimeDurationInMs(req._time, process.hrtime()) + hrTimeDurationInMs(req._timeStart, process.hrtime()) ); } } diff --git a/lib/plugins/index.js b/lib/plugins/index.js index 11de0fc50..756018074 100644 --- a/lib/plugins/index.js +++ b/lib/plugins/index.js @@ -10,6 +10,7 @@ module.exports = { authorizationParser: require('./authorization'), bodyParser: require('./bodyParser'), bodyReader: require('./bodyReader'), + conditionalHandler: require('./conditionalHandler'), conditionalRequest: require('./conditionalRequest'), cpuUsageThrottle: require('./cpuUsageThrottle.js'), dateParser: require('./date'), diff --git a/lib/plugins/inflightRequestThrottle.js b/lib/plugins/inflightRequestThrottle.js index 679637993..e5234b412 100644 --- a/lib/plugins/inflightRequestThrottle.js +++ b/lib/plugins/inflightRequestThrottle.js @@ -2,7 +2,6 @@ var assert = require('assert-plus'); var ServiceUnavailableError = require('restify-errors').ServiceUnavailableError; -var defaultResponse = new ServiceUnavailableError('resource exhausted'); /** * The `inflightRequestThrottle` module allows you to specify an upper limit to @@ -49,7 +48,7 @@ function inflightRequestThrottle(opts) { } var plugin = {}; - plugin._err = opts.err || defaultResponse; + plugin._err = opts.err || new ServiceUnavailableError('resource exhausted'); plugin._limit = opts.limit; plugin._server = opts.server; @@ -65,8 +64,7 @@ function inflightRequestThrottle(opts) { }, 'maximum inflight requests exceeded, rejecting request' ); - res.send(plugin._err); - return next(false); + return next(plugin._err); } return next(); diff --git a/lib/plugins/jsonBodyParser.js b/lib/plugins/jsonBodyParser.js index d1226a2ad..174b5eba4 100644 --- a/lib/plugins/jsonBodyParser.js +++ b/lib/plugins/jsonBodyParser.js @@ -56,8 +56,8 @@ function jsonBodyParser(options) { ) { return next( new errors.InternalServerError( - 'Cannot map POST body of ' + - '[Array array] onto req.params' + 'Cannot map POST body of [Array array] onto ' + + 'req.params' ) ); } diff --git a/lib/plugins/metrics.js b/lib/plugins/metrics.js index eddf7f165..da6f74d4c 100644 --- a/lib/plugins/metrics.js +++ b/lib/plugins/metrics.js @@ -3,6 +3,26 @@ var assert = require('assert-plus'); var hrTimeDurationInMs = require('./utils/hrTimeDurationInMs'); +/** + * Timing internals + * + * Timings are also saved when there is no handler in the given category. + * Some handler categories are optional, for example there is no + * `use` and `route` for 404. + * + * @private + * + * req._timeStart - request lifecycle started in restify + * req._timePreStart - pre handlers started + * req._timePreEnd - all pre handlers finished + * req._timeUseStart - use handlers started + * req._timeUseEnd - all use handlers finished + * req._timeRouteStart - route handlers started + * req._timeRouteEnd - all route handlers finished + * req._timeFlushed - request flushed, may happens before handlers finished + * req._timeFinished - both all handlers finished and request flushed + */ + ///--- API /** @@ -40,15 +60,28 @@ function createMetrics(opts, callback) { // REST verb method: req.method, // overall request latency - latency: hrTimeDurationInMs(req._time, process.hrtime()), + totalLatency: hrTimeDurationInMs(req._timeStart, req._timeFinished), + latency: hrTimeDurationInMs(req._timeStart, req._timeFlushed), + preLatency: hrTimeDurationInMs(req._timePreStart, req._timePreEnd), + useLatency: hrTimeDurationInMs(req._timeUseStart, req._timeUseEnd), + routeLatency: hrTimeDurationInMs( + req._timeRouteStart, + req._timeRouteEnd + ), // the cleaned up url path // e.g., /foo?a=1 => /foo path: req.path(), // connection state can currently only have the following values: - // 'close' | 'aborted' | undefined. + // 'close' | undefined. // - // if the connection state is 'close' or 'aborted' + // if the connection state is 'close' // the status code will be set to 444 + // it is possible to get a 200 statusCode with a connectionState + // value of 'close'. i.e., the client timed out, + // but restify thinks it "sent" a response. connectionState should + // always be the primary source of truth here, and check it first + // before consuming statusCode. otherwise, it may result in skewed + // metrics. connectionState: req.connectionState && req.connectionState(), unfinishedRequests: opts.server.inflightRequests && opts.server.inflightRequests(), @@ -68,16 +101,21 @@ function createMetrics(opts, callback) { * @param {Number} metrics.statusCode status code of the response. can be * undefined in the case of an uncaughtException * @param {String} metrics.method http request verb - * @param {Number} metrics.latency request latency + * @param {Number} metrics.totalLatency latency includes both request is flushed + * and all handlers finished + * @param {Number} metrics.latency latency when request is flushed + * @param {Number|null} metrics.preLatency pre handlers latency + * @param {Number|null} metrics.useLatency use handlers latency + * @param {Number|null} metrics.routeLatency route handlers latency * @param {String} metrics.path `req.path()` value * @param {Number} metrics.inflightRequests Number of inflight requests pending * in restify. * @param {Number} metrics.unifinishedRequests Same as `inflightRequests` - * @param {String} metrics.connectionState can be either `'close'`, - * `'aborted'`, or `undefined`. If this value is set, err will be a - * corresponding `RequestCloseError` or `RequestAbortedError`. + * @param {String} metrics.connectionState can be either `'close'` or + * `undefined`. If this value is set, err will be a + * corresponding `RequestCloseError`. * If connectionState is either - * `'close'` or `'aborted'`, then the `statusCode` is not applicable since the + * `'close'`, then the `statusCode` is not applicable since the * connection was severed before a response was written. * @param {Request} req the request obj * @param {Response} res the response obj diff --git a/lib/plugins/static.js b/lib/plugins/static.js index fa6fff58d..ef555632a 100644 --- a/lib/plugins/static.js +++ b/lib/plugins/static.js @@ -33,7 +33,7 @@ var ResourceNotFoundError = errors.ResourceNotFoundError; * The serveStatic module is different than most of the other plugins, in that * it is expected that you are going to map it to a route, as below: * - * server.get(/\/docs\/current\/?.*\/, restify.plugins.serveStatic({ + * server.get('/docs/current/*', restify.plugins.serveStatic({ * directory: './documentation/v1', * default: 'index.html' * })); @@ -64,7 +64,12 @@ var ResourceNotFoundError = errors.ResourceNotFoundError; * serveStatic method as an option. The following will serve index.html from * the documentation/v1/ directory anytime a client requests `/home/`. * - * server.get(/\/home\//, restify.plugins.serveStatic({ + * server.get('/home/*', restify.plugins.serveStatic({ + * directory: './documentation/v1', + * file: 'index.html' + * })); + * // or + * server.get('/home/([a-z]+[.]html)', restify.plugins.serveStatic({ * directory: './documentation/v1', * file: 'index.html' * })); @@ -88,11 +93,7 @@ function serveStatic(options) { var re = new RegExp('^' + escapeRE(p) + '/?.*'); function serveFileFromStats(file, err, stats, isGzip, req, res, next) { - if ( - typeof req.connectionState === 'function' && - (req.connectionState() === 'close' || - req.connectionState() === 'aborted') - ) { + if (typeof req.closed === 'function' && req.closed()) { next(false); return; } diff --git a/lib/plugins/utils/hrTimeDurationInMs.js b/lib/plugins/utils/hrTimeDurationInMs.js index 87a472231..07321dc02 100644 --- a/lib/plugins/utils/hrTimeDurationInMs.js +++ b/lib/plugins/utils/hrTimeDurationInMs.js @@ -9,9 +9,13 @@ var MS_PER_NS = 1e6; * @function hrTimeDurationInMs * @param {Array} startTime - [seconds, nanoseconds] * @param {Array} endTime - [seconds, nanoseconds] -* @returns {Number} durationInMs +* @returns {Number|null} durationInMs */ function hrTimeDurationInMs(startTime, endTime) { + if (!Array.isArray(startTime) || !Array.isArray(endTime)) { + return null; + } + var secondDiff = endTime[0] - startTime[0]; var nanoSecondDiff = endTime[1] - startTime[1]; var diffInNanoSecond = secondDiff * NS_PER_SEC + nanoSecondDiff; diff --git a/lib/request.js b/lib/request.js index 47ed10016..ff36ca2a5 100644 --- a/lib/request.js +++ b/lib/request.js @@ -766,14 +766,16 @@ function patch(Request) { self._timerMap[name] = process.hrtime(); - dtrace._rstfy_probes['handler-start'].fire(function fire() { - return [ - self.serverName, - self._currentRoute, // set in server._run - name, - self._dtraceId - ]; - }); + if (self.dtrace) { + dtrace._rstfy_probes['handler-start'].fire(function fire() { + return [ + self.serverName, + self._currentRoute, // set in server._run + name, + self._dtraceId + ]; + }); + } }; /** @@ -807,32 +809,47 @@ function patch(Request) { time: self._timerMap[name] }); - dtrace._rstfy_probes['handler-done'].fire(function fire() { - return [ - self.serverName, - self._currentRoute, // set in server._run - name, - self._dtraceId - ]; - }); + if (self.dtrace) { + dtrace._rstfy_probes['handler-done'].fire(function fire() { + return [ + self.serverName, + self._currentRoute, // set in server._run + name, + self._dtraceId + ]; + }); + } }; /** * Returns the connection state of the request. Current possible values are: * - `close` - when the request has been closed by the clien - * - `aborted` - when the socket was closed unexpectedly * * @public * @memberof Request * @instance * @function connectionState - * @returns {String} connection state (`"close"`, `"aborted"`) + * @returns {String} connection state (`"close"`) */ Request.prototype.connectionState = function connectionState() { var self = this; return self._connectionState; }; + /** + * Returns true when connection state is "close" + * + * @private + * @memberof Request + * @instance + * @function closed + * @returns {Boolean} is closed + */ + Request.prototype.closed = function closed() { + var self = this; + return self.connectionState() === 'close'; + }; + /** * Returns the route object to which the current request was matched to. * diff --git a/lib/response.js b/lib/response.js index df329d766..96d22bf47 100644 --- a/lib/response.js +++ b/lib/response.js @@ -374,17 +374,11 @@ function patch(Response) { var body = opts.body; var headers = opts.headers || {}; + self._sent = true; + // Now lets try to derive values for optional arguments that we were not // provided, otherwise we choose sane defaults. - // Request was aborted or closed. Override the status code - if ( - self.req.connectionState() === 'close' || - self.req.connectionState() === 'aborted' - ) { - code = 444; - } - // If the body is an error object and we were not given a status code, // try to derive it from the error object, otherwise default to 500 if (!code && body instanceof Error) { @@ -833,7 +827,7 @@ function flush(res, formattedBody) { res.write(res._data); } - // Finish resuest + // Finish request res.end(); // If log level is set to trace, log the entire response object diff --git a/lib/router.js b/lib/router.js index c1c57afd7..b6f6db8ec 100644 --- a/lib/router.js +++ b/lib/router.js @@ -1,150 +1,22 @@ -// Copyright 2012 Mark Cavage, Inc. All rights reserved. - 'use strict'; var EventEmitter = require('events').EventEmitter; -var url = require('url'); var util = require('util'); +var http = require('http'); -var LRU = require('lru-cache'); -var Negotiator = require('negotiator'); var _ = require('lodash'); var assert = require('assert-plus'); -var cloneRegexp = require('clone-regexp'); var errors = require('restify-errors'); -var semver = require('semver'); +var uuid = require('uuid'); -var utils = require('./utils'); +var Chain = require('./chain'); +var RouterRegistryRadix = require('./routerRegistryRadix'); +var dtrace = require('./dtrace'); ///--- Globals -var DEF_CT = 'application/octet-stream'; - -var BadRequestError = errors.BadRequestError; -var InternalError = errors.InternalError; -var InvalidArgumentError = errors.InvalidArgumentError; -var InvalidVersionError = errors.InvalidVersionError; var MethodNotAllowedError = errors.MethodNotAllowedError; var ResourceNotFoundError = errors.ResourceNotFoundError; -var UnsupportedMediaTypeError = errors.UnsupportedMediaTypeError; - -var shallowCopy = utils.shallowCopy; - -///--- Helpers - -/** - * Given a request, try to match it against the regular expression to - * get the route params. - * i.e., /foo/:param1/:param2 - * - * @private - * @function matchURL - * @param {String | RegExp} re - a string or regular expression - * @param {Object} req - the request object - * @returns {Object} params - */ -function matchURL(re, req) { - var i = 0; - var result = re.exec(req.path()); - var params = {}; - - if (!result) { - return false; - } - - // This means the user original specified a regexp match, not a url - // string like /:foo/:bar - if (!re.restifyParams) { - for (i = 1; i < result.length; i++) { - params[i - 1] = result[i]; - } - - return params; - } - - // This was a static string, like /foo - if (re.restifyParams.length === 0) { - return params; - } - - // This was the "normal" case, of /foo/:id - re.restifyParams.forEach(function forEach(p) { - if (++i < result.length) { - params[p] = decodeURIComponent(result[i]); - } - }); - - return params; -} - -/** - * Called while installing routes. attempts to compile the passed in string - * or regexp and register it. - * - * @private - * @function compileURL - * @param {Object} options - an options object - * @returns {RegExp} url regexp - */ -function compileURL(options) { - if (options.url instanceof RegExp) { - return options.url; - } - assert.string(options.url, 'url'); - - var params = []; - var pattern = '^'; - var re; - var _url = url.parse(options.url).pathname; - _url.split('/').forEach(function forEach(frag) { - if (frag.length <= 0) { - return false; - } - - pattern += '\\/'; - - if (frag.charAt(0) === ':') { - var label = frag; - var index = frag.indexOf('('); - var subexp; - - if (index === -1) { - if (options.urlParamPattern) { - subexp = options.urlParamPattern; - } else { - subexp = '[^/]*'; - } - } else { - label = frag.substring(0, index); - subexp = frag.substring(index + 1, frag.length - 1); - } - pattern += '(' + subexp + ')'; - params.push(label.slice(1)); - } else { - pattern += frag; - } - return true; - }); - - if (options.strict && _url.slice(-1) === '/') { - pattern += '\\/'; - } - - if (!options.strict) { - pattern += '[\\/]?'; - } - - if (pattern === '^') { - pattern += '\\/'; - } - - pattern += '$'; - - re = new RegExp(pattern, options.flags); - re.restifyParams = params; - - return re; -} ///--- API @@ -155,534 +27,351 @@ function compileURL(options) { * @class * @public * @param {Object} options - an options object + * @param {Bunyan} options.log - Bunyan logger instance + * @param {Boolean} [options.onceNext=false] - Prevents calling next multiple + * times + * @param {Boolean} [options.strictNext=false] - Throws error when next() is + * called more than once, enabled onceNext option + * @param {Object} [options.registry] - route registry */ function Router(options) { assert.object(options, 'options'); assert.object(options.log, 'options.log'); + assert.optionalBool(options.onceNext, 'options.onceNext'); + assert.optionalBool(options.strictNext, 'options.strictNext'); EventEmitter.call(this); - // eslint-disable-next-line new-cap - this.cache = LRU({ max: 100 }); - this.contentType = options.contentType || []; - - if (!Array.isArray(this.contentType)) { - this.contentType = [this.contentType]; - } - assert.arrayOfString(this.contentType, 'options.contentType'); - - this.strict = Boolean(options.strictRouting); this.log = options.log; - this.mounts = {}; + this.onceNext = !!options.onceNext; + this.strictNext = !!options.strictNext; this.name = 'RestifyRouter'; - // A list of methods to routes - this.routes = { - DELETE: [], - GET: [], - HEAD: [], - OPTIONS: [], - PATCH: [], - POST: [], - PUT: [] - }; + // Internals + this._anonymousHandlerCounter = 0; + this._registry = options.registry || new RouterRegistryRadix(options); +} +util.inherits(Router, EventEmitter); - // So we can return 405 vs 404, we maintain a reverse mapping of URLs - // to method - this.reverse = {}; +/** + * Lookup for route + * + * @public + * @memberof Router + * @instance + * @function lookup + * @param {Request} req - request + * @param {Response} res - response + * @returns {Chain|undefined} handler or undefined + */ +Router.prototype.lookup = function lookup(req, res) { + var self = this; + var pathname = req.getUrl().pathname; - this.versions = options.versions || options.version || []; + // Find route + self._dtraceStart(req); + var registryRoute = this._registry.lookup(req.method, pathname); + self._dtraceEnd(req, res); - if (!Array.isArray(this.versions)) { - this.versions = [this.versions]; + // Not found + if (!registryRoute) { + return undefined; } - assert.arrayOfString(this.versions, 'options.versions'); - this.versions.forEach(function forEach(v) { - if (semver.valid(v)) { - return true; - } - - throw new InvalidArgumentError('%s is not a valid semver', v); - }); - this.versions.sort(); -} -util.inherits(Router, EventEmitter); + // Decorate req + req.params = Object.assign(req.params, registryRoute.params); + req.route = registryRoute.route; -module.exports = Router; + // Call handler chain + return registryRoute.handler; +}; /** - * Takes an object of route params and query params, and 'renders' a URL. + * Lookup by name * * @public - * @function render - * @param {String} routeName - the route name - * @param {Object} params - an object of route params - * @param {Object} query - an object of query params - * @returns {String} URL + * @memberof Router + * @instance + * @function lookupByName + * @param {String} name - route name + * @param {Request} req - request + * @param {Response} res - response + * @returns {Chain|undefined} handler or undefined */ -Router.prototype.render = function render(routeName, params, query) { - function pathItem(match, key) { - if (params.hasOwnProperty(key) === false) { - throw new Error( - 'Route <' + routeName + '> is missing parameter <' + key + '>' - ); - } - return '/' + encodeURIComponent(params[key]); - } - - function queryItem(key) { - return encodeURIComponent(key) + '=' + encodeURIComponent(query[key]); - } - - var route = this.mounts[routeName]; +Router.prototype.lookupByName = function lookupByName(name, req, res) { + var self = this; + var route = self._registry.get()[name]; if (!route) { - return null; + return undefined; } - var _path = route.spec.path; - var _url = _path.replace(/\/:([A-Za-z0-9_]+)(\([^\\]+?\))?/g, pathItem); - var items = Object.keys(query || {}).map(queryItem); - var queryString = items.length > 0 ? '?' + items.join('&') : ''; - return _url + queryString; + // Decorate req + req.route = route; + + return route.chain.run.bind(route.chain); }; /** * Adds a route. * * @public + * @memberof Router + * @instance * @function mount - * @param {Object} options - an options object + * @param {Object} opts - an options object + * @param {String} opts.name - name + * @param {String} opts.method - method + * @param {String} opts.path - path can be any String + * @param {Function[]} handlers - handlers * @returns {String} returns the route name if creation is successful. + * @fires ...String#mount */ -Router.prototype.mount = function mount(options) { - assert.object(options, 'options'); - assert.string(options.method, 'options.method'); - assert.string(options.name, 'options.name'); - - var exists; - var name = options.name; - var route; - var routes = this.routes[options.method]; +Router.prototype.mount = function mount(opts, handlers) { var self = this; - var type = options.contentType || self.contentType; - var versions = options.versions || options.version || self.versions; - - if (type) { - if (!Array.isArray(type)) { - type = [type]; - } - type - .filter(function filter(t) { - return t; - }) - .sort() - .join(); - } - if (versions) { - if (!Array.isArray(versions)) { - versions = [versions]; - } - versions.sort(); - } + assert.object(opts, 'opts'); + assert.string(opts.method, 'opts.method'); + assert.arrayOfFunc(handlers, 'handlers'); + assert.optionalString(opts.name, 'opts.name'); - exists = routes.some(function some(r) { - return r.name === name; + var chain = new Chain({ + onceNext: self.onceNext, + strictNext: self.strictNext }); - if (exists) { - return false; - } - - route = { - name: name, - method: options.method, - path: compileURL({ - url: options.path || options.url, - flags: options.flags, - urlParamPattern: options.urlParamPattern, - strict: self.strict - }), - spec: options, - types: type, - versions: versions + // Route + var route = { + name: self._getRouteName(opts.name, opts.method, opts.path), + method: opts.method, + path: opts.path, + spec: opts, + chain: chain }; - routes.push(route); - if (!this.reverse[route.path.source]) { - this.reverse[route.path.source] = []; - } + handlers.forEach(function forEach(handler) { + // Assign name to anonymous functions + handler._name = + handler.name || 'handler-' + self._anonymousHandlerCounter++; - if (this.reverse[route.path.source].indexOf(route.method) === -1) { - this.reverse[route.path.source].push(route.method); - } - - this.mounts[route.name] = route; + // Attach to middleware chain + chain.add(handler); + }); - this.emit('mount', route.method, route.path, route.types, route.versions); + self._registry.add(route); + self.emit('mount', route.method, route.path); - return route.name; + return route; }; /** * Unmounts a route. * * @public + * @memberof Router + * @instance * @function unmount * @param {String} name - the route name - * @returns {String} the name of the deleted route. + * @returns {Object|undefined} removed route if found */ Router.prototype.unmount = function unmount(name) { - var route = this.mounts[name]; - - if (!route) { - this.log.warn('router.unmount(%s): route does not exist', name); - return false; - } - - var reverse = this.reverse[route.path.source]; - var routes = this.routes[route.method]; - this.routes[route.method] = routes.filter(function filter(r) { - return r.name !== route.name; - }); - - if (!this.findByPath(route.spec.path, { method: route.method })) { - this.reverse[route.path.source] = reverse.filter(function filter(r) { - return r !== route.method; - }); - - if (this.reverse[route.path.source].length === 0) { - delete this.reverse[route.path.source]; - } - } - - delete this.mounts[name]; + assert.string(name, 'name'); - var cache = this.cache; - cache.dump().forEach(function forEach(i) { - if (i.v.name === name) { - cache.del(i.k); - } - }); - - return name; + var route = this._registry.remove(name); + return route; }; /** - * Get a route from the router. + * toString() serialization. * * @public - * @function get - * @param {String} name - the name of the route to retrieve - * @param {Object} req - the request object - * @param {Function} cb - callback function - * @returns {undefined} no return value + * @memberof Router + * @instance + * @function toString + * @returns {String} stringified router */ -Router.prototype.get = function get(name, req, cb) { - var params; - var route = false; - var routes = this.routes[req.method] || []; - - for (var i = 0; i < routes.length; i++) { - if (routes[i].name === name) { - route = routes[i]; - - try { - params = matchURL(route.path, req); - } catch (e) { - // if we couldn't match the URL, log it out. - console.log(e); - } - break; - } - } - - if (route) { - cb(null, route, params || {}); - } else { - cb(new InternalError('Route not found: ' + name)); - } +Router.prototype.toString = function toString() { + return this._registry.toString(); }; /** - * Find a route from inside the router, handles versioned routes. + * Return information about the routes registered in the router. * * @public - * @function find - * @param {Object} req - the request object - * @param {Object} res - the response object - * @param {Function} callback - callback function - * @returns {undefined} no return value + * @memberof Router + * @instance + * @returns {object} The routes in the router. */ -Router.prototype.find = function find(req, res, callback) { - var candidates = []; - var ct = req.headers['content-type'] || DEF_CT; - var cacheKey = req.method + req.url + req.version() + ct; - var cacheVal; - var neg; - var params; - var r; - var reverse; - var routes = this.routes[req.method] || []; - var typed; - var versioned; - var maxV; - - if ((cacheVal = this.cache.get(cacheKey))) { - res.methods = cacheVal.methods.slice(); - req._matchedVersion = cacheVal.matchedVersion; - callback(null, cacheVal, shallowCopy(cacheVal.params)); - return; - } - - for (var i = 0; i < routes.length; i++) { - try { - params = matchURL(routes[i].path, req); - } catch (e) { - this.log.trace({ err: e }, 'error parsing URL'); - callback(new BadRequestError(e.message)); - return; - } - - if (params === false) { - continue; - } - - reverse = this.reverse[routes[i].path.source]; - - if (routes[i].types.length && req.isUpload()) { - candidates.push({ - p: params, - r: routes[i] - }); - typed = true; - continue; - } - - // GH-283: we want to find the latest version for a given route, - // not the first one. However, if neither the client nor - // server specified any version, we're done, because neither - // cared - if (routes[i].versions.length === 0) { - if (req.version() === '*') { - r = routes[i]; - break; - } - callback( - new InvalidVersionError( - '%s is not supported by %s %s', - req.version() || '?', - req.method, - req.path() - ) - ); - return; - } - - if (routes[i].versions.length > 0) { - candidates.push({ - p: params, - r: routes[i] - }); - versioned = true; - } - } +Router.prototype.getDebugInfo = function getDebugInfo() { + var routes = this._registry.get(); + return _.mapValues(routes, function mapValues(route, routeName) { + return { + name: route.name, + method: route.method.toLowerCase(), + path: route.path, + handlers: route.chain.getHandlers() + }; + }); +}; - if (!r) { - // If upload and typed - if (typed) { - var _t = ct.split(/\s*,\s*/); - candidates = candidates.filter(function filter(c) { - neg = new Negotiator({ - headers: { - accept: c.r.types.join(', ') - } - }); - var tmp = neg.preferredMediaType(_t); - return tmp && tmp.length; - }); - - // Pick the first one in case not versioned - if (candidates.length) { - r = candidates[0].r; - params = candidates[0].p; - } - } - - if (versioned) { - candidates.forEach(function forEach(c) { - var k = c.r.versions; - var v = semver.maxSatisfying(k, req.version()); - - if (v) { - if (!r || !maxV || semver.gt(v, maxV)) { - r = c.r; - params = c.p; - maxV = v; - } - } - }); - } - } +/** + * Return mounted routes + * + * @public + * @memberof Router + * @instance + * @returns {object} The routes in the router. + */ +Router.prototype.getRoutes = function getRoutes() { + return this._registry.get(); +}; - // In order, we check if the route exists, in which case, we're good. - // Otherwise we look to see if ver was set to false; that would tell us - // we indeed did find a matching route (method+url), but the version - // field didn't line up, so we return bad version. If no route and no - // version, we now need to go walk the reverse map and look at whether - // we should return 405 or 404. - if (params && r) { - cacheVal = { - methods: reverse, - name: r.name, - params: params, - spec: r.spec - }; +/** + * Returns true if the router generated a 404 for an options request. + * + * TODO: this is relevant for CORS only. Should move this out eventually to a + * userland middleware? This also seems a little like overreach, as there is no + * option to opt out of this behavior today. + * + * @private + * @static + * @function _optionsError + * @param {Object} req - the request object + * @param {Object} res - the response object + * @returns {Boolean} is options error + */ +Router._optionsError = function _optionsError(req, res) { + var pathname = req.getUrl().pathname; + return req.method === 'OPTIONS' && pathname === '*'; +}; - if (versioned) { - req._matchedVersion = maxV; - cacheVal.matchedVersion = maxV; - } +/** + * Default route, when no route found + * Responds with a ResourceNotFoundError error. + * + * @private + * @memberof Router + * @instance + * @function defaultRoute + * @param {Request} req - request + * @param {Response} res - response + * @param {Function} next - next + * @returns {undefined} no return value + */ +Router.prototype.defaultRoute = function defaultRoute(req, res, next) { + var self = this; + var pathname = req.getUrl().pathname; - this.cache.set(cacheKey, cacheVal); - res.methods = reverse.slice(); - callback(null, cacheVal, shallowCopy(params)); + // Allow CORS + if (Router._optionsError(req, res, pathname)) { + res.send(200); + next(null, req, res); return; } - if (typed) { - callback(new UnsupportedMediaTypeError(ct)); - return; - } + // Check for 405 instead of 404 + var allowedMethods = http.METHODS.filter(function some(method) { + return method !== req.method && self._registry.lookup(method, pathname); + }); - if (versioned) { - callback( - new InvalidVersionError( - '%s is not supported by %s %s', - req.version() || '?', - req.method, - req.path() - ) + if (allowedMethods.length) { + res.methods = allowedMethods; + res.setHeader('Allow', allowedMethods.join(', ')); + var methodErr = new MethodNotAllowedError( + '%s is not allowed', + req.method ); + next(methodErr, req, res); return; } - // Check for 405 instead of 404 - var j; - var urls = Object.keys(this.reverse); - - for (j = 0; j < urls.length; j++) { - if (matchURL(new RegExp(urls[j]), req)) { - res.methods = this.reverse[urls[j]].slice(); - res.setHeader('Allow', res.methods.join(', ')); - - var err = new MethodNotAllowedError( - '%s is not allowed', - req.method - ); - callback(err); - return; - } - } - // clean up the url in case of potential xss // https://github.com/restify/node-restify/issues/1018 - var cleanedUrl = url.parse(req.url).pathname; - callback(new ResourceNotFoundError('%s does not exist', cleanedUrl)); + var err = new ResourceNotFoundError('%s does not exist', pathname); + next(err, req, res); }; /** - * Find a route by path. Scans the route list for a route with the same RegEx. - * i.e. /foo/:param1/:param2 would match an existing route with different - * parameter names /foo/:id/:name since the compiled RegExs match. + * Generate route name * - * @public - * @function findByPath - * @param {String | RegExp} path - a path to find a route for. - * @param {Object} options - an options object - * @returns {Object} returns the route if a match is found + * @private + * @memberof Router + * @instance + * @function _getRouteName + * @param {String|undefined} name - Name of the route + * @param {String} method - HTTP method + * @param {String} path - path + * @returns {String} name of the route */ -Router.prototype.findByPath = function findByPath(path, options) { - assert.string(path, 'path'); - assert.object(options, 'options'); - assert.string(options.method, 'options.method'); - - var route; - var routes = this.routes[options.method] || []; - var routeRegex = compileURL({ - url: path, - flags: options.flags, - urlParamPattern: options.urlParamPattern, - strict: this.strict - }); +Router.prototype._getRouteName = function _getRouteName(name, method, path) { + // Generate name + if (!name) { + name = method + '-' + path; + name = name.replace(/\W/g, '').toLowerCase(); + } - for (var i = 0; i < routes.length; i++) { - if (routeRegex.toString() === routes[i].path.toString()) { - route = routes[i]; - break; - } + // Avoid name conflict: GH-401 + if (this._registry.get()[name]) { + name += uuid.v4().substr(0, 7); } - return route; + + return name; }; /** - * toString() serialization. + * Setup request and calls _onRequest to run middlewares and call router * - * @public - * @function toString - * @returns {String} stringified router + * @private + * @memberof Router + * @instance + * @function _dtraceStart + * @param {Request} req - the request object + * @returns {undefined} no return value + * @fires Request,Response#request */ -Router.prototype.toString = function toString() { - var self = this; - var str = this.name + ':\n'; - - Object.keys(this.routes).forEach(function forEach(k) { - var routes = self.routes[k].map(function map(r) { - return r.name; - }); +Router.prototype._dtraceStart = function _dtraceStart(req) { + if (!req.dtrace) { + return; + } - str += '\t\t' + k + ': [' + routes.join(', ') + ']\n'; + dtrace._rstfy_probes['route-start'].fire(function fire() { + return [ + req.serverName, + req.route.name, + req._dtraceId, + req.method, + req.href(), + req.headers + ]; }); - - return str; }; /** - * Return information about the routes registered in the router. + * Setup request and calls _onRequest to run middlewares and call router * - * @public - * @returns {object} The routes in the router. + * @private + * @memberof Router + * @instance + * @function _dtraceEnd + * @param {Request} req - the request object + * @param {Response} res - the response object + * @returns {undefined} no return value + * @fires Request,Response#request */ -Router.prototype.getDebugInfo = function getRoutes() { - var self = this; - var routeInfo = []; - - _.forOwn(self.mounts, function forOwn(value, routeName) { - if (self.mounts.hasOwnProperty(routeName)) { - var mountedRoute = self.mounts[routeName]; - var routeRegex = mountedRoute.path; - - routeInfo.push({ - name: mountedRoute.name, - method: mountedRoute.method.toLowerCase(), - input: mountedRoute.spec.path, - compiledRegex: cloneRegexp(routeRegex), - // any url params are saved on the regex object as a key/val - // bucket. - compiledUrlParams: - routeRegex.restifyParams && - Object.keys(routeRegex.restifyParams).length > 0 - ? shallowCopy(routeRegex.restifyParams) - : null, - versions: - mountedRoute.versions.length > 1 - ? mountedRoute.versions - : null - }); - } - }); +Router.prototype._dtraceEnd = function _dtraceEnd(req, res) { + if (!req.dtrace) { + return; + } - return routeInfo; + dtrace._rstfy_probes['route-done'].fire(function fire() { + return [ + req.serverName, + req.route.name, + req._dtraceId, + res.statusCode || 200, + res.headers() + ]; + }); }; + +module.exports = Router; diff --git a/lib/routerRegistryRadix.js b/lib/routerRegistryRadix.js new file mode 100644 index 000000000..5a291a329 --- /dev/null +++ b/lib/routerRegistryRadix.js @@ -0,0 +1,137 @@ +'use strict'; + +var assert = require('assert-plus'); +var FindMyWay = require('find-my-way'); +var Chain = require('./chain'); + +/** + * Radix tree based router registry backed by `find-my-way` + * + * @class RouterRegistryRadix + * @public + */ +function RouterRegistryRadix() { + this._findMyWay = new FindMyWay(); + this._routes = {}; +} + +/** + * Adds a route. + * + * @public + * @memberof Router + * @instance + * @function add + * @param {Object} route - an route object + * @param {String} route.name - name of the route + * @param {String} route.method - HTTP method + * @param {String} route.path - any String accepted by + * [find-my-way](https://github.com/delvedor/find-my-way) + * @param {Chain} route.chain - Chain instance + * @returns {Boolean} true + */ +RouterRegistryRadix.prototype.add = function add(route) { + assert.object(route, 'route'); + assert.string(route.method, 'route.method'); + assert.string(route.path, 'path'); + assert.ok(route.chain instanceof Chain, 'route.chain'); + + this._findMyWay.on( + route.method, + route.path, + function onRoute(req, res, next) { + route.chain.run(req, res, next); + }, + { + route: route + } + ); + + this._routes[route.name] = route; + + return route; +}; + +/** + * Removes a route. + * + * @public + * @memberof RouterRegistryRadix + * @instance + * @function remove + * @param {String} name - the route name + * @returns {Object|undefined} removed route if found + */ +RouterRegistryRadix.prototype.remove = function remove(name) { + assert.string(name, 'name'); + + // check for route + var route = this._routes[name]; + if (!route) { + return undefined; + } + + // remove from registry + this._findMyWay.off(route.method, route.path); + delete this._routes[name]; + + return route; +}; + +/** + * Registry for route + * + * @public + * @memberof RouterRegistryRadix + * @instance + * @function Registry + * @param {String} method - method + * @param {String} pathname - pathname + * @returns {Chain|undefined} handler or undefined + */ +RouterRegistryRadix.prototype.lookup = function lookup(method, pathname) { + assert.string(method, 'method'); + assert.string(pathname, 'pathname'); + + var fmwRoute = this._findMyWay.find(method, pathname); + + // Not found + if (!fmwRoute) { + return undefined; + } + + // Call handler chain + return { + route: fmwRoute.store.route, + params: fmwRoute.params, + handler: fmwRoute.handler + }; +}; + +/** + * Get registry + * + * @public + * @memberof RouterRegistryRadix + * @instance + * @function toString + * @returns {String} stringified RouterRegistryRadix + */ +RouterRegistryRadix.prototype.get = function get() { + return this._routes; +}; + +/** + * toString() serialization. + * + * @public + * @memberof RouterRegistryRadix + * @instance + * @function toString + * @returns {String} stringified RouterRegistryRadix + */ +RouterRegistryRadix.prototype.toString = function toString() { + return this._findMyWay.prettyPrint(); +}; + +module.exports = RouterRegistryRadix; diff --git a/lib/server.js b/lib/server.js index 976babb59..40de7104e 100644 --- a/lib/server.js +++ b/lib/server.js @@ -12,12 +12,10 @@ var _ = require('lodash'); var assert = require('assert-plus'); var errors = require('restify-errors'); var mime = require('mime'); -var once = require('once'); -var semver = require('semver'); var spdy = require('spdy'); -var uuid = require('uuid'); var vasync = require('vasync'); +var Chain = require('./chain'); var dtrace = require('./dtrace'); var formatters = require('./formatters'); var shallowCopy = require('./utils').shallowCopy; @@ -36,8 +34,6 @@ patchRequest(http.IncomingMessage); ///--- Globals var sprintf = util.format; -var maxSatisfying = semver.maxSatisfying; -var ResourceNotFoundError = errors.ResourceNotFoundError; var PROXY_EVENTS = [ 'clientError', 'close', @@ -56,11 +52,10 @@ var PROXY_EVENTS = [ * @class * @param {Object} options - an options object * @param {String} options.name - Name of the server. + * @param {Boolean} [options.dtrace=false] - enable DTrace support * @param {Router} options.router - Router * @param {Object} options.log - [bunyan](https://github.com/trentm/node-bunyan) * instance. - * @param {String|Array} [options.version] - Default version(s) to use in all - * routes. * @param {String[]} [options.acceptable] - List of content-types this * server can respond with. * @param {String} [options.url] - Once listen() is called, this will be filled @@ -74,6 +69,7 @@ var PROXY_EVENTS = [ * @param {Boolean} [options.handleUncaughtExceptions=false] - When true restify * will use a domain to catch and respond to any uncaught * exceptions that occur in it's handler stack. + * Comes with significant negative performance impact. * [bunyan](https://github.com/trentm/node-bunyan) instance. * response header, default is `restify`. Pass empty string to unset the header. * @param {Object} [options.spdy] - Any options accepted by @@ -83,13 +79,17 @@ var PROXY_EVENTS = [ * @param {Boolean} [options.handleUpgrades=false] - Hook the `upgrade` event * from the node HTTP server, pushing `Connection: Upgrade` requests through the * regular request handling chain. + * @param {Boolean} [options.onceNext=false] - Prevents calling next multiple + * times + * @param {Boolean} [options.strictNext=false] - Throws error when next() is + * called more than once, enabled onceNext option * @param {Object} [options.httpsServerOptions] - Any options accepted by * [node-https Server](http://nodejs.org/api/https.html#https_https). * If provided the following restify server options will be ignored: * spdy, ca, certificate, key, passphrase, rejectUnauthorized, requestCert and * ciphers; however these can all be specified on httpsServerOptions. - * @param {Boolean} [options.strictRouting=false] - If set, Restify - * will treat "/foo" and "/foo/" as different paths. + * @param {Boolean} [options.noWriteContinue=false] - prevents + * `res.writeContinue()` in `server.on('checkContinue')` when proxing * @example * var restify = require('restify'); * var server = restify.createServer(); @@ -103,22 +103,36 @@ function Server(options) { assert.object(options.log, 'options.log'); assert.object(options.router, 'options.router'); assert.string(options.name, 'options.name'); + assert.optionalBool( + options.handleUncaughtExceptions, + 'options.handleUncaughtExceptions' + ); + assert.optionalBool(options.dtrace, 'options.dtrace'); + assert.optionalBool(options.socketio, 'options.socketio'); + assert.optionalBool(options.onceNext, 'options.onceNext'); + assert.optionalBool(options.strictNext, 'options.strictNext'); var self = this; EventEmitter.call(this); - this.before = []; - this.chain = []; + this.onceNext = !!options.onceNext; + this.strictNext = !!options.strictNext; + this.preChain = new Chain({ + onceNext: this.onceNext, + strictNext: this.strictNext + }); + this.useChain = new Chain({ + onceNext: this.onceNext, + strictNext: this.strictNext + }); this.log = options.log; this.name = options.name; this.handleUncaughtExceptions = options.handleUncaughtExceptions || false; this.router = options.router; - this.routes = {}; this.secure = false; this.socketio = options.socketio || false; - this._once = options.strictNext === false ? once : once.strict; - this.versions = options.versions || options.version || []; + this.dtrace = options.dtrace || false; this._inflightRequests = 0; var fmt = mergeFormatters(options.formatters); @@ -191,29 +205,18 @@ function Server(options) { res.writeContinue(); } - self._setupRequest(req, res); - self._handle(req, res, true); + self._onRequest(req, res); }); if (options.handleUpgrades) { this.server.on('upgrade', function onUpgrade(req, socket, head) { req._upgradeRequest = true; var res = upgrade.createResponse(req, socket, head); - self._setupRequest(req, res); - self._handle(req, res); + self._onRequest(req, res); }); } - this.server.on('request', function onRequest(req, res) { - self.emit('request', req, res); - - if (options.socketio && /^\/socket\.io.*/.test(req.url)) { - return; - } - - self._setupRequest(req, res); - self._handle(req, res); - }); + this.server.on('request', this._onRequest.bind(this)); this.__defineGetter__('maxHeadersCount', function getMaxHeadersCount() { return self.server.maxHeadersCount; @@ -321,19 +324,18 @@ Server.prototype.close = function close(callback) { * @typedef {String|Regexp |Object} Server~methodOpts * @type {Object} * @property {String} name a name for the route - * @property {String|Regexp} path a string or regex matching the route - * @property {String|String[]} version versions supported by this route + * @property {String} path can be any String accepted by + * [find-my-way](https://github.com/delvedor/find-my-way) * @example * // a static route * server.get('/foo', function(req, res, next) {}); * // a parameterized route * server.get('/foo/:bar', function(req, res, next) {}); * // a regular expression - * server.get(/^\/([a-zA-Z0-9_\.~-]+)\/(.*)/, function(req, res, next) {}); + * server.get('/example/:file(^\\d+).png', function(req, res, next) {}); * // an options object * server.get({ * path: '/foo', - * version: ['1.0.0', '2.0.0'] * }, function(req, res, next) {}); */ @@ -467,8 +469,9 @@ Server.prototype.pre = function pre() { var self = this; var handlers = Array.prototype.slice.call(arguments); - argumentsToChain(handlers).forEach(function forEach(h) { - self.before.push(h); + argumentsToChain(handlers).forEach(function forEach(handler) { + handler._name = handler.name || 'pre-' + self.preChain.count(); + self.preChain.add(handler); }); return this; @@ -497,8 +500,9 @@ Server.prototype.use = function use() { var self = this; var handlers = Array.prototype.slice.call(arguments); - argumentsToChain(handlers).forEach(function forEach(h) { - self.chain.push(h); + argumentsToChain(handlers).forEach(function forEach(handler) { + handler._name = handler.name || 'use-' + self.useChain.count(); + self.useChain.add(handler); }); return this; @@ -538,57 +542,6 @@ Server.prototype.param = function param(name, fn) { return this; }; -/** - * Piggy-backs on the `server.use` method. It attaches a new middleware - * function that only fires if the specified version matches the request. - * - * Note that if the client does not request a specific version, the middleware - * function always fires. If you don't want this set a default version with a - * pre handler on requests where the client omits one. - * - * Exposes an API: - * server.versionedUse("version", function (req, res, next, ver) { - * // do stuff that only applies to routes of this API version - * }); - * - * @public - * @memberof Server - * @instance - * @function versionedUse - * @param {String|Array} versions - the version(s) the URL to respond to - * @param {Function} fn - the middleware function to execute, the - * fourth parameter will be the selected - * version - * @returns {undefined} no return value - */ -Server.prototype.versionedUse = function versionedUse(versions, fn) { - if (!Array.isArray(versions)) { - versions = [versions]; - } - assert.arrayOfString(versions, 'versions'); - - versions.forEach(function forEach(v) { - if (!semver.valid(v)) { - throw new TypeError('%s is not a valid semver', v); - } - }); - - this.use(function _versionedUse(req, res, next) { - var ver; - - if ( - req.version() === '*' || - (ver = maxSatisfying(versions, req.version()) || false) - ) { - fn.call(this, req, res, next, ver); - } else { - next(); - } - }); - - return this; -}; - /** * Removes a route from the server. * You pass in the route 'blob' you got from a mount call. @@ -598,17 +551,12 @@ Server.prototype.versionedUse = function versionedUse(versions, fn) { * @instance * @function rm * @throws {TypeError} on bad input. - * @param {String} route - the route name. + * @param {String} routeName - the route name. * @returns {Boolean} true if route was removed, false if not. */ -Server.prototype.rm = function rm(route) { - var r = this.router.unmount(route); - - if (r && this.routes[r]) { - delete this.routes[r]; - } - - return r; +Server.prototype.rm = function rm(routeName) { + var route = this.router.unmount(routeName); + return !!route; }; ///--- Info and debug methods @@ -667,7 +615,6 @@ Server.prototype.inflightRequests = function inflightRequests() { * input: '/', * compiledRegex: /^[\/]*$/, * compiledUrlParams: null, - * versions: null, * handlers: [Array] * } * ], @@ -704,22 +651,30 @@ Server.prototype.getDebugInfo = function getDebugInfo() { // output the actual routes registered with restify var routeInfo = self.router.getDebugInfo(); + + var preHandlers = self.preChain.getHandlers().map(funcNameMapper); + var useHandlers = self.useChain.getHandlers().map(funcNameMapper); + // get each route's handler chain - _.forEach(routeInfo, function forEach(value, key) { - var routeName = value.name; - value.handlers = self.routes[routeName].map(funcNameMapper); + var routes = _.map(routeInfo, function mapValues(route) { + route.handlers = Array.prototype.concat.call( + // TODO: should it contain use handlers? + useHandlers, + route.handlers.map(funcNameMapper) + ); + return route; }); self._debugInfo = { - routes: routeInfo, + routes: routes, server: { formatters: self.formatters, // if server is not yet listening, addressInfo may be null address: addressInfo && addressInfo.address, port: addressInfo && addressInfo.port, inflightRequests: self.inflightRequests(), - pre: self.before.map(funcNameMapper), - use: self.chain.map(funcNameMapper), + pre: preHandlers, + use: useHandlers, after: self.listeners('after').map(funcNameMapper) } }; @@ -762,7 +717,6 @@ Server.prototype.getDebugInfo = function getDebugInfo() { Server.prototype.toString = function toString() { var LINE_FMT = '\t%s: %s\n'; var SUB_LINE_FMT = '\t\t%s: %s\n'; - var self = this; var str = ''; function handlersToString(arr) { @@ -780,421 +734,342 @@ Server.prototype.toString = function toString() { str += sprintf(LINE_FMT, 'Accepts', this.acceptable.join(', ')); str += sprintf(LINE_FMT, 'Name', this.name); - str += sprintf(LINE_FMT, 'Pre', handlersToString(this.before)); - str += sprintf(LINE_FMT, 'Router', this.router.toString()); + str += sprintf( + LINE_FMT, + 'Pre', + handlersToString(this.preChain.getHandlers()) + ); + str += sprintf(LINE_FMT, 'Router', ''); + this.router + .toString() + .split('\n') + .forEach(function forEach(line) { + str += sprintf('\t\t%s\n', line); + }); str += sprintf(LINE_FMT, 'Routes', ''); - Object.keys(this.routes).forEach(function forEach(k) { - var handlers = handlersToString(self.routes[k]); - str += sprintf(SUB_LINE_FMT, k, handlers); + _.forEach(this.router.getRoutes(), function forEach(route, routeName) { + var handlers = handlersToString(route.chain.getHandlers()); + str += sprintf(SUB_LINE_FMT, routeName, handlers); }); str += sprintf(LINE_FMT, 'Secure', this.secure); str += sprintf(LINE_FMT, 'Url', this.url); - str += sprintf( - LINE_FMT, - 'Version', - Array.isArray(this.versions) ? this.versions.join() : this.versions - ); return str; }; ///--- Private methods +// Lifecycle: +// +// 1. _onRequest (handle new request, setup request and triggers pre) +// 2. _runPre +// 3. _afterPre (runs after pre handlers are finisehd, triggers route) +// 4. _runRoute (route lookup) +// 5. _runUse (runs use handlers, if route exists) +// 6. Runs route handlers +// 7. _afterRoute (runs after route handlers are finised, +// triggers use) +// 8. _finishReqResCycle (on response "finish" and "error" events) +// +// Events: +// e.1 after (triggered when response is flushed) +// +// Errors: +// e.1 _onHandlerError (runs when next was called with an Error) +// e.2 _routeErrorResponse +// e.1 _onHandlerError (when, next('string') called, trigger route by name) +// e.2 _afterRoute + /** - * Route and run + * Setup request and calls _onRequest to run middlewares and call router * * @private * @memberof Server * @instance - * @function _routeAndRun - * @param {Request} req - request - * @param {Response} res - response - * @returns {undefined} no return value + * @function _onRequest + * @param {Object} req - the request object + * @param {Object} res - the response object + * @returns {undefined} no return value + * @fires Request,Response#request */ -Server.prototype._routeAndRun = function _routeAndRun(req, res) { +Server.prototype._onRequest = function _onRequest(req, res) { var self = this; - self._route(req, res, function _route(route, context) { - // emit 'routed' event after the req has been routed - self.emit('routed', req, res, route); - req.context = req.params = context; - req.route = route.spec; + this.emit('request', req, res); - var r = route ? route.name : null; - var chain = self.routes[r]; + // Skip Socket.io endpoints + if (this.socketio && /^\/socket\.io.*/.test(req.url)) { + return; + } - self._run(req, res, route, chain, function done(e) { - self._finishReqResCycle(req, res, route, e); + // Decorate req and res objects + self._setupRequest(req, res); + + // increment number of requests + self._inflightRequests++; + + // Run in domain to catch async errors + // It has significant negative performance impact + // Warning: this feature depends on the deprecated domains module + if (self.handleUncaughtExceptions) { + var handlerDomain = domain.create(); + handlerDomain.add(req); + handlerDomain.add(res); + handlerDomain.on('error', function onError(err) { + self._onHandlerError(err, req, res); }); - }); + handlerDomain.run(function run() { + self._runPre(req, res); + }); + } else { + self._runPre(req, res); + } }; /** - * Upon receivng a request, route the request, then run the chain of handlers. + * Run pre handlers * * @private * @memberof Server * @instance - * @function _handle - * @param {Object} req - the request object - * @param {Object} res - the response object + * @function _runPre + * @param {Object} req - the request object + * @param {Object} res - the response object * @returns {undefined} no return value + * @fires Request,Response#request */ -Server.prototype._handle = function _handle(req, res) { +Server.prototype._runPre = function _runPre(req, res) { var self = this; - // increment number of requests - self._inflightRequests++; // emit 'pre' event before we run the pre handlers self.emit('pre', req, res); - // run pre() handlers first before routing and running - if (self.before.length > 0) { - self._run(req, res, null, self.before, function _run(err) { - // Like with regular handlers, if we are provided an error, we - // should abort the middleware chain and fire after events. - if (err === false || err instanceof Error) { - self._finishReqResCycle(req, res, null, err); - return; - } + // Run "pre" + req._currentHandler = 'pre'; + req._timePreStart = process.hrtime(); - self._routeAndRun(req, res); - }); - } else { - self._routeAndRun(req, res); - } + self.preChain.run(req, res, function preChainDone(err) { + // Execution time of a handler with error can be significantly lower + req._timePreEnd = process.hrtime(); + self._afterPre(err, req, res); + }); }; /** - * Helper function to, when on router error, emit error events and then - * flush the err. + * After pre handlers finished * * @private * @memberof Server * @instance - * @function _routeErrorResponse - * @param {Request} req - the request object - * @param {Response} res - the response object - * @param {Error} err - error - * @returns {undefined} no return value + * @function _afterPre + * @param {Error|false|undefined} err - pre handler error + * @param {Request} req - request + * @param {Response} res - response + * @returns {undefined} no return value */ -Server.prototype._routeErrorResponse = function _routeErrorResponse( - req, - res, - err -) { +Server.prototype._afterPre = function _afterPre(err, req, res) { var self = this; - return self._emitErrorEvents( - req, - res, - null, - err, - function _emitErrorEvents() { - if (!res.headersSent) { - res.send(err); - } - return self._finishReqResCycle(req, res, null, err); - } - ); + // Handle error + if (err) { + self._onHandlerError(err, req, res); + self._finishReqResCycle(req, res, err); + return; + } + + // Stop + if (err === false) { + self._onHandlerStop(req, res); + return; + } + + self._runRoute(req, res); }; /** - * look into the router, find the route object that should match this request. - * if a route cannot be found, fire error events then flush the error out. + * Find route and run handlers * * @private * @memberof Server * @instance - * @function _route - * @param {Object} req - the request object - * @param {Object} res - the response object - * @param {String} [name] - name of the route - * @param {Function} cb - callback function + * @function _runRoute + * @param {Object} req - the request object + * @param {Object} res - the response object * @returns {undefined} no return value + * @fires Request,Response#request */ -Server.prototype._route = function _route(req, res, name, cb) { +Server.prototype._runRoute = function _runRoute(req, res) { var self = this; - if (typeof name === 'function') { - cb = name; - name = null; - - return this.router.find(req, res, function onRoute(err, route, ctx) { - var r = route ? route.name : null; - - if (err) { - // TODO: if its a 404 for OPTION method (likely a CORS - // preflight), return OK. This should move into userland. - if (optionsError(err, req, res)) { - res.send(200); - return self._finishReqResCycle(req, res, null, null); - } else { - return self._routeErrorResponse(req, res, err); - } - } else if (!r || !self.routes[r]) { - err = new ResourceNotFoundError(req.path()); - return self._routeErrorResponse(req, res, err); - } else { - // else no err, continue - return cb(route, ctx); - } - }); - } else { - return this.router.get(name, req, function get(err, route, ctx) { - if (err) { - return self._routeErrorResponse(req, res, err); - } else { - // else no err, continue - return cb(route, ctx); - } + var routeHandler = self.router.lookup(req, res); + + if (!routeHandler) { + self.router.defaultRoute(req, res, function afterRouter(err) { + self._afterRoute(err, req, res); }); + return; } + + // Emit routed + self.emit('routed', req, res, req.route); + + self._runUse(req, res, function afterUse() { + req._timeRouteStart = process.hrtime(); + routeHandler(req, res, function afterRouter(err) { + // Execution time of a handler with error can be significantly lower + req._timeRouteEnd = process.hrtime(); + self._afterRoute(err, req, res); + }); + }); }; /** - * `cb()` is called when execution is complete. "completion" can occur when: - * 1) all functions in handler chain have been executed - * 2) users invoke `next(false)`. this indicates the chain should stop - * executing immediately. - * 3) users invoke `next(err)`. this is sugar for calling res.send(err) and - * firing any error events. after error events are done firing, it will also - * stop execution. - * - * The goofy checks in next() are to make sure we fire the DTrace - * probes after an error might have been sent, as in a handler - * return next(new Error) is basically shorthand for sending an - * error via res.send(), so we do that before firing the dtrace - * probe (namely so the status codes get updated in the - * response). - * - * there are two important closure variables in this function as a result of - * the way `next()` is currently implemented. `next()` assumes logic is sync, - * and automatically calls cb() when a route is considered complete. however, - * for case #3, emitted error events are async and serial. this means the - * automatic invocation of cb() cannot occur: - * - * 1) `emittingErrors` - this boolean is set to true when the server is still - * emitting error events. this var is used to avoid automatic invocation of - * cb(), which is delayed until all async error events are fired. - * 2) `done` - when next is invoked with a value of `false`, or handler if + * After use handlers finished * * @private * @memberof Server * @instance - * @function _run - * @param {Object} req - the request object - * @param {Object} res - the response object - * @param {Object} route - the route object - * @param {Function[]} chain - array of handler functions - * @param {Function} cb - callback function - * @fires redirect - * @returns {undefined} no return value + * @function _afterRoute + * @param {Error|false|undefined} err - use handler error + * @param {Request} req - request + * @param {Response} res - response + * @returns {undefined} no return value */ -Server.prototype._run = function _run(req, res, route, chain, cb) { - var i = -1; - var id = dtrace.nextId(); - req._dtraceId = id; - - if (!req._anonFuncCount) { - // Counter used to keep track of anonymous functions. Used when a - // handler function is anonymous. This ensures we're using a - // monotonically increasing int for anonymous handlers through out the - // the lifetime of this request - req._anonFuncCount = 0; - } - var log = this.log; +Server.prototype._afterRoute = function _afterRoute(err, req, res) { var self = this; - var handlerName = null; - var emittingErrors = false; - cb = self._once(cb); - // attach a listener for 'close' and 'aborted' events, this will let us set - // a flag so that we can stop processing the request if the client closes - // the connection (or we lose the connection). - function _requestClose() { - req._connectionState = 'close'; - } - function _requestAborted() { - req._connectionState = 'aborted'; + res._handlersFinished = true; + + // Handle error + if (err) { + self._onHandlerError(err, req, res); + self._finishReqResCycle(req, res, err); + return; } - req.once('close', _requestClose); - req.once('aborted', _requestAborted); - // attach a listener for the response's 'redirect' event - res.on('redirect', function onRedirect(redirectLocation) { - self.emit('redirect', redirectLocation); - }); + // Trigger finish + self._finishReqResCycle(req, res, err); +}; - function next(arg) { - // default value of done determined by whether or not there is another - // function in the chain and/or if req was not already closed. we will - // consume the value of `done` after dealing with any passed in values - // of `arg`. - var done = false; - - if (typeof arg !== 'undefined') { - if (arg instanceof Error) { - // the emitting of the error events are async, so we can not - // complete this invocation of run() until it returns. set a - // flag so that the automatic invocation of cb() at the end of - // this function is bypassed. - emittingErrors = true; - // set the done flag - allows us to stop execution of handler - // chain now that an error has occurred. - done = true; - // now emit error events in serial and async - self._emitErrorEvents( - req, - res, - route, - arg, - function emitErrorsDone() { - res.send(arg); - return cb(arg); - } - ); - } else if (typeof arg === 'string') { - // GH-193, allow redirect - if (req._rstfy_chained_route) { - var _e = new errors.InternalError(); - log.error( - { - err: _e - }, - 'Multiple next("chain") calls not ' + 'supported' - ); - res.send(_e); - return false; - } - - // Stop running the rest of this route since we're redirecting. - // do this instead of setting done since the route technically - // isn't complete yet. - return self._route(req, res, arg, function _route(r, ctx) { - req.context = req.params = ctx; - req.route = r.spec; - - var _c = chain.slice(0, i + 1); - - function _uniq(fn) { - return _c.indexOf(fn) === -1; - } - - var _routes = self.routes[r.name] || []; - var _chain = _routes.filter(_uniq); - - req._rstfy_chained_route = true; - - // Need to fire DTrace done for previous handler here too. - if (i + 1 > 0 && chain[i] && !chain[i]._skip) { - req.endHandlerTimer(handlerName); - } - self._run(req, res, r, _chain, cb); - }); - } else if (arg === false) { - done = true; - } - } +/** + * Run use handlers + * + * @private + * @memberof Server + * @instance + * @function _runUse + * @param {Object} req - the request object + * @param {Object} res - the response object + * @param {Function} next - next + * @returns {undefined} no return value + * @fires Request,Response#request + */ +Server.prototype._runUse = function _runUse(req, res, next) { + var self = this; - // Fire DTrace done for the previous handler. - if (i + 1 > 0 && chain[i] && !chain[i]._skip) { - req.endHandlerTimer(handlerName); - } + // Run "use" + req._currentHandler = 'use'; + req._timeUseStart = process.hrtime(); - // Run the next handler up - if (!done && chain[++i] && !_reqClosed(req)) { - if (chain[i]._skip) { - return next(); - } + self.useChain.run(req, res, function useChainDone(err) { + // Execution time of a handler with error can be significantly lower + req._timeUseEnd = process.hrtime(); + self._afterUse(err, req, res, next); + }); +}; - if (log.trace()) { - log.trace('running %s', chain[i].name || '?'); - } +/** + * After use handlers finished + * + * @private + * @memberof Server + * @instance + * @function _afterUse + * @param {Error|false|undefined} err - use handler error + * @param {Request} req - request + * @param {Response} res - response + * @param {Function} next - next + * @returns {undefined} no return value + */ +Server.prototype._afterUse = function _afterUse(err, req, res, next) { + var self = this; - req._currentRoute = route !== null ? route.name : 'pre'; - handlerName = chain[i].name || 'handler-' + req._anonFuncCount++; - req._currentHandler = handlerName; - req.startHandlerTimer(handlerName); + // Handle error + if (err) { + self._onHandlerError(err, req, res); + self._finishReqResCycle(req, res, err); + return; + } - var n = self._once(next); + // Stop + if (err === false) { + self._onHandlerStop(req, res); + return; + } - // support ifError only if domains are on - if (self.handleUncaughtExceptions === true) { - n.ifError = ifError(n); - } - return chain[i].call(self, req, res, n); - } + next(); +}; - // if we have reached this last section of next(), then we are 'done' - // with this route. - dtrace._rstfy_probes['route-done'].fire(function fire() { - return [ - self.name, - route !== null ? route.name : 'pre', - id, - res.statusCode || 200, - res.headers() - ]; - }); +/** + * Runs after next(false) is called + * + * @private + * @memberof Server + * @instance + * @function _onHandlerStop + * @param {Request} req - request + * @param {Response} res - response + * @returns {undefined} no return value + */ +Server.prototype._onHandlerStop = function _onHandlerStop(req, res) { + res._handlersFinished = true; + this._finishReqResCycle(req, res); +}; - // if there is no route, it's because this is running the `pre` handler - // chain. - if (route === null) { - self.emit('preDone', req, res); - } else { - req.removeListener('close', _requestClose); - req.removeListener('aborted', _requestAborted); - self.emit('done', req, res, route); - } +/** + * After route handlers finished + * NOTE: only called when last handler calls next([err]) + * + * @private + * @memberof Server + * @instance + * @function _onHandlerError + * @param {Error|String|undefined} err - router handler error or route name + * @param {Request} req - request + * @param {Response} res - response + * @returns {undefined} no return value + */ +Server.prototype._onHandlerError = function _onHandlerError(err, req, res) { + var self = this; - // if we have reached here, there are no more handlers in the chain, or - // we next(err), and we are done with the request. if errors are still - // being emitted (due to being async), skip calling cb now, that will - // happen after all error events are done being emitted. - if (emittingErrors === false) { - return cb(arg); + // Call route by name + if (_.isString(err)) { + var routeName = err; + var routeHandler = self.router.lookupByName(routeName, req, res); + + // Cannot find route by name, called when next('route-name') doesn't + // find any route, it's a 5xx error as it's a programatic error + if (!routeHandler) { + var routeByNameErr = new errors.RouteMissingError( + "Route by name doesn't exist" + ); + routeByNameErr.code = 'ENOEXIST'; + self._afterRoute(routeByNameErr, req, res); + return; } - - // don't really need to return anything, returning null to placate - // eslint. - return null; + routeHandler(req, res, function afterRouter(routeErr) { + self._afterRoute(routeErr, req, res); + }); + return; } - var n1 = self._once(next); - - dtrace._rstfy_probes['route-start'].fire(function fire() { - return [ - self.name, - route !== null ? route.name : 'pre', - id, - req.method, - req.href(), - req.headers - ]; - }); + res._handlersFinished = true; - req.timers = []; + // Preserve handler err for finish event + res.err = res.err || err; - if (!self.handleUncaughtExceptions) { - return n1(); - } else { - n1.ifError = ifError(n1); - // Add the uncaughtException error handler. - var d = domain.create(); - d.add(req); - d.add(res); - d.on('error', function onError(err) { - if (err._restify_next) { - err._restify_next(err); - } else { - log.trace({ err: err }, 'uncaughtException'); - self.emit('uncaughtException', req, res, route, err); - self._finishReqResCycle(req, res, route, err); - } - }); - return d.run(n1); - } + // Error happened in router handlers + self._routeErrorResponse(req, res, err); }; /** @@ -1210,21 +1085,169 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { */ Server.prototype._setupRequest = function _setupRequest(req, res) { var self = this; + + // Extend request + req._dtraceId = dtrace.nextId(); req.log = res.log = self.log; req._date = new Date(); - req._time = process.hrtime(); + req._timeStart = process.hrtime(); req.serverName = self.name; + req.params = {}; + req.timers = []; + req.dtrace = self.dtrace; + // Extend response res.acceptable = self.acceptable; res.formatters = self.formatters; res.req = req; res.serverName = self.name; + res._handlersFinished = false; + res._flushed = false; // set header only if name isn't empty string if (self.name !== '') { res.setHeader('Server', self.name); } - res.version = self.router.versions[self.router.versions.length - 1]; + + // Request lifecycle events + // attach a listener for 'close' events, this will let us set + // a flag so that we can stop processing the request if the client closes + // the connection (or we lose the connection). + // we consider a closed request as flushed from metrics point of view + function onClose() { + res._flushed = true; + req._timeFlushed = process.hrtime(); + + res.removeListener('finish', onFinish); + res.removeListener('error', onError); + req._connectionState = 'close'; + + // Request was aborted or closed. Override the status code + res.statusCode = 444; + + self._finishReqResCycle(req, res, new errors.RequestCloseError()); + } + req.once('close', onClose); + + // Response lifecycle events + function onFinish() { + var processHrTime = process.hrtime(); + + res._flushed = true; + req._timeFlushed = processHrTime; + + // Response may get flushed before handler callback is triggered + req._timeFlushed = processHrTime; + req._timePreEnd = req._timePreEnd || processHrTime; + req._timeUseEnd = req._timeUseEnd || processHrTime; + req._timeRouteEnd = req._timeRouteEnd || processHrTime; + + req.removeListener('close', onClose); + res.removeListener('error', onError); + + // Do not trigger false + self._finishReqResCycle(req, res); + } + function onError(err) { + res._flushed = true; + req._timeFlushed = process.hrtime(); + + req.removeListener('close', onClose); + res.removeListener('finish', onFinish); + self._finishReqResCycle(req, res, err); + } + res.once('finish', onFinish); + res.once('error', onError); + + // attach a listener for the response's 'redirect' event + res.on('redirect', function onRedirect(redirectLocation) { + self.emit('redirect', redirectLocation); + }); +}; + +/** + * Maintaining the end of the request-response cycle: + * - emitting after event + * - updating inflight requests metrics + * Check if the response is finished, and if not, wait for it before firing the + * response object. + * + * @private + * @memberof Server + * @instance + * @function _finishReqResCycle + * @param {Object} req - the request object + * @param {Object} res - the response object + * @param {Object} [err] - a possible error as a result of failed route matching + * or failed execution of the handler array. + * @returns {undefined} no return value + */ +Server.prototype._finishReqResCycle = function _finishReqResCycle( + req, + res, + err +) { + var self = this; + var route = req.route; // can be undefined when 404 or error + + if (res._finished) { + return; + } + + if (res._flushed && res._handlersFinished) { + // decrement number of requests + self._inflightRequests--; + res._finished = true; + req._timeFinished = process.hrtime(); + + // after event has signature of function(req, res, route, err) {...} + self.emit('after', req, res, route, err || res.err); + } else { + // preserve error for actual finish + res.err = err; + } +}; + +/** + * Helper function to, when on router error, emit error events and then + * flush the err. + * + * @private + * @memberof Server + * @instance + * @function _routeErrorResponse + * @param {Request} req - the request object + * @param {Response} res - the response object + * @param {Error} err - error + * @returns {undefined} no return value + */ +Server.prototype._routeErrorResponse = function _routeErrorResponse( + req, + res, + err +) { + var self = this; + + // if (self.handleUncaughtExceptions) { + if (self.listenerCount('uncaughtException') > 1) { + self.emit('uncaughtException', req, res, req.route, err); + return; + } + + self._emitErrorEvents(req, res, null, err, function emitError(emitErr) { + // Prevent double handling + if (res._sent) { + return; + } + + // Expose only knonw errors + if (_.isNumber(err.statusCode)) { + res.send(err); + return; + } + + res.send(new errors.InternalError(emitErr || err)); + }); }; /** @@ -1297,78 +1320,8 @@ Server.prototype._emitErrorEvents = function _emitErrorEvents( ); }; -/** - * Wrapper method for emitting the after event. this is needed in scenarios - * where the async formatter has failed, and the ot assume that the - * original res.send() status code is no longer valid (it is now a 500). check - * if the response is finished, and if not, wait for it before firing the - * response object. - * - * @private - * @memberof Server - * @instance - * @function _finishReqResCycle - * @param {Object} req - the request object - * @param {Object} res - the response object - * @param {Object} [route] - the matched route - * @param {Object} [err] - a possible error as a result of failed route matching - * or failed execution of the handler array. - * @returns {undefined} no return value - */ -Server.prototype._finishReqResCycle = function _finishReqResCycle( - req, - res, - route, - err -) { - var self = this; - - // res.finished is set by node core's response object, when - // res.end() completes. if the request was closed by the client, then emit - // regardless of res status. - - // after event has signature of function(req, res, route, err) {...} - if (!res.finished && !_reqClosed(req)) { - res.once('finish', function resFinished() { - self.emit('after', req, res, route, err || res.formatterErr); - }); - } else { - // if there was an error in the processing of that request, use it. - // if not, check to see if the request was closed or aborted early and - // create an error out of that for audit logging. - var afterErr = err; - - if (!afterErr) { - if (req._connectionState === 'close') { - afterErr = new errors.RequestCloseError(); - } else if (req._connectionState === 'aborted') { - afterErr = new errors.RequestAbortedError(); - } - } - - self.emit('after', req, res, route, afterErr); - } - - // decrement number of requests - self._inflightRequests--; -}; - ///--- Helpers -/** - * Helper function that returns true if the request was closed or aborted. - * - * @private - * @function _reqClosed - * @param {Object} req - the request object - * @returns {Boolean} is req closed or aborted - */ -function _reqClosed(req) { - return ( - req._connectionState === 'close' || req._connectionState === 'aborted' - ); -} - /** * Verify and flatten a nested array of request handlers. * @@ -1465,54 +1418,6 @@ function mergeFormatters(fmt) { }; } -/** - * Attaches ifError function attached to the `next` function in handler chain. - * uses a closure to maintain ref to next. - * - * @private - * @deprecated since 5.x - * @function ifError - * @param {Function} next - the next function - * @returns {Function} error handler - */ -function ifError(next) { - /** - * @throws will throw if an error is passed in. - * @private - * @function _ifError - * @param {Object} err - an error object - * @returns {undefined} no return value - */ - function _ifError(err) { - if (err) { - err._restify_next = next; - throw err; - } - } - - return _ifError; -} - -/** - * Returns true if the router generated a 404 for an options request. - * - * TODO: this is relevant for CORS only. Should move this out eventually to a - * userland middleware? This also seems a little like overreach, as there is no - * option to opt out of this behavior today. - * - * @private - * @function optionsError - * @param {Object} err - an error object - * @param {Object} req - the request object - * @param {Object} res - the response object - * @returns {Boolean} is options error - */ -function optionsError(err, req, res) { - return ( - err.statusCode === 404 && req.method === 'OPTIONS' && req.url === '*' - ); -} - /** * Map an Error's .name property into the actual event name that is emitted * by the restify server object. @@ -1558,51 +1463,16 @@ function serverMethodFactory(method) { throw new TypeError('handler (function) required'); } - var chain = []; - var route; - var self = this; - - function addHandler(h) { - assert.func(h, 'handler'); - - chain.push(h); - } - opts.method = method; - opts.versions = opts.versions || opts.version || self.versions; - - if (!Array.isArray(opts.versions)) { - opts.versions = [opts.versions]; - } + opts.path = opts.path || opts.url; - if (!opts.name) { - opts.name = method + '-' + (opts.path || opts.url); - - if (opts.versions.length > 0) { - opts.name += '-' + opts.versions.join('--'); - } - - opts.name = opts.name.replace(/\W/g, '').toLowerCase(); - - if (this.router.mounts[opts.name]) { - // GH-401 - opts.name += uuid.v4().substr(0, 7); - } - } - - if (!(route = this.router.mount(opts))) { - return false; - } - - this.chain.forEach(addHandler); // We accept both a variable number of handler functions, a // variable number of nested arrays of handler functions, or a mix // of both - argumentsToChain(Array.prototype.slice.call(arguments, 1)).forEach( - addHandler - ); - this.routes[route] = chain; + var handlers = Array.prototype.slice.call(arguments, 1); + var chain = argumentsToChain(handlers); + var route = this.router.mount(opts, chain); - return route; + return route.name; }; } diff --git a/lib/upgrade.js b/lib/upgrade.js index 9c6116e90..50d59c0ba 100644 --- a/lib/upgrade.js +++ b/lib/upgrade.js @@ -107,9 +107,7 @@ function notImplemented(method) { }; } else { return function throws() { - throw new Error( - 'Method ' + method.name + ' is not ' + 'implemented!' - ); + throw new Error('Method ' + method.name + ' is not implemented!'); }; } } diff --git a/package.json b/package.json index 55233d7fb..fd7de8fd0 100644 --- a/package.json +++ b/package.json @@ -94,10 +94,10 @@ "dependencies": { "assert-plus": "^1.0.0", "bunyan": "^1.8.12", - "clone-regexp": "^1.0.0", "csv": "^1.1.1", "escape-regexp-component": "^1.0.2", "ewma": "^2.0.1", + "find-my-way": "^1.10.0", "formidable": "^1.1.1", "http-signature": "^1.2.0", "lodash": "^4.17.4", diff --git a/test/chain.test.js b/test/chain.test.js new file mode 100644 index 000000000..80ca19946 --- /dev/null +++ b/test/chain.test.js @@ -0,0 +1,280 @@ +'use strict'; +/* eslint-disable func-names */ + +var Chain = require('../lib/chain'); + +if (require.cache[__dirname + '/lib/helper.js']) { + delete require.cache[__dirname + '/lib/helper.js']; +} +var helper = require('./lib/helper.js'); + +///--- Globals + +var test = helper.test; + +test('calls all the handlers', function(t) { + var chain = new Chain(); + var counter = 0; + + chain.add(function(req, res, next) { + counter++; + next(); + }); + chain.add(function(req, res, next) { + counter++; + next(); + }); + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function() { + t.equal(counter, 2); + t.done(); + } + ); +}); + +test('abort with Error in next', function(t) { + var chain = new Chain(); + var counter = 0; + var myError = new Error('Foo'); + + chain.add(function(req, res, next) { + counter++; + next(myError); + }); + chain.add(function(req, res, next) { + counter++; + next(); + }); + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function(err) { + t.deepEqual(err, myError); + t.equal(counter, 1); + t.done(); + } + ); +}); + +test('abort with false in next', function(t) { + var chain = new Chain(); + + chain.add(function(req, res, next) { + next(false); + }); + chain.add(function(req, res, next) { + t.fail('Should not be here'); + next(); + }); + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function(err) { + t.equal(err, false); + t.done(); + } + ); +}); + +test('abort with closed request', function(t) { + var chain = new Chain(); + var closed = false; + + chain.add(function(req, res, next) { + closed = true; + next(); + }); + chain.add(function(req, res, next) { + t.fail('Should not be here'); + }); + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return closed; + } + }, + {}, + function(err) { + t.ifError(err); + t.done(); + } + ); +}); + +test('cals error middleware', function(t) { + t.expect(2); + var chain = new Chain(); + var myError = new Error('Foo'); + + chain.add(function(req, res, next) { + next(myError); + }); + chain.add(function(err, req, res, next) { + t.deepEqual(err, myError); + next(err); + }); + chain.add(function(req, res, next) { + t.fail('Should not be here'); + }); + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function(err) { + t.deepEqual(err, myError); + t.done(); + } + ); +}); + +test('onceNext prevents double next calls', function(t) { + var doneCalled = 0; + var chain = new Chain({ + onceNext: true + }); + + chain.add(function foo(req, res, next) { + next(); + next(); + }); + + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function(err) { + t.ifError(err); + doneCalled++; + t.equal(doneCalled, 1); + t.done(); + } + ); +}); + +test('throws error for double next calls in strictNext mode', function(t) { + var doneCalled = 0; + var chain = new Chain({ + strictNext: true + }); + + chain.add(function foo(req, res, next) { + next(); + next(); + }); + + try { + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function(err) { + t.ifError(err); + doneCalled++; + t.equal(doneCalled, 1); + t.done(); + } + ); + } catch (err) { + t.equal(err.message, "next shouldn't be called more than once"); + } +}); + +test('calls req.startHandlerTimer', function(t) { + var chain = new Chain(); + + chain.add(function foo(req, res, next) { + next(); + }); + + chain.run( + { + startHandlerTimer: function(handleName) { + t.equal(handleName, 'foo'); + t.done(); + }, + endHandlerTimer: function() {}, + closed: function() { + return false; + } + }, + {}, + function() {} + ); +}); + +test('calls req.endHandlerTimer', function(t) { + var chain = new Chain(); + + chain.add(function foo(req, res, next) { + next(); + }); + + chain.run( + { + startHandlerTimer: function() {}, + endHandlerTimer: function(handleName) { + t.equal(handleName, 'foo'); + t.done(); + }, + closed: function() { + return false; + } + }, + {}, + function() {} + ); +}); + +test('count returns with the number of registered handlers', function(t) { + var chain = new Chain(); + chain.add(function(req, res, next) {}); + chain.add(function(req, res, next) {}); + t.equal(chain.count(), 2); + t.end(); +}); + +test('getHandlers returns with the array of handlers', function(t) { + var chain = new Chain(); + var handlers = [function(req, res, next) {}, function(req, res, next) {}]; + chain.add(handlers[0]); + chain.add(handlers[1]); + t.deepEqual(chain.getHandlers(), handlers); + t.end(); +}); diff --git a/test/formatter.test.js b/test/formatter.test.js index a451425cd..b1ff61cef 100644 --- a/test/formatter.test.js +++ b/test/formatter.test.js @@ -210,23 +210,21 @@ test( } ); -test( - 'default jsonp formatter should escape ' + 'line and paragraph separators', - function(t) { - // ensure client accepts only a type not specified by server - var opts = { - path: '/jsonpSeparators', - headers: { - accept: 'application/javascript' - } - }; +// eslint-disable-next-line +test('default jsonp formatter should escape line and paragraph separators', function(t) { + // ensure client accepts only a type not specified by server + var opts = { + path: '/jsonpSeparators', + headers: { + accept: 'application/javascript' + } + }; - CLIENT.get(opts, function(err, req, res, data) { - t.ifError(err); - t.ok(req); - t.ok(res); - t.equal(data, '"\\u2028\\u2029"'); - t.end(); - }); - } -); + CLIENT.get(opts, function(err, req, res, data) { + t.ifError(err); + t.ok(req); + t.ok(res); + t.equal(data, '"\\u2028\\u2029"'); + t.end(); + }); +}); diff --git a/test/lib/helper.js b/test/lib/helper.js index 2c1227ebb..92fc2473d 100644 --- a/test/lib/helper.js +++ b/test/lib/helper.js @@ -84,15 +84,6 @@ module.exports = { }, get dtrace() { - var dtp; - - try { - var d = require('dtrace-provider'); - dtp = d.createDTraceProvider('restifyUnitTest'); - } catch (e) { - dtp = null; - } - - return dtp; + return true; } }; diff --git a/test/plugins/audit.test.js b/test/plugins/audit.test.js index 9332b71d8..ed7bab084 100644 --- a/test/plugins/audit.test.js +++ b/test/plugins/audit.test.js @@ -199,9 +199,7 @@ describe('audit logger', function() { // don't sporadically fail due to timing issues. }); - CLIENT.get('/audit', function(err, req, res) { - assert.ifError(err); - + SERVER.on('after', function() { var record = ringbuffer.records && ringbuffer.records[0]; // check timers @@ -237,6 +235,10 @@ describe('audit logger', function() { ); done(); }); + + CLIENT.get('/audit', function(err, req, res) { + assert.ifError(err); + }); }); it('should log anonymous handler timers', function(done) { @@ -263,6 +265,20 @@ describe('audit logger', function() { }) ); + SERVER.pre(function(req, res, next) { + next(); + }); + SERVER.pre(function(req, res, next) { + next(); + }); + + SERVER.use(function(req, res, next) { + next(); + }); + SERVER.use(function(req, res, next) { + next(); + }); + SERVER.get( '/audit', function(req, res, next) { @@ -281,9 +297,7 @@ describe('audit logger', function() { } ); - CLIENT.get('/audit', function(err, req, res) { - assert.ifError(err); - + SERVER.on('after', function() { // check timers var record = ringbuffer.records && ringbuffer.records[0]; assert.ok(record, 'no log records'); @@ -292,6 +306,30 @@ describe('audit logger', function() { 1, 'should only have 1 log record' ); + assertIsAtLeastWithTolerate( + record.req.timers['pre-0'], + 0, + TOLERATED_MICROSECONDS, + 'pre-0' + ); + assertIsAtLeastWithTolerate( + record.req.timers['pre-1'], + 0, + TOLERATED_MICROSECONDS, + 'pre-1' + ); + assertIsAtLeastWithTolerate( + record.req.timers['use-0'], + 0, + TOLERATED_MICROSECONDS, + 'use-0' + ); + assertIsAtLeastWithTolerate( + record.req.timers['use-1'], + 0, + TOLERATED_MICROSECONDS, + 'use-1' + ); assertIsAtLeastWithTolerate( record.req.timers['handler-0'], WAIT_IN_MILLISECONDS * MILLISECOND_IN_MICROSECONDS, @@ -324,6 +362,10 @@ describe('audit logger', function() { ); done(); }); + + CLIENT.get('/audit', function(err, req, res) { + assert.ifError(err); + }); }); it('restify-GH-1435 should accumulate log handler timers', function(done) { @@ -363,9 +405,7 @@ describe('audit logger', function() { // don't sporadically fail due to timing issues. }); - CLIENT.get('/audit', function(err, req, res) { - assert.ifError(err); - + SERVER.on('after', function() { var record = ringbuffer.records && ringbuffer.records[0]; // check timers @@ -389,6 +429,10 @@ describe('audit logger', function() { ); done(); }); + + CLIENT.get('/audit', function(err, req, res) { + assert.ifError(err); + }); }); it('restify-GH-812 audit logger has query params string', function(done) { @@ -417,9 +461,7 @@ describe('audit logger', function() { next(); }); - CLIENT.get('/audit?a=1&b=2', function(err, req, res) { - assert.ifError(err); - + SERVER.on('after', function() { // check timers assert.ok(ringbuffer.records[0], 'no log records'); assert.equal( @@ -430,6 +472,10 @@ describe('audit logger', function() { assert.ok(ringbuffer.records[0].req.query, 'a=1&b=2'); done(); }); + + CLIENT.get('/audit?a=1&b=2', function(err, req, res) { + assert.ifError(err); + }); }); it('restify-GH-812 audit logger has query params obj', function(done) { @@ -461,9 +507,7 @@ describe('audit logger', function() { } ]); - CLIENT.get('/audit?a=1&b=2', function(err, req, res) { - assert.ifError(err); - + SERVER.on('after', function() { // check timers assert.ok(ringbuffer.records[0], 'no log records'); assert.equal( @@ -477,6 +521,10 @@ describe('audit logger', function() { }); done(); }); + + CLIENT.get('/audit?a=1&b=2', function(err, req, res) { + assert.ifError(err); + }); }); it('should work with pre events', function(done) { @@ -606,41 +654,6 @@ describe('audit logger', function() { }); }); - it('should log 444 status code for aborted request', function(done) { - SERVER.once( - 'after', - restify.plugins.auditLogger({ - log: bunyan.createLogger({ - name: 'audit', - streams: [ - { - level: 'info', - stream: process.stdout - } - ] - }), - server: SERVER, - event: 'after' - }) - ); - - SERVER.once('audit', function(data) { - assert.ok(data); - assert.ok(data.req_id); - assert.isNumber(data.latency); - assert.equal(444, data.res.statusCode); - done(); - }); - - SERVER.get('/audit', function(req, res, next) { - req.emit('aborted'); - res.send(); - next(); - }); - - CLIENT.get('/audit', function(err, req, res) {}); - }); - it('should log 444 for closed request', function(done) { SERVER.once( 'after', diff --git a/test/plugins/bodyReader.test.js b/test/plugins/bodyReader.test.js index 1cfe0595e..b799fef90 100644 --- a/test/plugins/bodyReader.test.js +++ b/test/plugins/bodyReader.test.js @@ -127,7 +127,7 @@ describe('body reader', function() { SERVER.use(restify.plugins.bodyParser()); SERVER.post('/compressed', function(req, res, next) { - res.send(200, { inflightRequests: SERVER.inflightRequests() }); + res.send('ok'); next(); }); @@ -142,7 +142,7 @@ describe('body reader', function() { var options = { hostname: '127.0.0.1', port: PORT, - path: '/compressed', + path: '/compressed?v=1', method: 'POST', headers: { 'Content-Type': 'application/json', @@ -156,6 +156,13 @@ describe('body reader', function() { assert.isNotOk(res); }); + SERVER.on('after', function(req2) { + if (req2.href() === '/compressed?v=2') { + assert.equal(SERVER.inflightRequests(), 0); + done(); + } + }); + // will get a req error after 100ms timeout req.on('error', function(e) { // make another request to verify in flight request is only 1 @@ -165,15 +172,13 @@ describe('body reader', function() { }); CLIENT.post( - '/compressed', + '/compressed?v=2', { apple: 'red' }, function(err, _, res, obj) { assert.ifError(err); assert.equal(res.statusCode, 200); - assert.equal(obj.inflightRequests, 1); - done(); } ); }); diff --git a/test/plugins/conditionalHandler.test.js b/test/plugins/conditionalHandler.test.js new file mode 100644 index 000000000..cb4dc0a0f --- /dev/null +++ b/test/plugins/conditionalHandler.test.js @@ -0,0 +1,610 @@ +'use strict'; +/* eslint-disable func-names */ + +var assert = require('chai').assert; +var restify = require('../../lib/index.js'); +var restifyClients = require('restify-clients'); +var parallel = require('vasync').parallel; + +// local files +var helper = require('../lib/helper'); + +// local globals +var SERVER; +var CLIENT; +var PORT; + +function handlerFactory(response) { + return function handler(req, res, next) { + res.send(response); + next(); + }; +} + +describe('conditional request', function() { + describe('version', function() { + beforeEach(function(done) { + SERVER = restify.createServer({ + dtrace: helper.dtrace, + log: helper.getLog('server') + }); + + SERVER.listen(0, '127.0.0.1', function() { + PORT = SERVER.address().port; + CLIENT = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + PORT, + dtrace: helper.dtrace, + retry: false + }); + + done(); + }); + }); + + afterEach(function(done) { + CLIENT.close(); + SERVER.close(done); + }); + + it('should find handler by string version', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.0'), + version: 'v1.1.0' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + parallel( + { + funcs: [ + function v1(callback) { + var opts = { + path: '/', + headers: { + 'accept-version': '1.1.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.1.0'); + callback(); + }); + }, + function v2(callback) { + var opts = { + path: '/', + headers: { + 'accept-version': '1.2.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal( + res.headers['api-version'], + 'v1.2.0' + ); + assert.equal(response, 'v1.2.0'); + callback(); + }); + } + ] + }, + function parallelDone(err) { + assert.ifError(err); + done(); + } + ); + }); + + it('should find handler by array of versions', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.x, v2.x'), + version: ['v1.1.0', 'v2.0.0'] + }, + { + handler: handlerFactory('v3.x'), + version: 'v3.0.0' + } + ]) + ); + + parallel( + { + funcs: [ + function v1(callback) { + var opts = { + path: '/', + headers: { + 'accept-version': '2.x' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.x, v2.x'); + callback(); + }); + }, + function v2(callback) { + var opts = { + path: '/', + headers: { + 'accept-version': '3.x' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v3.x'); + callback(); + }); + } + ] + }, + function parallelDone(err) { + assert.ifError(err); + done(); + } + ); + }); + + it('should find handler with 1.x', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.0'), + version: 'v1.1.0' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: { + 'accept-version': '1.x' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.2.0'); + done(); + }); + }); + + it('should find handler with ~1.1.0', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.1'), + version: 'v1.1.1' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: { + 'accept-version': '~1.1.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.1.1'); + done(); + }); + }); + + it('should find handler with ^1.1.0', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.1'), + version: 'v1.1.1' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: { + 'accept-version': '^1.1.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.2.0'); + done(); + }); + }); + + it('should find largest version with missing header', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.0'), + version: 'v1.1.0' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: {} + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'v1.2.0'); + done(); + }); + }); + + it('should throw invalid version error', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('v1.1.0'), + version: 'v1.1.0' + }, + { + handler: handlerFactory('v1.2.0'), + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: { + 'accept-version': '1.3.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.equal(err.name, 'InvalidVersionError'); + assert.equal(err.message, '1.3.0 is not supported by GET /'); + assert.equal(res.statusCode, 400); + done(); + }); + }); + }); + + describe('content type', function() { + beforeEach(function(done) { + SERVER = restify.createServer({ + dtrace: helper.dtrace, + log: helper.getLog('server') + }); + + SERVER.listen(0, '127.0.0.1', function() { + PORT = SERVER.address().port; + CLIENT = restifyClients.createStringClient({ + url: 'http://127.0.0.1:' + PORT, + dtrace: helper.dtrace, + retry: false + }); + + done(); + }); + }); + + afterEach(function(done) { + CLIENT.close(); + SERVER.close(done); + }); + + it('should find handler by content type by string', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/json'), + contentType: 'application/json' + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + parallel( + { + funcs: [ + function v1(callback) { + var opts = { + path: '/', + headers: { + accept: 'application/json' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, '"application/json"'); + callback(); + }); + }, + function v2(callback) { + var opts = { + path: '/', + headers: { + accept: 'text/plain' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'text/plain'); + callback(); + }); + } + ] + }, + function parallelDone(err) { + assert.ifError(err); + done(); + } + ); + }); + + it('should find handler by array of content types', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/*'), + contentType: [ + 'application/json', + 'application/javascript' + ] + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + var opts = { + path: '/', + headers: { + accept: 'application/javascript' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, '"application/*"'); + done(); + }); + }); + + it('should find handler with multiple accept', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/*'), + contentType: 'application/json' + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + var opts = { + path: '/', + headers: { + accept: 'text/html,text/plain' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, 'text/plain'); + done(); + }); + }); + + it('should find handler with application/*', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/*'), + contentType: 'application/json' + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + var opts = { + path: '/', + headers: { + accept: 'application/json' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, '"application/*"'); + done(); + }); + }); + + it('should find handler with content type and version', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/json, 1.1.0'), + contentType: 'application/json', + version: '1.1.0' + }, + { + handler: handlerFactory('application/json, 1.2.0'), + contentType: 'application/json', + version: '1.2.0' + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + var opts = { + path: '/', + headers: { + accept: 'application/json', + 'accept-version': '1.2.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(response, '"application/json, 1.2.0"'); + done(); + }); + }); + + it('should throw invalid media type error', function(done) { + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: handlerFactory('application/json'), + contentType: 'application/json' + }, + { + handler: handlerFactory('text/plain'), + contentType: 'text/plain' + } + ]) + ); + + var opts = { + path: '/', + headers: { + accept: 'text/html' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.equal(err.name, 'UnsupportedMediaTypeError'); + assert.equal( + err.message, + '{"code":"UnsupportedMediaType","message":"text/html"}' + ); + assert.equal(res.statusCode, 415); + done(); + }); + }); + }); + + describe('multiple handlers', function() { + beforeEach(function(done) { + SERVER = restify.createServer({ + dtrace: helper.dtrace, + log: helper.getLog('server') + }); + + SERVER.listen(0, '127.0.0.1', function() { + PORT = SERVER.address().port; + CLIENT = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + PORT, + dtrace: helper.dtrace, + retry: false + }); + + done(); + }); + }); + + afterEach(function(done) { + CLIENT.close(); + SERVER.close(done); + }); + + it('should run each of the handlers', function(done) { + var counter = 0; + + SERVER.get( + '/', + restify.plugins.conditionalHandler([ + { + handler: [ + function handler1(req, res, next) { + counter += 1; + next(); + }, + function handler2(req, res, next) { + counter += 1; + next(); + }, + function handler3(req, res, next) { + counter += 1; + res.send('v1.2.0'); + } + ], + version: 'v1.2.0' + } + ]) + ); + + var opts = { + path: '/', + headers: { + 'accept-version': '1.2.0' + } + }; + CLIENT.get(opts, function(err, _, res, response) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(counter, 3, 'calls all of the handlers'); + assert.equal(response, 'v1.2.0'); + done(); + }); + }); + }); +}); diff --git a/test/plugins/dedupeSlashes.test.js b/test/plugins/dedupeSlashes.test.js index 3d668cab2..445df94d2 100644 --- a/test/plugins/dedupeSlashes.test.js +++ b/test/plugins/dedupeSlashes.test.js @@ -15,144 +15,62 @@ var CLIENT; var PORT; describe('dedupe forward slashes in URL', function() { - describe('non-strict routing', function() { - before(function(done) { - SERVER = restify.createServer({ - dtrace: helper.dtrace, - log: helper.getLog('server') - }); - - SERVER.pre(restify.plugins.pre.dedupeSlashes()); - - SERVER.get('/foo/bar', function respond(req, res, next) { - res.send(req.url); - next(); - }); - - SERVER.listen(0, '127.0.0.1', function() { - PORT = SERVER.address().port; - CLIENT = restifyClients.createJsonClient({ - url: 'http://127.0.0.1:' + PORT, - dtrace: helper.dtrace, - retry: false - }); - - done(); - }); + before(function(done) { + SERVER = restify.createServer({ + dtrace: helper.dtrace, + log: helper.getLog('server') }); - after(function(done) { - CLIENT.close(); - SERVER.close(done); - }); + SERVER.pre(restify.plugins.pre.dedupeSlashes()); - it('should not remove single slashes', function(done) { - CLIENT.get('/foo/bar', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar'); - done(); - }); - }); - - it( - 'should not remove single slashes ' + 'including trailing slashes', - function(done) { - CLIENT.get('/foo/bar/', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); - } - ); - - it('should remove duplicate slashes', function(done) { - CLIENT.get('//foo//bar', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar'); - done(); - }); + SERVER.get('/foo/bar/', function respond(req, res, next) { + res.send(req.url); + next(); }); - // eslint-disable-next-line - it('should remove duplicate slashes including trailing slashes', function(done) { - CLIENT.get('//foo//bar//', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); - }); - it('should merge multiple slashes', function(done) { - CLIENT.get('//////foo///bar///////', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); - }); - }); - - describe('strict routing', function() { - before(function(done) { - SERVER = restify.createServer({ - strictRouting: true, + SERVER.listen(0, '127.0.0.1', function() { + PORT = SERVER.address().port; + CLIENT = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + PORT, dtrace: helper.dtrace, - log: helper.getLog('server') - }); - - SERVER.pre(restify.plugins.pre.dedupeSlashes()); - - SERVER.get('/foo/bar/', function respond(req, res, next) { - res.send(req.url); - next(); + retry: false }); - SERVER.listen(0, '127.0.0.1', function() { - PORT = SERVER.address().port; - CLIENT = restifyClients.createJsonClient({ - url: 'http://127.0.0.1:' + PORT, - dtrace: helper.dtrace, - retry: false - }); - - done(); - }); + done(); }); + }); - after(function(done) { - CLIENT.close(); - SERVER.close(done); - }); + after(function(done) { + CLIENT.close(); + SERVER.close(done); + }); - it('should not remove single slashes', function(done) { - CLIENT.get('/foo/bar/', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); + it('should not remove single slashes', function(done) { + CLIENT.get('/foo/bar/', function(err, _, res, data) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(data, '/foo/bar/'); + done(); }); + }); - it('should remove duplicate slashes', function(done) { - CLIENT.get('//////foo///bar///////', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); + it('should remove duplicate slashes', function(done) { + CLIENT.get('//////foo///bar///////', function(err, _, res, data) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(data, '/foo/bar/'); + done(); }); + }); - // eslint-disable-next-line - it('should remove duplicate slashes including trailing slashes', function(done) { - CLIENT.get('//foo//bar//', function(err, _, res, data) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - assert.equal(data, '/foo/bar/'); - done(); - }); + // eslint-disable-next-line + it('should remove duplicate slashes including trailing slashes', + function(done) { + CLIENT.get('//foo//bar//', function(err, _, res, data) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + assert.equal(data, '/foo/bar/'); + done(); }); }); }); diff --git a/test/plugins/fieldedTextParser.test.js b/test/plugins/fieldedTextParser.test.js index 83dc584ce..da80b2aa9 100644 --- a/test/plugins/fieldedTextParser.test.js +++ b/test/plugins/fieldedTextParser.test.js @@ -92,47 +92,45 @@ describe('fielded text parser', function() { }); }); - it( - '#100 should parse CSV body even ' + 'if bodyparser declared twice', - function(done) { - SERVER.use(restify.plugins.bodyParser()); - var options = { - path: '/data', - headers: { - 'Content-Type': 'text/csv' - } - }; - - SERVER.post('/data', function respond(req, res, next) { - res.send({ - status: 'okay', - parsedReq: req.body - }); - return next(); + // eslint-disable-next-line + it('#100 should parse CSV body even if bodyparser declared twice', function(done) { + SERVER.use(restify.plugins.bodyParser()); + var options = { + path: '/data', + headers: { + 'Content-Type': 'text/csv' + } + }; + + SERVER.post('/data', function respond(req, res, next) { + res.send({ + status: 'okay', + parsedReq: req.body }); + return next(); + }); - CLIENT.post(options, function(err, req) { - assert.ifError(err); - req.on('result', function(errReq, res) { - assert.ifError(errReq); - res.body = ''; - res.setEncoding('utf8'); - res.on('data', function(chunk) { - res.body += chunk; - }); - res.on('end', function() { - res.body = JSON.parse(res.body); - var parsedReqStr = JSON.stringify(res.body.parsedReq); - var objectStr = JSON.stringify(OBJECT_CSV); - assert.equal(parsedReqStr, objectStr); - done(); - }); + CLIENT.post(options, function(err, req) { + assert.ifError(err); + req.on('result', function(errReq, res) { + assert.ifError(errReq); + res.body = ''; + res.setEncoding('utf8'); + res.on('data', function(chunk) { + res.body += chunk; + }); + res.on('end', function() { + res.body = JSON.parse(res.body); + var parsedReqStr = JSON.stringify(res.body.parsedReq); + var objectStr = JSON.stringify(OBJECT_CSV); + assert.equal(parsedReqStr, objectStr); + done(); }); - req.write(DATA_CSV); - req.end(); }); - } - ); + req.write(DATA_CSV); + req.end(); + }); + }); it('should parse TSV body', function(done) { var options = { diff --git a/test/plugins/inflightRequestThrottle.test.js b/test/plugins/inflightRequestThrottle.test.js index 2b7245c29..d3b363f2a 100644 --- a/test/plugins/inflightRequestThrottle.test.js +++ b/test/plugins/inflightRequestThrottle.test.js @@ -25,8 +25,9 @@ describe('inlfightRequestThrottle', function() { assert(body instanceof Error, 'Defaults to error body'); done(); } - function next(cont) { - assert.isFalse(cont, 'Should call next with false'); + function next(err) { + assert.equal(err.name, 'ServiceUnavailableError'); + done(); } function trace() { logged = true; @@ -43,10 +44,10 @@ describe('inlfightRequestThrottle', function() { var plugin = inflightRequestThrottle(opts); function send(body) { assert.equal(body, err, 'Overrides body'); - done(); } - function next() { - assert(false, 'Should not call next'); + function next(nextErr) { + assert.equal(err, nextErr); + done(); } var fakeReq = { log: { trace: function() {} } }; plugin(fakeReq, { send: send }, next); diff --git a/test/plugins/jsonBodyParser.test.js b/test/plugins/jsonBodyParser.test.js index 0a9d71519..c0ce2b7ca 100644 --- a/test/plugins/jsonBodyParser.test.js +++ b/test/plugins/jsonBodyParser.test.js @@ -141,6 +141,7 @@ describe('JSON body parser', function() { }); }); + // TODO: router param mapping runs later it('should map req.body onto req.params', function(done) { SERVER.use( restify.plugins.jsonBodyParser({ @@ -267,45 +268,40 @@ describe('JSON body parser', function() { }); }); - it( - 'restify-GH-318 get request with body ' + '(requestBodyOnGet=true)', - function(done) { - SERVER.use( - restify.plugins.bodyParser({ - mapParams: true, - requestBodyOnGet: true - }) - ); - - SERVER.get('/getWithBody', function(req, res, next) { - assert.equal(req.params.foo, 'bar'); - res.send(); - next(); - }); + // eslint-disable-next-line + it('restify-GH-318 get request with body (requestBodyOnGet=true)', function(done) { + SERVER.use( + restify.plugins.bodyParser({ + mapParams: true, + requestBodyOnGet: true + }) + ); - var request = - 'GET /getWithBody HTTP/1.1\r\n' + - 'Content-Type: application/json\r\n' + - 'Content-Length: 13\r\n' + - '\r\n' + - '{"foo":"bar"}'; - - var client = net.connect( - { host: '127.0.0.1', port: PORT }, - function() { - client.write(request); - } - ); + SERVER.get('/getWithBody', function(req, res, next) { + assert.equal(req.params.foo, 'bar'); + res.send(); + next(); + }); - client.once('data', function(data) { - client.end(); - }); + var request = + 'GET /getWithBody HTTP/1.1\r\n' + + 'Content-Type: application/json\r\n' + + 'Content-Length: 13\r\n' + + '\r\n' + + '{"foo":"bar"}'; - client.once('end', function() { - done(); - }); - } - ); + var client = net.connect({ host: '127.0.0.1', port: PORT }, function() { + client.write(request); + }); + + client.once('data', function(data) { + client.end(); + }); + + client.once('end', function() { + done(); + }); + }); it('restify-GH-111 JSON Parser not right for arrays', function(done) { SERVER.use( diff --git a/test/plugins/metrics.test.js b/test/plugins/metrics.test.js index 5795fcb0b..e99315817 100644 --- a/test/plugins/metrics.test.js +++ b/test/plugins/metrics.test.js @@ -42,6 +42,10 @@ describe('request metrics plugin', function() { }); it('should return metrics for a given request', function(done) { + SERVER.on('uncaughtException', function(req, res, route, err) { + assert.ifError(err); + }); + SERVER.on( 'after', restify.plugins.metrics( @@ -53,7 +57,11 @@ describe('request metrics plugin', function() { assert.isObject(metrics, 'metrics'); assert.equal(metrics.statusCode, 202); - assert.isAtLeast(metrics.latency, 100); + assert.isAtLeast(metrics.preLatency, 50); + assert.isAtLeast(metrics.useLatency, 50); + assert.isAtLeast(metrics.routeLatency, 50); + assert.isAtLeast(metrics.latency, 150); + assert.isAtLeast(metrics.totalLatency, 150); assert.equal(metrics.path, '/foo'); assert.equal(metrics.connectionState, undefined); assert.equal(metrics.method, 'GET'); @@ -66,11 +74,23 @@ describe('request metrics plugin', function() { ) ); + SERVER.pre(function(req, res, next) { + setTimeout(function() { + return next(); + }, 50); + }); + + SERVER.use(function(req, res, next) { + setTimeout(function() { + return next(); + }, 50); + }); + SERVER.get('/foo', function(req, res, next) { setTimeout(function() { res.send(202, 'hello world'); return next(); - }, 100); + }, 50); }); CLIENT.get('/foo?a=1', function(err, _, res) { @@ -80,10 +100,10 @@ describe('request metrics plugin', function() { }); }); - it("should return 'RequestCloseError' err", function(done) { - // we test that the client times out and closes the request. server - // flushes request responsibly but connectionState should indicate it - // was closed by the server. + it('should return metrics with pre error', function(done) { + SERVER.on('uncaughtException', function(req, res, route, err) { + assert.ok(err); + }); SERVER.on( 'after', @@ -93,39 +113,79 @@ describe('request metrics plugin', function() { }, function(err, metrics, req, res, route) { assert.ok(err); - assert.equal(err.name, 'RequestCloseError'); assert.isObject(metrics, 'metrics'); - assert.equal(metrics.statusCode, 444); - assert.isAtLeast(metrics.latency, 200); - assert.equal(metrics.path, '/foo'); - assert.equal(metrics.method, 'GET'); - assert.equal(metrics.connectionState, 'close'); - assert.isNumber(metrics.inflightRequests); + assert.isAtLeast(metrics.preLatency, 50); + assert.equal(metrics.useLatency, null); + assert.equal(metrics.routeLatency, null); + assert.isAtLeast(metrics.latency, 50); + return done(); } ) ); - SERVER.get('/foo', function(req, res, next) { + SERVER.pre(function(req, res, next) { setTimeout(function() { - res.send(202, 'hello world'); - return next(); - }, 250); + return next(new Error('My Error')); + }, 50); + }); + + CLIENT.get('/foo?a=1', function(err, _, res) { + assert.ok(err); + }); + }); + + it('should return metrics with use error', function(done) { + SERVER.on('uncaughtException', function(req, res, route, err) { + assert.ok(err); + }); + + SERVER.on( + 'after', + restify.plugins.metrics( + { + server: SERVER + }, + function(err, metrics, req, res, route) { + assert.ok(err); + + assert.isObject(metrics, 'metrics'); + assert.isAtLeast(metrics.preLatency, 0); + assert.isAtLeast(metrics.useLatency, 50); + assert.equal(metrics.routeLatency, null); + assert.isAtLeast(metrics.latency, 50); + + return done(); + } + ) + ); + + SERVER.use(function(req, res, next) { + setTimeout(function() { + return next(new Error('My Error')); + }, 50); + }); + + SERVER.get('/foo', function(req, res, next) { + res.send(202, 'hello world'); + return next(); }); CLIENT.get('/foo?a=1', function(err, _, res) { - // request should timeout assert.ok(err); - assert.equal(err.name, 'RequestTimeoutError'); }); }); - it("should return 'RequestAbortedError' err", function(done) { + it("should return 'RequestCloseError' err", function(done) { // we test that the client times out and closes the request. server // flushes request responsibly but connectionState should indicate it // was closed by the server. + SERVER.on('uncaughtException', function(req, res, route, err) { + assert.ifError(err); + }); + SERVER.on( 'after', restify.plugins.metrics( @@ -134,28 +194,58 @@ describe('request metrics plugin', function() { }, function(err, metrics, req, res, route) { assert.ok(err); - assert.equal(err.name, 'RequestAbortedError'); + assert.equal(err.name, 'RequestCloseError'); assert.isObject(metrics, 'metrics'); - assert.equal(metrics.statusCode, 444); - assert.isAtLeast(metrics.latency, 200); + assert.equal(metrics.statusCode, 444); // router doesn't run + // However the timeout value is 200, + // it's calculated by the client, + // but setTimeout is happening on the server, tolerate 10ms + assert.isAtLeast(metrics.preLatency, 50); + assert.isAtLeast(metrics.useLatency, 50); + assert.isAtLeast(metrics.routeLatency, 250); + assert.isAtLeast(metrics.latency, 200 - 10); + // latency should dbe lower as request timeouts + assert.isAbove(metrics.routeLatency, metrics.latency); assert.equal(metrics.path, '/foo'); assert.equal(metrics.method, 'GET'); - assert.equal(metrics.connectionState, 'aborted'); + assert.equal(metrics.connectionState, 'close'); assert.isNumber(metrics.inflightRequests); + return done(); } ) ); - SERVER.get('/foo', function(req, res, next) { - // simulate request being aborted by TCP socket being closed - req.emit('aborted'); - res.send(202, 'hello world'); - return next(); + SERVER.pre(function(req, res, next) { + setTimeout(function() { + return next(); + }, 50); + }); + + SERVER.use(function(req, res, next) { + setTimeout(function() { + return next(); + }, 50); }); + SERVER.get( + '/foo', + function(req, res, next) { + setTimeout(function() { + return next(); + }, 250); + }, + function(req, res, next) { + assert.fail('Client has already closed request'); + res.send(202, 'hello world'); + return next(); + } + ); + CLIENT.get('/foo?a=1', function(err, _, res) { - return done(); + // request should timeout + assert.ok(err); + assert.equal(err.name, 'RequestTimeoutError'); }); }); @@ -183,6 +273,7 @@ describe('request metrics plugin', function() { assert.equal(metrics.method, 'GET'); assert.equal(metrics.connectionState, undefined); assert.isNumber(metrics.inflightRequests); + return done(); } ) ); @@ -193,7 +284,6 @@ describe('request metrics plugin', function() { CLIENT.get('/foo?a=1', function(err, _, res) { assert.ok(err); - return done(); }); }); }); diff --git a/test/plugins/oauth2.test.js b/test/plugins/oauth2.test.js index cdd866faf..6ae5446a0 100644 --- a/test/plugins/oauth2.test.js +++ b/test/plugins/oauth2.test.js @@ -70,26 +70,23 @@ describe('oauth2 token parser', function() { }); }); - it( - 'should do nothing (token is null) ' + - 'if there is no oauth2 token set', - function(done) { - var opts = { - path: '/test2/do/nothing' - }; - SERVER.get(opts, function(req, res, next) { - assert.isNull(req.oauth2.accessToken); - assert.equal(res.statusCode, 200); - res.send(); - next(); - }); - CLIENT.get(opts, function(err, _, res) { - assert.ifError(err); - assert.equal(res.statusCode, 200); - done(); - }); - } - ); + // eslint-disable-next-line + it('should do nothing (token is null) if there is no oauth2 token set', function(done) { + var opts = { + path: '/test2/do/nothing' + }; + SERVER.get(opts, function(req, res, next) { + assert.isNull(req.oauth2.accessToken); + assert.equal(res.statusCode, 200); + res.send(); + next(); + }); + CLIENT.get(opts, function(err, _, res) { + assert.ifError(err); + assert.equal(res.statusCode, 200); + done(); + }); + }); it('should parse from request body', function(done) { var test3Url = '/test3/contenttype/ok'; @@ -119,36 +116,34 @@ describe('oauth2 token parser', function() { client.end(); }); - it( - 'should parse oauth2 token from ' + 'request body(case-insensitive)', - function(done) { - var test4Url = '/test4/contenttype/mixedcase'; + // eslint-disable-next-line + it('should parse oauth2 token from request body(case-insensitive)', function(done) { + var test4Url = '/test4/contenttype/mixedcase'; - SERVER.post(test4Url, function(req, res, next) { - assert.isNotNull(req.oauth2.accessToken); - assert.equal(req.oauth2.accessToken, TEST_TOKEN); - res.send(); - next(); - }); + SERVER.post(test4Url, function(req, res, next) { + assert.isNotNull(req.oauth2.accessToken); + assert.equal(req.oauth2.accessToken, TEST_TOKEN); + res.send(); + next(); + }); - var opts = { - hostname: '127.0.0.1', - port: PORT, - path: test4Url, - agent: false, - method: 'POST', - headers: { - 'Content-Type': 'APPLICATION/x-www-form-urlencoded' - } - }; - var client = http.request(opts, function(res) { - assert.equal(res.statusCode, 200); - done(); - }); - client.write('access_token=' + TEST_TOKEN); - client.end(); - } - ); + var opts = { + hostname: '127.0.0.1', + port: PORT, + path: test4Url, + agent: false, + method: 'POST', + headers: { + 'Content-Type': 'APPLICATION/x-www-form-urlencoded' + } + }; + var client = http.request(opts, function(res) { + assert.equal(res.statusCode, 200); + done(); + }); + client.write('access_token=' + TEST_TOKEN); + client.end(); + }); it('should ignore token from request body', function(done) { var test5Url = '/test5/contenttype/missing/1'; @@ -177,32 +172,25 @@ describe('oauth2 token parser', function() { client.end(); }); - it( - 'should fail if more than one method ' + - 'is used to set the oauth2 token', - function(done) { - SERVER.post('/test6/multi/method/fail', function(req, res, next) { - assert.isNull(req.oauth2.accessToken); - res.send(); - next(); - }); - var opts = { - path: '/test6/multi/method/fail', - headers: { - Authorization: 'Bearer ' + TEST_TOKEN, - 'Content-Type': 'application/x-www-form-urlencoded' - } - }; - - CLIENT.post(opts, { access_token: TEST_TOKEN }, function( - err, - _, - res - ) { - assert.ok(err); - assert.equal(res.statusCode, 400); - done(); - }); - } - ); + // eslint-disable-next-line + it('should fail if more than one method is used to set the oauth2 token', function(done) { + SERVER.post('/test6/multi/method/fail', function(req, res, next) { + assert.isNull(req.oauth2.accessToken); + res.send(); + next(); + }); + var opts = { + path: '/test6/multi/method/fail', + headers: { + Authorization: 'Bearer ' + TEST_TOKEN, + 'Content-Type': 'application/x-www-form-urlencoded' + } + }; + + CLIENT.post(opts, { access_token: TEST_TOKEN }, function(err, _, res) { + assert.ok(err); + assert.equal(res.statusCode, 400); + done(); + }); + }); }); diff --git a/test/plugins/plugins.test.js b/test/plugins/plugins.test.js index ca9c4bb81..60214da11 100644 --- a/test/plugins/plugins.test.js +++ b/test/plugins/plugins.test.js @@ -115,7 +115,6 @@ describe('all other plugins', function() { assert.ok(headers['response-time'] >= 0); assert.equal(headers.server, 'restify'); assert.equal(headers.connection, 'Keep-Alive'); - assert.equal(headers['api-version'], '2.0.0'); done(); }); }); diff --git a/test/plugins/query.test.js b/test/plugins/query.test.js index 0a802bf34..5201dc38b 100644 --- a/test/plugins/query.test.js +++ b/test/plugins/query.test.js @@ -239,7 +239,7 @@ describe('query parser', function() { mapParams: true }) ); - SERVER.get(/\/.*/, function(req, res, next) { + SERVER.get('/', function(req, res, next) { res.send(req.params); }); diff --git a/test/plugins/reqIdHeaders.test.js b/test/plugins/reqIdHeaders.test.js index 36e570063..40f58fa31 100644 --- a/test/plugins/reqIdHeaders.test.js +++ b/test/plugins/reqIdHeaders.test.js @@ -94,28 +94,26 @@ describe('request id headers', function() { }); }); - it( - 'GH-1086: should use default uuid request id ' + 'if none provided', - function(done) { - SERVER.get('/1', function(req, res, next) { - assert.ok(req.getId()); - assert.ok(validator.isUUID(req.getId())); - res.send('hello world'); - return next(); - }); + // eslint-disable-next-line + it('GH-1086: should use default uuid request id if none provided', function(done) { + SERVER.get('/1', function(req, res, next) { + assert.ok(req.getId()); + assert.ok(validator.isUUID(req.getId())); + res.send('hello world'); + return next(); + }); - // create new client since we new specific headers - CLIENT = restifyClients.createJsonClient({ - url: 'http://127.0.0.1:' + PORT - }); + // create new client since we new specific headers + CLIENT = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + PORT + }); - CLIENT.get('/1', function(err, req, res, data) { - assert.ifError(err); - assert.equal(data, 'hello world'); - return done(); - }); - } - ); + CLIENT.get('/1', function(err, req, res, data) { + assert.ifError(err); + assert.equal(data, 'hello world'); + return done(); + }); + }); it('GH-1086: empty request id should be ignored', function(done) { SERVER.get('/1', function(req, res, next) { diff --git a/test/plugins/static.test.js b/test/plugins/static.test.js index fcd200a0f..b80bff32b 100644 --- a/test/plugins/static.test.js +++ b/test/plugins/static.test.js @@ -99,11 +99,10 @@ describe('static resource plugin', function() { opts.default = testFileName; routeName += ' with default'; } - var re = regex || new RegExp('/' + testDir + '/?.*'); SERVER.get( { - path: re, + path: '/' + testDir + '/*', name: routeName }, restify.plugins.serveStatic(opts) @@ -158,11 +157,10 @@ describe('static resource plugin', function() { opts.default = testFileName; routeName += ' with default'; } - var re = regex || new RegExp('/' + testDir + '/?.*'); SERVER.get( { - path: re, + path: '/' + testDir + '/*', name: routeName }, restify.plugins.serveStatic(opts) @@ -191,25 +189,22 @@ describe('static resource plugin', function() { }); it('static serves static files in with a root regex', function(done) { - serveStaticTest(done, false, '.tmp', new RegExp('/.*')); + serveStaticTest(done, false, '.tmp', '/.*'); }); - it('static serves static files ' + 'with a root, !greedy, regex', function( - done - ) { - serveStaticTest(done, false, '.tmp', new RegExp('/?.*')); + // eslint-disable-next-line + it('static serves static files with a root, !greedy, regex', function(done) { + serveStaticTest(done, false, '.tmp', '/?.*'); }); it('static serves default file', function(done) { serveStaticTest(done, true, '.tmp'); }); - it( - 'restify-GH-379 static serves file ' + 'with parentheses in path', - function(done) { - serveStaticTest(done, false, '.(tmp)'); - } - ); + // eslint-disable-next-line + it('restify-GH-379 static serves file with parentheses in path', function(done) { + serveStaticTest(done, false, '.(tmp)'); + }); it('restify-GH-719 serve a specific static file', function(done) { // serve the same default file .tmp/public/index.json @@ -217,34 +212,27 @@ describe('static resource plugin', function() { serveStaticTest(done, false, '.tmp', null, true); }); - it( - 'static serves static file with ' + 'appendRequestPath = false', - function(done) { - testNoAppendPath(done, false, '.tmp'); - } - ); + // eslint-disable-next-line + it('static serves static file with appendRequestPath = false', function(done) { + testNoAppendPath(done, false, '.tmp'); + }); - it( - 'static serves default file with ' + 'appendRequestPath = false', - function(done) { - testNoAppendPath(done, true, '.tmp'); - } - ); + // eslint-disable-next-line + it('static serves default file with appendRequestPath = false', function(done) { + testNoAppendPath(done, true, '.tmp'); + }); - it( - 'restify serve a specific static file ' + - 'with appendRequestPath = false', - function(done) { - testNoAppendPath(done, false, '.tmp', null, true); - } - ); + // eslint-disable-next-line + it('restify serve a specific static file with appendRequestPath = false', function(done) { + testNoAppendPath(done, false, '.tmp', null, true); + }); it('static responds 404 for missing file', function(done) { var p = '/public/no-such-file.json'; var tmpPath = path.join(process.cwd(), '.tmp'); SERVER.get( - new RegExp('/public/.*'), + '/public/.*', restify.plugins.serveStatic({ directory: tmpPath }) ); @@ -256,25 +244,23 @@ describe('static resource plugin', function() { }); }); - it( - 'GH-1382 static responds 404 for missing file ' + 'with percent-codes', - function(done) { - var p = '/public/no-%22such-file.json'; - var tmpPath = path.join(process.cwd(), '.tmp'); - - SERVER.get( - new RegExp('/public/.*'), - restify.plugins.serveStatic({ directory: tmpPath }) - ); - - CLIENT.get(p, function(err, req, res, obj) { - assert.ok(err); - assert.equal(err.statusCode, 404); - assert.equal(err.restCode, 'ResourceNotFound'); - done(); - }); - } - ); + // eslint-disable-next-line + it('GH-1382 static responds 404 for missing file with percent-codes', function(done) { + var p = '/public/no-%22such-file.json'; + var tmpPath = path.join(process.cwd(), '.tmp'); + + SERVER.get( + '/public/.*', + restify.plugins.serveStatic({ directory: tmpPath }) + ); + + CLIENT.get(p, function(err, req, res, obj) { + assert.ok(err); + assert.equal(err.statusCode, 404); + assert.equal(err.restCode, 'ResourceNotFound'); + done(); + }); + }); // To ensure this will always get properly restored (even in case of a test // failure) we do it here. @@ -327,7 +313,7 @@ describe('static resource plugin', function() { directory: TMP_PATH }); - SERVER.get(/.*/, function(req, res, next) { + SERVER.get('/index.html', function(req, res, next) { serve(req, res, function(nextRoute) { assert.strictEqual(streamWasClosed, true); assert.strictEqual(nextRoute, false); @@ -364,12 +350,17 @@ describe('static resource plugin', function() { directory: TMP_PATH }); - SERVER.get(/.*/, function(req, res, next) { + SERVER.get('/index.html', function(req, res, next) { + // closed before serve serve(req, res, function(nextRoute) { assert.strictEqual(nextRoute, false); done(); }); }); + SERVER.on('after', function(req, res, route, afterErr) { + assert(afterErr.name, 'RequestCloseError'); + done(); + }); var socket = new net.Socket(); socket.connect({ host: '127.0.0.1', port: PORT }, function() { @@ -387,7 +378,7 @@ describe('static resource plugin', function() { var tmpPath = path.join(process.cwd(), '.tmp'); SERVER.get( - new RegExp('/public/.*'), + '/public/.*', restify.plugins.serveStatic({ directory: tmpPath }) ); @@ -399,23 +390,21 @@ describe('static resource plugin', function() { }); }); - it( - 'GH-1382 static responds 404 for missing file with ' + 'percent-codes', - function(done) { - var p = '/public/no-%22such-file.json'; - var tmpPath = path.join(process.cwd(), '.tmp'); - - SERVER.get( - new RegExp('/public/.*'), - restify.plugins.serveStatic({ directory: tmpPath }) - ); - - CLIENT.get(p, function(err, req, res, obj) { - assert.ok(err); - assert.equal(err.statusCode, 404); - assert.equal(err.restCode, 'ResourceNotFound'); - return done(); - }); - } - ); + // eslint-disable-next-line + it('GH-1382 static responds 404 for missing file with percent-codes', function(done) { + var p = '/public/no-%22such-file.json'; + var tmpPath = path.join(process.cwd(), '.tmp'); + + SERVER.get( + '/public/.*', + restify.plugins.serveStatic({ directory: tmpPath }) + ); + + CLIENT.get(p, function(err, req, res, obj) { + assert.ok(err); + assert.equal(err.statusCode, 404); + assert.equal(err.restCode, 'ResourceNotFound'); + return done(); + }); + }); }); diff --git a/test/plugins/strictQueryParams.test.js b/test/plugins/strictQueryParams.test.js index ec8f6ba4f..66065081e 100644 --- a/test/plugins/strictQueryParams.test.js +++ b/test/plugins/strictQueryParams.test.js @@ -19,7 +19,8 @@ describe('strictQueryParams', function() { beforeEach(function(done) { SERVER = restify.createServer({ dtrace: helper.dtrace, - log: helper.getLog('server') + log: helper.getLog('server'), + handleUncaughtExceptions: true }); SERVER.listen(0, '127.0.0.1', function() { @@ -163,42 +164,36 @@ describe('strictQueryParams', function() { }); }); - it( - 'should respond to non-strict key/val query param ' + 'value with 400', - function(done) { - SERVER.pre( - restify.plugins.pre.strictQueryParams({ - message: MESSAGE - }) - ); - - SERVER.use( - restify.plugins.queryParser({ - mapParams: true, - overrideParams: true - }) - ); - - SERVER.get('/query/:id', function(req, res, next) { - res.send(); - next(); - }); + // eslint-disable-next-line + it('should respond to non-strict key/val query param value with 400', function(done) { + SERVER.pre( + restify.plugins.pre.strictQueryParams({ + message: MESSAGE + }) + ); + + SERVER.use( + restify.plugins.queryParser({ + mapParams: true, + overrideParams: true + }) + ); + + SERVER.get('/query/:id', function(req, res, next) { + res.send(); + next(); + }); - CLIENT.get('/query/foo?id=bar&name=josep&jorge', function( - err, - _, - res - ) { - assert.equal(typeof err, 'object'); - assert.equal(res.statusCode, 400); - assert.deepEqual(JSON.parse(res.body), { - code: 'BadRequest', - message: MESSAGE - }); - done(); + CLIENT.get('/query/foo?id=bar&name=josep&jorge', function(err, _, res) { + assert.equal(typeof err, 'object'); + assert.equal(res.statusCode, 400); + assert.deepEqual(JSON.parse(res.body), { + code: 'BadRequest', + message: MESSAGE }); - } - ); + done(); + }); + }); it('should respond to valid query param value with 200', function(done) { SERVER.pre( @@ -247,7 +242,7 @@ describe('strictQueryParams', function() { }) ); - SERVER.get('/query/:id', function(req, res, next) { + SERVER.get('/query', function(req, res, next) { assert.equal(req.params.id, 'bar'); assert.equal(req.params.name, 'josep & jorge'); assert.deepEqual(req.query, req.params); @@ -255,7 +250,7 @@ describe('strictQueryParams', function() { next(); }); - CLIENT.get('/query/foo?id=bar&name=josep%20%26%20jorge', function( + CLIENT.get('/query?id=bar&name=josep%20%26%20jorge', function( err, _, res diff --git a/test/plugins/throttle.test.js b/test/plugins/throttle.test.js index aa0f66309..64d5743c4 100644 --- a/test/plugins/throttle.test.js +++ b/test/plugins/throttle.test.js @@ -23,8 +23,10 @@ function setupClientServer(ip, throttleOptions, done) { }); server.use(function ghettoAuthenticate(req, res, next) { - if (req.params.name) { - req.username = req.params.name; + var username = req.url.match(/test\/([a-z]+)/)[1]; + + if (username) { + req.username = username; } next(); diff --git a/test/plugins/userAgent.test.js b/test/plugins/userAgent.test.js index 69de6fa18..6466214fa 100644 --- a/test/plugins/userAgent.test.js +++ b/test/plugins/userAgent.test.js @@ -93,41 +93,39 @@ describe('userAgent pre-route handler', function() { // the userAgentConnection should not remove the content-length header from // the response, and it should not replace the value of the 'connection' // header by 'close'. - it( - 'sets proper headers for HEAD requests ' + 'from non-curl clients', - function(done) { - var req = http.request( - { - hostname: SERVER_ADDRESS, - port: SERVER_PORT, - path: TEST_PATH, - method: 'HEAD', - headers: { - 'user-agent': 'foobar', - connection: 'keep-alive' - } - }, - function onResponse(res) { - var responseHeaders = res.headers; - - assert.ok(responseHeaders.hasOwnProperty('content-length')); - assert.equal(responseHeaders.connection, 'keep-alive'); - - // destroy the socket explicitly now since the request was - // explicitly requesting to not destroy the socket by - // setting its connection header to 'keep-alive'. - req.abort(); - - done(); + // eslint-disable-next-line + it('sets proper headers for HEAD requests from non-curl clients', function(done) { + var req = http.request( + { + hostname: SERVER_ADDRESS, + port: SERVER_PORT, + path: TEST_PATH, + method: 'HEAD', + headers: { + 'user-agent': 'foobar', + connection: 'keep-alive' } - ); + }, + function onResponse(res) { + var responseHeaders = res.headers; + + assert.ok(responseHeaders.hasOwnProperty('content-length')); + assert.equal(responseHeaders.connection, 'keep-alive'); + + // destroy the socket explicitly now since the request was + // explicitly requesting to not destroy the socket by setting + // its connection header to 'keep-alive'. + req.abort(); - req.on('error', function onReqError(err) { - assert.ifError(err); done(); - }); + } + ); - req.end(); - } - ); + req.on('error', function onReqError(err) { + assert.ifError(err); + done(); + }); + + req.end(); + }); }); diff --git a/test/response.test.js b/test/response.test.js index 8f77f32b1..a3866af21 100644 --- a/test/response.test.js +++ b/test/response.test.js @@ -396,57 +396,50 @@ test('redirect using default hostname with custom port', function(t) { }); }); -// jscs:disable maximumLineLength -test( - 'redirect should cause InternalError ' + 'when invoked without next', - function(t) { - SERVER.get('/9', function(req, res, next) { - res.redirect(); - }); +// eslint-disable-next-line +test('redirect should cause InternalError when invoked without next', function(t) { + SERVER.get('/9', function(req, res, next) { + res.redirect(); + }); - CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res, body) { - t.equal(res.statusCode, 500); + CLIENT.get(join(LOCALHOST, '/9'), function(err, _, res, body) { + t.equal(res.statusCode, 500); - // json parse the response - t.equal(body.code, 'Internal'); - t.end(); - }); + // json parse the response + t.equal(body.code, 'Internal'); + t.end(); + }); +}); + +// eslint-disable-next-line +test('redirect should call next with false to stop handler stack execution', function(t) { + var wasRun = false; + + function A(req, res, next) { + req.a = 1; + next(); + } + function B(req, res, next) { + req.b = 2; + wasRun = true; + next(); + } + function redirect(req, res, next) { + res.redirect('/10', next); } -); -// jscs:enable maximumLineLength + SERVER.get('/10', [A, redirect, B]); -test( - 'redirect should call next with false to stop ' + 'handler stack execution', - function(t) { - var wasRun = false; - - function A(req, res, next) { - req.a = 1; - next(); - } - function B(req, res, next) { - req.b = 2; - wasRun = true; - next(); - } - function redirect(req, res, next) { - res.redirect('/10', next); - } - - SERVER.get('/10', [A, redirect, B]); - - CLIENT.get(join(LOCALHOST, '/10'), function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 302); - t.equal(res.headers.location, '/10'); + CLIENT.get(join(LOCALHOST, '/10'), function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 302); + t.equal(res.headers.location, '/10'); - // handler B should not be executed - t.equal(wasRun, false); - t.end(); - }); - } -); + // handler B should not be executed + t.equal(wasRun, false); + t.end(); + }); +}); test('redirect should emit a redirect event', function(t) { var wasEmitted = false; diff --git a/test/router.test.js b/test/router.test.js index 019793567..27d551095 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -1,10 +1,10 @@ -// Copyright 2012 Mark Cavage, Inc. All rights reserved. - 'use strict'; /* eslint-disable func-names */ var restify = require('../lib'); +var Router = require('../lib/router'); var clients = require('restify-clients'); +var _ = require('lodash'); if (require.cache[__dirname + '/lib/helper.js']) { delete require.cache[__dirname + '/lib/helper.js']; @@ -14,141 +14,98 @@ var helper = require('./lib/helper.js'); ///--- Globals var test = helper.test; -var mockResponse = function respond(req, res, next) { - res.send(200); +var mockReq = { + params: {}, + closed: function() { + return false; + }, + startHandlerTimer: function() {}, + endHandlerTimer: function() {} +}; +var mockRes = { + setHeader: function() {}, + send: function() {} }; ///--- Tests -test('render route', function(t) { - var server = restify.createServer(); - server.get({ name: 'countries', path: '/countries' }, mockResponse); - server.get({ name: 'country', path: '/countries/:name' }, mockResponse); - server.get( - { name: 'cities', path: '/countries/:name/states/:state/cities' }, - mockResponse - ); - - var countries = server.router.render('countries', {}); - t.equal(countries, '/countries'); - - var country = server.router.render('country', { name: 'Australia' }); - t.equal(country, '/countries/Australia'); +test('mounts a route', function(t) { + function handler(req, res, next) { + res.send('Hello world'); + } - var cities = server.router.render('cities', { - name: 'Australia', - state: 'New South Wales' + var router = new Router({ + log: {} }); - t.equal(cities, '/countries/Australia/states/New%20South%20Wales/cities'); - - t.end(); -}); - -test('render route (missing params)', function(t) { - var server = restify.createServer(); - server.get( - { name: 'cities', path: '/countries/:name/states/:state/cities' }, - mockResponse + router.mount({ method: 'GET', path: '/' }, [handler]); + router.mount({ method: 'POST', path: '/' }, [handler]); + router.mount({ method: 'GET', path: '/ab' }, [handler]); + + t.deepEqual(Object.keys(router.getRoutes()), ['get', 'post', 'getab']); + + // Route names are unique + router.mount({ name: 'get', method: 'GET', path: '/get' }, [handler]); + router.mount({ method: 'GET', path: '/a/b' }, [handler]); + t.deepEqual( + _.uniq(Object.keys(router.getRoutes())), + Object.keys(router.getRoutes()) ); - try { - server.router.render('cities', { name: 'Australia' }); - } catch (ex) { - t.equal(ex, 'Error: Route is missing parameter '); - } - - t.end(); + t.done(); }); -test('GH #704: render route (special charaters)', function(t) { - var server = restify.createServer(); - server.get({ name: 'my-route', path: '/countries/:name' }, mockResponse); +test('unmounts a route', function(t) { + function handler(req, res, next) { + res.send('Hello world'); + } - var link = server.router.render('my-route', { name: 'Australia' }); - t.equal(link, '/countries/Australia'); + var router = new Router({ + log: {} + }); - t.end(); -}); + // Mount + router.mount({ method: 'GET', path: '/a' }, [handler]); + router.mount({ method: 'POST', path: '/b' }, [handler]); + t.deepEqual(Object.keys(router.getRoutes()), ['geta', 'postb']); -test('GH #704: render route (with sub-regex param)', function(t) { - var server = restify.createServer(); - server.get( - { - name: 'my-route', - path: '/countries/:code([A-Z]{2,3})' - }, - mockResponse - ); - - var link = server.router.render('my-route', { code: 'FR' }); - t.equal(link, '/countries/FR'); + // Unmount + var route = router.unmount('geta'); + t.ok(route); + t.equal(route.name, 'geta'); - link = server.router.render('my-route', { code: '111' }); - t.equal(link, '/countries/111'); - t.end(); -}); + // Removes from mounted routes + t.deepEqual(Object.keys(router.getRoutes()), ['postb']); -test('GH-796: render route (with multiple sub-regex param)', function(t) { - var server = restify.createServer(); - server.get( - { - name: 'my-route', - path: '/countries/:code([A-Z]{2,3})/:area([0-9]+)' - }, - mockResponse + // 404 + var handlerFound = router.lookup( + Object.assign( + { + getUrl: function() { + return { pathname: '/a' }; + }, + method: 'GET' + }, + mockReq + ), + mockRes ); - var link = server.router.render('my-route', { code: '111', area: 42 }); - t.equal(link, '/countries/111/42'); - t.end(); -}); - -test('render route (with encode)', function(t) { - var server = restify.createServer(); - server.get({ name: 'my-route', path: '/countries/:name' }, mockResponse); - - var link = server.router.render('my-route', { name: 'Trinidad & Tobago' }); - t.equal(link, '/countries/Trinidad%20%26%20Tobago'); - + t.notOk(handlerFound); t.end(); }); -test('render route (query string)', function(t) { - var server = restify.createServer(); - server.get({ name: 'country', path: '/countries/:name' }, mockResponse); - - var country1 = server.router.render( - 'country', - { - name: 'Australia' - }, - { - state: 'New South Wales', - 'cities/towns': 5 - } - ); - - t.equal( - country1, - '/countries/Australia?state=New%20South%20Wales&cities%2Ftowns=5' - ); - - var country2 = server.router.render( - 'country', - { - name: 'Australia' - }, - { - state: 'NSW & VIC', - 'cities&towns': 5 - } - ); +test('unmounts a route that does not exist', function(t) { + function handler(req, res, next) { + res.send('Hello world'); + } - t.equal( - country2, - '/countries/Australia?state=NSW%20%26%20VIC&cities%26towns=5' - ); + var router = new Router({ + log: {} + }); + // Mount + router.mount({ method: 'GET', path: '/a' }, [handler]); + t.notOk(router.unmount('non-existing')); t.end(); }); @@ -186,103 +143,206 @@ test('clean up xss for 404', function(t) { }); }); -test('Strict routing handles root path', function(t) { - var server = restify.createServer({ strictRouting: true }); - function noop() {} - server.get('/', noop); +test('lookupByName runs a route by name and calls next', function(t) { + var router = new Router({ + log: {} + }); + + function handler(req, res, next) { + res.send('hello world'); + next(); + } - var root = server.router.routes.GET[0]; - t.ok(root.path.test('/')); + router.mount({ method: 'GET', path: '/', name: 'my-route' }, [handler]); - t.end(); + var handlerFound = router.lookupByName('my-route', mockReq, mockRes); + t.ok(handlerFound); + + handlerFound(mockReq, mockRes, function next(err) { + t.ifError(err); + t.end(); + }); }); -test('Strict routing distinguishes trailing slash', function(t) { - var server = restify.createServer({ strictRouting: true }); - function noop() {} +test('lookupByName calls next with err', function(t) { + var router = new Router({ + log: {} + }); + var myErr = new Error('My Error'); + router.mount({ method: 'GET', path: '/', name: 'my-route' }, [ + function(req, res, next) { + next(myErr); + } + ]); - server.get('/trailing/', noop); - server.get('/no-trailing', noop); + var handlerFound = router.lookupByName('my-route', mockReq, mockRes); + t.ok(handlerFound); - var trailing = server.router.routes.GET[0]; - t.ok(trailing.path.test('/trailing/'), 'Single trailing slash is ok'); - t.notOk(trailing.path.test('/trailing'), 'No trailing slash is not ok'); - t.notOk( - trailing.path.test('/trailing//'), - 'Double trailing slash is not ok' - ); - t.notOk( - trailing.path.test('//trailing/'), - 'Double heading slash is not ok' - ); + handlerFound(mockReq, mockRes, function next(err) { + t.deepEqual(err, myErr); + t.end(); + }); +}); - var noTrailing = server.router.routes.GET[1]; - t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok')); - t.notOk( - noTrailing.path.test('/no-trailing/'), - 'Single trailing slash is not ok' - ); - t.notOk( - noTrailing.path.test('/no-trailing//'), - 'Double trailing slash is not ok' - ); - t.notOk( - noTrailing.path.test('//no-trailing'), - 'Double heading slash is not ok' +test('lookup runs a route chain by path and calls next', function(t) { + var router = new Router({ + log: {} + }); + router.mount({ method: 'GET', path: '/', name: 'my-route' }, [ + function(req, res, next) { + res.send('Hello world'); + next(); // no _afterRoute without next() + } + ]); + + var handlerFound = router.lookup( + Object.assign( + { + getUrl: function() { + return { pathname: '/' }; + }, + method: 'GET' + }, + mockReq + ), + mockRes ); + t.ok(handlerFound); - t.end(); + handlerFound(mockReq, mockRes, function next(err) { + t.ifError(err); + t.end(); + }); }); -test('Default non-strict routing ignores trailing slash(es)', function(t) { - var server = restify.createServer(); - function noop() {} - - server.get('/trailing/', noop); - server.get('/no-trailing', noop); +test('lookup calls next with err', function(t) { + var router = new Router({ + log: {} + }); + var myErr = new Error('My Error'); + router.mount({ method: 'GET', path: '/', name: 'my-route' }, [ + function(req, res, next) { + next(myErr); + } + ]); - var trailing = server.router.routes.GET[0]; - t.ok(trailing.path.test('/trailing/', 'Single trailing slash is ok')); - t.ok(trailing.path.test('/trailing'), 'No trailing slash is not ok'); - t.notOk( - trailing.path.test('/trailing//'), - 'Double trailing slash is not ok' - ); - t.notOk(trailing.path.test('//trailing'), 'Double heading slash is not ok'); - - var noTrailing = server.router.routes.GET[1]; - t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok')); - t.ok(noTrailing.path.test('/no-trailing/'), 'Single trailing slash is ok'); - t.notOk( - noTrailing.path.test('/no-trailing//'), - 'Double trailing slash is not ok' - ); - t.notOk( - noTrailing.path.test('//no-trailing'), - 'Double heading slash is not ok' + var handlerFound = router.lookup( + Object.assign( + { + getUrl: function() { + return { pathname: '/' }; + }, + method: 'GET' + }, + mockReq + ), + mockRes ); + t.ok(handlerFound); - t.end(); + handlerFound(mockReq, mockRes, function next(err) { + t.deepEqual(err, myErr); + t.end(); + }); }); -test('Find existing route with path', function(t) { - var server = restify.createServer(); - function noop() {} +test('route handles 404', function(t) { + var router = new Router({ + log: {} + }); + router.defaultRoute( + Object.assign( + { + getUrl: function() { + return { pathname: '/' }; + }, + method: 'GET' + }, + mockReq + ), + mockRes, + function next(err) { + t.equal(err.statusCode, 404); + t.end(); + } + ); +}); - var routePath = '/route/:withParam'; - server.get(routePath, noop); +test('route handles method not allowed (405)', function(t) { + var router = new Router({ + log: {} + }); + router.mount({ method: 'GET', path: '/', name: 'my-route' }, [ + function(req, res, next) { + res.send('Hello world'); + } + ]); - var foundRoute = server.router.findByPath( - '/route/:withADifferentParamName', - { method: 'GET' } + router.defaultRoute( + Object.assign( + { + getUrl: function() { + return { pathname: '/' }; + }, + method: 'POST' + }, + mockReq + ), + mockRes, + function next(err) { + t.equal(err.statusCode, 405); + t.end(); + } ); - t.equal(foundRoute.spec.path, routePath); +}); - var notFoundRoute = server.router.findByPath( - '/route/:withADifferentParamName([A-Z]{2,3})', - { method: 'GET' } - ); - t.notOk(notFoundRoute); +test('prints debug info', function(t) { + function handler1(req, res, next) { + res.send('Hello world'); + } + function handler2(req, res, next) { + res.send('Hello world'); + } + var router = new Router({ + log: {} + }); + router.mount({ method: 'GET', path: '/' }, [handler1]); + router.mount({ method: 'POST', path: '/' }, [handler1, handler2]); + + t.deepEqual(router.getDebugInfo(), { + get: { + name: 'get', + method: 'get', + path: '/', + handlers: [handler1] + }, + post: { + name: 'post', + method: 'post', + path: '/', + handlers: [handler1, handler2] + } + }); + t.end(); +}); + +test('toString()', function(t) { + function handler(req, res, next) { + res.send('Hello world'); + } + + var router = new Router({ + log: {} + }); + router.mount({ method: 'GET', path: '/' }, [handler]); + router.mount({ method: 'GET', path: '/a' }, [handler]); + router.mount({ method: 'GET', path: '/a/b' }, [handler]); + router.mount({ method: 'POST', path: '/' }, [handler]); + + t.deepEqual( + router.toString(), + '└── / (GET|POST)\n' + ' └── a (GET)\n' + ' └── /b (GET)\n' + ); t.end(); }); diff --git a/test/routerRegistryRadix.test.js b/test/routerRegistryRadix.test.js new file mode 100644 index 000000000..b2684d9c9 --- /dev/null +++ b/test/routerRegistryRadix.test.js @@ -0,0 +1,101 @@ +'use strict'; +/* eslint-disable func-names */ + +var RouterRegistryRadix = require('../lib/routerRegistryRadix'); +var Chain = require('../lib/chain'); + +if (require.cache[__dirname + '/lib/helper.js']) { + delete require.cache[__dirname + '/lib/helper.js']; +} +var helper = require('./lib/helper.js'); + +///--- Globals + +var test = helper.test; + +function getTestRoute(opts) { + var chain = new Chain(); + var name = opts.method + '-' + opts.path; + name = name.replace(/\W/g, '').toLowerCase(); + + return { + name: name, + method: opts.method, + path: opts.path, + spec: opts, + chain: chain + }; +} + +///--- Tests + +test('adds a route', function(t) { + var registry = new RouterRegistryRadix(); + registry.add(getTestRoute({ method: 'GET', path: '/' })); + registry.add(getTestRoute({ method: 'POST', path: '/' })); + registry.add(getTestRoute({ method: 'GET', path: '/ab' })); + + t.deepEqual(Object.keys(registry.get()), ['get', 'post', 'getab']); + + t.done(); +}); + +test('removes a route', function(t) { + var registry = new RouterRegistryRadix(); + + // Mount + registry.add(getTestRoute({ method: 'GET', path: '/a' })); + registry.add(getTestRoute({ method: 'POST', path: '/b' })); + t.deepEqual(Object.keys(registry.get()), ['geta', 'postb']); + + // Unmount + var route = registry.remove('geta'); + t.ok(route); + t.equal(route.name, 'geta'); + + // Removes from registry + t.deepEqual(Object.keys(registry.get()), ['postb']); + + t.end(); +}); + +test('lookups a route', function(t) { + var registry = new RouterRegistryRadix(); + var route = getTestRoute({ method: 'GET', path: '/a/:b' }); + registry.add(route); + + var result = registry.lookup('GET', '/a/b'); + + t.deepEqual(result, { + route: route, + params: { b: 'b' }, + handler: result.handler + }); + + t.done(); +}); + +test('get registered routes', function(t) { + var registry = new RouterRegistryRadix(); + registry.add(getTestRoute({ method: 'GET', path: '/' })); + registry.add(getTestRoute({ method: 'GET', path: '/a' })); + registry.add(getTestRoute({ method: 'GET', path: '/a/b' })); + registry.add(getTestRoute({ method: 'POST', path: '/' })); + + t.deepEqual(Object.keys(registry.get()), ['get', 'geta', 'getab', 'post']); + t.end(); +}); + +test('toString()', function(t) { + var registry = new RouterRegistryRadix(); + registry.add(getTestRoute({ method: 'GET', path: '/' })); + registry.add(getTestRoute({ method: 'GET', path: '/a' })); + registry.add(getTestRoute({ method: 'GET', path: '/a/b' })); + registry.add(getTestRoute({ method: 'POST', path: '/' })); + + t.deepEqual( + registry.toString(), + '└── / (GET|POST)\n' + ' └── a (GET)\n' + ' └── /b (GET)\n' + ); + t.end(); +}); diff --git a/test/server.test.js b/test/server.test.js index 0a5155978..babbdf151 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -178,7 +178,7 @@ test('use + get (path only)', function(t) { }); test('rm', function(t) { - var route = SERVER.get('/foo/:id', function foosy(req, res, next) { + var routeName = SERVER.get('/foo/:id', function foosy(req, res, next) { next(); }); @@ -189,7 +189,7 @@ test('rm', function(t) { next(); }); - t.ok(SERVER.rm(route)); + t.ok(SERVER.rm(routeName)); CLIENT.get('/foo/bar', function(err, _, res) { t.ok(err); @@ -202,31 +202,9 @@ test('rm', function(t) { }); }); -test('rm route and clear cached route', function(t) { - t.equal(SERVER.router.cache.dump().length, 0); - - var route = SERVER.get('/cached/route', function cachey(req, res, next) { - res.send({ foo: 'bar' }); - next(); - }); - - CLIENT.get('/cached/route', function(err, _, res) { - t.equal(SERVER.router.cache.dump().length, 1); - t.equal(SERVER.router.cache.dump()[0].v.name, route); - t.equal(res.statusCode, 200); - t.ok(SERVER.rm(route)); - CLIENT.get('/cached/route', function(err2, _2, res2) { - t.ok(err2); - t.equal(SERVER.router.cache.dump().length, 0); - t.equal(res2.statusCode, 404); - t.end(); - }); - }); -}); - test( - '_routeErrorResponse does not cause uncaughtException' + - 'when called when header has already been sent', + '_routeErrorResponse does not cause uncaughtException when called when' + + 'header has already been sent', function(t) { SERVER.on('MethodNotAllowed', function(req, res, error, next) { res.json(405, { status: 'MethodNotAllowed' }); @@ -252,73 +230,6 @@ test( } ); -test( - 'GH-1171: rm one version of the routes, ' + - 'other versions should still work', - function(t) { - var routeOne = SERVER.get( - { path: '/hello/:name', version: '1.0.0' }, - function(req, res, next) { - res.send('hello ' + req.params.name); - next(); - } - ); - var routeTwo = SERVER.get( - { path: '/hello/:name', version: '2.0.0' }, - function(req, res, next) { - res.send('hello ' + req.params.name); - next(); - } - ); - - var routeThree = SERVER.get( - { path: '/hello/:name', version: '3.0.0' }, - function(req, res, next) { - res.send('hello ' + req.params.name); - next(); - } - ); - - t.ok(SERVER.rm(routeThree)); - - var opts = { - path: '/hello/friend', - headers: { - 'accept-version': '3.0.0' - } - }; - CLIENT.get(opts, function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 400); - - opts.headers = { - 'accept-version': '1.0.0' - }; - CLIENT.get(opts, function(err2, _2, res2) { - t.ifError(err2); - t.equal(res2.statusCode, 200); - - opts.headers = { - 'accept-version': '2.0.0' - }; - CLIENT.get(opts, function(err3, _3, res3) { - t.ifError(err3); - t.equal(res3.statusCode, 200); - - t.ok(SERVER.rm(routeOne)); - t.ok(SERVER.rm(routeTwo)); - - CLIENT.get('/hello/friend', function(err4, _4, res4) { - t.ok(err4); - t.equal(res4.statusCode, 404); - t.end(); - }); - }); - }); - }); - } -); - test('use - throws TypeError on non function as argument', function(t) { var errMsg = 'handler (function) is required'; @@ -498,12 +409,15 @@ test('OPTIONS', function(t) { }); test('RegExp ok', function(t) { - SERVER.get(/\/foo/, function tester(req, res, next) { + SERVER.get('/example/:file(^\\d+).png', function tester(req, res, next) { + t.deepEqual(req.params, { + file: '12' + }); res.send('hi there'); next(); }); - CLIENT.get('/foo', function(err, _, res, obj) { + CLIENT.get('/example/12.png', function(err, _, res, obj) { t.ifError(err); t.equal(res.statusCode, 200); t.equal(obj, 'hi there'); @@ -538,29 +452,6 @@ test('get (path and version ok)', function(t) { }); }); -test('get (path and version not ok)', function(t) { - function respond(req, res, next) { - res.send(); - next(); - } - - SERVER.get({ url: '/foo/:id', version: '1.2.3' }, respond); - SERVER.get({ url: '/foo/:id', version: '3.2.1' }, respond); - - var opts = { - path: '/foo/bar', - headers: { - 'accept-version': '~2.1' - } - }; - CLIENT.get(opts, function(err, _, res) { - t.ok(err); - t.equal(err.body.message, '~2.1 is not supported by GET /foo/bar'); - t.equal(res.statusCode, 400); - t.end(); - }); -}); - test('GH-56 streaming with filed (download)', function(t) { SERVER.get('/', function tester(req, res, next) { filed(__filename).pipe(res); @@ -732,136 +623,6 @@ test('GH-77 uncaughtException (with custom handler)', function(t) { }); }); -test('GH-97 malformed URI breaks server', function(t) { - SERVER.get('/echo/:name', function(req, res, next) { - res.send(200); - next(); - }); - - CLIENT.get('/echo/mark%', function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 400); - t.end(); - }); -}); - -test('GH-109 RegExp flags not honored', function(t) { - SERVER.get(/\/echo\/(\w+)/i, function(req, res, next) { - res.send(200, req.params[0]); - next(); - }); - - CLIENT.get('/ECHO/mark', function(err, _, res, obj) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(obj, 'mark'); - t.end(); - }); -}); - -test('upload routing based on content-type ok', function(t) { - var opts = { - path: '/', - contentType: '*/json' - }; - SERVER.put(opts, function(req, res, next) { - res.send(204); - next(); - }); - - CLIENT.put('/', { foo: 'foo' }, function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 204); - t.end(); - }); -}); - -test('upload routing based on content-type fail', function(t) { - var opts = { - path: '/', - contentType: 'text/*' - }; - SERVER.put(opts, function(req, res, next) { - res.send(204); - next(); - }); - - CLIENT.put('/', { foo: 'foo' }, function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 415); - t.end(); - }); -}); - -test('path+flags ok', function(t) { - SERVER.get({ path: '/foo', flags: 'i' }, function(req, res, next) { - res.send('hi'); - next(); - }); - - CLIENT.get('/FoO', function(err, _, res, obj) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(obj, 'hi'); - t.end(); - }); -}); - -test('test matches params with custom regex', function(t) { - var Router = require('../lib/router'); - var router = new Router({ - log: helper.getLog() - }); - t.ok(router); - router.mount({ - method: 'GET', - name: 'test', - url: '/foo/:bar', - urlParamPattern: '[a-zA-Z0-9-_~%!;@=+\\$\\*\\.]+' - }); - - var count = 0; - var done = 0; - - function find(p, exp) { - count++; - var obj = { - headers: {}, - method: 'GET', - contentType: function() {}, - path: function() { - return p; - }, - version: function() { - return '*'; - }, - url: p - }; - - process.nextTick(function() { - router.find(obj, {}, function(err, r, ctx) { - if (exp) { - t.ifError(err); - t.ok(r); - t.ok(ctx); - t.deepEqual(ctx, { bar: exp }); - } else { - t.ok(err); - } - - if (++done === count) { - t.end(); - } - }); - }); - } - - find('/foo/a%40b.com', 'a@b.com'); - find('/foo/a@b.com', 'a@b.com'); - find('/foo/a*b.com', 'a*b.com'); - find('/foo/a%40b.com/bar', false); -}); - test('GH-180 can parse DELETE body', function(t) { SERVER.use(restify.plugins.bodyParser({ mapParams: false })); @@ -984,347 +745,6 @@ test('gh-278 missing router error events (405)', function(t) { }); }); -test('gh-278 missing router error events invalid version', function(t) { - var p = '/' + uuid.v4(); - SERVER.get( - { - path: p, - version: '1.2.3' - }, - function(req, res, next) { - res.send(200); - next(); - } - ); - SERVER.once('VersionNotAllowed', function(req, res) { - res.send(449, 'foo'); - }); - - var opts = { - path: p, - headers: { - 'accept-version': '3.2.1' - } - }; - CLIENT.get(opts, function(err, _, res) { - t.ok(err); - t.equal(err.message, '"foo"'); - t.equal(res.statusCode, 449); - t.end(); - }); -}); - -test('gh-278 missing router error events (415)', function(t) { - var p = '/' + uuid.v4(); - SERVER.post( - { - path: p, - contentType: 'text/xml' - }, - function(req, res, next) { - res.send(200); - next(); - } - ); - - SERVER.once('UnsupportedMediaType', function(req, res) { - res.send(415, 'foo'); - }); - - CLIENT.post(p, {}, function(err, _, res) { - t.ok(err); - t.equal(err.message, '"foo"'); - t.equal(res.statusCode, 415); - t.end(); - }); -}); - -test('next.ifError', function(t) { - var port = 3000; - var myServer = restify.createServer({ - handleUncaughtExceptions: true - }); - - myServer.use(function(req, res, next) { - next.ifError(null); - next(); - }); - - myServer.get('/foo/:id', function tester(req, res, next) { - process.nextTick(function() { - var e = new RestError({ - statusCode: 400, - restCode: 'Foo', - message: 'screw you client' - }); - next.ifError(e); - t.notOk(true); - res.send(200); - next(); - }); - }); - - myServer.listen(port, function() { - var myClient = restifyClients.createJsonClient({ - url: 'http://127.0.0.1:' + port, - headers: { - connection: 'close' - } - }); - - myClient.get('/foo/bar', function(err) { - t.ok(err); - t.equal(err.message, ''); - myServer.close(function() { - t.end(); - }); - }); - }); -}); - -test('next.ifError is not available by default', function(t) { - var port = 3000; - var myServer = restify.createServer(); - - myServer.get('/', function(req, res, next) { - t.throws( - function() { - next.ifError(new Error('boom')); - }, - 'TypeError', - 'next.ifError is not a function' - ); - - res.send('hi'); - t.end(); - }); - - myServer.listen(port, function() { - var myClient = restifyClients.createStringClient({ - url: 'http://127.0.0.1:' + port, - headers: { - connection: 'close' - } - }); - - myClient.get('/', function(err) { - t.ifError(err); - myServer.close(function() { - t.end(); - }); - }); - }); -}); - -test('gh-283 maximum available versioned route matching', function(t) { - var p = '/' + uuid.v4(); - var versions = ['1.0.0', '1.1.0']; - var i; - - function mnt(v) { - SERVER.get( - { - path: p, - version: v - }, - function(req, res, next) { - res.json(200, { version: v }); - next(); - } - ); - } - - for (i = 0; i < versions.length; i++) { - mnt(versions[i]); - } - - var opts = { - path: p, - headers: { - 'accept-version': '~1' - } - }; - - CLIENT.get(opts, function(err, _, res, obj) { - t.equal(obj.version, '1.1.0'); - t.end(); - }); -}); - -test('gh-635 routes match the maximum version', function(t) { - var p = '/' + uuid.v4(); - - SERVER.get( - { - path: p, - version: ['1.2.0', '1.2.1', '1.2.2'] - }, - function(req, res, next) { - res.json(200, { - requestedVersion: req.version(), - matchedVersion: req.matchedVersion() - }); - next(); - } - ); - - var opts = { - path: p, - headers: { - 'accept-version': '<1.2.2' - } - }; - - CLIENT.get(opts, function(err, _, res, obj) { - t.equal(obj.requestedVersion, '<1.2.2'); - t.equal(obj.matchedVersion, '1.2.1'); - t.end(); - }); -}); - -test('versioned route matching should prefer \ - first match if equal versions', function(t) { - var p = '/' + uuid.v4(); - - SERVER.get( - { - path: p, - version: ['1.1.0', '1.2.0'] - }, - function(req, res, next) { - res.json(200, { route: p }); - next(); - } - ); - - SERVER.get( - { - path: '/:id', - version: ['1.1.0', '1.2.0'] - }, - function(req, res, next) { - res.json(200, { route: 'id' }); - next(); - } - ); - - var opts = { - path: p, - headers: { - 'accept-version': '~1' - } - }; - - CLIENT.get(opts, function(err, _, res, obj) { - t.equal(obj.route, p); - t.end(); - }); -}); - -test('versioned route matching should not throw TypeError', function(t) { - var p = '/path/' + uuid.v4(); - - SERVER.post( - { - path: p, - version: ['1.1.0', '1.2.0'], - contentType: 'application/json' - }, - function(req, res, next) { - res.json(200, { route: p }); - next(); - } - ); - - SERVER.post( - { - path: '/path/:id', - version: ['1.1.0', '1.2.0'] - }, - function(req, res, next) { - res.json(200, { route: 'id' }); - next(); - } - ); - - var opts = { - path: p, - headers: { - 'accept-version': '~1' - } - }; - - CLIENT.post(opts, function(err, _, res, obj) { - t.equal(obj.route, p); - t.end(); - }); -}); - -test('GH-652 throw InvalidVersion on version mismatch', function(t) { - function response(req, res, next) { - return res.send(req.route.version); - } - SERVER.get({ path: '/ping', version: '1.0.1' }, response); - var opts = { - path: '/ping', - headers: { - 'accept-version': '1.0.2' - } - }; - CLIENT.get(opts, function(err, req, res, data) { - t.equal(res.statusCode, 400); - t.equal(data.code, 'InvalidVersion'); - t.done(); - }); -}); - -test('GH-652 throw InvalidVersion on non-versioned route', function(t) { - function response(req, res, next) { - return res.send(req.route.version); - } - SERVER.get({ path: '/ping' }, response); - var opts = { - path: '/ping', - headers: { - 'accept-version': '1.0.1' - } - }; - CLIENT.get(opts, function(err, req, res, data) { - t.equal(res.statusCode, 400); - t.equal(data.code, 'InvalidVersion'); - t.done(); - }); -}); - -test('GH-959 matchedVersion() should return on cached routes', function(t) { - SERVER.get( - { - path: '/test', - version: '0.5.0' - }, - function(req, res, next) { - res.send({ - version: req.version(), - matchedVersion: req.matchedVersion() - }); - return next(); - } - ); - - CLIENT.get('/test', function(err, _, res, body) { - t.ifError(err); - t.equal(body.version, '*'); - t.equal(body.matchedVersion, '0.5.0'); - - CLIENT.get('/test', function(err2, _2, res2, body2) { - t.ifError(err2); - t.equal(body.version, '*'); - t.equal(body.matchedVersion, '0.5.0'); - t.end(); - }); - }); -}); - test('gh-329 wrong values in res.methods', function(t) { function route(req, res, next) { res.send(200); @@ -1339,7 +759,7 @@ test('gh-329 wrong values in res.methods', function(t) { SERVER.once('MethodNotAllowed', function(req, res, cb) { t.ok(res.methods); - t.deepEqual(res.methods, ['GET', 'PUT', 'DELETE']); + t.deepEqual(res.methods, ['DELETE', 'GET', 'PUT']); res.send(405); }); @@ -1369,7 +789,7 @@ test('GH #704: Route with a valid RegExp params', function(t) { }); }); -test('GH #704: Route with an unvalid RegExp params', function(t) { +test('GH #704: Route with an invalid RegExp params', function(t) { SERVER.get( { name: 'regexp_param2', @@ -1389,144 +809,6 @@ test('GH #704: Route with an unvalid RegExp params', function(t) { }); }); -test('content-type routing vendor', function(t) { - SERVER.post( - { - name: 'foo', - path: '/', - contentType: 'application/vnd.joyent.com.foo+json' - }, - function(req, res, next) { - res.send(201); - } - ); - - SERVER.post( - { - name: 'bar', - path: '/', - contentType: 'application/vnd.joyent.com.bar+json' - }, - function(req, res, next) { - res.send(202); - } - ); - - var _done = 0; - - function done() { - if (++_done === 2) { - t.end(); - } - } - - var opts = { - path: '/', - headers: { - 'content-type': 'application/vnd.joyent.com.foo+json' - } - }; - CLIENT.post(opts, {}, function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 201); - done(); - }); - - var opts2 = { - path: '/', - headers: { - 'content-type': 'application/vnd.joyent.com.bar+json' - } - }; - CLIENT.post(opts2, {}, function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 202); - done(); - }); -}); - -test('content-type routing params only', function(t) { - SERVER.post( - { - name: 'foo', - path: '/', - contentType: 'application/json; type=foo' - }, - function(req, res, next) { - res.send(201); - } - ); - - SERVER.post( - { - name: 'bar', - path: '/', - contentType: 'application/json; type=bar' - }, - function(req, res, next) { - res.send(202); - } - ); - - var _done = 0; - - function done() { - if (++_done === 2) { - t.end(); - } - } - - var opts = { - path: '/', - headers: { - 'content-type': 'application/json; type=foo' - } - }; - CLIENT.post(opts, {}, function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 201); - done(); - }); - - var opts2 = { - path: '/', - headers: { - 'content-type': 'application/json; type=bar' - } - }; - CLIENT.post(opts2, {}, function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 202); - done(); - }); -}); - -test('malformed content type', function(t) { - SERVER.post( - { - name: 'foo', - path: '/', - contentType: 'application/json' - }, - function(req, res, next) { - res.send(201); - } - ); - - var opts = { - path: '/', - headers: { - 'content-type': 'boom' - } - }; - - CLIENT.post(opts, {}, function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 415); - t.end(); - }); -}); - test('gh-193 basic', function(t) { SERVER.get( { @@ -1622,9 +904,7 @@ test('run param only with existing req.params', function(t) { next(); }); - SERVER.param('userId', function(req, res, next, param, name) { - t.equal(param, '1'); - t.equal(name, 'userId'); + SERVER.param('userId', function(req, res, next) { count++; next(); }); @@ -1641,7 +921,7 @@ test('run param only with existing req.params', function(t) { }); }); -test('run param with false value', function(t) { +test('run param only with existing req.params', function(t) { var count = 0; SERVER.param('name', function(req, res, next) { @@ -1650,7 +930,7 @@ test('run param with false value', function(t) { }); SERVER.param('userId', function(req, res, next, param, name) { - t.equal(param, ''); + t.equal(param, '1'); t.equal(name, 'userId'); count++; next(); @@ -1660,7 +940,7 @@ test('run param with false value', function(t) { res.send(200); }); - CLIENT.get('/users//', function(err, _, res) { + CLIENT.get('/users/1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); t.equal(count, 1); @@ -1745,8 +1025,8 @@ test('gh-193 route chained', function(t) { ); CLIENT.get('/foo', function(err, _, res) { - t.ok(err); - t.equal(res.statusCode, 500); + t.ifError(err); + t.equal(res.statusCode, 200); t.equal(count, 1); t.end(); }); @@ -1791,45 +1071,6 @@ test('gh-193 route params basic', function(t) { }); }); -test('gh-193 same url w/params', function(t) { - var count = 0; - - SERVER.use(function(req, res, next) { - count++; - next(); - }); - - SERVER.get( - { - name: 'foo', - path: '/foo/:id' - }, - function(req, res, next) { - t.equal(req.params.id, 'blah'); - next('foo2'); - } - ); - - SERVER.get( - { - name: 'foo2', - path: '/foo/:baz' - }, - function(req, res, next) { - t.equal(req.params.baz, 'blah'); - res.send(200); - next(); - } - ); - - CLIENT.get('/foo/blah', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(count, 1); - t.end(); - }); -}); - test('gh-193 next("route") from a use plugin', function(t) { var count = 0; @@ -1915,30 +1156,6 @@ test('GH-384 res.json(200, {}) broken', function(t) { }); }); -test('GH-401 regex routing broken', function(t) { - function handle(req, res, next) { - res.send(204); - next(); - } - - var done = 0; - - function client_cb(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 204); - - if (++done === 2) { - t.end(); - } - } - - SERVER.get('/image', handle); - SERVER.get(/^(\/image\/)(.*)/, handle); - - CLIENT.get('/image', client_cb); - CLIENT.get('/image/1.jpg', client_cb); -}); - test('explicitly sending a 403 with custom error', function(t) { function MyCustomError() {} @@ -2017,34 +1234,32 @@ test('error handler defers "after" event', function(t) { }); }); -test( - 'gh-757 req.absoluteUri() ' + 'defaults path segment to req.path()', - function(t) { - SERVER.get('/the-original-path', function(req, res, next) { - var prefix = 'http://127.0.0.1:' + PORT; - t.equal( - req.absoluteUri('?key=value'), - prefix + '/the-original-path/?key=value' - ); - t.equal( - req.absoluteUri('#fragment'), - prefix + '/the-original-path/#fragment' - ); - t.equal( - req.absoluteUri('?key=value#fragment'), - prefix + '/the-original-path/?key=value#fragment' - ); - res.send(); - next(); - }); +// eslint-disable-next-line +test('gh-757 req.absoluteUri() defaults path segment to req.path()', function(t) { + SERVER.get('/the-original-path', function(req, res, next) { + var prefix = 'http://127.0.0.1:' + PORT; + t.equal( + req.absoluteUri('?key=value'), + prefix + '/the-original-path/?key=value' + ); + t.equal( + req.absoluteUri('#fragment'), + prefix + '/the-original-path/#fragment' + ); + t.equal( + req.absoluteUri('?key=value#fragment'), + prefix + '/the-original-path/?key=value#fragment' + ); + res.send(); + next(); + }); - CLIENT.get('/the-original-path', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.end(); - }); - } -); + CLIENT.get('/the-original-path', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.end(); + }); +}); test('GH-693 sending multiple response header values', function(t) { SERVER.get('/', function(req, res, next) { @@ -2146,13 +1361,6 @@ test( } ); -test('gh-630 handle server versions as an array or string', function(t) { - t.ok(SERVER.toString().indexOf('0.5.4,1.4.3,2.0.0') > -1); - SERVER.versions = '3.0.0'; - t.ok(SERVER.toString().indexOf('3.0.0') > -1); - t.end(); -}); - test('GH-877 content-type should be case insensitive', function(t) { SERVER.use(restify.plugins.bodyParser({ maxBodySize: 1024 })); @@ -2219,9 +1427,9 @@ test( }, function second(req, res, next) { req.startHandlerTimer('second'); + numCount++; + req.endHandlerTimer('second'); setTimeout(function() { - numCount++; - req.endHandlerTimer('second'); return next(); }, 300); }, @@ -2233,7 +1441,65 @@ test( } ]); - CLIENT.get('/audit', function(err, req, res, data) { + // set up audit logs + var ringbuffer = new bunyan.RingBuffer({ limit: 1 }); + SERVER.on( + 'after', + restify.plugins.auditLogger({ + log: bunyan.createLogger({ + name: 'audit', + streams: [ + { + level: 'info', + type: 'raw', + stream: ringbuffer + } + ] + }), + event: 'after' + }) + ); + + SERVER.on('after', function(req, res, route, err) { + if (req.href() === '/audit?v=2') { + // should request timeout error + t.ok(err); + t.equal(err.name, 'RequestCloseError'); + + // check records + t.ok(ringbuffer.records[0], 'no log records'); + t.equal( + ringbuffer.records.length, + 1, + 'should only have 1 log record' + ); + // TODO: fix this after plugin is fixed to use + // req.connectionState() + // t.equal(ringbuffer.records[0].req.clientClosed, true); + + // check timers + var handlers = Object.keys(ringbuffer.records[0].req.timers); + t.equal(handlers.length, 2, 'should only have 2 req timers'); + t.equal( + handlers[0], + 'first', + 'first handler timer not in order' + ); + t.equal( + handlers[handlers.length - 1], + 'second', + 'second handler not last' + ); + t.end(); + + // ensure third handler never ran + t.equal(numCount, 2); + + t.end(); + } + }); + + CLIENT.get('/audit?v=1', function(err, req, res, data) { t.ifError(err); t.deepEqual(data, { hello: 'world' }); t.equal(numCount, 3); @@ -2241,69 +1507,9 @@ test( // reset numCount numCount = 0; - // set up audit logs - var ringbuffer = new bunyan.RingBuffer({ limit: 1 }); - SERVER.once( - 'after', - restify.plugins.auditLogger({ - log: bunyan.createLogger({ - name: 'audit', - streams: [ - { - level: 'info', - type: 'raw', - stream: ringbuffer - } - ] - }), - event: 'after' - }) - ); - - FAST_CLIENT.get('/audit', function(err2, req2, res2, data2) { - setTimeout(function() { - // should request timeout error - t.ok(err2); - t.equal(err2.name, 'RequestTimeoutError'); - t.deepEqual(data2, {}); - - // check records - t.ok(ringbuffer.records[0], 'no log records'); - t.equal( - ringbuffer.records.length, - 1, - 'should only have 1 log record' - ); - // TODO: fix this after plugin is fixed to use - // req.connectionState() - // t.equal(ringbuffer.records[0].req.clientClosed, true); - - // check timers - var handlers = Object.keys( - ringbuffer.records[0].req.timers - ); - t.equal( - handlers.length, - 2, - 'should only have 2 req timers' - ); - t.equal( - handlers[0], - 'first', - 'first handler timer not in order' - ); - t.equal( - handlers[handlers.length - 1], - 'second', - 'second handler not last' - ); - t.end(); - - // ensure third handler never ran - t.equal(numCount, 2); - }, 500); - // don't start tests until a little after the request times - // out so that server can start the audit logs. + FAST_CLIENT.get('/audit?v=2', function(err2, req2, res2, data2) { + t.ok(err2); + t.equal(err2.name, 'RequestTimeoutError'); }); }); } @@ -2367,26 +1573,24 @@ test('GH-667 emit error event for generic Errors', function(t) { /*eslint-enable no-shadow*/ }); -test( - 'GH-667 returning error in error handler ' + 'should not do anything', - function(t) { - SERVER.on('ImATeapot', function(req, res, err, cb) { - // attempt to pass a new error back - return cb(new errors.LockedError('oh noes')); - }); +// eslint-disable-next-line +test('GH-667 returning error in error handler should not do anything', function(t) { + SERVER.on('ImATeapot', function(req, res, err, cb) { + // attempt to pass a new error back + return cb(new errors.LockedError('oh noes')); + }); - SERVER.get('/1', function(req, res, next) { - return next(new errors.ImATeapotError('foobar')); - }); + SERVER.get('/1', function(req, res, next) { + return next(new errors.ImATeapotError('foobar')); + }); - CLIENT.get('/1', function(err, req, res, data) { - t.ok(err); - // should still get the original error - t.equal(err.name, 'ImATeapotError'); - t.end(); - }); - } -); + CLIENT.get('/1', function(err, req, res, data) { + t.ok(err); + // should still get the original error + t.equal(err.name, 'ImATeapotError'); + t.end(); + }); +}); test('GH-958 RCS does not write triggering record', function(t) { var passThrough = new stream.PassThrough(); @@ -2524,6 +1728,32 @@ test('calling next(false) should early exit from pre handlers', function(t) { }); }); +test('calling next(false) should early exit from use handlers', function(t) { + var steps = 0; + + SERVER.use(function(req, res, next) { + res.send('early exit'); + return next(false); + }); + + SERVER.get('/1', function(req, res, next) { + res.send('hello world'); + return next(); + }); + + SERVER.on('after', function() { + steps++; + t.equal(steps, 2); + t.end(); + }); + + CLIENT.get('/1', function(err, req, res, data) { + t.ifError(err); + t.equal(data, 'early exit'); + steps++; + }); +}); + test('calling next(err) from pre should still emit after event', function(t) { setTimeout(function() { t.fail('Timed out'); @@ -2595,35 +1825,33 @@ test('GH-1078: server name should be customizable', function(t) { }); }); -test( - 'GH-1078: server name should be overridable ' + 'and not sent down', - function(t) { - var myServer = restify.createServer({ - name: '' - }); - var port = 3000; +// eslint-disable-next-line +test('GH-1078: server name should be overridable and not sent down', function(t) { + var myServer = restify.createServer({ + name: '' + }); + var port = 3000; - myServer.get('/', function(req, res, next) { - res.send('hi'); - return next(); - }); + myServer.get('/', function(req, res, next) { + res.send('hi'); + return next(); + }); - var myClient = restifyClients.createStringClient({ - url: 'http://127.0.0.1:' + port, - headers: { - connection: 'close' - } - }); + var myClient = restifyClients.createStringClient({ + url: 'http://127.0.0.1:' + port, + headers: { + connection: 'close' + } + }); - myServer.listen(port, function() { - myClient.get('/', function(err, req, res, data) { - t.ifError(err); - t.equal(res.headers.hasOwnProperty('server'), false); - myServer.close(t.end); - }); + myServer.listen(port, function() { + myClient.get('/', function(err, req, res, data) { + t.ifError(err); + t.equal(res.headers.hasOwnProperty('server'), false); + myServer.close(t.end); }); - } -); + }); +}); test("should emit 'after' on successful request", function(t) { SERVER.on('after', function(req, res, route, err) { @@ -2722,33 +1950,6 @@ test( } ); -test( - "should 'emit' after on aborted request " + - "(req.connectionState(): 'aborted')", - function(t) { - SERVER.on('after', function(req, res, route, err) { - t.ok(err); - t.equal(req.connectionState(), 'aborted'); - t.equal(err.name, 'RequestAbortedError'); - }); - - SERVER.get('/foobar', function(req, res, next) { - req.emit('aborted'); - // fast client times out at 500ms, wait for 800ms which should cause - // client to timeout - setTimeout(function() { - return next(); - }, 800); - }); - - FAST_CLIENT.get('/foobar', function(err, _, res) { - t.ok(err); - t.equal(err.name, 'RequestTimeoutError'); - t.end(); - }); - } -); - test('should increment/decrement inflight request count', function(t) { SERVER.get('/foo', function(req, res, next) { t.equal(SERVER.inflightRequests(), 1); @@ -2756,46 +1957,48 @@ test('should increment/decrement inflight request count', function(t) { return next(); }); + SERVER.on('after', function() { + t.equal(SERVER.inflightRequests(), 0); + t.end(); + }); + CLIENT.get('/foo', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); - t.equal(SERVER.inflightRequests(), 0); - t.end(); + t.equal(SERVER.inflightRequests(), 1); }); }); -test( - 'should increment/decrement inflight request count ' + - 'for concurrent reqs', - function(t) { - SERVER.get('/foo1', function(req, res, next) { - t.equal(SERVER.inflightRequests(), 1); - setTimeout(function() { - res.send(); - return next(); - }, 250); - }); - - SERVER.get('/foo2', function(req, res, next) { - t.equal(SERVER.inflightRequests(), 2); +// eslint-disable-next-line +test('should increment/decrement inflight request count for concurrent reqs', function(t) { + SERVER.get('/foo1', function(req, res, next) { + // other request is already sent + t.equal(SERVER.inflightRequests() >= 1, true); + setTimeout(function() { res.send(); return next(); - }); + }, 250); + }); - CLIENT.get('/foo1', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(SERVER.inflightRequests(), 0); - t.end(); - }); + SERVER.get('/foo2', function(req, res, next) { + t.equal(SERVER.inflightRequests(), 2); + res.send(); + return next(); + }); - CLIENT.get('/foo2', function(err, _, res) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(SERVER.inflightRequests(), 1); - }); - } -); + CLIENT.get('/foo1', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.equal(SERVER.inflightRequests(), 1); + t.end(); + }); + + CLIENT.get('/foo2', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.equal(SERVER.inflightRequests(), 2); + }); +}); test("should emit 'close' on server close", function(t) { var server = restify.createServer(); @@ -2815,23 +2018,32 @@ test('should cleanup inflight requests count for 404s', function(t) { return next(); }); + SERVER.on('after', function(req) { + if (req.path() === '/doesnotexist') { + t.equal(SERVER.inflightRequests(), 0); + t.end(); + } + }); + CLIENT.get('/foo1', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); - t.equal(SERVER.inflightRequests(), 0); + t.equal(SERVER.inflightRequests(), 1); CLIENT.get('/doesnotexist', function(err2, _2, res2) { t.ok(err2); t.equal(res2.statusCode, 404); t.equal(SERVER.inflightRequests(), 0); - t.end(); }); }); }); test('should cleanup inflight requests count for timeouts', function(t) { + t.equal(SERVER.inflightRequests(), 0); + SERVER.get('/foo1', function(req, res, next) { - t.equal(SERVER.inflightRequests(), 1); + // othr request is already sent + t.equal(SERVER.inflightRequests() >= 1, true); setTimeout(function() { res.send(); return next(); @@ -2844,45 +2056,44 @@ test('should cleanup inflight requests count for timeouts', function(t) { return next(); }); + SERVER.on('after', function(req) { + if (req.path() === '/foo1') { + t.equal(SERVER.inflightRequests(), 0); + t.end(); + } else if (req.path() === '/foo2') { + t.equal(SERVER.inflightRequests(), 1); + } + }); + FAST_CLIENT.get('/foo1', function(err, _, res) { t.ok(err); t.equal(SERVER.inflightRequests(), 1); - - setTimeout(function() { - // wait for server to flush response, 600 extra plus the already - // 500ms we waited should be enough to cover the 1000 response time - // of server. - t.equal(SERVER.inflightRequests(), 0); - t.end(); - }, 600); }); CLIENT.get('/foo2', function(err, _, res) { t.ifError(err); t.equal(res.statusCode, 200); - t.equal(SERVER.inflightRequests(), 1); + t.equal(SERVER.inflightRequests(), 2); }); }); -test( - 'should cleanup inflight requests ' + 'count on uncaughtExceptions', - function(t) { - SERVER.on('uncaughtException', function(req, res, route, err) { - res.send(500, 'asplode'); - }); +// eslint-disable-next-line +test('should cleanup inflight requests count on uncaughtExceptions', function(t) { + SERVER.on('uncaughtException', function(req, res, route, err) { + res.send(500, 'asplode'); + }); - SERVER.get('/foo1', function(req, res, next) { - t.equal(SERVER.inflightRequests(), 1); - throw new Error('oh noes'); - }); + SERVER.get('/foo1', function(req, res, next) { + t.equal(SERVER.inflightRequests(), 1); + throw new Error('oh noes'); + }); - CLIENT.get('/foo1', function(err, _, res) { - t.ok(err); - t.equal(SERVER.inflightRequests(), 0); - t.end(); - }); - } -); + CLIENT.get('/foo1', function(err, _, res) { + t.ok(err); + t.equal(SERVER.inflightRequests(), 0); + t.end(); + }); +}); test('should show debug information', function(t) { SERVER.pre(function pre(req, res, next) { @@ -2916,11 +2127,7 @@ test('should show debug information', function(t) { return next(); }); - SERVER.get(/^\/([a-zA-Z0-9_\.~-]+)\/(.*)/, function freeform( - req, - res, - next - ) { + SERVER.get('/example/:file(^\\d+).png', function freeform(req, res, next) { res.end(); return next(); }); @@ -2934,16 +2141,6 @@ test('should show debug information', function(t) { t.ok(route); t.equal(typeof route.name, 'string'); t.equal(typeof route.method, 'string'); - t.ok( - typeof route.input === 'string' || - route.input instanceof RegExp === true - ); - t.equal(typeof route.compiledRegex, 'object'); - - t.equal(route.versions instanceof Array, true); - route.versions.forEach(function(v) { - t.equal(typeof v, 'string'); - }); t.equal(route.handlers instanceof Array, true); route.handlers.forEach(function(handlerFn) { @@ -2951,7 +2148,8 @@ test('should show debug information', function(t) { }); }); - // check /foo + // // check /foo + // TODO: should it contain use handlers? t.equal(debugInfo.routes[0].handlers[0], 'use'); t.equal(debugInfo.routes[0].handlers[1], 'use2'); t.equal(debugInfo.routes[0].handlers[2], 'anonymous'); @@ -2977,25 +2175,12 @@ test('should show debug information', function(t) { // detailed test for compiled regex // verify url parameter regex - t.deepEqual(debugInfo.routes[1].name, 'getbarab054143200'); + t.deepEqual(debugInfo.routes[1].name, 'getbarab'); t.deepEqual(debugInfo.routes[1].method, 'get'); - t.deepEqual(debugInfo.routes[1].input, '/bar/:a/:b'); - t.ok(debugInfo.routes[1].compiledRegex instanceof RegExp); - t.deepEqual(debugInfo.routes[1].compiledUrlParams, { - 0: 'a', - 1: 'b' - }); + // verify freeform regex - t.deepEqual(debugInfo.routes[2].name, 'getazaz09_054143200'); + t.deepEqual(debugInfo.routes[2].name, 'getexamplefiledpng'); t.deepEqual(debugInfo.routes[2].method, 'get'); - t.ok(debugInfo.routes[2].input instanceof RegExp); - t.ok(debugInfo.routes[2].compiledRegex instanceof RegExp); - // freeform regex input should equal output - t.equal( - debugInfo.routes[2].input.toString(), - debugInfo.routes[2].compiledRegex.toString() - ); - t.deepEqual(debugInfo.routes[2].compiledUrlParams, null); // verify other server details t.deepEqual(Object.keys(debugInfo.server.formatters), [ @@ -3125,20 +2310,43 @@ test('should emit restifyError even for router errors', function(t) { }); }); -test('calling next twice should throw', function(t) { - SERVER.get('/', function(req, res, next) { - res.send(200); - next(); - next(); +test('should emit error with multiple next calls with strictNext', function(t) { + var server = restify.createServer({ + dtrace: helper.dtrace, + strictNext: true, + handleUncaughtExceptions: true, + log: helper.getLog('server') }); + var client; + var port; - SERVER.on('uncaughtException', function(req, res, route, err) { - t.ok(err); - t.equal(err.message, "next shouldn't be called more than once"); - t.end(); - }); + server.listen(PORT + 1, '127.0.0.1', function() { + port = server.address().port; + client = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + port, + dtrace: helper.dtrace, + retry: false + }); - CLIENT.get('/', function(err, req, res, data) { - t.ifError(err); + server.get('/strict-next', function(req, res, next) { + next(); + next(); + }); + + server.on('uncaughtException', function(req, res, route, err) { + t.ok(err); + t.equal(err.message, "next shouldn't be called more than once"); + res.send(err); + }); + + client.get('/strict-next', function(err, _, res) { + t.ok(err); + t.equal(res.statusCode, 500); + + client.close(); + server.close(function() { + t.end(); + }); + }); }); }); diff --git a/tools/docsBuild.js b/tools/docsBuild.js index e760c6418..ce3a6e60d 100644 --- a/tools/docsBuild.js +++ b/tools/docsBuild.js @@ -66,6 +66,7 @@ var docsConfig = [ 'plugins/inflightRequestThrottle.js' ), path.join(__dirname, LIB_PATH, 'plugins/cpuUsageThrottle.js'), + path.join(__dirname, LIB_PATH, 'plugins/conditionalHandler.js'), path.join(__dirname, LIB_PATH, 'plugins/conditionalRequest.js'), path.join(__dirname, LIB_PATH, 'plugins/audit.js'), path.join(__dirname, LIB_PATH, 'plugins/metrics.js')