Add mergeParams option to Router

fixes #2153
fixes #2203

History.md

@@ -4,6 +4,8 @@ unreleased
  * add deprecation message to non-plural `req.accepts*`
  * add deprecation message to `res.send(body, status)`
  * add deprecation message to `res.vary()`
+ * add `mergeParams` option to `Router`
+   - merges `req.params` from parent routes
  * deprecate things with `depd` module
  * fix behavior when handling request without routes
  * fix handling when `route.all` is only route

lib/router/index.js

@@ -5,6 +5,7 @@
 var Route = require('./route');
 var Layer = require('./layer');
 var methods = require('methods');
+var mixin = require('utils-merge');
 var debug = require('debug')('express:router');
 var parseUrl = require('parseurl');
 var slice = Array.prototype.slice;
@@ -30,6 +31,7 @@ var proto = module.exports = function(options) {
   router.params = {};
   router._params = [];
   router.caseSensitive = options.caseSensitive;
+  router.mergeParams = options.mergeParams;
   router.strict = options.strict;
   router.stack = [];
 
@@ -132,6 +134,7 @@ proto.handle = function(req, res, done) {
   var stack = self.stack;
 
   // manage inter-router variables
+  var parentParams = req.params;
   var parentUrl = req.baseUrl || '';
   done = restore(done, req, 'baseUrl', 'next', 'params');
 
@@ -197,7 +200,9 @@ proto.handle = function(req, res, done) {
       }
 
       // Capture one-time layer values
-      req.params = layer.params;
+      req.params = self.mergeParams
+        ? mergeParams(layer.params, parentParams)
+        : layer.params;
       layerPath = layer.path;
 
       // this should be done for the layer
@@ -441,6 +446,42 @@ methods.concat('all').forEach(function(method){
   };
 });
 
+// merge params with parent params
+function mergeParams(params, parent) {
+  if (typeof parent !== 'object' || !parent) {
+    return params;
+  }
+
+  // make copy of parent for base
+  var obj = mixin({}, parent);
+
+  // simple non-numeric merging
+  if (!(0 in params) || !(0 in parent)) {
+    return mixin(obj, params);
+  }
+
+  var i = 0;
+  var o = 0;
+
+  // determine numeric gaps
+  while (i === o || o in parent) {
+    if (i in params) i++;
+    if (o in parent) o++;
+  }
+
+  // offset numeric indices in params before merge
+  for (i--; i >= 0; i--) {
+    params[i + o] = params[i];
+
+    // create holes for the merge when necessary
+    if (i < o) {
+      delete params[i];
+    }
+  }
+
+  return mixin(parent, params);
+}
+
 // restore obj props after function
 function restore(fn, obj) {
   var props = new Array(arguments.length - 2);

test/app.router.js

@@ -5,6 +5,33 @@ var express = require('../')
   , methods = require('methods');
 
 describe('app.router', function(){
+  it('should restore req.params after leaving router', function(done){
+    var app = express();
+    var router = new express.Router();
+
+    function handler1(req, res, next){
+      res.setHeader('x-user-id', req.params.id);
+      next()
+    }
+
+    function handler2(req, res){
+      res.send(req.params.id);
+    }
+
+    router.use(function(req, res, next){
+      res.setHeader('x-router', req.params.id);
+      next();
+    });
+
+    app.get('/user/:id', handler1, router, handler2);
+
+    request(app)
+    .get('/user/1')
+    .expect('x-router', 'undefined')
+    .expect('x-user-id', '1')
+    .expect(200, '1', done);
+  })
+
   describe('methods supported', function(){
     methods.concat('del').forEach(function(method){
       if (method === 'connect') return;
@@ -183,6 +210,106 @@ describe('app.router', function(){
     })
   })
 
+  describe('params', function(){
+    it('should overwrite existing req.params by default', function(done){
+      var app = express();
+      var router = new express.Router();
+
+      router.get('/:action', function(req, res){
+        res.send(req.params);
+      });
+
+      app.use('/user/:user', router);
+
+      request(app)
+      .get('/user/1/get')
+      .expect(200, '{"action":"get"}', done);
+    })
+
+    it('should allow merging existing req.params', function(done){
+      var app = express();
+      var router = new express.Router({ mergeParams: true });
+
+      router.get('/:action', function(req, res){
+        var keys = Object.keys(req.params).sort();
+        res.send(keys.map(function(k){ return [k, req.params[k]] }));
+      });
+
+      app.use('/user/:user', router);
+
+      request(app)
+      .get('/user/tj/get')
+      .expect(200, '[["action","get"],["user","tj"]]', done);
+    })
+
+    it('should use params from router', function(done){
+      var app = express();
+      var router = new express.Router({ mergeParams: true });
+
+      router.get('/:thing', function(req, res){
+        var keys = Object.keys(req.params).sort();
+        res.send(keys.map(function(k){ return [k, req.params[k]] }));
+      });
+
+      app.use('/user/:thing', router);
+
+      request(app)
+      .get('/user/tj/get')
+      .expect(200, '[["thing","get"]]', done);
+    })
+
+    it('should merge numeric indices req.params', function(done){
+      var app = express();
+      var router = new express.Router({ mergeParams: true });
+
+      router.get('/*.*', function(req, res){
+        var keys = Object.keys(req.params).sort();
+        res.send(keys.map(function(k){ return [k, req.params[k]] }));
+      });
+
+      app.use('/user/id:(\\d+)', router);
+
+      request(app)
+      .get('/user/id:10/profile.json')
+      .expect(200, '[["0","10"],["1","profile"],["2","json"]]', done);
+    })
+
+    it('should merge numeric indices req.params when more in parent', function(done){
+      var app = express();
+      var router = new express.Router({ mergeParams: true });
+
+      router.get('/*', function(req, res){
+        var keys = Object.keys(req.params).sort();
+        res.send(keys.map(function(k){ return [k, req.params[k]] }));
+      });
+
+      app.use('/user/id:(\\d+)/name:(\\w+)', router);
+
+      request(app)
+      .get('/user/id:10/name:tj/profile')
+      .expect(200, '[["0","10"],["1","tj"],["2","profile"]]', done);
+    })
+
+    it('should ignore invalid incoming req.params', function(done){
+      var app = express();
+      var router = new express.Router({ mergeParams: true });
+
+      router.get('/:name', function(req, res){
+        var keys = Object.keys(req.params).sort();
+        res.send(keys.map(function(k){ return [k, req.params[k]] }));
+      });
+
+      app.use('/user/', function (req, res, next) {
+        req.params = 3; // wat?
+        router(req, res, next);
+      });
+
+      request(app)
+      .get('/user/tj')
+      .expect(200, '[["name","tj"]]', done);
+    })
+  })
+
   describe('trailing slashes', function(){
     it('should be optional by default', function(done){
       var app = express();
@@ -656,33 +783,6 @@ describe('app.router', function(){
     .expect(200, '0,1,2,3,4,5', done);
   })
 
-  it('should provide req.params to all handlers', function(done){
-    var app = express();
-    var router = new express.Router();
-
-    function handler1(req, res, next){
-      res.setHeader('x-user-id', req.params.id);
-      next()
-    }
-
-    function handler2(req, res){
-      res.send(req.params.id);
-    }
-
-    router.use(function(req, res, next){
-      res.setHeader('x-router', req.params.id);
-      next();
-    });
-
-    app.get('/user/:id', handler1, router, handler2);
-
-    request(app)
-    .get('/user/1')
-    .expect('x-router', 'undefined')
-    .expect('x-user-id', '1')
-    .expect(200, '1', done);
-  })
-
   it('should be chainable', function(){
     var app = express();
     app.get('/', function(){}).should.equal(app);