From 84af4194fbba69f593896ef0edcfdaf38b7df115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Wed, 10 Dec 2014 09:36:45 +0100 Subject: [PATCH] Rework phased middleware, fix several bugs Bugs fixed: - express helpers like `req.get` are now available in middleware handlers registered via `app.middleware` - `req.url` does not include the mountpath prefix now, this is consistent with the behaviour of `app.use` The implementation of phased middleware was completely rewritten. - We no longer use Phase and PhaseList objects from loopback-phase. - Handler functions are registered via the `Layer` mechanism used by express router. - The app keeps the layers sorted according to phases. --- lib/server-app.js | 192 ++++++++++++++++++++-------------------------- package.json | 3 +- test/app.test.js | 161 +++++++++++++++++++++++++++++++++++++- 3 files changed, 244 insertions(+), 112 deletions(-) diff --git a/lib/server-app.js b/lib/server-app.js index b75835da..7788dd9e 100644 --- a/lib/server-app.js +++ b/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 = []; - } - - if (typeof paths === 'string' || paths instanceof RegExp) { - paths = [paths]; + paths = undefined; } 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; - var handlerName = handler.name || '(anonymous)'; + if (paths === undefined) { + paths = '/'; + } + + var fullPhaseName = name; + var handlerName = handler.name || ''; - var hook = 'use'; var m = name.match(/^(.+):(before|after)$/); if (m) { name = m[1]; - hook = m[2]; } - var phase = this._requestHandlingPhases.find(name); - if (!phase) + if (this._requestHandlingPhases.indexOf(name) === -1) throw new Error('Unknown middleware phase ' + name); - var matches = createRequestMatcher(paths); + debug('use %s %s %s', fullPhaseName, paths, handlerName); - var wrapper; - if (handler.length === 4) { - // handler is function(err, req, res, next) - debug('Add error handler %j to phase %j', handlerName, fullName); + this._skipLayerSorting = true; + this.use(paths, handler); + this._router.stack[this._router.stack.length - 1].phase = fullPhaseName; + this._skipLayerSorting = false; - wrapper = function errorHandler(ctx, next) { - 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)); - } - }; - } + this._sortLayersByPhase(); - 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 - // 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; + var router = self._router; - self._requestHandlingPhases = new PhaseList(); - self._requestHandlingPhases.add([ + // Mark all middleware added by Router ctor as builtin + // 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. - // This is achieved by adding a phase-handler that wraps the error - // into `req` object and then a router-handler that unwraps the error - // and calls `next(err)`. - // It is important to register these two handlers at the very beginning, - // before any other handlers are added. - self.middleware('routes', function wrapError(err, req, res, next) { - req.__err = err; - next(); - }); - - self.use(function unwrapError(req, res, next) { - var err = req.__err; - req.__err = undefined; - next(err); - }); - - self.middleware('routes', function runRootHandlers(req, res, next) { - self._router.__expressHandle(req, res, next); - }); - - // Overwrite the original handle() function provided by express, - // replace it with our implementation based on PhaseList - self._router.handle = function(req, res, next) { - var ctx = { req: req, res: res }; - self._requestHandlingPhases.run(ctx, function(err) { - next(err || ctx.err); - }); - }; + ]; +}; + +proto._sortLayersByPhase = function() { + if (this._skipLayerSorting) return; + + var phaseOrder = {}; + this._requestHandlingPhases.forEach(function(name, ix) { + phaseOrder[name + ':before'] = ix * 3; + phaseOrder[name] = ix * 3 + 1; + phaseOrder[name + ':after'] = ix * 3 + 2; + }); + + var router = this._router; + router.stack.sort(compareLayers); + + function compareLayers(left, right) { + var leftPhase = left.phase; + var rightPhase = right.phase; + + if (leftPhase === rightPhase) return 0; + + // Builtin middleware is always first + if (leftPhase === BUILTIN_MIDDLEWARE) return -1; + if (rightPhase === BUILTIN_MIDDLEWARE) return 1; + + // Layers registered via app.use and app.route + // are executed as the first items in `routes` phase + if (leftPhase === undefined) { + 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]; + } }; diff --git a/package.json b/package.json index 1b70b9fa..bae83f44 100644 --- a/package.json +++ b/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", diff --git a/test/app.test.js b/test/app.test.js index 452089f1..978773ac 100644 --- a/test/app.test.js +++ b/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) {