Conversation
| from starlette.middleware import Middleware, _MiddlewareClass | ||
| from starlette.middleware.base import BaseHTTPMiddleware | ||
| from starlette.middleware.errors import ServerErrorMiddleware | ||
| from starlette.middleware.exceptions import ExceptionMiddleware |
There was a problem hiding this comment.
We need to remove ExceptionMiddleware from here. And maybe delete that file as well.
There was a problem hiding this comment.
Where should we register the starlette.exception_handlers now?
There was a problem hiding this comment.
Sorry, I guess we need to get rid of is this part: https://github.com/encode/starlette/blob/966f0fc85764bd478d7c582fc8e2e5f91dbebae6/starlette/middleware/exceptions.py#L62
There was a problem hiding this comment.
Ah! Just to confirm: you mean that the ExceptionMiddleware should just register the exception_handlers in the scope, but the wrap_app_handling_exceptions should NOT be called there, only on the other places we have them (request_response and websocket_session). Correct?
There was a problem hiding this comment.
I cannot come up with a solution for this, FYI.
There was a problem hiding this comment.
@adriangb how do you want to proceed here? We talked in PyCon US a bit, but I don't recall how we left this.
This is the only blocker for 1.0.
The only thing is that I don't want to add any breaking change.
There was a problem hiding this comment.
If you want to do this without any breaking change we can just leave ExceptionMiddleware where it is and ignore this thread.
But conceptually I think the thing is that:
- After routing once we have a Request/Response exception handling can be done via a Request/Response (the current API).
- Outside of that (e.g. in other middleware or routing) we're in pure ASGI land. We could still let you catch
HTTPException's but I think the API should essentially be an ASGI appHTTPException | int, Scope, Receive, Sendor something like that. But that would be a breaking change. Otherwise I fear we're going to get bugs like "I wanted to catch the404HTTPException and log the request body but when I called.body()I got aStreamConsumederror".
To do that we'd have to replace ExceptionMiddleware with AppExceptionMiddleware or something like that with the new API and have two different arguments to Starlette for each type of exception handler.
There was a problem hiding this comment.
Leaving the pure ASGI realm, I have a minimalistic and effective design. https://github.com/abersheeran/baize/blob/master/baize/asgi/shortcut.py#L59
Kludex
force-pushed
the
version/1.0.0
branch
2 times, most recently
from
1bad72d to
9f57bf0
Compare
Kludex
force-pushed
the
version/1.0.0
branch
from
82cd228 to
d25651c
Compare
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Alex Oleshkevich <635848+alex-oleshkevich@users.noreply.github.com>
|
I reckon Starlette 1.0 needs a clear resolution to #2673. |
I've been trying to push this forward for 2 years. |
|
The simplest approach fro your side is #2534 (comment). |
5 participants
I've tried to be careful on the commit sequence here - each commit passes the pipeline by itself.
httpcoreon Version 1.0 encode/httpcore#809.@encode/maintainers Does someone want to add a breaking change on Starlette before we go to 1.0.0? Please, let's focus on breaking changes, if there are features that can be added on minor, let's not bother with them here.