diff --git a/4TO5GUIDE.md b/4TO5GUIDE.md index 3bdee6d8d..fcb3a3f43 100644 --- a/4TO5GUIDE.md +++ b/4TO5GUIDE.md @@ -206,60 +206,6 @@ Connection: keep-alive {"code":"Gone","message":"gone girl"} ``` -## sync vs async formatters - -Restify now supports both sync and async formatters. In 4.x, all formatters had -an async signature despite not being async. For example, the text formatter in -4.x might have looked like this: - -```js -function formatText(req, res, body, cb) { - return cb(null, body.toString()); -} -``` - -This caused a scenario where formatting could potentially fail, but the handler -chain would continue on. To address this gap, as of 5.x, any formatters that -are async require a callback to be passed into `res.send()`. For example, -imagine this async formatter: - -```js -function specialFormat(req, res, body, cb) { - return asyncSerializer.format(body, cb); -} - -server.get('/', function(req, res, next) { - res.send('hello world', function(err) { - if (err) { - res.end('some other backup content when formatting fails'); - } - return next(); - }); -}); - -server.get('/', function(req, res, next) { - // alternatively, you can pass the error to next, which will render the - // error to the client. - res.send('hello world', next); -}); -``` - -This way we are able to block the handler chain from moving on when an async -formatter fails. If you have any custom formatters, migrating from 4.x will -require you to change the formatter to be sync. Imagine the previous text -formatter changed to sync. Notice that the signature no longer takes a -callback. This hints to restify that the formatter is sync: - -```js -function formatText(req, res, body) { - return body.toString(); -} -``` - -Thus, if your formatter takes 4 parameters (i.e., takes a callback), -invocations of `res.send()` must take a callback, or else restify will throw. - - ## Deprecations The following are still currently supported, but are on life support and may be diff --git a/docs/pages/components/response.md b/docs/pages/components/response.md index a21a433e1..362ac8968 100644 --- a/docs/pages/components/response.md +++ b/docs/pages/components/response.md @@ -128,7 +128,6 @@ res.status(201); * `code` {Number} an http status code * `body` {String | Object | Array | Buffer} the payload to send * `[headers]` {Object} an optional object of headers (key/val pairs) -* `[callback]` {Function} an optional callback, used in conjunction with async formatters You can use `send()` to wrap up all the usual `writeHead()`, `write()`, `end()` diff --git a/docs/pages/guides/server.md b/docs/pages/guides/server.md index bced7d76a..56b744fe8 100644 --- a/docs/pages/guides/server.md +++ b/docs/pages/guides/server.md @@ -484,7 +484,7 @@ want: ```js restify.createServer({ formatters: { - 'application/foo; q=0.9': function formatFoo(req, res, body, cb) { + 'application/foo; q=0.9': function formatFoo(req, res, body) { if (body instanceof Error) return body.stack; @@ -511,53 +511,6 @@ res.write(body); res.end(); ``` -Formatters can also be async, in which case a callback is available to you as -the fourth parameter. If you choose to use an async formatter for a particular -content-type, it is required to pass a callback parameter to `res.send()`. If a -callback is not provided, then restify will throw an error: - -```js -var server = restify.createServer({ - formatters: { - 'application/foo-async': function formatFoo(req, res, body, cb) { - - someAsyncMethod(body, function(err, formattedBody) { - if (err) { - return cb(err); - } - return cb(null, formattedBody); - }); - } - } -}); - -server.get('/fooAsync', function(req, res, next) { - res.send(fooData, next); // => callback required! -}); -``` - -In the event of an error with your async formatter, ensure that the error is -passed to the callback. The default behavior in this scenario is for restify to -send an empty 500 response to the client, since restify can make no assumptions -about the correct format for your data. Alternatively, you can listen to a -`FormatterError` event on the server. This event is fired whenever an async -formatter returns an error, allowing you to write a custom response if -necessary. response if necessary. If you listen to this event, you _must_ -flush a response, or else the request will hang. It is highly recommended to -avoid using `res.send()` within this handler to avoid any issues that might -have caused the async formatter to fail to begin with. Instead, use the native -node response methods: - - -```js -server.on('FormatterError', function(req, res, route, err) { - // the original body that caused formatting failure is available provided - // on the error object - err.context.rawBody - res.end('boom! could not format body!'); -}); -``` - - ## Error handling It is common to want to handle an error conditions the same way. As an example, diff --git a/lib/response.js b/lib/response.js index bde681530..4dab12071 100644 --- a/lib/response.js +++ b/lib/response.js @@ -96,66 +96,6 @@ Response.prototype.charSet = function charSet(type) { }; -/** - * finds a formatter that is both acceptable and works for the content-type - * specified on the response. Can return two errors: - * NotAcceptableError - couldn't find a suitable formatter - * InternalServerError - couldn't find a fallback formatter - * @public - * @function _findFormatter - * @param {Function} callback a callback fn - * @returns {undefined} - */ -Response.prototype._findFormatter = function _findFormatter(callback) { - - var formatter; - var type = this.contentType || this.getHeader('Content-Type'); - - if (!type) { - if (this.req.accepts(this.acceptable)) { - type = this.req.accepts(this.acceptable); - } - - if (!type) { - return callback(new errors.NotAcceptableError({ - message: 'could not find suitable formatter' - })); - } - } else if (type.indexOf(';') !== '-1') { - type = type.split(';')[0]; - } - - if (!(formatter = this.formatters[type])) { - - if (type.indexOf('/') === -1) { - type = mime.lookup(type); - } - - if (this.acceptable.indexOf(type) === -1) { - type = 'application/octet-stream'; - } - - formatter = this.formatters[type] || this.formatters['*/*']; - - // this is a catastrophic case - should always fall back on - // octet-stream but if for some reason that's gone, return a 500. - if (!formatter) { - return callback(new errors.InternalServerError({ - message: 'could not find formatter for application/octet-stream' - })); - } - } - - if (this._charSet) { - type = type + '; charset=' + this._charSet; - } - - this.setHeader('Content-Type', type); - - return callback(null, formatter, type); -}; - - /** * retrieves a header off the response. * @public @@ -266,16 +206,16 @@ Response.prototype.link = function link(l, rel) { * formatter based on the content-type header. * @public * @function send - * @param {Number} code http status code - * @param {Object | Buffer | Error} body the content to send - * @param {Object} headers any add'l headers to set - * @param {Function} callback a callback for use with async formatters - * @returns {Object | undefined} when async formatter in use, returns null, - * otherwise returns the response object + * @param {Number} [code] http status code + * @param {Object | Buffer | Error} [body] the content to send + * @param {Object} [headers] any add'l headers to set + * @returns {Object} the response object */ -Response.prototype.send = function send(code, body, headers, callback) { +Response.prototype.send = function send(code, body, headers) { var self = this; - return self.__send(code, body, headers, callback, true); + var args = Array.prototype.slice.call(arguments); + args.push(true); // Append format = true to __send invocation + return self.__send.apply(self, args); }; @@ -284,114 +224,134 @@ Response.prototype.send = function send(code, body, headers, callback) { * formatters entirely and sends the content as is. * @public * @function sendRaw - * @param {Number} code http status code - * @param {Object | Buffer | Error} body the content to send - * @param {Object} headers any add'l headers to set - * @param {Function} callback a callback for use with async formatters - * @returns {Object | undefined} when async formatter in use, returns null, - * otherwise returns the response object + * @param {Number} [code] http status code + * @param {Object | Buffer | Error} [body] the content to send + * @param {Object} [headers] any add'l headers to set + * @returns {Object} the response object */ -Response.prototype.sendRaw = function sendRaw(code, body, headers, callback) { +Response.prototype.sendRaw = function sendRaw(code, body, headers) { var self = this; - return self.__send(code, body, headers, callback, false); + var args = Array.prototype.slice.call(arguments); + args.push(false); // Append format = false to __send invocation + return self.__send.apply(self, args); }; /** * internal implementation of send. convenience method that handles: * writeHead(), write(), end(). + * + * Both body and headers are optional, but you MUST provide body if you are + * providing headers. + * * @private * @private - * @param {Number} [maybeCode] http status code - * @param {Object | Buffer | Error} [maybeBody] the content to send - * @param {Object} [maybeHeaders] any add'l headers to set - * @param {Function} [maybeCallback] optional callback for async formatters - * @param {Function} format when false, skip formatting + * @param {Number} [code] http status code + * @param {Object | Buffer | String | Error} [body] the content to send + * @param {Object} [headers] any add'l headers to set + * @param {Boolean} [format] When false, skip formatting * @returns {Object} returns the response object */ -Response.prototype.__send = -function __send(maybeCode, maybeBody, maybeHeaders, maybeCallback, format) { - +Response.prototype.__send = function __send() { var self = this; var isHead = (self.req.method === 'HEAD'); var log = self.log; - var code; - var body; - var headers; - var callback; - - // normalize variadic args. - if (typeof maybeCode === 'number') { - // if status code was passed in, then signature should look like jsdoc - // signature and we only need to figure out headers/callback variation. - code = maybeCode; - - // signature should look like jsdoc signature - body = maybeBody; - - if (typeof maybeHeaders === 'function') { - callback = maybeHeaders; - } else { - callback = maybeCallback; - headers = maybeHeaders; - } - } else { - // if status code was omitted, then first arg must be body. - body = maybeCode; + var code, body, headers, format; - // now figure out headers/callback variation - if (typeof maybeBody === 'function') { - callback = maybeBody; - } else { - callback = maybeHeaders; - headers = maybeBody; - } + // derive arguments from types, one by one + var index = 0; + // Check to see if the first argument is a status code + if (typeof arguments[index] === 'number') { + code = arguments[index++]; + } + + // Check to see if the next argument is a body + if (typeof arguments[index] === 'object' || + typeof arguments[index] === 'string') { + body = arguments[index++]; } - // if an error object is being sent and a status code isn't explicitly set, - // infer one from the error object itself. + // Check to see if the next argument is a collection of headers + if (typeof arguments[index] === 'object') { + headers = arguments[index++]; + } + + // Check to see if the next argument is the format boolean + if (typeof arguments[index] === 'boolean') { + format = arguments[index++]; + } + + // Ensure the function was provided with arguments of the proper types, + // if we reach this line and there are still arguments, either one of the + // optional arguments was of an invalid type or we were provided with + // too many arguments + assert(arguments[index] === undefined, + 'Unknown argument: ' + arguments[index] + '\nProvided: ' + arguments); + + // Now lets try to derive values for optional arguments that we were not + // provided, otherwise we choose sane defaults. + + // 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) { - self.statusCode = body.statusCode || 500; - } else { - self.statusCode = code || 200; + code = body.statusCode || 500; } + // Set sane defaults for optional arguments if they were not provided and + // we failed to derive their values + code = code || 200; headers = headers || {}; + // Populate our response object with the derived arguments + self.statusCode = code; + self._body = body; + Object.keys(headers).forEach(function (k) { + self.setHeader(k, headers[k]); + }); + + // If log level is set to trace, output our constructed response object if (log.trace()) { var _props = { code: self.statusCode, - headers: headers + headers: self._headers }; if (body instanceof Error) { - _props.err = body; + _props.err = self._body; } else { - _props.body = body; + _props.body = self._body; } log.trace(_props, 'response::send entered'); } - self._body = body; - + // Flush takes our constructed response object and sends it to the client function _flush(formattedBody) { self._data = formattedBody; - Object.keys(headers).forEach(function (k) { - self.setHeader(k, headers[k]); - }); - + // Flush headers self.writeHead(self.statusCode); - if (self._data && !(isHead || code === 204 || code === 304)) { + // Send body if it was provided + if (self._data) { self.write(self._data); } + // Finish request self.end(); + // If log level is set to trace, log the entire response object if (log.trace()) { log.trace({res: self}, 'response sent'); } + + // Return the response object back out to the caller of __send + return self; + } + + // 204 = No Content and 304 = Not Modified, we don't want to send the + // body in these cases. HEAD never provides a body. + if (isHead || code === 204 || code === 304) { + return _flush(); } // if no formatting, assert that the value to be written is a string @@ -404,68 +364,77 @@ function __send(maybeCode, maybeBody, maybeHeaders, maybeCallback, format) { // 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 === null || - body === undefined || - (body instanceof Error && body.domain)) { + if (body === undefined || (body instanceof Error && body.domain)) { return _flush(); } - // otherwise, try to find a formatter - self._findFormatter( - function foundFormatter(findErr, formatter, contentType) { - - // handle missing formatters - if (findErr) { - // if user set a status code outside of the 2xx range, it probably - // outweighs being unable to format the response. set a status code - // then flush empty response. - if (self.statusCode >= 200 && self.statusCode < 300) { - self.statusCode = findErr.statusCode; - } - log.warn({ - req: self.req, - err: findErr - }, 'error retrieving formatter'); - return _flush(); + // 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 + + // _formatterError is used to handle any case where we were unable to + // properly format the provided body + function _formatterError(err) { + // If the user provided a non-success error code, we don't want to mess + // with it since their error is probably more important than our + // inability to format their message. + if (self.statusCode >= 200 && self.statusCode < 300) { + self.statusCode = err.statusCode; } - // if we have formatter, happy path. - var asyncFormat = (formatter && formatter.length === 4) ? true : false; + log.warn({ + req: self.req, + err: err + }, 'error retrieving formatter'); - if (asyncFormat === true) { + return _flush(); + } - assert.func(callback, 'async formatter for ' + contentType + - ' requires callback to res.send()'); + var formatter; + var type = self.contentType || self.getHeader('Content-Type'); + + // Check to see if we can find a valid formatter + if (!type && !self.req.accepts(self.acceptable)) { + return _formatterError(new errors.NotAcceptableError({ + message: 'could not find suitable formatter' + })); + } - // if async formatter error, propagate error back up to - // res.send() caller, most likely a handler. - return formatter.call(self, self.req, self, body, - function _formatDone(formatErr, formattedBody) { + // Derive type if not provided by the user + if (!type) { + type = self.req.accepts(self.acceptable); + } - if (formatErr) { + type = type.split(';')[0]; - return callback(new errors.FormatterError(formatErr, { - message: 'unable to format response for ' + - self.header('content-type'), - context: { - rawBody: body - } - })); - } - return _flush(formattedBody); - }); - } - // for sync formatters, invoke formatter and send response body. - else { - _flush(formatter.call(self, self.req, self, body), body); - // users can always opt to pass in next, even when formatter is not - // async. invoke callback in that case. return null when no - // callback because eslint wants consistent-return. - return (callback) ? callback() : null; - } - }); + if (!self.formatters[type] && type.indexOf('/') === -1) { + type = mime.lookup(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'; + } + + 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(new errors.InternalServerError({ + message: 'could not find formatter for application/octet-stream' + })); + } + + if (self._charSet) { + type = type + '; charset=' + self._charSet; + } + + // Update header to the derived content type for our formatter + self.setHeader('Content-Type', type); - return self; + // Finally, invoke the formatter and flush the request with it's results + return _flush(formatter(self.req, self, body)); }; diff --git a/test/formatter.test.js b/test/formatter.test.js index 174da541e..d6304bd85 100644 --- a/test/formatter.test.js +++ b/test/formatter.test.js @@ -37,16 +37,6 @@ before(function (callback) { // this is a bad formatter, on purpose. return x.toString(); // eslint-disable-line no-undef }, - 'text/async': function (req, res, body, cb) { - return process.nextTick(function () { - cb(null, 'async fmt'); - }); - }, - 'text/asyncerror': function (req, res, body, cb) { - return process.nextTick(function () { - cb(new Error('foobar')); - }); - }, 'application/foo; q=0.9': function (req, res, body) { return 'foo!'; }, @@ -70,17 +60,6 @@ before(function (callback) { res.send('dummy response'); return next(); }); - SERVER.get('/async', function (req, res, next) { - res.send('dummy response', next); - }); - SERVER.get('/asyncHandle', function (req, res, next) { - res.send('dummy resposne', function (err) { - res.writeHead(201); - res.write('panda'); - res.end(); - return next(); - }); - }); SERVER.get('/missingFormatter', function (req, res, next) { delete res.formatters['application/octet-stream']; res.setHeader('content-type', 'text/html'); @@ -151,126 +130,6 @@ test('GH-845: sync formatter should blow up', function (t) { }); -test('GH-845: async formatter', function (t) { - - CLIENT.get({ - path: '/async', - headers: { - accept: 'text/async' - } - }, function (err, req, res, data) { - t.ifError(err); - t.ok(req); - t.ok(res); - t.equal(data, 'async fmt'); - t.end(); - }); -}); - - -test('GH-845: async formatter error should invoke res.send() callback with err', -function (t) { - - CLIENT.get({ - path: '/asyncHandle', - headers: { - accept: 'text/asyncerror' - } - }, function (err, req, res, data) { - t.ifError(err); - t.ok(req); - t.ok(res); - t.equal(res.statusCode, 201); - t.equal(data, 'panda'); - t.end(); - }); - -}); - - -test('GH-845: async formatter error should send empty string if passed to next', -function (t) { - - CLIENT.get({ - path: '/async', - headers: { - accept: 'text/asyncerror' - } - }, function (err, req, res, data) { - t.ok(err); - t.equal(res.statusCode, 500); - t.equal(data, ''); - t.end(); - }); -}); - - -test('GH-845: async formatter error should emit FormatterError', function (t) { - - SERVER.on('FormatterError', function (req, res, route, err) { - t.ok(err); - t.equal(err.code, 'Formatter'); - res.end('custom formatter error'); - }); - - CLIENT.get({ - path: '/async', - headers: { - accept: 'text/asyncerror' - } - }, function (err, req, res, data) { - t.ok(err); - t.equal(res.statusCode, 500); - t.equal(data, 'custom formatter error'); - t.end(); - }); -}); - - -test('GH-1129: sync formatter should invoke res.send callback', function (t) { - - SERVER.on('after', function () { - // only end the test when server considers request complete - t.end(); - }); - - CLIENT.get({ - path: '/async', - headers: { - accept: 'text/sync' - } - }, function (err, req, res, data) { - t.ifError(err); - t.equal(res.statusCode, 200); - t.equal(data, 'sync fmt'); - }); -}); - - -test('GH-845: should blow up when using async formatter ' + - 'without res.send callback', function (t) { - - SERVER.on('uncaughtException', function (req, res, route, err) { - t.ok(err); - t.equal(err.name, 'AssertionError'); - t.equal(err.message, 'async formatter for text/async requires ' + - 'callback to res.send() (func) is required'); - res.write('uncaughtException'); - res.end(); - }); - - CLIENT.get({ - path: '/sync', - headers: { - accept: 'text/async' - } - }, function (err, req, res, data) { - t.equal(data, 'uncaughtException'); - t.end(); - }); -}); - - test('q-val priority', function (t) { var opts = { path: '/sync',