-
-
Notifications
You must be signed in to change notification settings - Fork 874
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
Version 1.0.0 #2384
base: master
Are you sure you want to change the base?
Version 1.0.0 #2384
Conversation
|
||
from typing_extensions import ParamSpec | ||
|
||
from starlette.datastructures import State, URLPath | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove ExceptionMiddleware from here. And maybe delete that file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should we register the starlette.exception_handlers
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I guess we need to get rid of is this part:
await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a problem. We had an oversight when implementing the per route exception handler.
There are places we have a raise HTTPException
that don't follow the request_response
or websocket_session
flow, and we can't just remove/replace by PlainTextResponse
because the user may be already be changing the behavior of those HTTPException
.
If you just remove the wrap_app_handling_exceptions
you mentioned above, you'll see the test failures that express what I'm saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try after the holidays but I’d say we should change those and users shouldn’t expect any request/response stuff in middleware (aside from BaseHTTPMiddleware which is beyond changing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on what you said, I don't think I made myself clear with my comment above.
I'll wait for later then. Thanks! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot come up with a solution for this, FYI.
1bad72d
to
9f57bf0
Compare
Co-authored-by: Adrian Garcia Badaracco <[email protected]> Co-authored-by: Alex Oleshkevich <[email protected]>
82cd228
to
d25651c
Compare
I've tried to be careful on the commit sequence here - each commit passes the pipeline by itself.
httpcore
on Version 1.0 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.