From de1833a44084e5f231de289421518ec646b86f60 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 7 Dec 2018 09:48:07 -0800 Subject: [PATCH] feat: add support for non-strict formatters (#1721) --- docs/guides/server.md | 57 ++++++++++-- docs/index.md | 65 +++++++++++-- lib/index.js | 5 + lib/response.js | 158 +++++++++++++++++++------------- lib/server.js | 12 +++ test/formatter-optional.test.js | 75 +++++++++++++++ 6 files changed, 290 insertions(+), 82 deletions(-) create mode 100644 test/formatter-optional.test.js diff --git a/docs/guides/server.md b/docs/guides/server.md index c1a7e5690..e90ecf610 100644 --- a/docs/guides/server.md +++ b/docs/guides/server.md @@ -425,12 +425,34 @@ server.get('/websocket/attach', function upgradeRoute(req, res, next) { }); ``` -## Content Negotiation +## Responses' Content Negotiation And Formatting -If you're using `res.send()` restify will automatically select the content-type -to respond with, by finding the first registered `formatter` defined. Note in -the examples above we've not defined any formatters, so we've been leveraging -the fact that restify ships with `application/json`, `text/plain` and +If you're using `res.send()` restify will determine the content-type to respond +with by, from highest priority to lowest priority: + +1. using the value of `res.contentType` if present +1. otherwise, using the value of the `Content-Type` response header if set +1. otherwise, using `application/json` if the body is an object that is not a + Buffer instance +1. otherwise, negotiating the content-type by matching available formatters with + the request's `accept` header + +If a content-type can't be determined, then restify will respond with an error. + +If a content-type can be negotiated, restify then determines what formatter to +use to format the response's content. + +If no formatter matching the content-type can be found, restify will by default +override the response's content-type to `'application/octet-stream'` and then +error if no formatter is found for that content-type. + +This default behavior can be changed by passing `strictFormatters: false` +(default is true) when creating the restify server instance. In that case, if no +formatter is found for the negotiated content-type, the response is flushed +without applying any formatter. + +Note in the examples above we've not defined any formatters, so we've been +leveraging the fact that restify ships with `application/json`, `text/plain` and `application/octet-stream` formatters. You can add additional formatters to restify by passing in a hash of content-type -> parser at server creation time: @@ -450,9 +472,8 @@ var server = restify.createServer({ }); ``` -If a content-type can't be negotiated, then restify will default to using the -`application/octet-stream` formatter. For example, attempting to send a -content-type that does not have a defined formatter: +For example, attempting to send a content-type that does not have a defined +formatter: ```js server.get('/foo', function(req, res, next) { @@ -473,6 +494,26 @@ Date: Thu, 02 Jun 2016 06:50:54 GMT Connection: keep-alive ``` +However, if the server instance is created with `strictFormatters: false`: + +```js +var server = restify.createServer({ + strictFormatters: false +}); +``` + +The response has a content-type of `text/css` even though no `'text/css'` +formatter is present: + +```sh +$ curl -i localhost:3000/ +HTTP/1.1 200 OK +Content-Type: text/css +Content-Length: 2 +Date: Thu, 02 Jun 2016 06:50:54 GMT +Connection: keep-alive +``` + As previously noted, restify ships with built-in formatters for json, text, and binary. When you override or append to this, the "priority" might change; to ensure that the priority is set to what you want, you should set a `q-value` diff --git a/docs/index.md b/docs/index.md index 93c83dab2..f6d17e2e3 100644 --- a/docs/index.md +++ b/docs/index.md @@ -435,12 +435,34 @@ server.get('/websocket/attach', function upgradeRoute(req, res, next) { }); ``` -## Content Negotiation +## Responses' Content Negotiation And Formatting -If you're using `res.send()` restify will automatically select the content-type -to respond with, by finding the first registered `formatter` defined. Note in -the examples above we've not defined any formatters, so we've been leveraging -the fact that restify ships with `application/json`, `text/plain` and +If you're using `res.send()` restify will determine the content-type to respond +with by, from highest priority to lowest priority: + +1. using the value of `res.contentType` if present +1. otherwise, using the value of the `Content-Type` response header if set +1. otherwise, using `application/json` if the body is an object that is not a + Buffer instance +1. otherwise, negotiating the content-type by matching available formatters with + the request's `accept` header + +If a content-type can't be determined, then restify will respond with an error. + +If a content-type can be negotiated, restify then determines what formatter to +use to format the response's content. + +If no formatter matching the content-type can be found, restify will by default +override the response's content-type to `'application/octet-stream'` and then +error if no formatter is found for that content-type. + +This default behavior can be changed by passing `strictFormatters: false` +(default is false) when creating the restify server instance. In that case, if +no formatter is found for the negotiated content-type, the response is flushed +without applying any formatter. + +Note in the examples above we've not defined any formatters, so we've been +leveraging the fact that restify ships with `application/json`, `text/plain` and `application/octet-stream` formatters. You can add additional formatters to restify by passing in a hash of content-type -> parser at server creation time: @@ -460,9 +482,8 @@ var server = restify.createServer({ }); ``` -If a content-type can't be negotiated, then restify will default to using the -`application/octet-stream` formatter. For example, attempting to send a -content-type that does not have a defined formatter: +For example, attempting to send a content-type that does not have a defined +formatter: ```js server.get('/foo', function(req, res, next) { @@ -483,6 +504,26 @@ Date: Thu, 02 Jun 2016 06:50:54 GMT Connection: keep-alive ``` +However, if the server instance is created with `strictFormatters:false`: + +```js +var server = restify.createServer({ + strictFormatters: false +}); +``` + +The response would has a content-type of `text/css` even though no `'text/css'` +formatter is present: + +```sh +$ curl -i localhost:3000/ +HTTP/1.1 200 OK +Content-Type: text/css +Content-Length: 2 +Date: Thu, 02 Jun 2016 06:50:54 GMT +Connection: keep-alive +``` + As previously noted, restify ships with built-in formatters for json, text, and binary. When you override or append to this, the "priority" might change; to ensure that the priority is set to what you want, you should set a `q-value` @@ -505,6 +546,14 @@ restify.createServer({ }); ``` +Restify ships with the following default formatters, which can be overridden +when passing a formatters options to `createServer()`: + +* application/javascript +* application/json +* text/plain +* application/octet-stream + The restify response object retains has all the "raw" methods of a node [ServerResponse](http://nodejs.org/docs/latest/api/http.html#http.ServerResponse) on it as well. diff --git a/lib/index.js b/lib/index.js index 76e35fa5c..6cc29fd29 100644 --- a/lib/index.js +++ b/lib/index.js @@ -59,6 +59,11 @@ require('./errorTypes'); * `res.writeContinue()` in `server.on('checkContinue')` when proxing * @param {Boolean} [options.ignoreTrailingSlash=false] - ignore trailing slash * on paths + * @param {Boolean} [options.strictFormatters=true] - enables strict formatters + * behavior: a formatter matching the response's content-type is required. If + * not found, the response's content-type is automatically set to + * 'application/octet-stream'. If a formatter for that content-type is not + * found, sending the response errors. * @example * var restify = require('restify'); * var server = restify.createServer(); diff --git a/lib/response.js b/lib/response.js index 96d22bf47..a7f817546 100644 --- a/lib/response.js +++ b/lib/response.js @@ -39,6 +39,8 @@ var HEADER_ARRAY_BLACKLIST = { * @returns {undefined} No return value */ function patch(Response) { + assert.func(Response, 'Response'); + /** * Wraps all of the node * [http.ServerResponse](https://nodejs.org/docs/latest/api/http.html) @@ -346,6 +348,11 @@ function patch(Response) { }; } + assert.ok( + typeof sendArgs.body === 'string' || Buffer.isBuffer(sendArgs.body), + 'res.sendRaw() accepts only strings or buffers' + ); + sendArgs.format = false; return self.__send(sendArgs); }; @@ -417,82 +424,93 @@ function patch(Response) { return flush(self); } - // if no formatting, assert that the value to be written is a string - // or a buffer, then send it. - if (opts.format === false) { - assert.ok( - typeof body === 'string' || Buffer.isBuffer(body), - 'res.sendRaw() accepts only strings or buffers' - ); - return flush(self, body); - } + if (opts.format === true) { + // if no body, then no need to format. if this was an error caught + // by a domain, don't send the domain error either. + if (body === undefined || (body instanceof Error && body.domain)) { + return flush(self); + } - // if no body, then no need to format. if this was an error caught by a - // domain, don't send the domain error either. - if (body === undefined || (body instanceof Error && body.domain)) { - return flush(self); - } + // At this point we know we have a body that needs to be formatted, + // so lets derive the formatter based on the response object's + // properties - // At this point we know we have a body that needs to be formatted, so - // lets derive the formatter based on the response object's properties + var formatter; + var type = self.contentType || self.getHeader('Content-Type'); - var formatter; - var type = self.contentType || self.getHeader('Content-Type'); + // Set Content-Type to application/json when + // res.send is called with an Object instead of calling res.json + if (!type && typeof body === 'object' && !Buffer.isBuffer(body)) { + type = 'application/json'; + } - // Set Content-Type to application/json when - // res.send is called with an Object instead of calling res.json - if (!type && typeof body === 'object' && !Buffer.isBuffer(body)) { - type = 'application/json'; - } + // Derive type if not provided by the user + type = type || self.req.accepts(self.acceptable); + + // Check to see if we could find a content type to use for the + // response. + if (!type) { + return formatterError( + self, + new errors.NotAcceptableError({ + message: + 'could not find suitable content-type to use ' + + 'for the response' + }) + ); + } - // Derive type if not provided by the user - type = type || self.req.accepts(self.acceptable); + type = type.split(';')[0]; - // Check to see if we can find a valid formatter - if (!type) { - return formatterError( - self, - new errors.NotAcceptableError({ - message: 'could not find suitable formatter' - }) - ); - } + if (!self.formatters[type] && type.indexOf('/') === -1) { + type = mime.lookup(type); + } - type = type.split(';')[0]; + // If finding a formatter matching the negotiated content-type is + // required, and we were unable to derive a valid type, default to + // treating it as arbitrary binary data per RFC 2046 Section 4.5.1 + if ( + this._strictFormatters && + !self.formatters[type] && + self.acceptable.indexOf(type) === -1 + ) { + type = 'application/octet-stream'; + } - if (!self.formatters[type] && type.indexOf('/') === -1) { - type = mime.lookup(type); - } + formatter = self.formatters[type] || self.formatters['*/*']; + + // If after the above attempts we were still unable to derive a + // formatter, provide a meaningful error message + if (this._strictFormatters && !formatter) { + return formatterError( + self, + new errors.InternalServerError({ + message: + 'could not find formatter for response ' + + 'content-type "' + + type + + '"' + }) + ); + } - // If we were unable to derive a valid type, default to treating it as - // arbitrary binary data per RFC 2046 Section 4.5.1 - if (!self.formatters[type] && self.acceptable.indexOf(type) === -1) { - type = 'application/octet-stream'; - } + if (self._charSet) { + type = type + '; charset=' + self._charSet; + } - formatter = self.formatters[type] || self.formatters['*/*']; - - // If after the above attempts we were still unable to derive a - // formatter, provide a meaningful error message - if (!formatter) { - return formatterError( - self, - new errors.InternalServerError({ - message: - 'could not find formatter for application/octet-stream' - }) - ); - } + // Update Content-Type header to the one originally set or to the + // type inferred from the most relevant formatter found. + self.setHeader('Content-Type', type); - if (self._charSet) { - type = type + '; charset=' + self._charSet; - } + if (formatter) { + // Finally, invoke the formatter and flush the request with it's + // results - // Update header to the derived content type for our formatter - self.setHeader('Content-Type', type); + return flush(self, formatter(self.req, self, body)); + } + } - // Finally, invoke the formatter and flush the request with it's results - return flush(self, formatter(self.req, self, body)); + return flush(self, body); }; /** @@ -813,11 +831,19 @@ function patch(Response) { * @private * @function flush * @param {Response} res - response - * @param {String|Buffer} formattedBody - formatted body + * @param {String|Buffer} body - response body * @returns {Response} response */ -function flush(res, formattedBody) { - res._data = formattedBody; +function flush(res, body) { + assert.ok( + body === null || + body === undefined || + typeof body === 'string' || + Buffer.isBuffer(body), + 'body must be a string or a Buffer instance' + ); + + res._data = body; // Flush headers res.writeHead(res.statusCode); diff --git a/lib/server.js b/lib/server.js index f47deb446..0b81f4424 100644 --- a/lib/server.js +++ b/lib/server.js @@ -82,6 +82,11 @@ var sprintf = util.format; * `res.writeContinue()` in `server.on('checkContinue')` when proxing * @param {Boolean} [options.ignoreTrailingSlash=false] - ignore trailing slash * on paths + * @param {Boolean} [options.strictFormatters=true] - enables strict formatters + * behavior: a formatter matching the response's content-type is required. If + * not found, the response's content-type is automatically set to + * 'application/octet-stream'. If a formatter for that content-type is not + * found, sending the response errors. * @example * var restify = require('restify'); * var server = restify.createServer(); @@ -103,6 +108,7 @@ function Server(options) { assert.optionalBool(options.socketio, 'options.socketio'); assert.optionalBool(options.onceNext, 'options.onceNext'); assert.optionalBool(options.strictNext, 'options.strictNext'); + assert.optionalBool(options.strictFormatters, 'options.strictFormatters'); var self = this; @@ -127,6 +133,11 @@ function Server(options) { this.dtrace = options.dtrace || false; this._inflightRequests = 0; + this.strictFormatters = true; + if (options.strictFormatters !== undefined) { + this.strictFormatters = options.strictFormatters; + } + var fmt = mergeFormatters(options.formatters); this.acceptable = fmt.acceptable; this.formatters = fmt.formatters; @@ -1138,6 +1149,7 @@ Server.prototype._setupRequest = function _setupRequest(req, res) { res.serverName = self.name; res._handlersFinished = false; res._flushed = false; + res._strictFormatters = this.strictFormatters; // set header only if name isn't empty string if (self.name !== '') { diff --git a/test/formatter-optional.test.js b/test/formatter-optional.test.js new file mode 100644 index 000000000..9ea729837 --- /dev/null +++ b/test/formatter-optional.test.js @@ -0,0 +1,75 @@ +'use strict'; +/* eslint-disable func-names */ + +var restifyClients = require('restify-clients'); + +var restify = require('../lib'); + +if (require.cache[__dirname + '/lib/helper.js']) { + delete require.cache[__dirname + '/lib/helper.js']; +} +var helper = require('./lib/helper.js'); + +///--- Globals + +var after = helper.after; +var before = helper.before; +var test = helper.test; + +var CLIENT; +var LOCALHOST; +var PORT = process.env.UNIT_TEST_PORT || 0; +var SERVER; + +///--- Tests + +before(function(callback) { + try { + SERVER = restify.createServer({ + handleUncaughtExceptions: true, + log: helper.getLog('server'), + strictFormatters: false + }); + SERVER.listen(PORT, '127.0.0.1', function() { + PORT = SERVER.address().port; + CLIENT = restifyClients.createJsonClient({ + url: 'http://127.0.0.1:' + PORT, + dtrace: helper.dtrace, + retry: false + }); + LOCALHOST = 'http://' + '127.0.0.1:' + PORT; + callback(); + }); + } catch (e) { + console.error(e.stack); + process.exit(1); + } +}); + +after(function(callback) { + try { + SERVER.close(callback); + CLIENT.close(); + } catch (e) { + console.error(e.stack); + process.exit(1); + } +}); + +test('send 200 on formatter missing and strictFormatters false', function(t) { + // When server is passed "strictFormatters: false" at creation time, + // res.send still sends a successful response even when a formatter is + // not set up for a specific content-type. + SERVER.get('/11', function handle(req, res, next) { + res.header('content-type', 'application/hal+json'); + res.send(200, JSON.stringify({ hello: 'world' })); + return next(); + }); + + CLIENT.get(LOCALHOST + '/11', function(err, _, res) { + t.ifError(err); + t.equal(res.statusCode, 200); + t.equal(res.headers['content-type'], 'application/hal+json'); + t.end(); + }); +});