Skip to content
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

[WIP] Allow None in ClientResponse.reason #3875

Closed
wants to merge 1 commit into from

Conversation

gwerbin
Copy link

@gwerbin gwerbin commented Jun 26, 2019

What do these changes do?

Since the work in #3533 is apparently slated for v4.0, this is a backport to v3.5.

Work around an AssertionError in v3.5 on ClientResponse.raise_for_status(), when the status is 400 or greater and the ClientResponse.reason is None (as opposed to an empty string or something else). The reason is meant purely for "human consumption" and the client's behavior shouldn't depend on it.

However, I feel like this might be the wrong place to make this fix. Perhaps real problem is that message.reason is allowed to be None, rather than defaulting to an empty string. See, for instance, the expected behavior in Requests: https://github.com/kennethreitz/requests/blob/bedd9284c9646e50c10b3defdf519d4ba479e2c7/requests/models.py#L920-L931

This PR is WIP -- should add some kind of test for this, as well as fill out the rest of the checklist items (CONTRIBUTORS.txt, CHANGES). Is any documentation change necessary?

Are there changes in behavior for the user?

None, other than fixing the unexpected AssertionErrors.

Related issue number

#3532
#3533

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

HTTP semantics don't require a reason.

However, I feel like this is the WRONG place to make this fix. Perhaps real problem is that `message.reason`
is allowed to be None, rather than defaulting to an empty string... making this commit for now, but
am prepared to revert and make a different change if needed.
@gwerbin gwerbin requested a review from asvetlov as a code owner June 26, 2019 18:03
@gwerbin
Copy link
Author

gwerbin commented Jun 26, 2019

Yikes, you guys probably just got a ton of pings + wasted CI cycles. I meant to mark this as a "draft". Sorry about that!

@webknjaz
Copy link
Member

Don't worry, just put WIP in the PR title.
In general, you can use cherry-picker tool to backport PRs. Or just do git cherry-pick manually.

@webknjaz webknjaz changed the title Allow None in ClientResponse.reason [WIP] Allow None in ClientResponse.reason Jun 26, 2019
@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

Merging #3875 into 3.5 will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              3.5    #3875      +/-   ##
==========================================
+ Coverage   97.86%   97.92%   +0.05%     
==========================================
  Files          44       44              
  Lines        8579     8579              
  Branches     1380     1381       +1     
==========================================
+ Hits         8396     8401       +5     
+ Misses         78       72       -6     
- Partials      105      106       +1
Impacted Files Coverage Δ
aiohttp/client_reqrep.py 97.14% <ø> (-0.01%) ⬇️
aiohttp/streams.py 98.2% <0%> (+0.26%) ⬆️
aiohttp/helpers.py 96.74% <0%> (+0.5%) ⬆️
aiohttp/locks.py 100% <0%> (+7.14%) ⬆️

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 a3fe4ec...1ac5cb5. Read the comment docs.

@asvetlov
Copy link
Member

asvetlov commented Jul 5, 2019

The PR is incorrect.
reason is None if ClientResponse object is created but the response headers are not received yet.
This state is not visible to user, raise_for_status() cannot be called in this state.
After receiving headers reason is always set to a message or empty string.

The check is always relaxed to accept empty string on 3.5 branch

@asvetlov asvetlov closed this Jul 5, 2019
@gwerbin
Copy link
Author

gwerbin commented Jul 5, 2019

@asvetlov I got an assertion error for this in production code, I don't know what you want me to tell you. I can try to reproduce if you like, although it will be difficult.

@asvetlov
Copy link
Member

asvetlov commented Jul 5, 2019

A unit test is the best judge, please provide it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants