Store matched routes in request #34

ajfranzoia · 2016-02-13T20:20:30Z

I'm transplanting this PR from the original I made at expressjs/express#2864 (issue discussed at expressjs/express#2501).

It allows to store every matched route in a request when travelling through nested routes. This may be useful for logging, ACL checks, etc.
So, for example, if you have your routing set like this:

var fooRouter = new Router();
var barRouter = new Router();
var bazRouter = new Router();

bazRouter.get(['/bez', '/baz/:subId'], function(req, res, next) {
  next();
});

fooRouter.use(['/foo/:id', '/foe'], barRouter);
barRouter.use(['/bar'], bazRouter);

A request for the URL /foo/10/bar/baz/30 will set an array of matched routes in the request object like this:

req.matchedRoutes = ['/foo/:id', '/bar', '/baz/:subId'];

Let me know if there is anything that needs to be modification, unmeaningful variable names, etc.

wesleytodd

There hasn't been much discussion on this since the original thread, but I just wanted to mention that I would really like to see this feature land for the router before express@5.0.

wesleytodd · 2017-03-25T16:29:00Z

index.js

@@ -286,6 +286,11 @@ Router.prototype.handle = function handle(req, res, callback) {
        return next(layerError || err)
      }

+      // store matched routes
+      if (layer.path) {
+        req.matchedRoutes = (req.matchedRoutes || []).concat(layer.matchedPath.path)


This is going to create unnecessary arrays. Can you instead only create a new array the first time, then push to it after?

req. matchedRoutes = req.matchedRoutes || [] req.matchedRoutes.push(layer.matchedPath.path)

Fixed in https://github.com/pillarjs/router/pull/86/files#diff-168726dbe96b3ce427e7fedce31bb0bcR295

wesleytodd · 2017-03-25T16:30:17Z

lib/layer.js

-  if (path === '/' && opts.end === false) {
-    this.regexp.fast_slash = true
+  if (paths === '/' && opts.end === false) {
+    this.fastSlash = true


V8 can do optimizations if the shape of an object remains the same. This addition will have only some Layer's with a fastSlash property. To optimize this, you should define it as undefined above, or false here.

Fixed in https://github.com/pillarjs/router/pull/86/files#diff-be883fbed8968009576c828e0ff70e36R47

wesleytodd · 2017-03-25T16:30:52Z

lib/layer.js

  }

-  debug('new %s', path)
+  debug('new %s', paths)
  var opts = options || {}

  this.handle = fn
  this.name = fn.name || '<anonymous>'
  this.params = undefined
  this.path = undefined


Add a line for this.matchedPath = undefined, see comment below for reasoning.

Fixed in https://github.com/pillarjs/router/pull/86/files#diff-be883fbed8968009576c828e0ff70e36R43

wesleytodd · 2017-03-25T16:33:26Z

lib/layer.js

    return true
  }

-  var m = this.regexp.exec(path)
+  var checkPath, m;


All of the express modules are working toward compliance with standard. Which includes the rule that variable declarations are each on a single line. We have doing it piecemeal because it would cause too many conflicts otherwise, so since this is a new line, can you make the change just here?

Fixed in https://github.com/pillarjs/router/pull/86/files#diff-be883fbed8968009576c828e0ff70e36R121

wesleytodd · 2017-03-25T16:34:23Z

test/router.js

@@ -985,6 +985,31 @@ describe('Router', function () {
  })
 })

+describe('req.matchedRoutes', function() {


Same as comment above, can you make the formatting through here function () {? As per standard.

Should be fixed in https://github.com/pillarjs/router/pull/86/files#diff-c6bcc5b8d170df93e5b3223fa590f1f7R1133

wesleytodd · 2017-03-25T16:45:20Z

Also, maybe we should change the property name to matchedPaths. There is something called a Route, and it may be less ambiguous to call it this. Thoughts on that?

- set `__mountpath` in `.use()` - override `process_params` to add `req.matchedRoutes` `req.matchedRoutes` is a proposed addition to express v5: pillarjs/router#34 Doing this because in goodeggs-stats, we need to know what route we are currently on when we are publishing metrics to librato. In express v4 there is no mechanism to do this, if the routes have parameters in them (like :id, etc) or if they have nesting (given /foo/bar, the middleware for /foo has no knowledge of /bar and vice-versa).

GuskiS · 2019-11-11T10:01:02Z

Is there a plan to get this merged?

EgilsZ · 2019-11-13T14:00:10Z

Can someone please give any timeline on when this could be merged? Anything blocking this to be completed?

wesleytodd · 2019-11-14T17:11:23Z

Nothing blocking AFAIK, there are just higher priorities. If you are interested in helping out you can take a look at these and start participating in the project:

https://github.com/expressjs/express/blob/master/Contributing.md
expressjs/express#4055

More people helping collaborate and traige issues would unblock the core team and allow us to spend more time on moving these features forward. Thanks in advance for your help!

saasen · 2019-12-05T12:22:15Z

I would love this feature, as we're trying to expose Prometheus metrics based on the resolved route, rather than the incoming URL and normalizing it (which is very error prone).

wesleytodd · 2019-12-05T16:53:34Z

@saasen, my original interest in this feature was the exact same as your use case. If you are looking for a good solution which is simple and does not have many edge cases, you can do what I did in that app:

const onFinished = require('on-finished')
function routeMetrics (route) {
  return [route, (req, res) => {
    onFinished(res, (err) => {
      reportMetric(err, route)
    })
  }]
}

app.get(routeMetrics('/:foo'), routeHandlerMiddleware)

The only thing to be aware of with a solution like this is that if a request would match two routes and both get run, both would report. You can prevent this if it is a concern for your app by setting a flag on res like res.locals.route = route and then reference that off of res inside the onFinished handler function. Make sense?

saasen · 2019-12-05T16:57:23Z

@wesleytodd: Would this work when defining routes inside a Router as well? AFAIK, the route resolved inside a Router doesn't contain the "parent"-route.

dougwilson · 2019-12-05T16:57:38Z

For the folks who keep asking when it will be merged / what is blocking: there are open review issues at the top that haven't been addressed yet.

wesleytodd · 2019-12-05T17:07:36Z

@saasen Hm, good question. Without actually trying that I am guessing no and that you would need to use a whole express app which has a mountpath. Maybe there is another way I am missing right now.

@dougwilson Oh yeah, I guess I did open a review! I wonder if @ajfranzoia is interested in making those updates? If not I might be able to find some time soon to take this over and clean it up.

gabegorelick · 2020-01-03T03:32:08Z

I rebased these changes into #86. But it's mostly unchanged from this PR.

gabegorelick · 2020-01-03T03:45:37Z

Sorry for the noise, but wanted to mark off all of the feedback from @wesleytodd to ensure that it's incorporated into #86.

closes pillarjs#34 closes pillarjs#85

dougwilson · 2020-03-24T13:16:18Z

Closing this in favor of the progress being made in #86

Store matched routes in request

0c77b88

dougwilson added the pr label Feb 18, 2017

wesleytodd requested changes Mar 25, 2017

View reviewed changes

wesleytodd mentioned this pull request Mar 25, 2017

Add Router events #49

Closed

haysmike mentioned this pull request Aug 11, 2017

Provide req.matchedRoutes to middlewares goodeggs/secure-router#7

Merged

wesleytodd mentioned this pull request Nov 22, 2017

Allow for identifying 404 vs. 405 #62

Closed

wangfengming mentioned this pull request Dec 20, 2017

handle 405 expressjs/express#3513

Closed

wesleytodd mentioned this pull request Jul 24, 2019

Add originalRoute like originalUrl expressjs/express#2501

Open

wesleytodd mentioned this pull request Nov 4, 2019

Provide access to original unparsed path #85

Open

gabegorelick mentioned this pull request Jan 3, 2020

Store matched routes in request #86

Open

gabegorelick pushed a commit to gabegorelick/router that referenced this pull request Jan 5, 2020

feat: store matched routes in request

f2f59a3

closes pillarjs#34 closes pillarjs#85

gabegorelick pushed a commit to gabegorelick/router that referenced this pull request Jan 5, 2020

feat: store matched routes in request

a0afed7

closes pillarjs#34 closes pillarjs#85

gabegorelick pushed a commit to gabegorelick/router that referenced this pull request Jan 6, 2020

feat: store matched routes in request

05babdf

closes pillarjs#34 closes pillarjs#85

dougwilson closed this Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store matched routes in request #34

Store matched routes in request #34

ajfranzoia commented Feb 13, 2016

wesleytodd left a comment

wesleytodd Mar 25, 2017

gabegorelick Jan 3, 2020

wesleytodd Mar 25, 2017

gabegorelick Jan 3, 2020

wesleytodd Mar 25, 2017

gabegorelick Jan 3, 2020

wesleytodd Mar 25, 2017

gabegorelick Jan 3, 2020

wesleytodd Mar 25, 2017

gabegorelick Jan 3, 2020

wesleytodd commented Mar 25, 2017

GuskiS commented Nov 11, 2019

EgilsZ commented Nov 13, 2019

wesleytodd commented Nov 14, 2019

saasen commented Dec 5, 2019 •

edited

Loading

wesleytodd commented Dec 5, 2019

saasen commented Dec 5, 2019

dougwilson commented Dec 5, 2019

wesleytodd commented Dec 5, 2019

gabegorelick commented Jan 3, 2020

gabegorelick commented Jan 3, 2020

dougwilson commented Mar 24, 2020

Store matched routes in request #34

Store matched routes in request #34

Conversation

ajfranzoia commented Feb 13, 2016

wesleytodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd commented Mar 25, 2017

GuskiS commented Nov 11, 2019

EgilsZ commented Nov 13, 2019

wesleytodd commented Nov 14, 2019

saasen commented Dec 5, 2019 • edited Loading

wesleytodd commented Dec 5, 2019

saasen commented Dec 5, 2019

dougwilson commented Dec 5, 2019

wesleytodd commented Dec 5, 2019

gabegorelick commented Jan 3, 2020

gabegorelick commented Jan 3, 2020

dougwilson commented Mar 24, 2020

saasen commented Dec 5, 2019 •

edited

Loading