-
-
Notifications
You must be signed in to change notification settings - Fork 890
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
Add 400 response when boundary
is missing
#1617
Conversation
boundary
is missing
f4a3ab0
to
d52dd9a
Compare
9a3d6ad
to
f1eb5ba
Compare
|
||
async def sender(message: Message) -> None: | ||
nonlocal response_started | ||
def __getattr__(name: str) -> typing.Any: # pragma: no cover |
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 need to add a test for this.
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.
Didn't realise you could do this at a module level. 😎
|
||
async def sender(message: Message) -> None: | ||
nonlocal response_started | ||
def __getattr__(name: str) -> typing.Any: # pragma: no cover |
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.
What's the plan to remove all of this deprecation code? I get why we need it, but it'd be nice to know when we're going to remove it so that:
- We can put it in the code for our knowledge down the road.
- We can put it in the error message.
I suppose this depends on #1623 , but I think we should make an executive decision (maybe @tomchristie ?) on when / if the deprecation shim is ever going to be removed.
Personally, I feel like 3 minor releases or 1 year, whichever comes first, sounds reasonable?
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 was more into 6 months and 3 minors... But any number is good for me... I'd just like to have a number 👍 But let's mention those stuff on that discussion.
For here, there's no plan yet. The plan should be defined on that discussion. If this is merged, it's just common sense i.e. in some arbitrary time we just remove it...
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.
This is an interesting change.
Overall the change (returning a proper exception to the client instead of a 500) makes absolute sense.
Unfortunately you ended up needing to do a bunch of surgery to the codebase to make it happen because raising an HTTPexception
from within a Request
requires that Request
reference HTTPException
, but ExceptionMiddleware
was in the same module as HTTPException
, which caused circular references. This makes the diff super ugly and huge relative to the small straightforward feature.
But I think that's okay: we are paying the price to fix an issue that already existed in our codebase that might have caused us issues down the road some other time. And personally I think moving the middleware into middlewares/
really cleans things up nicely.
I'm approving this, I do however want 1 more approver and some consensus on the deprecation policy because this is a breaking change for our downstream dependencies.
Yep 👍 Thanks for the review! 🙏 |
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.
Some questions which could be addressed, but they're not blockers so will leave this in your hands at this point @Kludex.
Really nice switch around with moving the middleware. 👍
|
||
async def sender(message: Message) -> None: | ||
nonlocal response_started | ||
def __getattr__(name: str) -> typing.Any: # pragma: no cover |
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.
Didn't realise you could do this at a module level. 😎
from starlette.types import ASGIApp, Message, Receive, Scope, Send | ||
|
||
|
||
class 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.
👍 Yup - much better to have this here.
Have confirmed to myself that the class here and the class when it was in exceptions.py
are identical.
multipart_parser = MultiPartParser(self.headers, self.stream()) | ||
self._form = await multipart_parser.parse() | ||
except MultiPartException as exc: | ||
if "app" in self.scope: |
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.
Do we need to have the switch here? Can we just always coerce to an HTTPException
here instead? Are we already using this pattern, and if so, where?
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.
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, we use the pattern in the code, as @adriangb pointed out.
Always coerce to an HTTPException
is an option. I opted to not do it because we only use HTTPException
s when we are in Starlette
(as application) e.g. "app" in self.scope
.
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.
Realistically, I think we could remove that pattern everywhere. IMO it's not worth supporting (nice) usage of Starlette's Request object outside of a Starlette app. If someone wants to do that, they can handle the HTTPException.
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.
If someone wants to do that, they can handle the HTTPException.
You have a point, but I think that's another discussion.
I really don't think it's a burden to maintain that, but I wonder if someone really uses that code outside Starlette
(app)... 🤔
client = test_client_factory(app) | ||
with pytest.raises(KeyError, match="boundary"): | ||
client.post( | ||
with expectation: |
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'd personally probably nudge towards the simpler pytest.raises(KeyError, match="boundary")
case here, just because it's more obviously readable to me.
The parameterised expectation
is neat, but also more complex to understand.
Just my personal perspective tho, happy to go either way.
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.
The idea was to avoid the creation of two very similar tests. Do you think is clear if I create two tests instead?
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 think your way or combining an exception and non-exception test into 1 is the cleanest I've seen for that pattern, but if it's
only 2 tests and not 10 I also think making it more explicit would be nice
This comment was marked as off-topic.
This comment was marked as off-topic.
I'm going to merge this. Some things left unanswered:
|
boundary
#1588Behavior of this PR:
"app" not in scope
.HTTPException(status_code=400)
in case"app" in scope
.Changes:
HTTPException
tohttp_exception.py
to avoid circular import.MultiPartException
on missingboundary
.MultiPartException
onform()
with the behavior mentioned above.Alternatives/thoughts related to this PR:
MultiPartException
can also be on an upper level:starlette/starlette/routing.py
Lines 250 to 261 in 830f348
This alternative would avoid creating the
http_exception.py
... 🤔