Merge pull request #592 from walmartlabs/user/eran

refactor router

lib/router.js

@@ -0,0 +1,127 @@
+// Load modules
+
+var Route = require('./route');
+var Utils = require('./utils');
+
+
+// Declare internals
+
+var internals = {};
+
+
+module.exports = internals.Router = function (server) {
+
+    this.server = server;
+    this.table = {};                                // Array per HTTP method, including * for catch-all
+
+    this.notfound = new Route({
+        method: 'notfound',
+        path: '/{p*}',
+        config: {
+            auth: { mode: 'none' },                 // In case defaults are set otherwise
+            handler: 'notFound'
+        }
+    }, server);
+
+    if (server.settings.cors) {
+        this.cors = new Route({
+            path: '/{p*}',
+            method: 'options',
+            config: {
+                auth: { mode: 'none' },                 // In case defaults are set otherwise
+                handler: function (request) {
+
+                    request.reply({});
+                }
+            }
+        }, server);
+    }
+};
+
+
+internals.Router.prototype.route = function (request) {
+
+    // Lookup route
+
+    var method = (request.method === 'head' ? 'get' : request.method);
+
+    var routes = this.table[method] || [];
+    for (var i = 0, il = routes.length; i<il;++i) {
+        var route = routes[i];
+        if (route.match(request)) {
+            return route;
+        }
+    }
+
+    // CORS
+
+    if (method === 'options' &&
+        this.cors) {
+
+        return this.cors;
+    }
+
+    // *
+
+    routes = this.table['*'] || [];
+    for (i = 0, il = routes.length; i < il; ++i) {
+        var route = routes[i];
+        if (route.match(request)) {
+            return route;
+        }
+    }
+
+    // Not found
+
+    return this.notfound;
+};
+
+
+internals.Router.prototype.add = function (configs) {
+
+    var self = this;
+
+    Utils.assert(configs, 'Routes configs must exist');
+
+    configs = (configs instanceof Array ? configs : [configs]);
+
+    var methods = {};
+    configs.forEach(function (config) {
+
+        var route = new Route(config, self.server);                                 // Do no use config beyond this point, use route members
+
+        self.table[route.method] = self.table[route.method] || [];
+
+        // Check for existing route with same fingerprint
+
+        methods[route.method] = true;
+        self.table[route.method].forEach(function (existing) {
+
+            Utils.assert(route.fingerprint !== existing.fingerprint, 'New route: ' + route.path + ' conflicts with existing: ' + existing.path);
+        });
+
+        self.table[route.method].push(route);
+    });
+
+    Object.keys(methods).forEach(function (method) {
+
+        self.table[method].sort(Route.sort);
+    });
+};
+
+
+internals.Router.prototype.routingTable = function () {
+
+    var self = this;
+
+    var table = [];
+    Object.keys(this.table).forEach(function (method) {
+
+        self.table[method].forEach(function (route) {
+
+            table.push(route);
+        });
+    });
+
+    return table;
+};

lib/server.js

@@ -11,7 +11,7 @@ var Catbox = require('catbox');
 var Defaults = require('./defaults');
 var Boom = require('boom');
 var Request = require('./request');
-var Route = require('./route');
+var Router = require('./router');
 var Schema = require('./schema');
 var Views = require('./views');
 var Utils = require('./utils');
@@ -23,8 +23,6 @@ var Utils = require('./utils');
 var internals = {};
 
 
-// Create and configure server instance
-
 module.exports = internals.Server = function (/* host, port, options */) {        // all optional
 
     var self = this;
@@ -113,18 +111,7 @@ module.exports = internals.Server = function (/* host, port, options */) {
 
     Utils.assert(!this.settings.router.routeDefaults || !this.settings.router.routeDefaults.handler, 'Route defaults cannot include a handler');
 
-    this._router = {
-        table: {}                                   // Array per HTTP method, including * for catch-all
-    };
-
-    this._router.notfound = new Route({
-        method: 'notfound',
-        path: '/{p*}',
-        config: {
-            auth: { mode: 'none' },                 // In case defaults are set otherwise
-            handler: 'notFound'
-        }
-    }, this);
+    this._router = new Router(this);
 
     // Plugin interface (pack)
 
@@ -161,20 +148,6 @@ module.exports = internals.Server = function (/* host, port, options */) {
     if (this.settings.cors) {
         this.settings.cors._headers = (this.settings.cors.headers || []).concat(this.settings.cors.additionalHeaders || []).join(', ');
         this.settings.cors._methods = (this.settings.cors.methods || []).concat(this.settings.cors.additionalMethods || []).join(', ');
-
-        var optionsConfig = {
-            path: '/{p*}',
-            method: 'options',
-            config: {
-                auth: { mode: 'none' },                 // In case defaults are set otherwise
-                handler: function (request) {
-
-                    request.reply({});
-                }
-            }
-        };
-
-        this._router.cors = new Route(optionsConfig, this);
     }
 
     // Initialize cache engine
@@ -232,87 +205,15 @@ internals.Server.prototype._dispatch = function (options) {
 
             // Lookup route
 
-            var method = (request.method === 'head' ? 'get' : request.method);
-
-            var routes = self._router.table[method];
-            if (routes) {
-                for (var i = 0, il = routes.length; i < il; ++i) {
-                    var route = routes[i];
-                    if (route.match(request)) {
-                        return request._execute(route);
-                    }
-                }
-            }
-
-            // CORS
-
-            if (method === 'options' &&
-                self.settings.cors) {
-
-                return request._execute(self._router.cors);
-            }
-
-            // *
-
-            routes = self._router.table['*'];
-            if (routes) {
-                for (i = 0, il = routes.length; i < il; ++i) {
-                    var route = routes[i];
-                    if (route.match(request)) {
-                        return request._execute(route);
-                    }
-                }
-            }
-
-            // Not found
-
-            return request._execute(self._router.notfound);
+            request._execute(self._router.route(request));
         });
     };
 };
 
 
-// Find a route match
-
-internals.Server.prototype._match = function (method, path) {
-
-    Utils.assert(method, 'The method parameter must be provided');
-    Utils.assert(path, 'The path parameter must be provided');
-
-    // Lookup route
+internals.Server.prototype.routingTable = function () {
 
-    method = method.toLowerCase();
-    method = (method === 'head' ? 'get' : method);
-    var routes = this._router.table[method];
-
-    if (routes) {
-        for (var i = 0, il = routes.length; i < il; ++i) {
-            var route = routes[i];
-            if (route.test(path)) {
-                return route;
-            }
-        }
-    }
-
-    return null;
-};
-
-
-internals.Server.prototype._routeTable = function () {
-
-    var self = this;
-
-    var flattenedRoutes = [];
-
-    Object.keys(this._router.table).forEach(function (method) {
-
-        self._router.table[method].forEach(function (route) {
-
-            flattenedRoutes.push(route);
-        });
-    });
-
-    return flattenedRoutes;
+    return this._router.routingTable();
 };
 
 
@@ -408,34 +309,7 @@ internals.Server.prototype.ext = function (event, func) {
 
 internals.Server.prototype.route = internals.Server.prototype.addRoute = internals.Server.prototype.addRoutes = function (configs) {
 
-    var self = this;
-
-    Utils.assert(configs, 'Routes configs must exist');
-
-    configs = (configs instanceof Array ? configs : [configs]);
-
-    var methods = {};
-    configs.forEach(function (config) {
-
-        var route = new Route(config, self);                               // Do no use config beyond this point, use route members
-
-        self._router.table[route.method] = self._router.table[route.method] || [];
-
-        // Check for existing route with same fingerprint
-
-        methods[route.method] = true;
-        var routes = self._router.table[route.method];
-        for (var ri = 0, rl = routes.length; ri < rl; ++ri) {
-            Utils.assert(route.fingerprint !== routes[ri].fingerprint, 'New route: ' + route.path + ' conflicts with existing: ' + routes[ri].path);
-        }
-
-        routes.push(route);
-    });
-
-    Object.keys(methods).forEach(function (method) {
-
-        self._router.table[method].sort(Route.sort);
-    });
+    this._router.add(configs);
 };
 
 

test/integration/proxy.js

@@ -224,7 +224,7 @@ describe('Proxy', function () {
             callback(new Error());
         };
 
-        var route = _server._match('get', '/proxyerror');
+        var route = _server._router.route({ method: 'get', path: '/proxyerror' });
         route.proxy.httpClient = requestStub;
 
         makeRequest({ path: '/proxyerror', method: 'get' }, function (rawRes) {

test/unit/server.js

@@ -162,55 +162,6 @@ describe('Server', function () {
         done();
     });
 
-    describe('#_match', function () {
-
-        it('throws an error when the method parameter is null', function (done) {
-
-            var fn = function () {
-
-                var server = new Hapi.Server('0.0.0.0', 0);
-                server._match(null, '/test');
-            };
-            expect(fn).to.throw(Error, 'The method parameter must be provided');
-            done();
-        });
-
-        it('throws an error when the path parameter is null', function (done) {
-
-            var fn = function () {
-
-                var server = new Hapi.Server('0.0.0.0', 0);
-                server._match('POST', null);
-            };
-            expect(fn).to.throw(Error, 'The path parameter must be provided');
-            done();
-        });
-
-        it('returns null when no routes are added', function (done) {
-
-            var server = new Hapi.Server('0.0.0.0', 0);
-            var result = server._match('GET', '/test');
-
-            expect(result).to.not.exist;
-            done();
-        });
-
-        it('returns the route when there is a match', function (done) {
-
-            var server = new Hapi.Server('0.0.0.0', 0);
-            server.route({
-                method: 'GET',
-                path: '/test',
-                handler: function () { }
-            });
-            var result = server._match('GET', '/test');
-
-            expect(result).exist;
-            expect(result.path).to.equal('/test');
-            done();
-        });
-    });
-
     describe('#start', function () {
 
         it('doesn\'t throw an error', function (done) {
@@ -558,7 +509,7 @@ describe('Server', function () {
         });
     });
 
-    describe('#_routeTable', function () {
+    describe('#routingTable', function () {
 
         it('returns an array of the current routes', function (done) {
 
@@ -567,7 +518,7 @@ describe('Server', function () {
             server.route({ path: '/test/', method: 'get', handler: function () { } });
             server.route({ path: '/test/{p}/end', method: 'get', handler: function () { } });
 
-            var routes = server._routeTable();
+            var routes = server.routingTable();
 
             expect(routes.length).to.equal(2);
             expect(routes[0].path).to.equal('/test/');