Merge pull request #478 from walmartlabs/user/eran

Cleanup ext options and error handling

docs/Reference.md

@@ -144,8 +144,6 @@ The extension points are:
 - `onRequest` - called upon new requests before any router processing. The _'request'_ object passed to the `onRequest` functions is decorated with the _'setUrl(url)'_ and _'setMethod(verb)'_ methods. Calls to these methods will impact how the request is router and can be used for rewrite rules.
 - `onPreHandler` - called after request passes validation and body parsing, before the request handler.
 - `onPostHandler` - called after the request handler, before sending the response. The actual state of the response depends on the response type used (e.g. direct, stream).
-- `onPostRoute` - called after the response was sent.
-- `onUnknownRoute` - if defined, overrides the default unknown resource (404) error response. The method must send the response manually via _request.raw.res_. Cannot be an array.
 
 For example:
 ```javascript

examples/extensions.js

@@ -42,21 +42,13 @@ internals.onPostHandler = function (request, next) {
 };
 
 
-internals.onPostRoute = function (request, next) {
-
-    Hapi.Log.event('onPostRoute');
-    next();
-};
-
-
 internals.main = function () {
 
     var http = new Hapi.Server(8080);
 
     http.ext('onRequest', internals.onRequest);                                         // New request, before handing over to the router (allows changes to method and url)
     http.ext('onPreHandler', [internals.onPreHandler1, internals.onPreHandler2]);       // After validation and body parsing, before route handler
     http.ext('onPostHandler', internals.onPostHandler);                                 // After route handler returns, before setting response
-    http.ext('onPostRoute', internals.onPostRoute);                                     // After response sent
 
     http.route({ method: 'GET', path: '/', handler: internals.get });
     http.start();

lib/request.js

@@ -43,7 +43,6 @@ exports = module.exports = internals.Request = function (server, req, res, optio
     this._setUrl(req.url);              // Sets: url, path, query
     this._setMethod(req.method);        // Sets: method
     this.id = now + '-' + process.pid + '-' + Math.floor(Math.random() * 0x10000);
-    this._timestamp = now;
 
     // Defined elsewhere:
 
@@ -72,6 +71,8 @@ exports = module.exports = internals.Request = function (server, req, res, optio
 
     // Private members
 
+    this._timestamp = now;
+    this._isReplied = false;
     this._route = null;
     this._response = null;
     this._log = [];
@@ -203,21 +204,53 @@ internals.Request.prototype.getLog = function (tags) {
 };
 
 
-internals.Request.prototype._onRequestExt = function (func, callback) {
+internals.Request.prototype._ext = function (event, callback) {
+
+    var self = this;
+
+    var handlers = this.server._ext[event];         // onRequest, onPreHandler, onPostHandler
+    if (!handlers) {
+        return callback();
+    }
+
+    Async.forEachSeries(handlers, function (func, next) {
+
+        func(self, next);
+    },
+    function (err) {
+
+        return callback(err);
+    });
+};
+
+
+internals.Request.prototype._onRequestExt = function (callback) {
+
+    var self = this;
 
     // Decorate request
 
     this.setUrl = this._setUrl;
     this.setMethod = this._setMethod;
 
-    Utils.executeRequestHandlers(func, this, function () {
+    this._ext('onRequest', function (err) {
 
         // Undecorate request
 
-        delete this.setUrl;
-        delete this.setMethod;
+        delete self.setUrl;
+        delete self.setMethod;
+
+        if (!err) {
+            return callback();
+        }
 
-        callback();
+        // Send error response
+
+        self._route = self.server._router.notFound;             // Only settings are used, not the handler
+        self._reply(err, function () {
+
+            return callback(err);
+        });
     });
 };
 
@@ -227,120 +260,111 @@ internals.Request.prototype._execute = function (route) {
     var self = this;
 
     this._route = route;
-    var isReplied = false;
 
-    var execute = function () {
+    var serverTimeout = self.server.settings.timeout.server;
+    if (serverTimeout) {
+        serverTimeout -= (Date.now() - self._timestamp);        // Calculate the timeout from when the request was constructed
+        var timeoutReply = function () {
 
-        var serverTimeout = self.server.settings.timeout.server;
-        if (serverTimeout) {
-            serverTimeout -= (Date.now() - self._timestamp);        // Calculate the timeout from when the request was constructed
-            var timeoutReply = function () {
-
-                reply(Err.serverTimeout('Server is taking too long to respond'));
-            };
-
-            if (serverTimeout <= 0) {
-                return timeoutReply();
-            }
+            self._reply(Err.serverTimeout());
+        };
 
-            self._serverTimeoutId = setTimeout(timeoutReply, serverTimeout);
+        if (serverTimeout <= 0) {
+            return timeoutReply();
         }
 
-        // Handler wrappers
-
-        var ext = function (func) {
-
-            return function (request, next) {
+        self._serverTimeoutId = setTimeout(timeoutReply, serverTimeout);
+    }
 
-                Utils.executeRequestHandlers(func, request, next);
-            };
-        };
+    var ext = function (event) { return function (request, next) { self._ext(event, next); }; };            // Handler wrappers
+
+    var funcs = [
+        // 'onRequest' in Server
+        internals.authenticate,
+        internals.processDebug,
+        Validation.query,
+        State.parseCookies,
+        Payload.read,
+        Validation.payload,
+        Validation.path,
+        ext('onPreHandler'),
+        internals.handler,                                      // Must not call next() with an Error
+        ext('onPostHandler'),                                   // An error from here on will override any result set in handler()
+        Validation.response
+    ];
+
+    Async.forEachSeries(funcs, function (func, next) {
+
+        func(self, next);
+    }, 
+    function (err) {
+
+        self._reply(err);
+    });
+};
 
-        var funcs = [
-            // self.server._ext.onRequest() in Server
-            internals.authenticate,
-            internals.processDebug,
-            Validation.query,
-            State.parseCookies,
-            Payload.read,
-            Validation.payload,
-            Validation.path,
-            ext(self.server._ext.onPreHandler),
-            internals.handler,                                      // Must not call next() with an Error
-            ext(self.server._ext.onPostHandler),                    // An error from here on will override any result set in handler()
-            Validation.response
-        ];
-
-        var funcsIterator = function (func, next) {
-
-            func(self, next);
-        };
 
-        Async.forEachSeries(funcs, funcsIterator, reply);
-    };
+internals.Request.prototype._reply = function (err, callback) {
 
-    var reply = function (err) {
+    var self = this;
 
-        if (isReplied === true) {                                   // Prevent any future responses to this request
-            return;
-        }
+    callback = callback || function () { };
 
-        isReplied = true;
-        clearTimeout(self._serverTimeoutId);
+    if (this._isReplied) {                                      // Prevent any future responses to this request
+        return callback();
+    }
 
-        // Error exit
+    this._isReplied = true;
+    clearTimeout(self._serverTimeoutId);
 
-        if (err) {
-            if (self._response &&
-                self._response instanceof Error === false) {
+    // Error exit
 
-                // Got error after valid result was already set
+    if (err) {
+        if (self._response &&
+            self._response instanceof Error === false) {
 
-                self._route.cache.drop(self.url.path, function (err) {
+            // Got error after valid result was already set
 
-                    self.log(['request', 'cache', 'drop'], err);
-                });
-            }
+            self._route.cache.drop(self.url.path, function (err) {
 
-            self._response = self._generateResponse(err);
+                self.log(['request', 'cache', 'drop'], err);
+            });
         }
 
-        var sendResponse = function () {
+        self._response = self._generateResponse(err);
+    }
 
-            if (!self._response) {                                  // Can only happen when request.reply.close() is called
-                self.raw.res.end();                                 // End the response in case it wasn't already closed
-                return postResponse();
-            }
+    var sendResponse = function () {
 
-            Response._respond(self._response, self, function () {
+        if (!self._response) {                                  // Can only happen when request.reply.close() is called
+            self.raw.res.end();                                 // End the response in case it wasn't already closed
+            return finalize();
+        }
 
-                return postResponse();
-            });
-        };
+        Response._respond(self._response, self, function () {
 
-        var postResponse = function () {
+            return finalize();
+        });
+    };
 
-            self.server.emit('response', self);
+    var finalize = function () {
 
-            Utils.executeRequestHandlers(self.server._ext.onPostRoute, self, function () {
+        self.server.emit('response', self);
 
-                self._isWagging = true;
-                delete self.addTail;
+        self._isWagging = true;
+        delete self.addTail;
 
-                if (Object.keys(self._tails).length === 0) {
-                    self.server.emit('tail', self);
-                }
-            });
-        };
+        if (Object.keys(self._tails).length === 0) {
+            self.server.emit('tail', self);
+        }
 
-        sendResponse();
+        return callback();
     };
 
-    execute();
+    sendResponse();
 };
 
 
-
 internals.authenticate = function (request, next) {
 
     var config = request._route.config.auth;
@@ -597,7 +621,7 @@ internals.handler = function (request, next) {
     var store = function (response) {
 
         request._response = response;
-        return next();
+        return next();                              // Must not include an argument
     };
 
     check();

lib/server.js

@@ -77,8 +77,7 @@ module.exports = internals.Server = function (/* host, port, options */) {
 
         onRequest: null,                            // New request, before handing over to the router (allows changes to the request method, url, etc.)
         onPreHandler: null,                         // After validation and body parsing, before route handler
-        onPostHandler: null,                        // After route handler returns, before sending response
-        onPostRoute: null                           // After response sent
+        onPostHandler: null                         // After route handler returns, before sending response
     };
 
     // Set optional configuration
@@ -224,11 +223,16 @@ internals.Server.prototype._dispatch = function (options) {
     return function (req, res) {
 
         // Create request object
+
         var request = new Request(self, req, res, options);
 
         // Execute onRequest extensions (can change request method and url)
 
-        request._onRequestExt(self._ext.onRequest, function () {
+        request._onRequestExt(function (err) {
+
+            if (err) {
+                return;         // Handled by the request
+            }
 
             // Lookup route
 
@@ -378,7 +382,7 @@ internals.Server.prototype.plugin = function (name, config, options) {
 
 internals.Server.prototype.ext = function (event, func) {
 
-    Utils.assert(['onRequest', 'onPreHandler', 'onPostHandler', 'onPostRoute'].indexOf(event) !== -1, 'Unknown event type: ' + event);
+    Utils.assert(['onRequest', 'onPreHandler', 'onPostHandler'].indexOf(event) !== -1, 'Unknown event type: ' + event);
     this._ext[event] = (this._ext[event] || []).concat(func);
 };
 

test/integration/ext.js

@@ -0,0 +1,38 @@
+// Load modules
+
+var Chai = require('chai');
+var Hapi = require('../helpers');
+
+
+// Declare internals
+
+var internals = {};
+
+
+// Test shortcuts
+
+var expect = Chai.expect;
+
+
+describe('Ext', function () {
+
+    describe('onRequest', function (done) {
+
+        it('replies with custom response', function (done) {
+
+            var server = new Hapi.Server();
+            server.ext('onRequest', function (request, next) {
+
+                return next(Hapi.error.badRequest('boom'));
+            });
+
+            server.route({ method: 'GET', path: '/', handler: function () { this.reply('ok'); } });
+
+            server.inject({ method: 'GET', url: '/' }, function (res) {
+
+                expect(res.result.message).to.equal('boom');
+                done();
+            });
+        });
+    });
+});

test/integration/response.js

@@ -20,7 +20,7 @@ var expect = Chai.expect;
 
 describe('Response', function () {
 
-    describe('Manual', function () {
+    describe('External', function () {
 
         it('returns a reply', function (done) {