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

Routers with Isolated middleware stack #38

Open
hacksparrow opened this issue Feb 29, 2016 · 15 comments
Open

Routers with Isolated middleware stack #38

hacksparrow opened this issue Feb 29, 2016 · 15 comments

Comments

@hacksparrow
Copy link
Member

The single stack middleware system of express seems to be a source of confusion for some developers. This is a proposal to isolate the middleware stack in a router from other routers mounted on the same app.

Current behavior:

  • A middleware defined without a path on router A will get executed for requests matching in router B.

Proposed behavior:

  • A middleware defined without a path on router A should get executed only for requests matching in router A, and not on any other router.

The current behavior of app.use(), which executes its middleware for all the requests to the app would remain the same and should be used for defining global middleware.

@frankxw
Copy link

frankxw commented Feb 29, 2016

What if two Routers are mounted like such:

  • Router A: /foo
  • Router B: /foo/bar

If a request comes in at /foo/bar, the proposed behavior is that if Router A matches, no other router's middleware should be executed, which I don't think would be desirable in the above situation. Does this imply that maybe the most specific match would be the router that gets executed (in this case Router B) - if so I feel like it would also not be desirable to ignore A.

@tunniclm
Copy link

@demaius You could have it so app.use('/foo', fn) would apply to both routers, but fooRouter.use(fn) would only apply to fooRouter and not barRouter.

@hacksparrow
Copy link
Member Author

@demalus the behavior would remain the way it is currently for that case - the router loaded earlier will get to process the request object first.

This proposal is limited to the behavior of pathless routes defined in routers.

@dougwilson
Copy link
Contributor

This proposal is limited to the behavior of pathless routes defined in routers.

Hi @hacksparrow, there is no concept at all for "pathless routes" in Express or this router. Just because the path argument is optional doesn't actually make not specifying it intrinsically different from specifying '/' at the path argument. This sounds like there is now a per-requisite on come up with "pathless routes" and how those are defined and work.

@hacksparrow
Copy link
Member Author

@dougwilson I didn't mean pathless literally, it was more about the interface when omitting the path parameter. Sorry about the confusion.

So, clarifying myself here:

This proposal is limited to the behavior of routes defined in routers without specifying a path.

@dougwilson
Copy link
Contributor

This proposal is limited to the behavior of routes defined in routers without specifying a path.

Right, but that's the problem: unless there is another change before this to the concepts in the router, app.use(fn) must behave identically to app.use('/', fn) especially because there are many configuration-based builders on top of this module that always provide that argument since it is promised to work in an identical way.

@hacksparrow
Copy link
Member Author

How about "behavior of routes defined in routers without specifying a path or at the root path"?

@dougwilson
Copy link
Contributor

Another major problem with the proposal as-is is request URL rewriting. A middleware is absolutely allowed to alter the URL and method of a request, and processing will continue with the new URL. Because of this, we cannot even understand if a request will match any given route until all prior middleware are executed.

@hacksparrow
Copy link
Member Author

Agreed.

@frankxw
Copy link

frankxw commented Feb 29, 2016

Agree with @dougwilson . There's no way to know if you are "inside" of a router when executing middleware. I also agree that single stack can be confusing for developers that haven't experienced it before.

Looking at the original ticket #2760, I'm not entirely sure a solution would work. If you have multiple Routers mounted at the same spot, the middleware will be executed for all of them until some piece of middleware exits the chain (usually a route). You could isolate the routers explicitly by doing something like the following:

  1. Instead of appending all of the Router's middleware to the global stack, you instead append a single function.
  2. This function checks if the path matches the Router, if it does it executes the Router's "sub"-stack.

This doesn't solve the problem either, because middleware is already implicitly isolated because each middleware is checked against Router.path + middleware.path.

The real problem I think is that people are expecting it to work like such: if a route is matched in a Router, then execute everything in the router. You could modify what I wrote above to change the match to also loop through all the routes in a router and see if one matches.

@frankxw
Copy link

frankxw commented Feb 29, 2016

If this wouldn't be feasible, I think my proposal at #35 would be a reasonable middle ground. It kind of simulates the effects of isolated middleware for route-handlers only. For #2760 it would be the workaround so "foo" doesn't have to be repeated many times.

@QuotableWater7
Copy link

Does anyone know if this kind of feature ever got anywhere? The existing behavior was totally unexpected and derailed a refactor I was working on 😒

@wesleytodd
Copy link
Member

@QuotableWater7, I do not believe so. I believe the current thinking on this would be, Express does not need to support every feature or configuration, but it should make it simple to swap in parts to get what you need.

So in this case, I would recommend using a router which supports the feature you need (router level isolation). If this means forking this to provide your features, go for it. The router is decoupled from much of express as long as you implement the interface.

IMHO, we should probably close this issue since it has not seen progress and is quite a big change in how the router works.

@karlismelderis
Copy link

would be really nice to support this:

clientRouter.use(authorizeUser);
clientRouter.get(/api/somethingA/:id, getIt());
clientRouter.get(/api/somethingB/:id, getIt());
callbackRouter.use(authorizeM2M);
callbackRouter.get(/api/somethingA/:id/callback, justDoIt());
callbackRouter.get(/api/somethingB/:id/callback, justDoIt());
app.use('/api', clientRouter)
app.use('/api', callbackRouter)

@Aditya-ds-1806
Copy link

OMG I was facing the same issue, it took me over an hour to get here. This is definitely not the expected behavior and causes lot of confusion. Has there been any progress?

esalling23 added a commit to esalling23/ecommerce-api that referenced this issue Aug 24, 2021
Router.use() is based on path, not router instance.
This means routerA can `.use` a middleware and end up effecting routerB.
The user router (and all others) was running through
the requireToken middleware as a result.

This is a documented issue with express router:
pillarjs/router#38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants