Merge pull request #908 from strongloop/fix/express-helpers-in-phased-middleware

Rework phased middleware, fix several bugs

lib/server-app.js

@@ -1,9 +1,10 @@
 var assert = require('assert');
 var express = require('express');
 var merge = require('util')._extend;
-var PhaseList = require('loopback-phase').PhaseList;
+var mergePhaseNameLists = require('loopback-phase').mergePhaseNameLists;
 var debug = require('debug')('loopback:app');
-var pathToRegexp = require('path-to-regexp');
+
+var BUILTIN_MIDDLEWARE = { builtin: true };
 
 var proto = {};
 
@@ -104,9 +105,12 @@ proto.defineMiddlewarePhases = function(nameOrArray) {
   this.lazyrouter();
 
   if (Array.isArray(nameOrArray)) {
-    this._requestHandlingPhases.zipMerge(nameOrArray);
+    this._requestHandlingPhases =
+      mergePhaseNameLists(this._requestHandlingPhases, nameOrArray);
   } else {
-    this._requestHandlingPhases.addBefore('routes', nameOrArray);
+    // add the new phase before 'routes'
+    var routesIx = this._requestHandlingPhases.indexOf('routes');
+    this._requestHandlingPhases.splice(routesIx - 1, 0, nameOrArray);
   }
 
   return this;
@@ -131,90 +135,39 @@ proto.middleware = function(name, paths, handler) {
 
   if (handler === undefined && typeof paths === 'function') {
     handler = paths;
-    paths = [];
+    paths = undefined;
-  }
-
-  if (typeof paths === 'string' || paths instanceof RegExp) {
-    paths = [paths];
   }
 
   assert(typeof name === 'string' && name, '"name" must be a non-empty string');
   assert(typeof handler === 'function', '"handler" must be a function');
-  assert(Array.isArray(paths), '"paths" must be an array');
 
-  var fullName = name;
+  if (paths === undefined) {
-  var handlerName = handler.name || '(anonymous)';
+    paths = '/';
+  }
+
+  var fullPhaseName = name;
+  var handlerName = handler.name || '<anonymous>';
 
-  var hook = 'use';
   var m = name.match(/^(.+):(before|after)$/);
   if (m) {
     name = m[1];
-    hook = m[2];
   }
 
-  var phase = this._requestHandlingPhases.find(name);
+  if (this._requestHandlingPhases.indexOf(name) === -1)
-  if (!phase)
     throw new Error('Unknown middleware phase ' + name);
 
-  var matches = createRequestMatcher(paths);
+  debug('use %s %s %s', fullPhaseName, paths, handlerName);
 
-  var wrapper;
+  this._skipLayerSorting = true;
-  if (handler.length === 4) {
+  this.use(paths, handler);
-    // handler is function(err, req, res, next)
+  this._router.stack[this._router.stack.length - 1].phase = fullPhaseName;
-    debug('Add error handler %j to phase %j', handlerName, fullName);
+  this._skipLayerSorting = false;
 
-    wrapper = function errorHandler(ctx, next) {
+  this._sortLayersByPhase();
-      if (ctx.err && matches(ctx.req)) {
-        var err = ctx.err;
-        ctx.err = undefined;
-        handler(err, ctx.req, ctx.res, storeErrorAndContinue(ctx, next));
-      } else {
-        next();
-      }
-    };
-  } else {
-    // handler is function(req, res, next)
-    debug('Add middleware %j to phase %j', handlerName , fullName);
-    wrapper = function regularHandler(ctx, next) {
-      if (ctx.err || !matches(ctx.req)) {
-        next();
-      } else {
-        handler(ctx.req, ctx.res, storeErrorAndContinue(ctx, next));
-      }
-    };
-  }
 
-  phase[hook](wrapper);
   return this;
 };
 
-function createRequestMatcher(paths) {
-  if (!paths.length) {
-    return function requestMatcher(req) { return true; };
-  }
-
-  var checks = paths.map(function(p) {
-    return pathToRegexp(p, {
-      sensitive: true,
-      strict: false,
-      end: false
-    });
-  });
-
-  return function requestMatcher(req) {
-    return checks.some(function(regex) {
-      return regex.test(req.url);
-    });
-  };
-}
-
-function storeErrorAndContinue(ctx, next) {
-  return function(err) {
-    if (err) ctx.err = err;
-    next();
-  };
-}
-
 // Install our custom PhaseList-based handler into the app
 proto.lazyrouter = function() {
   var self = this;
@@ -222,45 +175,70 @@ proto.lazyrouter = function() {
 
   self.__expressLazyRouter();
 
-  // Storing the fn in another property of the router object
+  var router = self._router;
-  // allows us to call the method with the router as `this`
-  // without the need to use slow `call` or `apply`.
-  self._router.__expressHandle = self._router.handle;
 
-  self._requestHandlingPhases = new PhaseList();
+  // Mark all middleware added by Router ctor as builtin
-  self._requestHandlingPhases.add([
+  // The sorting algo will keep them at beginning of the list
+  router.stack.forEach(function(layer) {
+    layer.phase = BUILTIN_MIDDLEWARE;
+  });
+
+  router.__expressUse = router.use;
+  router.use = function useAndSort() {
+    var retval = this.__expressUse.apply(this, arguments);
+    self._sortLayersByPhase();
+    return retval;
+  };
+
+  router.__expressRoute = router.route;
+  router.route = function routeAndSort() {
+    var retval = this.__expressRoute.apply(this, arguments);
+    self._sortLayersByPhase();
+    return retval;
+  };
+
+  self._requestHandlingPhases = [
     'initial', 'session', 'auth', 'parse',
     'routes', 'files', 'final'
-  ]);
+  ];
-
+};
-  // In order to pass error into express router, we have
+
-  // to pass it to a middleware executed from within the router.
+proto._sortLayersByPhase = function() {
-  // This is achieved by adding a phase-handler that wraps the error
+  if (this._skipLayerSorting) return;
-  // into `req` object and then a router-handler that unwraps the error
+
-  // and calls `next(err)`.
+  var phaseOrder = {};
-  // It is important to register these two handlers at the very beginning,
+  this._requestHandlingPhases.forEach(function(name, ix) {
-  // before any other handlers are added.
+    phaseOrder[name + ':before'] = ix * 3;
-  self.middleware('routes', function wrapError(err, req, res, next) {
+    phaseOrder[name] = ix * 3 + 1;
-    req.__err = err;
+    phaseOrder[name + ':after'] = ix * 3 + 2;
-    next();
+  });
-  });
+
-
+  var router = this._router;
-  self.use(function unwrapError(req, res, next) {
+  router.stack.sort(compareLayers);
-    var err = req.__err;
+
-    req.__err = undefined;
+  function compareLayers(left, right) {
-    next(err);
+    var leftPhase = left.phase;
-  });
+    var rightPhase = right.phase;
-
+
-  self.middleware('routes', function runRootHandlers(req, res, next) {
+    if (leftPhase === rightPhase) return 0;
-    self._router.__expressHandle(req, res, next);
+
-  });
+    // Builtin middleware is always first
-
+    if (leftPhase === BUILTIN_MIDDLEWARE) return -1;
-  // Overwrite the original handle() function provided by express,
+    if (rightPhase === BUILTIN_MIDDLEWARE) return 1;
-  // replace it with our implementation based on PhaseList
+
-  self._router.handle = function(req, res, next) {
+    // Layers registered via app.use and app.route
-    var ctx = { req: req, res: res };
+    // are executed as the first items in `routes` phase
-    self._requestHandlingPhases.run(ctx, function(err) {
+    if (leftPhase === undefined) {
-      next(err || ctx.err);
+      if (rightPhase === 'routes')
-    });
+        return -1;
-  };
+
+      return phaseOrder['routes'] - phaseOrder[rightPhase];
+    }
+
+    if (rightPhase === undefined)
+      return -compareLayers(right, left);
+
+    // Layers registered via `app.middleware` are compared via phase & hook
+    return phaseOrder[leftPhase] - phaseOrder[rightPhase];
+  }
 };

package.json

@@ -42,10 +42,9 @@
     "express": "^4.10.2",
     "inflection": "~1.4.2",
     "loopback-connector-remote": "^1.0.1",
-    "loopback-phase": "^1.0.1",
+    "loopback-phase": "^1.2.0",
     "nodemailer": "~1.3.0",
     "nodemailer-stub-transport": "~0.1.4",
-    "path-to-regexp": "^1.0.1",
     "serve-favicon": "^2.1.6",
     "strong-remoting": "^2.4.0",
     "uid2": "0.0.3",

test/app.test.js

@@ -4,6 +4,7 @@ var async = require('async');
 var path = require('path');
 
 var http = require('http');
+var express = require('express');
 var loopback = require('../');
 var PersistedModel = loopback.PersistedModel;
 
@@ -41,6 +42,17 @@ describe('app', function() {
       });
     });
 
+    it('preserves order of handlers in the same phase', function(done) {
+      app.middleware('initial', namedHandler('first'));
+      app.middleware('initial', namedHandler('second'));
+
+      executeMiddlewareHandlers(app, function(err) {
+        if (err) return done(err);
+        expect(steps).to.eql(['first', 'second']);
+        done();
+      });
+    });
+
     it('supports `before:` and `after:` prefixes', function(done) {
       app.middleware('routes:before', namedHandler('routes:before'));
       app.middleware('routes:after', namedHandler('routes:after'));
@@ -151,6 +163,140 @@ describe('app', function() {
         });
     });
 
+    it('sets req.url to a sub-path', function(done) {
+      app.middleware('initial', ['/scope'], function(req, res, next) {
+        steps.push(req.url);
+        next();
+      });
+
+      executeMiddlewareHandlers(app, '/scope/id', function(err) {
+        if (err) return done(err);
+        expect(steps).to.eql(['/id']);
+        done();
+      });
+    });
+
+    it('exposes express helpers on req and res objects', function(done) {
+      var req;
+      var res;
+
+      app.middleware('initial', function(rq, rs, next) {
+        req = rq;
+        res = rs;
+        next();
+      });
+
+      executeMiddlewareHandlers(app, function(err) {
+        if (err) return done(err);
+        expect(getObjectAndPrototypeKeys(req), 'request').to.include.members([
+          'accepts',
+          'get',
+          'param',
+          'params',
+          'query',
+          'res'
+        ]);
+
+        expect(getObjectAndPrototypeKeys(res), 'response').to.include.members([
+          'cookie',
+          'download',
+          'json',
+          'jsonp',
+          'redirect',
+          'req',
+          'send',
+          'sendFile',
+          'set'
+        ]);
+
+        done();
+      });
+    });
+
+    it('sets req.baseUrl and req.originalUrl', function(done) {
+      var reqProps;
+      app.middleware('initial', function(req, res, next) {
+        reqProps = { baseUrl: req.baseUrl, originalUrl: req.originalUrl };
+        next();
+      });
+
+      executeMiddlewareHandlers(app, '/test/url', function(err) {
+        if (err) return done(err);
+        expect(reqProps).to.eql({ baseUrl: '', originalUrl: '/test/url' });
+        done();
+      });
+    });
+
+    it('preserves correct order of routes vs. middleware', function(done) {
+      // This test verifies that `app.route` triggers sort of layers
+      app.middleware('files', namedHandler('files'));
+      app.get('/test', namedHandler('route'));
+
+      executeMiddlewareHandlers(app, '/test', function(err) {
+        if (err) return done(err);
+        expect(steps).to.eql(['route', 'files']);
+        done();
+      });
+    });
+
+    it('correctly mounts express apps', function(done) {
+      var data;
+      var mountWasEmitted;
+      var subapp = express();
+      subapp.use(function(req, res, next) {
+        data = {
+          mountpath: req.app.mountpath,
+          parent: req.app.parent
+        };
+        next();
+      });
+      subapp.on('mount', function() { mountWasEmitted = true; });
+
+      app.middleware('routes', '/mountpath', subapp);
+
+      executeMiddlewareHandlers(app, '/mountpath/test', function(err) {
+        if (err) return done(err);
+        expect(mountWasEmitted, 'mountWasEmitted').to.be.true();
+        expect(data).to.eql({
+          mountpath: '/mountpath',
+          parent: app
+        });
+        done();
+      });
+    });
+
+    it('restores req & res on return from mounted express app', function(done) {
+      // jshint proto:true
+      var expected = {};
+      var actual = {};
+
+      var subapp = express();
+      subapp.use(function verifyTestAssumptions(req, res, next) {
+        expect(req.__proto__).to.not.equal(expected.req);
+        expect(res.__proto__).to.not.equal(expected.res);
+        next();
+      });
+
+      app.middleware('initial', function saveOriginalValues(req, res, next) {
+        expected.req = req.__proto__;
+        expected.res = res.__proto__;
+        next();
+      });
+      app.middleware('routes', subapp);
+      app.middleware('final', function saveActualValues(req, res, next) {
+        actual.req = req.__proto__;
+        actual.res = res.__proto__;
+        next();
+      });
+
+      executeMiddlewareHandlers(app, function(err) {
+        if (err) return done(err);
+        expect(actual.req, 'req').to.equal(expected.req);
+        expect(actual.res, 'res').to.equal(expected.res);
+        done();
+      });
+    });
+
     function namedHandler(name) {
       return function(req, res, next) {
         steps.push(name);
@@ -160,10 +306,19 @@ describe('app', function() {
 
     function pathSavingHandler() {
       return function(req, res, next) {
-        steps.push(req.url);
+        steps.push(req.originalUrl);
         next();
       };
     }
+
+    function getObjectAndPrototypeKeys(obj) {
+      var result = [];
+      for (var k in obj) {
+        result.push(k);
+      }
+      result.sort();
+      return result;
+    }
   });
 
   describe.onServer('.middlewareFromConfig', function() {
@@ -223,7 +378,7 @@ describe('app', function() {
       app.middlewareFromConfig(
         function factory() {
           return function(req, res, next) {
-            steps.push(req.url);
+            steps.push(req.originalUrl);
             next();
           };
         },
@@ -286,7 +441,7 @@ describe('app', function() {
     it('throws helpful error on ordering conflict', function() {
       app.defineMiddlewarePhases(['first', 'second']);
       expect(function() { app.defineMiddlewarePhases(['second', 'first']); })
-        .to.throw(/ordering conflict.*first.*second/);
+        .to.throw(/Ordering conflict.*first.*second/);
     });
 
     function verifyMiddlewarePhases(names, done) {