Permit empty reason strings in ClientResponse.raise_for_status() #3533

rhwlo · 2019-01-13T18:37:41Z

What do these changes do?

Allows ClientResponse.raise_for_status to raise a ClientResponseError in the case of an HTTP Reason-Phrase that is an empty string
Raises a TypeError if the ClientResponse.reason is None (since we can’t make a ClientResponseError with a message of None)
Fixes ClientResponse.raise_for_status() raises an AssertionError for empty reason #3532

Are there changes in behavior for the user?

Calling raise_for_status on a status with a reason phrase that’s an empty string will raise a ClientResponseError now instead of an AssertionError

Related issue number

#3532

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .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.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

…e_for_status()

codecov-io · 2019-01-13T19:04:26Z

Codecov Report

Merging #3533 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            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

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.12% <0%> (-0.32%)`	⬇️
aiohttp/web_fileresponse.py	`96.55% <0%> (-1.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8aa12...1270774. Read the comment docs.

codecov-io · 2019-01-13T19:04:27Z

Codecov Report

Merging #3533 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.43% <100%> (ø)`	⬆️
aiohttp/web_fileresponse.py	`96.55% <0%> (-1.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8aa12...3b9d0d3. Read the comment docs.

asvetlov · 2019-01-13T21:00:37Z

aiohttp/client_reqrep.py

@@ -931,8 +931,10 @@ def release(self) -> Any:
        return noop()

    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")


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.

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

I’ll switch it to assert self.reason is not None if that’s the desired outcome.

asvetlov · 2019-01-14T21:53:40Z

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]>

ctg3 · 2019-02-07T22:12:26Z

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

vaultah · 2019-04-04T22:43:19Z

Maybe make a release that contains a fix for this? No hurry though, it's only been three months.

Solvero · 2019-04-08T08:49:52Z

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:

The Status-Code is intended for use by automata and the Reason-Phrase is intended for the human user. The client is not required to examine or display the Reason- Phrase.

Please see also:
https://stackoverflow.com/questions/17517086/can-an-http-response-omit-the-reason-phrase

gwerbin · 2019-06-26T17:13:40Z

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
        )

webknjaz · 2019-06-26T17:25:18Z

@gwerbin master branch is for 4.0. If you want, you can submit a backport PR against 3.5 branch.

gwerbin · 2019-06-26T17:52:38Z

Thanks @webknjaz, My one concern is about this line:

reason should always be not None for a started response

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 or '' fix.

Alternatively, why would reason be None here, rather than defaulting to an empty string? I feel like fixing this at the raise_for_status point doesn't actually address the problem, that reason can be None in the first place instead of an empty string.

webknjaz · 2019-06-26T18:06:59Z

@gwerbin you should normally cherry-pick the entire PR and not try to rip off some parts of it. This includes tests.

fiendish · 2023-01-26T23:19:46Z

raise_for_status on a response object is still giving AssertionError instead of ClientResponseError in the latest release?

Permit empty reason strings on responses that use ClientResponse.rais…

1270774

…e_for_status()

rhwlo requested review from asvetlov and webknjaz as code owners January 13, 2019 18:37

aio-libs-bot added the bug label Jan 13, 2019

asvetlov reviewed Jan 13, 2019

View reviewed changes

Change from raising a TypeError to asserting reason is not None

3b9d0d3

asvetlov approved these changes Jan 14, 2019

View reviewed changes

asvetlov merged commit f590bfd into aio-libs:master Jan 14, 2019

asvetlov pushed a commit that referenced this pull request Jan 14, 2019

[3.5] Permit empty reason strings in ClientResponse.raise_for_status() (

27f061f

#3533) (cherry picked from commit f590bfd) Co-authored-by: Joshu Coats <[email protected]>

asvetlov added a commit that referenced this pull request Jan 15, 2019

[3.5] Permit empty reason strings in ClientResponse.raise_for_status() (

24aff29

#3533) (#3541) (cherry picked from commit f590bfd) Co-authored-by: Joshu Coats <[email protected]>

webknjaz assigned asvetlov Apr 8, 2019

gwerbin mentioned this pull request Jun 26, 2019

[WIP] Allow None in ClientResponse.reason #3875

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit empty reason strings in ClientResponse.raise_for_status() #3533

Permit empty reason strings in ClientResponse.raise_for_status() #3533

rhwlo commented Jan 13, 2019

codecov-io commented Jan 13, 2019

codecov-io commented Jan 13, 2019 •

edited

Loading

asvetlov Jan 13, 2019

asvetlov Jan 13, 2019

rhwlo Jan 14, 2019

asvetlov commented Jan 14, 2019

ctg3 commented Feb 7, 2019

vaultah commented Apr 4, 2019

Solvero commented Apr 8, 2019 •

edited

Loading

gwerbin commented Jun 26, 2019 •

edited

Loading

webknjaz commented Jun 26, 2019

gwerbin commented Jun 26, 2019 •

edited

Loading

webknjaz commented Jun 26, 2019

fiendish commented Jan 26, 2023 •

edited

Loading

Permit empty reason strings in ClientResponse.raise_for_status() #3533

Permit empty reason strings in ClientResponse.raise_for_status() #3533

Conversation

rhwlo commented Jan 13, 2019

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Jan 13, 2019

Codecov Report

codecov-io commented Jan 13, 2019 • edited Loading

Codecov Report

asvetlov Jan 13, 2019

Choose a reason for hiding this comment

asvetlov Jan 13, 2019

Choose a reason for hiding this comment

rhwlo Jan 14, 2019

Choose a reason for hiding this comment

asvetlov commented Jan 14, 2019

ctg3 commented Feb 7, 2019

vaultah commented Apr 4, 2019

Solvero commented Apr 8, 2019 • edited Loading

gwerbin commented Jun 26, 2019 • edited Loading

webknjaz commented Jun 26, 2019

gwerbin commented Jun 26, 2019 • edited Loading

webknjaz commented Jun 26, 2019

fiendish commented Jan 26, 2023 • edited Loading

codecov-io commented Jan 13, 2019 •

edited

Loading

Solvero commented Apr 8, 2019 •

edited

Loading

gwerbin commented Jun 26, 2019 •

edited

Loading

gwerbin commented Jun 26, 2019 •

edited

Loading

fiendish commented Jan 26, 2023 •

edited

Loading