Eliminate side-effects from the `ClientResponse.ok` property #5404

adamko147 · 2021-01-13T16:01:52Z

Make ClientResponse.ok property only check the self.status for 400+ status codes and reuse it in raise_for_status method

Are there changes in behavior for the user?

Yes. User will be allowed to use code

res = await session.get('url-returning-400-status')
if not res.ok:
    body = await res.text()

Related issue number

fixes #5403

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

webknjaz · 2021-01-13T16:03:18Z

@adamko147 your local Git is misconfigured: the commit author email doesn't match any emails verified on GitHub. You may want to fix that.

codecov · 2021-01-13T16:08:19Z

Codecov Report

Merging #5404 (c48553c) into master (3401bef) will increase coverage by 0.40%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5404      +/-   ##
==========================================
+ Coverage   96.74%   97.15%   +0.40%     
==========================================
  Files          41       41              
  Lines        8736     8742       +6     
  Branches     1402     1402              
==========================================
+ Hits         8452     8493      +41     
+ Misses        149      130      -19     
+ Partials      135      119      -16

Flag	Coverage Δ
unit	`97.04% <100.00%> (+0.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.52% <100.00%> (+0.15%)`	⬆️
aiohttp/cookiejar.py	`98.78% <0.00%> (+<0.01%)`	⬆️
aiohttp/web_request.py	`95.71% <0.00%> (+0.01%)`	⬆️
aiohttp/multipart.py	`96.27% <0.00%> (+0.01%)`	⬆️
aiohttp/streams.py	`97.45% <0.00%> (+0.01%)`	⬆️
aiohttp/payload.py	`95.37% <0.00%> (+0.02%)`	⬆️
aiohttp/web.py	`97.43% <0.00%> (+0.02%)`	⬆️
aiohttp/web_response.py	`98.09% <0.00%> (+0.47%)`	⬆️
aiohttp/http_parser.py	`97.15% <0.00%> (+0.65%)`	⬆️
... and 6 more

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 3401bef...c48553c. Read the comment docs.

adamko147 · 2021-01-13T16:15:15Z

@webknjaz thanks! Do I need to open new PR with correct email?

Dreamsorcerer · 2021-01-13T16:18:16Z

I think we should be able to make a simple regression test in test_client_response.py to ensure this doesn't happen again.

webknjaz · 2021-01-13T17:03:10Z

@adamko147 you can set up the correct name and email locally with git config --global user.name 'Adam Horacek' and git config --global user.email '[email protected]'. Then, do git commit --amend --reset-author. After this, your local and remote commits will differ and you'll need to git push --force-with-lease -u origin HEAD:fix-5403 so that it appears rewritten on GitHub. Updating a branch that is attached to a PR will automatically update the PR so you don't need to open a new one, just update your branch.

adamko147 · 2021-01-13T17:17:34Z

@webknjaz thanks, wasn't sure whether I'm able to do force-push. should be ok now

CHANGES/5403.bugfix

webknjaz · 2021-01-13T17:20:45Z

wasn't sure whether I'm able to do force-push. should be ok now

The branch is in your own fork, you can do anything you please there with no restrictions unless you set up your fork to forbid things.

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

adamko147 · 2021-01-13T17:42:37Z

Hi @Dreamsorcerer, I'm looking into the tests... most of the tests there mock the content.read method, however in this case mocking content.read doesn't seems to me right as the exception is raised there based on the content._exception
So I was thinking of test calling response.ok and assert that response.release() has not been called. Do you think it would be ok?

Dreamsorcerer · 2021-01-13T17:46:15Z

Sounds good to me. Probably just a assert not resp.closed() will cover it.

webknjaz · 2021-01-13T23:59:12Z

tests/test_client_response.py

@@ -1243,3 +1243,22 @@ def test_response_links_empty(loop: Any, session: Any) -> None:
    )
    response._headers = CIMultiDict()
    assert response.links == {}
+
+
+def test_response_not_closed_after_get_ok() -> None:


You could use mocker.spy() on response.raise_for_status method to verify that it wasn't called in addition to checking the side-effects.

https://github.com/pytest-dev/pytest-mock#spy

webknjaz · 2021-01-14T11:24:21Z

Hm... Automatic backport failed for some obscure reason: https://github.com/aio-libs/aiohttp/runs/1701449441?check_suite_focus=true#step:3:31.

@adamko147 mind creating a backport PR to 3.8 using cherry_picker?
Ref: https://docs.aiohttp.org/en/latest/contributing.html#backporting

@adamko147

…5407) This change makes it so accessing `ClientResponse.ok` only does the status code check. Prior to this commit, it'd call `ClientResponse.raise_for_status()` which in turn, closed the underlying TCP session whenever the status was 400 or higher making it effectively impossible to keep working with the response, including reading the HTTP response payload. PR #5404 by @adamko147 Fixes #5403 Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit 3250c5d) Co-authored-by: Adam Horacek <[email protected]>

@adamko147

This change makes it so accessing `ClientResponse.ok` only does the status code check. Prior to this commit, it'd call `ClientResponse.raise_for_status()` which in turn, closed the underlying TCP session whenever the status was 400 or higher making it effectively impossible to keep working with the response, including reading the HTTP response payload. PR aio-libs#5404 by @adamko147 Fixes aio-libs#5403 Co-authored-by: Sviatoslav Sydorenko <[email protected]>

@adamko147

This change makes it so accessing `ClientResponse.ok` only does the status code check. Prior to this commit, it'd call `ClientResponse.raise_for_status()` which in turn, closed the underlying TCP session whenever the status was 400 or higher making it effectively impossible to keep working with the response, including reading the HTTP response payload. PR aio-libs#5404 by @adamko147 Fixes aio-libs#5403 Co-authored-by: Sviatoslav Sydorenko <[email protected]>

@adamko147

This change makes it so accessing `ClientResponse.ok` only does the status code check. Prior to this commit, it'd call `ClientResponse.raise_for_status()` which in turn, closed the underlying TCP session whenever the status was 400 or higher making it effectively impossible to keep working with the response, including reading the HTTP response payload. PR aio-libs#5404 by @adamko147 Fixes aio-libs#5403 Co-authored-by: Sviatoslav Sydorenko <[email protected]>

adamko147 requested review from asvetlov and webknjaz as code owners January 13, 2021 16:01

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 13, 2021

fixes #5403

21b5308

webknjaz reviewed Jan 13, 2021

View reviewed changes

CHANGES/5403.bugfix Outdated Show resolved Hide resolved

rephrase based on review suggestion

f9898a9

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

webknjaz added the backport-3.8 label Jan 13, 2021

unit test

fc29651

adamko147 requested a review from webknjaz January 13, 2021 20:42

webknjaz reviewed Jan 13, 2021

View reviewed changes

assert raise_for_status not called

c48553c

webknjaz changed the title ~~ClientResponse.ok property only check the self.status for 400+ codes~~ Eliminate side-effects from the ClientResponse.ok property Jan 14, 2021

webknjaz approved these changes Jan 14, 2021

View reviewed changes

webknjaz merged commit 3250c5d into aio-libs:master Jan 14, 2021

adamko147 deleted the fix-5403 branch January 14, 2021 14:25

adamko147 mentioned this pull request Jan 14, 2021

[3.8] Eliminate side-effects from the ClientResponse.ok property #5407

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate side-effects from the `ClientResponse.ok` property #5404

Eliminate side-effects from the `ClientResponse.ok` property #5404

adamko147 commented Jan 13, 2021

webknjaz commented Jan 13, 2021

codecov bot commented Jan 13, 2021 •

edited

Loading

adamko147 commented Jan 13, 2021

Dreamsorcerer commented Jan 13, 2021

webknjaz commented Jan 13, 2021

adamko147 commented Jan 13, 2021

webknjaz commented Jan 13, 2021

adamko147 commented Jan 13, 2021

Dreamsorcerer commented Jan 13, 2021

webknjaz Jan 13, 2021 •

edited

Loading

webknjaz commented Jan 14, 2021

Eliminate side-effects from the ClientResponse.ok property #5404

Eliminate side-effects from the ClientResponse.ok property #5404

Conversation

adamko147 commented Jan 13, 2021

Are there changes in behavior for the user?

Related issue number

Checklist

webknjaz commented Jan 13, 2021

codecov bot commented Jan 13, 2021 • edited Loading

Codecov Report

adamko147 commented Jan 13, 2021

Dreamsorcerer commented Jan 13, 2021

webknjaz commented Jan 13, 2021

adamko147 commented Jan 13, 2021

webknjaz commented Jan 13, 2021

adamko147 commented Jan 13, 2021

Dreamsorcerer commented Jan 13, 2021

webknjaz Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

webknjaz commented Jan 14, 2021

Eliminate side-effects from the `ClientResponse.ok` property #5404

Eliminate side-effects from the `ClientResponse.ok` property #5404

codecov bot commented Jan 13, 2021 •

edited

Loading

webknjaz Jan 13, 2021 •

edited

Loading