starlette
starlette copied to clipboard
encode
feat: add middleware to Router
Closes #1286
Instead of adding middleware to each BaseRoute, we can add it to Router.
This can then be composed with Mount and such to get middleware for a single BaseRoute if needed, although this does make adding middleware to a single BaseRoute more complicated compared to #1286 since you may have to do a mount -> router -> route just to add middleware. But the most part I think users would be adding middleware to a group of routes not a single route, which works well with having it on Router.
One nice thing about this is that the behavior of middleware is preserved: you can even modify scope["path"] and change the outcome of routing.
Another advantage this has over #1286 is that it leaves the door open for us to move middleware from the Starlette app to Router, so we'd only have middleware in 1 class (I've tested this, it works nicely but requires some braking changes like removing Starlette.build_middleware_stack()).
@alex-oleshkevich what do you think?
For what it's worth, I implemented this in Xpresso and it works very nicely. I can also confirm that, aside from breaking the deprecated decorators, moving all middleware to Router and out of App/Starlette would also work in the future, should we choose to take that path (this is what I did in Xpresso).
@alex-oleshkevich Your opinion is highly appreciated here. :)
As we talked on Gitter some time ago, I was pretty skeptical about non-app-level-middlewares at the beginning, but as no one could find a better solution (myself included), I think this is a great addition. I also think this is simpler than #1286 .
In any case, I'm going to test this in the following weeks. Everybody, feel free to review this as well.
I see that in #1286 Tom mentioned the possibility of passing middleware=... on Mount as well.
https://github.com/encode/starlette/pull/1286#issuecomment-966156328
Let's get the same thing added to Mount as well. Having middleware on mounts kinda makes more sense to middleware on routes to me.
On this PR, usage is as follows...
# apps/admin.py
router = Router(
routes=[
Route(
"/dashboard,
endpoint=...,
)
],
middleware=[Middleware(AdminAuthMiddleware, ...)], # Sub-router-specific middleware
)
# app.py
from .apps import admin
routes = [
Mount(
"/admin",
app=admin.router,
)
]
app = Starlette(
routes=routes,
middleware=..., # App-wide middleware
)
But what about Mount(..., routes=...)? Personally, I use this style more often, and it's also the one that's advertised in the docs: Sub-mounting routes.
It looks like only allowing Router(..., middleware=...) would make Mount(..., routes=...) sort of ill-defined: if one needs middleware on a sub-router, one would now always have to define a Router instance, and then pass it as app=....
Instead, with Mount(..., middleware=...), we'd have this...
# apps/admin.py
routes = [
Route(
"/dashboard,
endpoint=...,
),
]
# app.py
from .apps import admin
routes = [
Mount(
"/admin",
routes=admin.routes,
middleware=[Middleware(AdminAuthMiddleware, ...)], # Sub-router-specific middleware
)
]
app = Starlette(
routes=routes,
middleware=..., # App-wide middleware
)
This style makes the definition of sub-router middleware both closer to where app-level middleware is defined (generally the app routes are defined near Starlette), and just next to the mounting path, which is defined on Mount. This provides nice locality of behavior.
This would not only still work with Mount(..., app=Router(...), middleware=...), but also work for cases where app is an arbitrary ASGI app, not just a Starlette router. This embraces the composability principle of ASGI.
It also seems consistent if we think of Mount as a sub-app of Starlette (which it is), given that Starlette used to be the sole point for defining middleware=.... We'd just be expanding this capability to sub-apps.
Lastly, given all this, and while Tom initially thought "let's also add this to Mount", I wonder if we need Router(..., middleware=...) at all. So, given that sub-routes always involves Mount anyway, I might be in favor of keeping this style only: Mount(..., middleware=...).
So……why not use this code?
AdminAuthMiddleware(Mount("/admin", routes=admin.routes), ...)
Just a question. I love routing middleware. But I don't think routing middleware is necessary in the Mount scenario.
@abersheeran You might be able to get that to work (does it?), but I suspect there might be issues related to root_path and other things that Mount() takes care of. Also, Mount(..., middleware=[A, B, ...]) would take care of applying multiple middleware in a reverse order, whereas you would need to write B(A(Mount(...), ...), ...).
I vote for Mount solution. This looks solid and does exactly the same things as Mount already does -> attach another routes/app with extra settings (middleware in our case).
Great points, thank you for bringing it up @florimondmanca
I'll cook up an implementation shortly (just rip it out of #1286).
For those coming here from FastAPI, you can achieve similar functionality using router dependencies as explained here: https://github.com/tiangolo/fastapi/issues/1174#issuecomment-1205071157
@HiperMaximus I think the team has decided to move forward with #1649 instead