Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to get route name of parent route? #2879

Closed
watson opened this issue Feb 4, 2016 · 23 comments
Closed

How to get route name of parent route? #2879

watson opened this issue Feb 4, 2016 · 23 comments

Comments

@watson
Copy link

watson commented Feb 4, 2016

Goal

For each incoming request I'd like to log the name of the route that matches the request. So given the request GET /users/42 which matches the app.use('/users/:id', ...) route handler I'd like to get the name /users/:id.

Partial solution

I originally used the route property on the request object to extract the route name:

// given a request to `/users/42`, the routeName below would be `/users/:id`
var routeName = req.route.path || req.route.regexp && req.route.regexp.source

Problem

But the above solution doesn't work for nested routes, e.g:

// create sub-app
var admin = express.Router()
admin.get('/users/:id', require('./admin/user'))

// create main app
var app = express()
app.get('/users/:id', require('./user'))
app.use('/a(dmin)?', admin) // mount sub-app under "/admin" and "/a"

When mounting a route like in the above example, the req.route.path property will not contain the parent route. So if someone requests GET /admin/users/42 then req.route.path would just contain /users/:id and not /a(dmin)?/users/:id.

Is there any way inside a route handler to figure out that this was routed through a parent route with the name /a(dmin)??

I first thought to use req.baseUrl, but that will give me the "resolved" path, so I'd get either /a or /admin depending on which actual URL the user typed - I'd like to get the generic route name instead: /a(dmin)?

If this is not possible at all in v4, it would be cool if this could be supported in v5 😃

@wesleytodd
Copy link
Member

I was actually looking at this exact feature recently, so we could stat the routes getting hit. We ended up just passing an identifier to the last middleware where we knew its "final form". We did this because the router/route do now know about mounting.

It would be a relatively simple task to make the req.route property into an array. And then instead of re-assigning the route, push onto the array. That would be a breaking change and probably need to go on the v5 router. If you open this issue over there I would love to discuss the other similar features that would be great for this (ie. named routes). And I could probably even whip up the PR if you weren't already interested in doing it.

@tunniclm
Copy link
Contributor

tunniclm commented Feb 4, 2016

Related to #2501

@wesleytodd
Copy link
Member

Haha, @tunniclm that PR is exactly what I just said. Thanks!

EDIT: #2864 <- This pull request linked in the bottom of the #2501

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd @tunniclm Thanks for your feedback - nice to see that I'm not the only one who've been struggling with this. I guess I'll have to monkey patch my way out of this for the time being and wait for proper support to land in Express v5 😃

@watson watson closed this as completed Feb 4, 2016
@wesleytodd
Copy link
Member

@watson NP! Also might want to take a look at using req.app.path(). You might be able to do something like:

var route = req.app.path() + (req.route.path || req.route.regexp && req.route.regexp.source);

Haven't tested that, but it might solve your issue.

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd It just returns an empty string in my case. It seems to depend on the req.app.parent property being set which apparent isn't set for me

@wesleytodd
Copy link
Member

It does return an empty string when NOT in a mounted app. But when you mount like in your OP you should get the mount path. Is that not working?

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd Seems not to work in my app.

I've made a simple express app showing it. The app in the linked gist will log the following if GET /foo is requested:

GET /foo
--> req.path /
--> req.baseUrl /foo
--> req.app.parent undefined
--> req.app.mountpath /
--> req.app.path() 
sub

Update: The string I'm looking to extract is /foo(bar)? which is the route under which the sub-app is mounted

@wesleytodd
Copy link
Member

Looks like it doesnt work with just a router instance, and only with a full express app instance. I pulled down your gist and changed express.Router() to express() and got this output:

node index.js
GET /
--> req.path /
--> req.baseUrl
--> req.app.parent undefined
--> req.app.mountpath /
--> req.app.path()
root
GET /favicon.ico
GET /foo
--> req.path /
--> req.baseUrl /foo
--> req.app.parent function (req, res, next) {
    app.handle(req, res, next);
  }
--> req.app.mountpath /foo(bar)?
--> req.app.path() /foo(bar)?
sub

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd Interesting. Thanks for clearing that up. Unfortunately I need it to work for instances of express.Router() as well 😞

@wesleytodd
Copy link
Member

Yeah, this clearly seems like something worth changing in express and the router. I think opening up an issue on the router is still a good idea.

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd Ah I see. The reason why the parent and mountpath isn't set when using express.Router() is because it's not seen as a proper Express app.

I've just been looking through the code and as far as I can see the path given as the 1st argument to the use function isn't stored anywhere on any property - so it's not accessible in it's raw form later. It ends up in the Layer constructor where it's just passed into the path-to-regexp module and then forgotten. The Layer constructor is called from the use function on the router - but here it's just discarded as well as far as I can see.

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd wouldn't it maybe be ok to check if the sub-app was an instance of the express.Router and set its parent and mountpath just as if it was a proper express app? Or would that be a breaking change if someone depends on those being empty in that case?

@wesleytodd
Copy link
Member

That behavior seems accurate to what I know. This might be a question for @dougwilson or @jasnell for the breaking change question.

@wesleytodd
Copy link
Member

I was at lunch when i answered last, so I didn't want to type too much. But now that I am back, I think that technically this is not a breaking change. But any change to a public api has fallout that can be un-anticipated, so maybe patching it in your application and then applying the changes to v5 would be best.

To patch it in your app all you need to do is override app.use and fix these lines: https://github.com/strongloop/express/blob/master/lib/application.js#L218-L220

@watson
Copy link
Author

watson commented Feb 4, 2016

@wesleytodd To prevent having to replicate the entire app.use function in my own code, I found a working monkey patch solution that "only" requires me to:

First shim the express.Router.use function:

var origUse = express.Router.use

express.Router.use = function (fn) {
  if (typeof fn === 'string' && Array.isArray(this.stack)) {
    var offset = this.stack.length
    var result = origUse.apply(this, arguments)
    var layer
    for (; offset < this.stack.length; offset++) {
      layer = this.stack[offset]
      // I'm not sure if my check for `fast_slash` is the way to go here
      // But if I don't check for it, each stack element will add a slash to the path
      if (layer && layer.regexp && !layer.regexp.fast_slash) layer.__mountpath = fn
    }
    return result
  } else {
    return origUse.apply(this, arguments)
  }
}

Then shim the express.Router.process_params function to take advantage of the new layer.__mountpath property:

var origPP = express.Router.process_params

express.Router.process_params = function (layer, called, req, res, done) {
  var path = req.route && (req.route.path || req.route.regexp && req.route.regexp.source) ||
    layer.__mountpath ||
    ''

  req.__route = (req.__route || '') + path

  return origPP.apply(this, arguments)
}

This will add a req.__route property containing the entire route name with both parent and child route names.

@wesleytodd
Copy link
Member

Nice! Certainly monkey patching is not fun in the long run, but hopefully we can add this in 5 and you can switch over sometime soon :)

@mlick
Copy link

mlick commented Jun 28, 2016

@watson That work for me! Thank you very much!

@jagadish-kb
Copy link

@watson The above works! Thank you.
Additionally, I found a bug as well. In case the middleware function throws an error and if we have an error handler configured, then the last path is appended several times.
for ex: if you have something like below:
router.use('/mountpath', anotherRouter);
router.use(middlewareErrorHandler);
router.use(middlewareErrorHandler2);

In the above case if my middleware against '/mountpath' throws an error, then inside 'middlewareErrorHandler' I would see __route with '/mountpath' duplicated once and in the next error handler, i see it twice. Am not sure the right way to fix this, but i put the below fix (ofcse the below will be erroneous if someone's URL naming scheme has duplicated path, but not sure if someone would do that :)). Nevertheless would appreciate if there is a better way, please advise.

express.Router.process_params = function (layer, called, req, res, done) {
    const path = req.route && (req.route.path || req.route.regexp && req.route.regexp.source) ||
      layer.__mountpath || '';
    if (req.__route && path) {
      const searchFromIdx = req.__route.length - path.length;
      if (req.__route.indexOf(path, searchFromIdx) > 0) {
        return origPP.apply(this, arguments);
      }
    }
    req.__route = (req.__route || '') + path;

    return origPP.apply(this, arguments);
  };

@OsoianMarcel
Copy link

Hello!
Any news related to this feature?
Thank you .

@asheba
Copy link

asheba commented Feb 4, 2021

Another approach that avoids the monkey-patching issue is the following:

const reverseMapParamValuesToKeys = (params) => Object.entries(params).reduce(
  (map, [k, v]) => ({ ...map, [v]: map[v] ? ':param' : `:${k}` }),
  {},
);

...
const paramValuesToKeys = reverseMapParamValuesToKeys(req.params);
return (req.originalUrl || '').split('/').map((part) => paramValuesToKeys[part] || part).join('/').split('?')[0];

Notice that conflict resolution (i.e two params that were resolved to the same value) can be implemented in whichever way you choose. Since we use uuids in our app, this should never happen in our routes. This is why we decided to resolve this conflict using :param to indicate something weird happened. You should change this part of the implementation to choose first/last or whichever conflict resolution you prefer.

@geekhunger
Copy link

Another approach that avoids the monkey-patching issue is the following:

const reverseMapParamValuesToKeys = (params) => Object.entries(params).reduce(
  (map, [k, v]) => ({ ...map, [v]: map[v] ? ':param' : `:${k}` }),
  {},
);
const paramValuesToKeys = reverseMapParamValuesToKeys(req.params);
return (req.originalUrl || '').split('/').map((part) => paramValuesToKeys[part] || part).join('/').split('?')[0];

@asheba Wow! Thank so so much for this! You saved my day! :)
It took me a already to much time to work on a solution for my implementation of multilingual urls. Your code will definitely help me further.

@LfxB
Copy link

LfxB commented Sep 15, 2022

Thank you all for the various solutions! It helped guide me to this alternative which also avoids monkey-patching, in addition to not requiring setting mergeParams: true for any inner Routers, and avoids naming conflicts.
Tested on Express v4.17.1.

// Middleware callback to provide to .use before Routes are defined
const logMatchedRoute = (req, res, next) => {
  // Add getter and setter for req.params
  Object.defineProperty(req, 'params', {
    get() {
      return this.__params;
    },
    set(params) {
      // If __matchroute is empty, set as full URL (except ? queries)
      if (!this.__matchroute) {
        this.__matchroute = this.baseUrl + this.path;
      }
      // Loop through params and replace the first matching value with :{param_name}
      for (const key in params) {
        if (Object.hasOwnProperty.call(params, key)) {
          const val = params[key];
          this.__matchroute = this.__matchroute.replace(`/${val}`, `/:${key}`);
        }
      }
      this.__params = params;
    }
  });

  // Log matched route when response is closed
  res.on('close', () => {
    console.log('__matchroute', req.__matchroute);
  });

  next();
};

@expressjs expressjs locked as resolved and limited conversation to collaborators Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants