Add originalRoute like originalUrl

[danieljuhl]

In a request we can get the originalUrl, which is the complete URL for the request after it has travelled through nested routes.

But I think an originalRoute would be at least as usefull, especially for request logging.

I don't think it should be that difficult to add.

Currently I've made a simple hack to accomplish this, but I rather see it built-in.

var _ = require('lodash');
var express = require('express');
var Router = express.Router;
var Router_process_params = Router.process_params;

Router.process_params = function (layer, called, req, res, done) {
  req.originalRoute = (req.originalRoute || '') + (req.route && req.route.path || layer.path || '');
  return Router_process_params.apply(this, _.toArray(arguments));
}

[dougwilson]

Can you show a little express app that makes use of this as well, please :)?

[danieljuhl]

@dougwilson Is the following good enough?

A request could be GET /api/user/123, and I would expect to be able to get the full route /api/user/:id in the request object.

server.js

var express = require('express');
var app = express();

app.use('/api', require('./api').router);

app.listen(port);

api/index.js

var express = require('express');
var router = express.Router();

router.use('/user', require('./user'));
router.use('/items', require('./items'));

exports.router = router;

api/user.js

var express = require('express');
var router = express.Router();

router.get('/:id', function(...) {});
router.get('/', function(...) {});

exports.router = router;

api/items.js

var express = require('express');
var router = express.Router();

router.get('/:id', function(...) {});
router.get('/', function(...) {});

exports.router = router;

[dougwilson]

How would it work for RegExp routes or array routes?

[dougwilson]

i.e. we need it to work if your main file has

app.use(['/api', '/api/v1'], require('./api').router);

or

app.use(/^\/api(?:\/v1)?\//i, require('./api').router);

[danieljuhl]

@dougwilson I see your concerns, and that we have to re-think it, to ensure all methods will be handled correctly.

Another approach could be to simply expose an array of routes in the order they were matched, and if you are going to use the data, you'll have to deal with the different data types.

Assume that you'd replaced my route in server.js with one of you version, then req.originalRoute === [['/api', '/api/v1'], '/user', '/:id']; or req.originalRoute === [/^\/api(?:\/v1)?\//i, '/user', '/:id']. In the "array" version, we might actually be able to identify which of the two was used for the request, and then show that specific one instead of an array - and then we only need to see if we can do something better on RegExp-route.

I don't know if newrelic is able to handle all cases (haven't investigated their code further), but they do something similar to my hacky workaround. https://github.com/newrelic/node-newrelic/blob/master/lib/instrumentation/express.js#L184

[dougwilson]

That's fine. I don't mind adding something for this. We just want to make sure that what gets added to Express core is compatible with all of Express's features, rather than only the features a particular user just happens to be using it is all :)

[danieljuhl]

I'm all in on that - I did not just want this to be a feature for me :) This issue was to rise a discussion on a good way to implement something like this, as the new Router in Express 4 is giving the power to split and nest routes, but the downside is, that it can be hard to know exactly where you are, and for logging purpose it is often useful to be able to group by route rather than exact URL, to know how many request of a specific type has happend.

Which way do you prefer? Some way to concat the routes or should we go with the array of routes a particular request has travelled from entry to the final route?

[dougwilson]

An array that gets built up sounds like the simplest method to me.

[danieljuhl]

An array is fine with me... if a use case needs another format, it simply has to handle the transformation from the array to something else, eg a concat-string.

[devlato]

Hey, I can confirm that it would be great to have originalRoute. I'm trying to implement declarative ACL based on routing so it's very uncomfortable to deal with it without knowing what route was matched =(

[dougwilson]

Any traction on a PR?

[ajfranzoia]

@dougwilson Created a PR about this (#2864).

[devlato]

@ajfranzoia Thank you very much

[evanshortiss]

@dougwilson @ajfranzoia @danieljuhl @devlato thoughts on #3100?

[dougwilson]

Hi @evanshortiss it looks like that PR doesn't address the concerns brought up in this PR.

[evanshortiss]

@ajfranzoia @danieljuhl @devlato, your input would be appreciated based on @dougwilson's latest comments on #3100.

Currently #3100 is building up an Array of all matched routes as discussed in this PR. @dougwilson rightly suggested that req.route could be used for some scenarios. req.route is workable so long as you only need the relative path.

An issue arises when you'd like to have access to all matched routes. The code below for example cannot provide the full matched path, only the local path, thus we lose "/hey" in logs, analytics, and wherever else your use case requires.

var app = require('express')();
var route = require('express').Router();

route.get('/you/:id', function (req, res) {
  res.end(req.route.path); // Will return "/you/:id", not "/hey/you/:id" or similar
});

app.use('/hey', route);

app.listen(3000);

There are ways around this using middleware or alternative structures, but think it would be nice if express had the ability to provide this information in a clean, clear built-in manner. My thoughts on a middleware is something to the effect of this, which I'd actually settle for if adding this behaviour to express doesn't make sense:

function expressMatchedMiddleware (req, res, next) {
  if (!req.matchedRoutes) {
    req.matchedRoutes = [];
  }

  if (req.route) {
    req.matchedRoutes.push(req.route);
  }

  next();
};

[hacksparrow]

WRT #3100 as well.

Since the path parameter of middleware and router handlers can be more than simple strings - examples, this kind of feature will have a hard time being universally useful.

Let's look at some scenarios.

1. The API should return an array of matched layers/routes.

   Consider the resulting array, when a request is made to /secret:

   // there are two built-in middleware in express already
   app.use(...) // logger
   app.use(...) // body parse
   app.use(...) // cookie parser
   app.use(...) // some more root-level middleware
   app.use([['restricted', 'hidden'], /lol/, 'ha*'], (req, res, next) => next())
   app.use('/secret', (req, res, next) => next())
   app.use('/secret', (req, res, next) => res.send('ABRACADABRA!'))
   

   Questions:

   a. Would the resulting array of matched layers/routes be useful?
   b. What advantage would it have over manually looking through the code?

2. The API should return the absolute path of the route (full route)

   What should it return when a request is made to /whatever/pineapple or /whatever/apple, in this case?

   var router = express.Router()
   
   router.get([['cat', 'dog'], '*apple'], (req, res, next) => {
   req.whatever = 'YOLO'
   next()
   })
   router.get(['/apple', '/pineapple'], (req, res) => res.send(req.whatever))
   
   app.use('/whatever', router)
   

   Simply returning /whatever/pineapple or /whatever/apple hides the significance of what router.get([['cat', 'dog'], '*apple'] does to req.

3. Best case scenario: the "path" is a simple string

   app.get('/apple', (req, res, next) => next())
   app.get('/apple', (req, res, next) => next())
   app.get('/apple', (req, res, next) => next())
   app.get('/apple', (req, res, next) => next())
   app.get('/apple', (req, res, next) => res.send('APPLE'))
   app.get('/apple', (req, res, next) => next())
   

   The API will return /apple, but there is no context about the route handler, unlike req.route.

My suggestion would be to let the users implement their own ways of retrieving the absolute path of the matched route(s), for use cases unique to their own requirements.

@dougwilson @ajfranzoia @danieljuhl @devlato @evanshortiss

[danieljuhl]

@hacksparrow your scenarios are valid, but it seems like you assume that the API should return a combined string. Or am I getting you wrong?

I believe it has already been discussed a lot, that the only solution that would make sense, is to return an array of the actual route definitions, that be a string, regex, an array or whatever is allowed by express.

As I see it, @evanshortiss has actually handled that, though more test scenarios should be created, to ensure the API is handling all kinds of route definitions.

[hacksparrow]

@danieljuhl I am not making any assumptions about what the API should return. In fact, I cannot think of anything that should be returned, which would make sense universally.

Scenarios 1 and 3 highlight the issue of using an array as implemented by @evanshortiss.

[evanshortiss]

@hacksparrow here are some thoughts.

1. I think populating with req.route makes sense, but this example (app.use) is definitely a valid gap, since you do actually respond in the final handler but it (req.matchedRoutes) would yield no matches in this case if we were to use req.route. However, I would argue that you should use app.all('/secrets', handler) or app.METHOD('/secrets', handler) for the last scenario since this would populate req.route and is more sensible from a semantic standpoint. For middleware such as body parser, nothing should be added to the matched routes, since they aren't routes, but are a passthrough.

   a. Yes, one would expect this is populating the Array using req.route and app.all: [{path: '/secret', /* other req.route props */}], or the current PR [ '/', '/', '/', '/secret', '/secret' ].
   b. This information can be useful at runtime for logging.

2. True, but it's a passthrough middleware. The purpose of this issue is purely to get the matched routes, not the state of the request object.

3. Correct, but again these are passthroughs/middleware. One can delete known passthroughs to easily construct the absolute URL, or use them if their scenario requires.

Ultimately the goal of this issue is to enable a developer to programatically determine matched routes which seems like a reasonable request. But I agree, we can see it's a challenge to achieve in a manner that works "universally" and raises the question of can and/or should express attempt provide this.

[hacksparrow]

@evanshortiss yes, I agree it is a reasonable request. It's just that the API is not very convincing or a lot of complexities will be involved (eg: detecting passthroughs). It would be great to have a really elegant solution.

👍 for "This information can be useful at runtime for logging", hadn't thought about it.

[danieljuhl]

@hacksparrow I think the purpose of this PR is to get the routes matched not the 'route handlers' matched. The purpose of this PR, IMHO, is to get the 'context', and not just the final route matched. I do aknowledge that it can look messy if you have a very complex route structure, but I can't see how you can make something messy, look elegant - at least not for now :)

[dougwilson]

Hi @danieljuhl which PR are you referring to? This is just an issue and references a few PRs, so just trying to get context on your statement.

[danieljuhl]

@dougwilson - sorry, PR #3100 as referenced by @hacksparrow

[dougwilson]

Gotcha. I was thinking that, @danieljuhl , but all the examples in the #3100 seem to indicate the intention of the PR is to build the final route matched, which seems to conflict what your description of your PR. Perhaps I'm just misunderstanding here?

[evanshortiss]

@dougwilson @danieljuhl correct, it's to get context. For me personally I'd like to construct a string that represents the matched URL. @hacksparrow has a nice solution here that would allow one to do what my PR does, without technically adding it as a feature to express.

[shivasurya]

any solution for this will make me happy ;)

[snawaz]

Another use case would be when supporting Prometheus metrics for all endpoints. To do that, one could use middleware functions at the app-level and use the originalRoute as value of one of the label, something like this:

app.use((req, res, next) => {
    requestsCount.inc({api: req.originalRoute, method: req.method});
});

Since req.url could have user-id, tokens, etc, in it, we cannot use it as the values as that way the number of time-series would be too many.. Not good. So I've kinda this workaround:

app.use((req, res, next) => {
    requestsCount.inc({api: normalize(req.url), method: req.method});
});

.. where normalize is a function which finds & replaces variable-values such as uuid, tokens, etc with something constant like - and just empty-string. It pointlessly consumes some CPU time. We could avoid paying for this if we have originalRoute, or originalPath (whichever sounds good).

[wesleytodd]

The router has been moved, and this would be a feature of the router. You can see what IMO is the best option for this on the Router repo: pillarjs/router#34

Any future conversation should be had over on that thread, as it is where we would make this kind of change.

[manvydasu]

So it's impossible to have metrics for "response times by endpoint" using express with the current implementation of router?