Permit empty reason strings in ClientResponse.raise_for_status()#3533
Permit empty reason strings in ClientResponse.raise_for_status()#3533asvetlov merged 2 commits intoaio-libs:masterfrom rhwlo:allow-empty-reason-strings-on-errors
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3533 +/- ##
==========================================
- Coverage 97.94% 97.89% -0.05%
==========================================
Files 44 44
Lines 8562 8563 +1
Branches 1381 1382 +1
==========================================
- Hits 8386 8383 -3
- Misses 71 73 +2
- Partials 105 107 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3533 +/- ##
==========================================
- Coverage 97.94% 97.92% -0.03%
==========================================
Files 44 44
Lines 8562 8562
Branches 1381 1381
==========================================
- Hits 8386 8384 -2
- Misses 71 72 +1
- Partials 105 106 +1
Continue to review full report at Codecov.
|
aiohttp/client_reqrep.py
Outdated
| def raise_for_status(self) -> None: | ||
| # self.reason is only None if self.start() hasn't been called: | ||
| if self.reason is None: | ||
| raise TypeError("Expected a Reason-Phrase as a string") |
There was a problem hiding this comment.
An exception type and especially text is meaningless for an end user.
The text describes a consequence (the reason field is not set) but hides an actual problem.
Moreover, assertion exists only for sake of mypy.
If you don't like the assertion -- please find another way to satisfy mypy.
assert self.reason is not None could be the simplest possible fix, even no test is needed for the case.
There was a problem hiding this comment.
Well, a test would be nice but please use a functional test for an empty reason.
https://github.com/aio-libs/aiohttp/blob/master/tests/test_client_functional.py can give an inspiration how a functional client test looks like
There was a problem hiding this comment.
I’ll switch it to assert self.reason is not None if that’s the desired outcome.
|
Thanks! |
#3533) (cherry picked from commit f590bfd) Co-authored-by: Joshu Coats <[email protected]>
#3533) (#3541) (cherry picked from commit f590bfd) Co-authored-by: Joshu Coats <[email protected]>
|
Why is reason string required? I have a webserver that doesn't return any reason phrase (None). If self.reason is None, can we just change it to self.reason = '' and still satisfy mypy? Also see a discussion here: https://stackoverflow.com/questions/17517086/can-an-http-response-omit-the-reason-phrase |
|
Maybe make a release that contains a fix for this? No hurry though, it's only been three months. |
|
I have encountered this problem after aiohttp upgrade to 3.5.4. My server doesn't return Response-Reason too. HTTP/1.1 specification says that:
Please see also: |
|
I appreciate all the work the contributors are doing on this library, but it would have been nice to merge and release this as a 3.5.5 "patch". It looks like you're waiting for 3.6 release or something like that? In the meantime, I'm using my own implementation of this, basically copy-and-pasted from the source code: def raise_for_status(response: aiohttp.ClientResponse) -> None:
if 400 <= response.status:
response.release()
raise aiohttp.ClientResponseError(
response.request_info,
response.history,
status=response.status,
message=response.reason or '',
headers=response.headers
) |
|
@gwerbin |
|
Thanks @webknjaz, My one concern is about this line:
Does some other part of the application or test suite depend on this being true? I don't want to break anything else with my quick Alternatively, why would reason be |
|
@gwerbin you should normally cherry-pick the entire PR and not try to rip off some parts of it. This includes tests. |
|
raise_for_status on a response object is still giving AssertionError instead of ClientResponseError in the latest release? |
10 participants
What do these changes do?
ClientResponse.raise_for_statusto raise aClientResponseErrorin the case of an HTTP Reason-Phrase that is an empty stringTypeErrorif theClientResponse.reasonisNone(since we can’t make aClientResponseErrorwith a message ofNone)Are there changes in behavior for the user?
raise_for_statuson a status with a reason phrase that’s an empty string will raise aClientResponseErrornow instead of anAssertionErrorRelated issue number
#3532
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.