Move Obj response stringify to _prepare, Closes hapijs#1190

docs/Reference.md

@@ -1345,13 +1345,16 @@ Each incoming request passes through a pre-defined set of steps, along with opti
 - Route prerequisites
 - Route handler
 - **`'onPostHandler'`** extension point
-    - the `request` object passed to the extension function is decorated with the `request.response()` method which returns the response object. The response object may be
-      modified. To return a different response (for example, replace an error with an HTML response), return the new response via `next(response)`.
+    - the `request` object passed to the extension function is decorated with the `request.response()` method which returns the response object.
+      The response object may be modified. To return a different response (for example, replace an error with an HTML response), return the new
+      response via `next(response)`.
 - Validate response payload
 - **`'onPreResponse'`** extension point
-    - always called
-    - the `request` object passed to the extension function is decorated with the `request.response()` method which returns the response object. The response object may be
-      modified. To return a different response (for example, replace an error with an HTML response), return the new response via `next(response)`.
+    - always called.
+    - the `request` object passed to the extension function is decorated with the `request.response()` method which returns the response object.
+      The response object cannot be modified. To return a different response (for example, replace an error with an HTML response), return the new
+      response via `next(response)`. Note that any errors generated after `next(response)` is called will not be passed back to the `'onPreResponse'`
+      extention method to prevent an infinite loop.
 - Send response (may emit `'internalError'` event)
 - Emits `'response'` event
 - Wait for tails
@@ -1714,7 +1717,7 @@ server.on('tail', function (request) {
 
 #### `request.setState(name, value, [options])`
 
-_Available until immediately after the `'onPreResponse'` extension point methods are called._
+_Available until immediately after the `'onPostHandler'` extension point methods are called._
 
 Sets a cookie which is sent with the response, where:
 
@@ -1729,7 +1732,7 @@ request.setState('preferences', { color: 'blue' }, { encoding: 'base64json' });
 
 #### `request.clearState(name)`
 
-_Available until immediately after the `'onPreResponse'` extension point methods are called._
+_Available until immediately after the `'onPostHandler'` extension point methods are called._
 
 Clears a cookie which sets an expired cookie and sent with the response, where:
 
@@ -1935,7 +1938,6 @@ server.ext('onPostHandler', function (request, next) {
     var response = request.response();
     if (response.variety === 'obj') {
         delete response.raw._id;        // Remove internal key
-        response.update();
     }
     next();
 });
@@ -2140,10 +2142,13 @@ var handler3 = function () {
 JavaScript object, sent stringified. The 'Content-Type' header defaults to 'application/json'. Supports all the methods provided by
 [`Generic`](#generic) as well as:
 
-- `raw` - the unmodified, unstringified object. Any direct manipulation must be followed with `update()`.
-- `update(options)` - updates the string representation of the object response after changes to `raw` where `options` includes:
-    - `type` - optional 'Content-Type' HTTP header value. Defaults to `'text/html'`.
-    - `encoding` - optional 'Content-Type' HTTP header encoding property. Defaults to `'utf-8'`.
+- `raw` - the unmodified, unstringified object. Can be directly manipulated before `onPreResponse` is called, afterwhich a new `Obj`
+  response must be created to modify the response payload.
+- `options(options)` - updates response configuration where `options` includes these optional keys:
+    - `type` - 'Content-Type' HTTP header value. Defaults to `'text/html'`.
+    - `encoding` - 'Content-Type' HTTP header encoding property. Defaults to `'utf-8'`.
+    - `replacer` - the `JSON.stringify()` replacer function or array. Defaults to no action.
+    - `space` - the `JSON.stringify()` number of spaces to indent nested object keys. Defaults to no indentation.
 
 ```javascript
 var Hapi = require('hapi');
@@ -2154,7 +2159,7 @@ server.ext('onPostHandler', function (request, next) {
     var response = request.response();
     if (response.variety === 'obj') {
         delete response.raw._id;        // Remove internal key
-        response.update();
+        response.options({ space: 4 });
     }
     next();
 });
@@ -2169,6 +2174,8 @@ Generated with:
     - `options` - an optional object with the following optional keys:
         - `type` - the 'Content-Type' HTTP header value. Defaults to `'text/html'`.
         - `encoding` - the 'Content-Type' HTTP header encoding property. Defaults to `'utf-8'`.
+        - `replacer` - the `JSON.stringify()` replacer function or array. Defaults to no action.
+        - `space` - the `JSON.stringify()` number of spaces to indent nested object keys. Defaults to no indentation.
 
 ```javascript
 var handler1 = function () {

lib/request.js

@@ -324,6 +324,9 @@ internals.Request.prototype._reply = function (exit) {
 
     clearTimeout(this._serverTimeoutId);
 
+    self.setState = undefined;
+    self.clearState = undefined;
+
     var process = function () {
 
         if (self._response &&
@@ -344,14 +347,17 @@ internals.Request.prototype._reply = function (exit) {
             self.server._ext.invoke(self, 'onPreResponse', function (err) {
 
                 self.response = undefined;
-                self.setState = undefined;
-                self.clearState = undefined;
 
-                if (err) {                                              // err can be valid response override
-                    override(err);
+                if (!err) {                                             // err can be valid response override
+                    return Response._respond(self._response, self, finalize);
                 }
 
-                Response._respond(self._response, self, finalize);
+                override(err);
+                Response._prepare(self._response, self, function (result) {
+
+                    self._response = result;
+                    Response._respond(self._response, self, finalize);
+                });
             });
         });
     };

lib/response/index.js

@@ -83,7 +83,7 @@ exports._generate = function (result, request, onSend) {
     }
 
     if (!response) {
-        response = new internals.Obj(result, request.server.settings.json);
+        response = new internals.Obj(result);
     }
 
     if (response.isHapiResponse) {
@@ -170,15 +170,15 @@ exports._respond = function (item, request, callback) {
         if (!response.isBoom) {
             return etag(response);
         }
-        
-        response = new internals.Error(response, request.server.settings.json);
+
+        response = new internals.Error(response);
         response._prepare(request, function (result) {
 
-            if (result.isBoom) {
-                return send(new internals.Error(result, request.server.settings.json));         // Do not prepare again to avoid error loop
+            if (!result.isBoom) {
+                return send(result);
             }
 
-            return send(result);
+            return send(response);      // Return the original error (which is partially prepared) instead of having to prepare the result error
         });
     };
 

lib/response/obj.js

@@ -16,20 +16,46 @@ exports = module.exports = internals.Obj = function (object, options) {
     this.variety = 'obj';
     this.varieties.obj = true;
 
-    // Convert immediately to snapshot content
-
+    this._stringify = {};                           // JSON.stringify options
     this.raw = object;                              // Can change if reference is modified
-    this.update(options);
+    this.options(options);
 };
 
 Utils.inherits(internals.Obj, Cacheable);
 
 
-internals.Obj.prototype.update = function (options) {
+internals.Obj.prototype.options = function (options) {
 
     options = options || {};
 
-    this._payload = [JSON.stringify(this.raw, options.replacer, options.space)];
+    if (options.hasOwnProperty('replacer')) {
+        this._stringify.replacer = options.replacer;
+    }
+
+    if (options.hasOwnProperty('space')) {
+        this._stringify.space = options.space;
+    }
+
     this._headers['content-type'] = options.type || this._headers['content-type'] || 'application/json';
     this.encoding(options.encoding);
 };
+
+
+internals.Obj.prototype._prepare = function (request, callback) {
+
+    var space = this._stringify.space || request.server.settings.json.space;
+    var replacer = this._stringify.replacer || request.server.settings.json.replacer;
+    this._payload = [JSON.stringify(this.raw, replacer, space)];
+    return Cacheable.prototype._prepare.call(this, request, callback);
+};
+
+
+internals.Obj.prototype.toCache = function () {
+
+    return {
+        code: this._code,
+        payload: JSON.stringify(this.raw, this._stringify.replacer, this._stringify.space),
+        headers: this._headers,
+        flags: this._flags
+    };
+};

test/integration/response.js

@@ -328,7 +328,7 @@ describe('Response', function () {
 
             var handler = function (request) {
 
-                request.reply(new Hapi.response.Obj({ a: 1, b: 2 }, { type: 'application/x-test' }));
+                request.reply(new Hapi.response.Obj({ a: 1, b: 2 }, { type: 'application/x-test', space: 0, replacer: null }));
             };
 
             var server = new Hapi.Server();
@@ -1689,24 +1689,26 @@ describe('Response', function () {
 
         it('returns a cached reply', function (done) {
 
+            var gen = 0;
             var cacheHandler = function (request) {
 
-                request.reply({ status: 'cached' });
+                request.reply({ status: 'cached', gen: gen++ });
             };
 
             var server = new Hapi.Server(0);
-            server.route({ method: 'GET', path: '/cache', config: { handler: cacheHandler, cache: { mode: 'server+client', expiresIn: 5000 } } });
+            server.route({ method: 'GET', path: '/cache', config: { handler: cacheHandler, cache: { mode: 'server', expiresIn: 5000 } } });
 
             server.start(function () {
 
                 server.inject('/cache', function (res1) {
 
                     expect(res1.result).to.exist;
                     expect(res1.result.status).to.equal('cached');
+                    expect(res1.result.gen).to.equal(0);
 
                     server.inject('/cache', function (res2) {
 
-                        expect(res2.result).to.deep.equal('{"status":"cached"}');
+                        expect(res2.payload).to.deep.equal('{"status":"cached","gen":0}');
                         done();
                     });
                 });
@@ -2348,7 +2350,7 @@ describe('Response', function () {
 
     describe('Extension', function () {
 
-        it('returns an internal error on error response loop', function (done) {
+        it('returns last known error on error response loop', function (done) {
 
             var handler = function () {
 
@@ -2372,7 +2374,7 @@ describe('Response', function () {
 
             server.inject('/', function (res) {
 
-                expect(res.result.code).to.equal(500);
+                expect(res.result.code).to.equal(400);
                 done();
             });
         });