refactor to make changes non-breaking

Author: Julien Gilli

docs/guides/server.md

@@ -425,17 +425,31 @@ server.get('/websocket/attach', function upgradeRoute(req, res, next) {
 });
 ```
 
-## Content Negotiation
+## Responses' Content Negotiation And Formatting
 
 If you're using `res.send()` restify will determine the content-type to respond
-with by:
+with by, from highest priority to lowest priority:
 
-1. negotiating the content-type by matching available formatters with the
-   request's `accept` header
+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, using the value of `res.contentType` if present
-1. otherwise, using the value of the `Content-Type` response header if set
+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 `optionalFormatters: true`
+(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
@@ -458,12 +472,6 @@ var server = restify.createServer({
 });
 ```
 
-If a content-type can't be determined, then restify will respond with an error.
-
-If a content-type can be negotiated, but that content-type doesn't have a
-corresponding formatter, restify will not format the response with any
-formatter.
-
 For example, attempting to send a content-type that does not have a defined
 formatter:
 
@@ -475,8 +483,27 @@ server.get('/foo', function(req, res, next) {
 });
 ```
 
-Will result in a response with a content-type of `text/css` even though no
-`'text/css'` formatter is present:
+Will result in a response with a content-type of `application/octet-stream`:
+
+```sh
+$ curl -i localhost:3000/
+HTTP/1.1 200 OK
+Content-Type: application/octet-stream
+Content-Length: 2
+Date: Thu, 02 Jun 2016 06:50:54 GMT
+Connection: keep-alive
+```
+
+However, if the server instance is created with `optionalFormatters: true`:
+
+```js
+var server = restify.createServer({
+  optionalFormatters: true
+});
+```
+
+The response has a content-type of `text/css` even though no `'text/css'`
+formatter is present:
 
 ```sh
 $ curl -i localhost:3000/

docs/index.md

@@ -435,17 +435,31 @@ server.get('/websocket/attach', function upgradeRoute(req, res, next) {
 });
 ```
 
-## Content Negotiation
+## Responses' Content Negotiation And Formatting
 
 If you're using `res.send()` restify will determine the content-type to respond
-with by:
+with by, from highest priority to lowest priority:
 
-1. negotiating the content-type by matching available formatters with the
-   request's `accept` header
+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, using the value of `res.contentType` if present
-1. otherwise, using the value of the `Content-Type` response header if set
+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 `optionalFormatters: true`
+(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
@@ -468,12 +482,6 @@ var server = restify.createServer({
 });
 ```
 
-If a content-type can't be determined, then restify will respond with an error.
-
-If a content-type can be negotiated, but that content-type doesn't have a
-corresponding formatter, restify will not format the response with any
-formatter.
-
 For example, attempting to send a content-type that does not have a defined
 formatter:
 
@@ -485,8 +493,27 @@ server.get('/foo', function(req, res, next) {
 });
 ```
 
-Will result in a response with a content-type of `text/css` even though no
-`'text/css'` formatter is present:
+Will result in a response with a content-type of `application/octet-stream`:
+
+```sh
+$ curl -i localhost:3000/
+HTTP/1.1 200 OK
+Content-Type: application/octet-stream
+Content-Length: 2
+Date: Thu, 02 Jun 2016 06:50:54 GMT
+Connection: keep-alive
+```
+
+However, if the server instance is created with `optionalFormatters:true`:
+
+```js
+var server = restify.createServer({
+  optionalFormatters: true
+});
+```
+
+The response would has a content-type of `text/css` even though no `'text/css'`
+formatter is present:
 
 ```sh
 $ curl -i localhost:3000/
@@ -519,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.

lib/index.js

@@ -59,6 +59,8 @@ require('./errorTypes');
  *  `res.writeContinue()` in `server.on('checkContinue')` when proxing
  * @param {Boolean} [options.ignoreTrailingSlash=false] - ignore trailing slash
  * on paths
+ * @param {Boolean} [options.optionalFormatters=false] - makes finding
+ * formatters matching responses' content-type optional when sending a response
  * @example
  * var restify = require('restify');
  * var server = restify.createServer();

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)
@@ -464,8 +466,33 @@ function patch(Response) {
                 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 (
+                !this._optionalFormatters &&
+                !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 (!this._optionalFormatters && !formatter) {
+                return formatterError(
+                    self,
+                    new errors.InternalServerError({
+                        message:
+                            'could not find formatter for response ' +
+                            'content-type "' +
+                            type +
+                            '"'
+                    })
+                );
+            }
+
             if (self._charSet) {
                 type = type + '; charset=' + self._charSet;
             }
@@ -477,6 +504,7 @@ function patch(Response) {
             if (formatter) {
                 // Finally, invoke the formatter and flush the request with it's
                 // results
+
                 return flush(self, formatter(self.req, self, body));
             }
         }

lib/server.js

@@ -26,11 +26,11 @@ var deprecationWarnings = require('./deprecationWarnings');
 var patchRequest = require('./request');
 var patchResponse = require('./response');
 
-var http2;
-
 patchResponse(http.ServerResponse);
 patchRequest(http.IncomingMessage);
 
+var http2;
+
 ///--- Globals
 
 var sprintf = util.format;
@@ -82,6 +82,8 @@ 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.optionalFormatters=false] - makes finding
+ * formatters matching responses' content-type optional when sending a response
  * @example
  * var restify = require('restify');
  * var server = restify.createServer();
@@ -103,6 +105,10 @@ function Server(options) {
     assert.optionalBool(options.socketio, 'options.socketio');
     assert.optionalBool(options.onceNext, 'options.onceNext');
     assert.optionalBool(options.strictNext, 'options.strictNext');
+    assert.optionalBool(
+        options.optionalFormatters,
+        'options.optionalFormatters'
+    );
 
     var self = this;
 
@@ -126,6 +132,7 @@ function Server(options) {
     this.socketio = options.socketio || false;
     this.dtrace = options.dtrace || false;
     this._inflightRequests = 0;
+    this.optionalFormatters = options.optionalFormatters || false;
 
     var fmt = mergeFormatters(options.formatters);
     this.acceptable = fmt.acceptable;
@@ -1138,6 +1145,7 @@ Server.prototype._setupRequest = function _setupRequest(req, res) {
     res.serverName = self.name;
     res._handlersFinished = false;
     res._flushed = false;
+    res._optionalFormatters = this.optionalFormatters;
 
     // set header only if name isn't empty string
     if (self.name !== '') {

test/formatter-optional.test.js

@@ -0,0 +1,77 @@
+'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'),
+            optionalFormatters: true
+        });
+        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('should send 200 on formatter missing but optional', function(t) {
+    // When server is passed "optionalFormatters: true" 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) {
+        console.log('got request');
+        res.header('content-type', 'application/hal+json');
+        res.send(200, JSON.stringify({ hello: 'world' }));
+        return next();
+    });
+
+    CLIENT.get(LOCALHOST + '/11', function(err, _, res) {
+        console.log('got response:', err);
+        t.ifError(err);
+        t.equal(res.statusCode, 200);
+        t.equal(res.headers['content-type'], 'application/hal+json');
+        t.end();
+    });
+});

test/formatter.test.js

@@ -189,8 +189,8 @@ test(
 );
 
 test(
-    'GH-937 should return 200 even when no formatter matching client found ' +
-        'but content-type set manually',
+    'GH-937 should return 500 when no default formatter found ' +
+        'and octet-stream is not available',
     function(t) {
         // ensure client accepts only a type not specified by server
         var opts = {
@@ -201,11 +201,10 @@ test(
         };
 
         CLIENT.get(opts, function(err, req, res, data) {
-            t.ifError(err);
+            t.ok(err);
             t.ok(req);
             t.ok(res);
-            t.equal(res.statusCode, 200);
-            t.equal(res.contentType(), 'text/html');
+            t.equal(res.statusCode, 500);
             t.end();
         });
     }