encode
Add Mount(..., middleware=[...])
This would be really useful. Is there a plan to merge and release this still at some point?
The implementation is simple and works well. However, there is a different behavior in both if an exception raised in handler.
This exception will be handled by ExceptionMiddleware and thus app-level middleware will still be able to change response such as appending extra tracking header. But this is not the case using router-level middleware (wherher #1649 or #1464).
I wonder if this behavior changing is expected and should be documented or we need build the middleware stack per Mount or Router? I currently remove the app-level middleware and wrap router-level middleware between ServerErrorMiddleware and ExceptionMiddleware.
Great point @progamesigner
One option would be to add ExceptionMiddleware automatically if the user supplies any other middleware. But that would get awkward because the exception handlers wouldn't propagate from the application level, so you'd be left juggling two sets of exception handlers. Sounds like a footgun to me. So maybe the best thing we can do is simply document this behavior?
I think this really only applies to things that plan to modify or look at the response, right? If you are trying to modify the response (e.g. GZipMiddleware) you probably want the exceptions to just tear through you (no point in compressing a 404 response). In the cases where you want to inspect the response it may make sense to have the route level middleware just "mark" the request by adding a key into the scope like scope["foobar.log-request"] = True and then have an application level middleware be what is actually doing the heavy lifting. That way the route-level middleware contains the routing specific logic but doesn't have to handle errors and if another middleware modifies the response (e.g. by compressing it) it will happen before the response info gets logged (so the content-length won't be off and such).
I added a note to the docs explaining https://github.com/encode/starlette/pull/1649#issuecomment-1189821544
I think we can take this opportunity to add a more real case usage instead of GZipMiddleware, maybe a middleware related to the issue that motivates this PR. What do you think?
I think we can take this opportunity to add a more real case usage instead of
GZipMiddleware, maybe a middleware related to the issue that motivates this PR. What do you think?
I was going to say yes but: https://github.com/encode/starlette/discussions/1837. So GZipMiddleware is actually a motivating example, right?
I think we can take this opportunity to add a more real case usage instead of
GZipMiddleware, maybe a middleware related to the issue that motivates this PR. What do you think?I was going to say yes but: #1837. So
GZipMiddlewareis actually a motivating example, right?
- I think the motivation was https://github.com/encode/starlette/issues/685, but fair enough. 👍
I'm going to go ahead and merge it so we can proceed with the next release, if anyone has any last minute concerns we can address them in a separate PR before the release.
A thought about this PR, Request objects and error handling. I wonder if we maybe should’ve had ExceptionMiddleware wrap each Route instead of being a middleware at the application level? That would have both resolved the concerns in https://github.com/encode/starlette/pull/1649#discussion_r962144793 and also https://github.com/encode/starlette/pull/1692#issuecomment-1266401636 because we’d be able to (1) make sure there is nothing in between ExceptionMiddleware and the route and (2) use the same Request object for error handlers and the endpoint. I wonder if this could be made backwards compatible…