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

Two routers mounted at same path do not isolate their middleware from each other #2760

Closed
mjm opened this issue Sep 22, 2015 · 13 comments
Closed

Comments

@mjm
Copy link

mjm commented Sep 22, 2015

Using router.use seems to be documented to allow you to add middleware that will only be called for routes added to that router, but that does not seem to be working for me. The issue seems to be because I'm mounting several routers at the same path. The decision for whether a middleware is run seems to be based on the path, not the router. This was confusing.

The use case here is that my application has several modules that are mounted under the same location, as the URLs for these don't always follow a common prefix. There's common middleware that is used on most, but not all, endpoints. I was attempting to refactor the application to use an express.Router for each module, with each router being able to .use whatever middleware is common to all of its routes.

The result has been that the same middleware is called multiple times, because several routers are all using it. They should be separated, if Router worked as described. Another case is that one module used a middleware to require auth on all of the endpoints in its router, but that spilled over to any other routers used after that one.

Is this intended behavior? Is there a workaround?

Here is a Gist demonstrating the issue: https://gist.github.com/mjm/497a23f68826f8ca473b

@dougwilson dougwilson self-assigned this Sep 22, 2015
@dougwilson
Copy link
Contributor

Hi! Yes, this is currently intended behavior in Express 4, but is open to change if you can whip up a PR for us :) The reason this happens is because Express 4's router is a simple stack; without executing the middleware in router1, Express has no way to know how to proceed, because that middleware may modify req.method, req.url or something else that modifies routing behavior.

@dougwilson
Copy link
Contributor

P.S., also feel free to open a PR at https://github.com/strongloop/expressjs.com to help document this behavior to your satisfaction, to help others who may run into the same surprise. Since Express 5 is not yet released, Express 5 is open to change. You can also always use another router module or your own router that behaves off something other than strict linear path matching.

@mjm
Copy link
Author

mjm commented Sep 22, 2015

Ah, that's disappointing. Basically, all I'm trying to do is go from:

app.get(foo, bar);
app.post(foo, baz);

to being able to not repeat foo so many times. Is there any built-in way in Express to (for lack of a better word) express this, whether it be with routers or not? I'm not really interested in using a different router module. If there's no way to do it, I'll just go back to mostly using what we had before, putting the middleware on each route. I appreciate the response, nonetheless.

And for what it's worth, my vote would be for this to be possible in Express 5 :)

@dougwilson
Copy link
Contributor

Would app.route (http://expressjs.com/4x/api.html#app.route) fit the bill here? For your example, you can do the following:

// assuming "foo" is a variable to a path:
app.route(foo)
.get(bar)
.post(baz)

// OR assuming "foo" is a middleware:
app.route('/foo')
.all(foo)
.get(bar)
.post(baz)

I could potentially find a good solution, but that example isn't the easiest to understand what you are trying trying to do :) If you provide the real use-case, that would help a lot, especially if there isn't a solution and one needs to be added here (and you're not up to making a PR). Let us help you :)

@mjm
Copy link
Author

mjm commented Sep 22, 2015

Your second example is correct, I left the path off my example by accident. As far as I can tell, app.route will help some, but not all of the repetition. In the cases, where I do have multiple methods for the same URL, your second example is very similar to what I've done.

What I was hoping to achieve was a grouping of routes that, independent of their path, could all have a common middleware applied to them, without repeating the middleware for each one. I'm sorry, I don't think I can be much more specific than that.

From what I can tell, though, it appears there are exactly two ways to control what middleware is applied:

  • Using a path to scope it down (using either router.all or router.use)
  • Using ordering

I somewhat understand why Express works the way it works now. Since there is so much flexibility in where a request will end up, it's hard to do this logical grouping in a way that makes sense, since I could go all the way through a route and then never actually finish the request there, instead leaving it to some subsequent route, possibly from another router. It's hard to tell what would happen there. That said, given I've structured my app in such a way that all my routes finish their requests, it seems straightforward enough to want to be able to do all the path filtering first, then see what middleware applies to the route that was chosen (and exclude the middleware from other routers).

@andrewosh
Copy link

Just want to leave my 👍 here. Let me know if there's something I'm doing wrong, but my use case is:

  1. I have two routers, one uses some authorization middleware and the other does not require authorization
  2. Both routers are mounted on the same path, and they might be responsible for handling different methods on the same endpoint

In this scenario, I'm seeing that the authorization middleware is being applied to the route/methods being handled by the second router as well. To be more specific, in the following chunk of code, authorization is being applied to the routes in openRouter as well:

var authRouter = express.Router()
authRouter.use(authHandler)
var openRouter = express.Router()

authRouter.route('/applications/:template')
  .get(this.cluster.getAllApplications.bind(this.cluster))
openRouter.route('/applications/:template')
  .post(this.cluster.createApplication.bind(this.cluster))

app.use('/', openRouter)
app.use('/', authRouter)

Any thoughts on this?

@dougwilson
Copy link
Contributor

At @andrewosh , you are just describing the issue here. There is no solution except mounting your two routers on non-conflicting routes or you have to push down the authHandler to the route-level. Effectively, all Express currently does is generate a completely linear route stack. This means your given code above turns into the following:

POST /applications/:template -> cluster.createApplication()
ANY / -> authHandler
GET /applications/:template -> cluster.getAllApplications()

So because all router is currently is basically a grouping, the code above is simply mounting authHandler at the path /* for all methods between the two routers, thus why it's called, regardless of if there are any matching routes within authRouter. Because middleware are allowed to alter the request path, method, etc., we cannot even know if the request matches a path without executing all prior middleware in case they alter the state of the request (this supports things like path rewrite, method overrides, etc.).

@ericuldall
Copy link

I'm having the same or a similar issue. It seems that we should be able to treat routers as encapsulated call stacks that extend the parent app functionality yet work independent of each other.

I think the problem is that the new routing is attached to the app via app.use(), so each router must be traversed in the order it was 'use'd by the app. (please correct me if i'm wrong)

Perhaps the solution is to no longer treat routers as their own middleware and rather create a new more specific implementation for attaching routers to the stack. I do believe this functionality is well warranted.

@frankxw
Copy link

frankxw commented Jan 25, 2016

Would my suggested changes in #2828 be of use for you guys? It doesn't make routers an isolated stack, but it does enable a shared stack to be used by each route within a router.

@Tomino2112
Copy link

I think this is an issue for quite a few people, logically Router.use() should isolate the middlewares just to its own scope otherwise it becomes app.use() and it goes out of its scope.

Also please see this: http://stackoverflow.com/questions/35489372/expressjs-applying-middleware-only-to-routes-in-router

@demalus suggestion sounds good

@hacksparrow
Copy link
Member

Sounds like a useful feature expectation. Let's take the discussion to the router middleware repo - pillarjs/router#38.

@kevinclarkadstech

This comment has been minimized.

@kevinclarkadstech

This comment has been minimized.

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

No branches or pull requests

8 participants