Store matched routes in request #86

gabegorelick · 2020-01-03T03:27:27Z

Lightly rebased version of #34 with the fixes suggested by @wesleytodd.

dougwilson

This is just a placeholder to remember to incorporate the review comments from PR #34 into the changes in this PR.

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

This is just a placeholder to remember to incorporate the review comments from PR #34 into the changes in this PR.

@dougwilson I think I got everything, but let me know if I missed anything.

dougwilson · 2020-01-03T03:33:15Z

Hi @gabegorelick just a glance through I don't think you got #34 (comment) ? If you want me to check every one of them one by one, though, please give me a few days to get back on the complete list :)

lib/layer.js

gabegorelick · 2020-01-03T03:38:39Z

just a glance through I don't think you got #34 (comment)

Thanks, I actually just fixed that. (I guess GitHub didn't display that under Files Changed since it was technically a comment on existing code?)

dougwilson · 2020-01-03T03:40:19Z

I guess GitHub didn't display that under Files Changed since it was technically a comment on existing code?

I'm not sure, I see it under Files Changed on my end. I wonder if you're not seeing that, what other review comments where missed? We'll definately want to go through them all one-by-one to be sure if that's the case here.

dougwilson · 2020-01-03T03:41:52Z

test/router.js

@@ -1129,6 +1129,31 @@ describe('Router', function () {
      .expect(200, 'saw GET /bar', done)
    })
  })
+
+  describe('req.matchedRoutes', function () {


One test doesn't seem adequate for this feature. Here are a few more tests cases I can think of off-hand:

Check that it works for regexp-defined routes

Check that is works as expected with sub routers

Yeah, I tend to agree. I'll try to add more tests.

Updated with some more tests. Let me know if that's what you had in mind. They don't totally match the style of the existing code, so let me know if it should be changed.

Nice! Can you add a test for what req.matchedRoutes looks like in the top router after exiting the sub router as well?

@dougwilson I think that's failing. I added a test that looks like this:

var router = new Router() var fooRouter = new Router() var server = createServer(router) var matchedFooRoutes var matchedBarRoutes router.use('/foo', function (req, res, next) { matchedFooRoutes = req.matchedRoutes next() }, fooRouter) fooRouter.get('/bar', function (req, res, next) { matchedBarRoutes = req.matchedRoutes next() }) router.use(saw) request(server) .get('/foo/bar') .expect(200, 'saw GET /foo/bar', function (err, res) { // matchedFooRoutes is [ '/foo', '/foo', '/bar' ] instead assert.deepEqual(matchedFooRoutes, ['/foo']) done(err) })

I'll see if I can fix it.

Fixed with my most recent commit, but not totally sure that's the right way to go. Let me know what you think.

index.js

lib/layer.js

gabegorelick · 2020-01-03T03:49:03Z

I'm not sure, I see it under Files Changed on my end. I wonder if you're not seeing that, what other review comments where missed?

User error on my part. I went through the changes again and marked them off. The only thing that should be missing is #34 (comment) ("should change the property name to matchedPaths") since I wasn't sure whether there was consensus for that yet.

dougwilson · 2020-01-03T03:53:29Z

lib/layer.js

+        this.matchedPath = checkPath
+        break
+      }
+    }
  }

  if (!match) {


I think this.matchedPath should be reset inside this conditional as well, otherwise it will look like there is a matched path when none of the paths matched, but they did in a previous attempt.

Updated. Is there a way to test this?

There should be by creating a specific order of routes and running though them. I can help make one if you need, perhaps this weekend

dougwilson · 2020-01-03T03:55:45Z

Hi @gabegorelick I appreciate the entheuseam in getting updated pushed, rock on 👍 the down side is that GitHub UI looses comments I am in the middle of writing when the code changes underneath me :( I need to go get dinner, so I will let you make changes for now and come back to continue to review later when we're not both altering the PR at the same time :)

gabegorelick · 2020-01-03T04:04:50Z

Sorry about that. I may need a day or two to add the tests anyway, so there should be more quiet time on this PR :)

wesleytodd · 2020-01-03T16:11:42Z

Hey! Thanks so much for picking this up @gabegorelick! I was planning on spending some time cleaning up many of the pending things in the router this weekend (time permitting), and that was on my list. I will take a look at your changes soon, but if possible it would be awesome if we could get this landed before the next 2.x pre-release.

I have a list of PRs and issues I would like to resolve, and if @dougwilson is on board with it I would like to land them all this weekend. I will pull them all into a 2.0.0-alpha.2 branch so that we can test and review. If all works out hopefully we can have the next alpha published with the next express@5.0.0-alpha.8 release in preparation for that major release (later this month hopefully).

gabegorelick · 2020-01-03T16:54:08Z

@wesleytodd That's great news! I'll try to address all feedback this weekend, but feel free to do whatever you need to do to this PR to land it.

gabegorelick · 2020-01-04T23:03:12Z

index.js

@@ -166,7 +166,8 @@ Router.prototype.handle = function handle(req, res, callback) {
  // manage inter-router variables
  var parentParams = req.params
  var parentUrl = req.baseUrl || ''
-  var done = restore(callback, req, 'baseUrl', 'next', 'params')
+  var parentMatchedRoutes = req.matchedRoutes
+  var done = restore(callback, req, 'baseUrl', 'next', 'params', 'matchedRoutes')


It seems like adding matchedRoutes is necessary here, but no tests fail. So we should either try to add a test for it, or remove it if it's not necessary.

It should be apparent once more tests are added with sub routers. This controls the behavior of what happens on a router exit.

wesleytodd · 2020-01-05T18:56:18Z

Also looks like it needs a lint fix due to the update to eslint which this is now rebased on top of. @gabegorelick, if you have time to knock this stuff out that would be awesome. If not, I will try to take a look at it this week.

dougwilson · 2020-01-05T18:59:53Z

test/router.js

+        .get('/foo/bar')
+        .expect(200, 'saw GET /foo/bar', function (err, res) {
+          assert.deepEqual(matchedFooRoutes, ['/foo'])
+          assert.deepEqual(matchedBarRoutes, ['/foo', '/bar'])


Do we think it makes sense for the matched foo routes to contain paths that do not even exist on the foo router? What if the foo router had a foo path, would this mean it had a match? That brings up a question for how this works with sub routers mounted at a path that is no / as well.

I like to think about the use case where this feature would be used, and if you do not get all matched routes at the end it seems much less useful. That said, your counter example makes it clear that what we have here is not correct either.

dougwilson · 2020-01-05T19:05:20Z

I made a comment above when looking at a test, but it made me think of an even bigger question I think we should think about:

This PR currently add a req.matchedRoutes property that is an array value of... something. It seems to be strings, but sometimes regular expressions. Sometimes it is still undefined according to the tests.

My question is what does this array represent? It is called "routes" but is "/foo" a "route"? Or just a pathname? Like should there be a difference if it was a GET or a POST or should it not matter? When the value is a regular expression, what does that mean, though? Is that useful data for the use case this is being designed for?

wesleytodd · 2020-01-05T19:09:10Z

Is that useful data for the use case this is being designed for?

I think it is useful in that it could be stringified and reported on as part of the metrics. I do think the nested router behavior is not good, but the main use case I have thought of for this is to report different metrics depending on which routes matched in the processing of the request. And for this use case all simple examples are really straight forward.

dougwilson · 2020-01-05T19:11:11Z

Gotcha. So should it be called like "matchedPathnames"? Seems more like what it would be vs "routes". Usually routes include what the method is as part of "route"?

dougwilson · 2020-01-05T19:15:18Z

P.S. what do we think the array should contain for a request to "GET /foo" given the following?

router.get('/foo', (req, res, next) => next())
router.get('/foo', (req, res) => ...)

Should be be two /foo or just one? Probably could add a test for this as well.

HISTORY.md

wesleytodd · 2020-01-05T19:17:09Z

So I think in all reality this conversation points to one of the long existing design flaws in the router. There are other approaches we could take to the internals which would still allow similar user facing API but not incur the issues we have with dynamic and arbitrarily deep trees. I don't think we should shoot for fixing this in 2.0, but I think a new approach for what could be 3.0 should be considered.

gabegorelick · 2020-01-05T19:18:02Z

lint fix due to the update to eslint which this is now rebased on top of. @gabegorelick, if you have time to knock this stuff out that would be awesome. If not, I will try to take a look at it this week.

Fixed the linting. Otherwise I'm staying quiet while all the other questions get sorted out :)

gabegorelick · 2020-01-05T19:42:18Z

what do we think the array should contain for a request to "GET /foo" given the following?

That should be [ '/foo' ]. I've added a failing test to cover that example. I'll try to get it passing.

gabegorelick · 2020-01-05T19:46:29Z

So should it be called like "matchedPathnames"

What about matchedPaths? That would align with req.path. The "name" in "pathname" to me also implies it would always be a string, while "path" is more abstract and so allows for things like regexes.

dougwilson · 2020-01-05T20:00:02Z

What about matchedPaths? That would align with req.path. The "name" in "pathname" to me also implies it would always be a string, while "path" is more abstract and so allows for things like regexes.

The word "path" and "pathname" are part of the URL syntax. The difference being "path" contains the query string and "pathname" does not. This module has no req.path at all, so there isn't any kind of alignment that would make sense in the context of this module. Node.js also does not have a req.path so, for example, when using this module against all the examples in the README here and tests, there is still nothing that would make sense in the "alignment" argument.

dougwilson · 2020-01-05T20:03:26Z

index.js

@@ -199,6 +200,7 @@ Router.prototype.handle = function handle(req, res, callback) {
      req.baseUrl = parentUrl
      req.url = protohost + removed + req.url.substr(protohost.length)
      removed = ''
+      req.matchedRoutes = parentMatchedRoutes


It seems like this is trying to restore the req.matchedRoutes back to what it used to be, but since the router is just directly manipulating the array, it still ends up containing the new additions that were made. I'm not 100% sure what the purpose of this restoration of the property is meant to do after every route call?

@dougwilson This whole approach needs to be tweaked anyway to to deal with the "sibling" routes example you mentioned in #86 (comment).

I'm open to suggestions for the best way to deal with the matchedRoutes "stack". I don't know enough yet about the router internals to know the best place to push and pop from it.

Well, due to the lack of documentation here or a full written explanation for how this all should work, I really have no idea what to suggest for the changes, because I'm still unclear how this feature is actually supposed to operate :) Maybe we should circle back to explaining exactly how this feature should work in text form, so then we can all help on the implementation by knowing what we're trying to achieve. Right now, I'm still unclear on what exactly the plain is, so don't have a way to suggest what changes to make to the code to get the outcome to be whatever it is it should be, if that makes sense.

@dougwilson Apologies. This PR has moved a number of times between expressjs and pillarjs, so much of the original context may have been lost.

AFAIK, this all got started with expressjs/express#2501 requesting req.originalRoute to complement req.originalUrl. The use case provided there is for logging route names instead of fully resolved paths, which is the same as what I need. Another use case I've seen is for recording metrics. E.g. when recording how many times an endpoint like GET /users/:id is called, you may want to use the full name of the endpoint without path parameters substituted.

As for the technical decisions in this PR, I can't speak to most of them. The original author (@ajfranzoia) seems to have abandoned his efforts, and I'm merely picking what he wrote back up. Perhaps @wesleytodd can provide more insight?

I assume req.originalRoute morphed into an array of matchedRoutes when it was realized that route names aren't always strings and that it wasn't as simple as joining path segments together.

gabegorelick · 2020-01-05T20:08:12Z

The word "path" and "pathname" are part of the URL syntax.

Fair enough. If there's consensus for matchedPathnames, I'll update it to use that.

dougwilson · 2020-01-05T20:09:50Z

lib/layer.js

+  this.fastStar = paths === '*'
+  this.fastSlash = paths === '/' && opts.end === false
+
+  this.paths = (!Array.isArray(paths) ? [paths] : paths).map(function (path) {


So I assume that the purpose of pre-expanding the array of paths is to store data long with them? If that is the case path-to-regexp supports arbitrarily-nested arrays like ["/foo",["/bar","/fizz"]] and this implementation would end up only accounting for the first level.

One of the big changes in this PR is to always call path-to-regexp with a single path segment instead of an array of paths. That allows us to identify the matching path element in the array of paths (it's the one that returns a match from the regex). But as you point out, that won't work for deeply nested arrays.

Perhaps a better approach is to use the return value of the regex to indicate which element in the array was the match. I'm not totally sure what that would look like for the deeply nested array case, or if that's feasible without making breaking changes to path-to-regexp, but it would allow us to avoid pre-expansion of the array here.

Is ["/foo",["/bar","/fizz"]] equivalent to ["/foo", "/bar", "/fizz"]? If it is, then recursively flattening the array of paths should be fine.

However, another issue is if evaluating multiple regexes hurts performance compared to evaluating a single regex with clauses separated by |. Is that something to worry about?

Is ["/foo",["/bar","/fizz"]] equivalent to ["/foo", "/bar", "/fizz"]? If it is, then recursively flattening the array of paths should be fine.

Currently this module delegates the argument to path-to-regexp and I don't know the answer off-hand and would have to look. If you're not even sure, then I think the entire way this functions needs to be re-examined, as if the code here is making assumptions for how path-to-regexp works, we should have a good grasp on how it is working.

However, another issue is if evaluating multiple regexes hurts performance compared to evaluating a single regex with clauses separated by |. Is that something to worry about?

It would probably depending on how much of a performance penalty it has. Typically adding features has a performance impact, but I it is really large, we may want to gate the feature. It would depend on (1) what % of users are going to use the feature and (2) how big of a penalty it has. For example, if 99% of users will use it, then it probably doesn't matter. But if only like 10% of users will use it and it will hurt performance by 20%, maybe a gate would be in order.

Currently this module delegates the argument to path-to-regexp and I don't know the answer off-hand and would have to look. If you're not even sure, then I think the entire way this functions needs to be re-examined, as if the code here is making assumptions for how path-to-regexp works, we should have a good grasp on how it is working.

A quick test of path-to-regexp indicates they are functionally equivalent (the only difference being an extra non-capturing group wrapper for each level of nesting):

pathToRegexp(['/', '/']) // /(?:^\/(?:\/)?$|^\/(?:\/)?$)/i pathToRegexp(['/', ['/']]) // /(?:^\/(?:\/)?$|(?:^\/(?:\/)?$))/i

Looking at the source, path-to-regexp does appear to recursively expand the array, so we should be able to do the same before calling it.

But if only like 10% of users will use it and it will hurt performance by 20%, maybe a gate would be in order.

Any theoretical hit to performance should only affect the use of arrays. So that should help.

Looking at the source, path-to-regexp does appear to recursively expand the array, so we should be able to do the same before calling it.

Be carful to look at the proper version of the module :) Your link is about version 6.1.0 module, several major versions ahead of what his module is actually using.

Good catch, although looks like the implementation hasn't changed since v3.0.0.

I've updated the PR to call array-flatten on the paths.

dougwilson · 2020-01-05T20:12:10Z

Fair enough. If there's consensus for matchedPathnames, I'll update it to use that.

TBF, I'm not saying there is consensus or you have to make a change. I am throwing things out for discussion. Maybe I'm unclear of the roles of the folks involved in the conversation :) Typically the PR author is the champion of the change, and is invested it in landing and has an explicit use-case they need in mind. So I am throwing points out for discusion with the author of the PR to get feedback and their own thoughts back, haha.

gabegorelick · 2020-01-05T20:23:43Z

Typically the PR author is the champion of the change, and is invested it in landing and has an explicit use-case they need in mind. So I am throwing points out for discusion with the author of the PR to get feedback and their own thoughts back, haha.

Many people have touched this PR, and my contributions are fairly minimal. I'm mostly just trying to get it over the finish line, so I don't have strong opinions. My use case is pretty much identical to what's been outlined in #34, #85, and expressjs/express#2501. As long as the final form of this PR accomplishes it's original goal, I'm fine no matter what it looks like :)

closes pillarjs#34 closes pillarjs#85

mmacai · 2020-03-10T10:58:53Z

Definitely useful feature, any update on this?

gabegorelick · 2020-03-10T14:53:06Z

Definitely useful feature, any update on this?

This PR works for simple cases, but there are a ton of edge cases that make this feature complicated (see the failing test, for instance). I sort of ran out of steam, but happy to help anyone else that wants to pick this up.

wesleytodd · 2024-03-16T18:48:17Z

Hey, I am finally getting around to cleaning this all up. Are you interested in trying to help get this landed for router@2 and express@5? If so please respond asap so we can get it accounted for in the plan.

dougwilson suggested changes Jan 3, 2020

View reviewed changes

dougwilson added enhancement pr needs docs labels Jan 3, 2020

gabegorelick mentioned this pull request Jan 3, 2020

Store matched routes in request #34

Closed

gabegorelick force-pushed the store-matched-routes branch from a08a2b6 to d337fc5 Compare January 3, 2020 03:35

dougwilson reviewed Jan 3, 2020

View reviewed changes

lib/layer.js Show resolved Hide resolved

dougwilson reviewed Jan 3, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

dougwilson reviewed Jan 3, 2020

View reviewed changes

lib/layer.js Outdated Show resolved Hide resolved

gabegorelick force-pushed the store-matched-routes branch from d337fc5 to ec00dba Compare January 3, 2020 03:51

dougwilson reviewed Jan 3, 2020

View reviewed changes

gabegorelick force-pushed the store-matched-routes branch from ec00dba to 3151f57 Compare January 3, 2020 03:53

gabegorelick force-pushed the store-matched-routes branch from 3151f57 to 0f5ceca Compare January 3, 2020 16:35

gabegorelick changed the base branch from master to 2.0 January 3, 2020 16:38

gabegorelick force-pushed the store-matched-routes branch from 0f5ceca to c3df1fb Compare January 3, 2020 16:40

gabegorelick commented Jan 4, 2020

View reviewed changes

wesleytodd added 4 commits January 5, 2020 09:40

build: disable package-lock.json in .npmrc

0637034

deps: mocha@7.0.0

a0f0c55

deps: supertest@4.0.2

2d84b21

deps: nyc@15.0.0

6e57d66

dougwilson reviewed Jan 5, 2020

View reviewed changes

gabegorelick force-pushed the store-matched-routes branch from 5e8a56e to 865c55e Compare January 5, 2020 19:12

dougwilson reviewed Jan 5, 2020

View reviewed changes

HISTORY.md Outdated Show resolved Hide resolved

gabegorelick force-pushed the store-matched-routes branch 2 times, most recently from f2f59a3 to a0afed7 Compare January 5, 2020 19:41

dougwilson reviewed Jan 5, 2020

View reviewed changes

feat: store matched routes in request

05babdf

closes pillarjs#34 closes pillarjs#85

gabegorelick force-pushed the store-matched-routes branch from a0afed7 to 05babdf Compare January 6, 2020 03:57

dougwilson changed the base branch from 2.0.0-alpha.2 to 2.0 March 13, 2020 04:46

dougwilson mentioned this pull request Apr 15, 2020

expose the matched routes for a request expressjs/express#3100

Closed

jonchurch added the help wanted label Apr 18, 2020

wesleytodd mentioned this pull request Sep 18, 2020

Use diagnostics_channel in layers #96

Draft

wesleytodd added the express-v5 label Mar 16, 2024

Store matched routes in request #86

Are you sure you want to change the base?

Store matched routes in request #86

Conversation

gabegorelick commented Jan 3, 2020

dougwilson left a comment

Choose a reason for hiding this comment

gabegorelick commented Jan 3, 2020

dougwilson commented Jan 3, 2020

gabegorelick commented Jan 3, 2020

dougwilson commented Jan 3, 2020

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

gabegorelick commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Jan 3, 2020

gabegorelick commented Jan 3, 2020

wesleytodd commented Jan 3, 2020

gabegorelick commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd commented Jan 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougwilson commented Jan 5, 2020

wesleytodd commented Jan 5, 2020

dougwilson commented Jan 5, 2020

dougwilson commented Jan 5, 2020

wesleytodd commented Jan 5, 2020

gabegorelick commented Jan 5, 2020

gabegorelick commented Jan 5, 2020 • edited

gabegorelick commented Jan 5, 2020 • edited

dougwilson commented Jan 5, 2020

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

gabegorelick commented Jan 5, 2020

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

dougwilson commented Jan 5, 2020

gabegorelick commented Jan 5, 2020

mmacai commented Mar 10, 2020

gabegorelick commented Mar 10, 2020

wesleytodd commented Mar 16, 2024 • edited

wesleytodd commented Jan 5, 2020 •

edited

gabegorelick commented Jan 5, 2020 •

edited

gabegorelick commented Jan 5, 2020 •

edited

wesleytodd commented Mar 16, 2024 •

edited