expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.48k stars 16.05k forks source link

Feature request: Add a custom event `render` (due `req.route` & routing middleware nature) #2474

Open diosney opened 9 years ago

diosney commented 9 years ago

Related links

https://github.com/strongloop/express/issues/2093 http://stackoverflow.com/a/19460598/881286

A practical and real application of req.route would be one that you have

At the view, it would be nice to do a conditional on a route definition instead of the actual url path.

To acomplish that right now it is needed to add a local variable in each route middleware definition, whis is a cumbersome task:

    router.route('/users/:id').get(function (req, res) {
      res.locals.route = req.route;
      res.render('users', {});
    });

Real deal

A nice way to overcome this would be having a general middleware that do it in a single place, but the issue is that req.route only gets filled when the routing middleware gets executed, so there is no way to code a separated middleware that gets executed AFTER req.route is defined and BEFORE the render method.

Conclusion

If an Expressjs custom event existed, that gets executed just after the render method gets executed, it could be used to overcome this issue:

app.use(function (req, res, next) {
    res.on('render', function(){
         res.locals.route = req.route;
    });
    next();
});

Thanks

dougwilson commented 9 years ago

I'm leaving this open for discussion and to think about, but the obvious solution that would work right now if you needed it would be the following (so you're not blocked on discussions here):

app.use(function (req, res, next) {
  res.render = createRouteRender(res.render)
  next()
})

function createRouteRender(_render) {
  return function render() {
    this.locals.route = this.req.route
    return _render.apply(this, arguments)
  }
}
diosney commented 9 years ago

@dougwilson That would really work, thanks for the tip, but it could be great that other solution is provided for the long term (being the 'render' event or another one). The thing is that I don't feel quite right overwriting .render (if you are suggesting this then maybe the side effects are minimum to null).

So, thanks again, expecting further discussion on this :smile:

rightaway commented 8 years ago

I have a similar issue. I'm writing an authorization middleware that goes above where the routes are all defined. In that case, req.route is null.

So if I'm trying to allow/deny access in the authorization middleware based on req.route.path to something like /user/:id, it becomes difficult (if it's checking a path that doesn't use params, like /user, then I don't need req.route to be available as I can just use req.originalUrl.

I could use regexes but that becomes error prone depending on what kind of routes exist.

algebris commented 8 years ago

@rightaway I'm trying to resolve the same problem. Did you get the result?

rzk commented 8 years ago

@algebris I had exact same task as @rightaway and I ended up with adding additional layer of middleware everywhere, inside I check authorization to various routes depending on what user is allowed to do.

Details: I have object defining all routes restrictions, e.g. all req.route.path'es that should be protected, depending on users role. If route has no restrictions, I just next(), else check if user role is allowed to execute the controller, if not - render 40x.

Would be great to somehow make this work without injecting another middleware layer.

algebris commented 8 years ago

@rzk yeah, that's would be great anyway. I need req.route.path in order to find a matches in my swagger declarations with auth, validation, caching etc. But i have a lot end-points to add middleware to all of them. So I'm still trying to find a way to make it right ...

algebris commented 8 years ago

Well, I don't know but I've made in yesterday dirty hack to express internals ... I have tested it with express@4.13.3 It intercepts your middleware stack with custom functions before first middleware call

https://gist.github.com/algebris/1f931e112b0253249a04

Could anybody verify how's it bad to do like that? I know it's just a hack but ...

  1. If you have a lot end-points you don't need to extend it with the same middleware bundle like ... app.get('/someroute', middleware1, middleware2, ... )
  2. You will gain an access to req.route.path route pattern. I need it to control validation, access, cache, etc.