option to disable automatic client response body decompression by thehesiod · Pull Request #2110 · aio-libs/aiohttp

thehesiod · 2017-07-17T23:55:26Z

enhancement for #1992

thehesiod · 2017-07-18T01:24:48Z

this PR only handles ClientResponse, other functionality can be added in separate PRs

codecov-io · 2017-07-18T02:57:44Z

Codecov Report

Merging #2110 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
+ Coverage   97.07%   97.07%   +<.01%     
==========================================
  Files          38       38              
  Lines        7723     7733      +10     
  Branches     1346     1347       +1     
==========================================
+ Hits         7497     7507      +10     
  Misses        102      102              
  Partials      124      124

Impacted Files	Coverage Δ
aiohttp/client_proto.py	`91.66% <ø> (ø)`	⬆️
aiohttp/client_reqrep.py	`97.2% <100%> (+0.01%)`	⬆️
aiohttp/http_parser.py	`97.25% <100%> (+0.01%)`	⬆️
aiohttp/web_urldispatcher.py	`99.42% <0%> (ø)`	⬆️
aiohttp/client_exceptions.py	`100% <0%> (ø)`	⬆️
aiohttp/web.py	`99.65% <0%> (ø)`	⬆️
aiohttp/test_utils.py	`98.6% <0%> (ø)`	⬆️
tests/autobahn/client.py	`96.85% <0%> (ø)`	⬆️
aiohttp/web_request.py	`98.94% <0%> (+0.01%)`	⬆️

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 91dc5b7...75f7814. Read the comment docs.

Pin version to 2.3

fafhrd91 · 2017-07-26T17:19:22Z

Is this too fine grained? Should it be just "disable all incoming stream transformations" setting?

asvetlov · 2017-07-26T18:02:53Z

Good note!
Do you object parameter name, isn't it?

fafhrd91 · 2017-07-26T18:06:30Z

right now aiohttp applies two type of transformation, transfer-encoding and content-encoding.
I'd disable both with one parameter.

asvetlov · 2017-07-26T18:09:53Z

Makes sense.

asvetlov · 2017-07-26T18:14:37Z

@cecton could you take a look on failed tests?
Sure they could accept loop fixture explicitly but I worry about backward degradation in our test tools.
Looks like the PR was passed without errors unless I've merged your changes into master.

cecton · 2017-07-26T18:31:35Z

tests/test_test_utils.py

+def test_auto_gzip_decompress():
+    with loop_context() as loop:
+        app = _create_example_app()
+        with _TestClient(app, loop=loop) as client:


@asvetlov Here is the problem. If you really want to use TestClient directly, you need a TestServer instance. It's best to use the test_client fixture if you can:

def test_auto_gzip_decompress(test_client): app = _create_example_app() with test_client(app) as client:

Otherwise:

with loop_context() as loop: app = _create_example_app() with _TestClient(_TestServer(app, loop=loop), loop=loop) as client:

I see no significant degradation.
@thehesiod please update tests to use test_client fixture explicitly

I've updated the tests to run like the tests surrounding it, I can't use test_client fixture because I use _create_example_app

@thehesiod I don't understand the problem. As far as I know you can give the app created by _create_example_app as argument to test_client. The fixture gives you a function, not a client instance.

I must have been looking at an old version of that function, in either case all the functions should be updated, so if @asvetlov says ok I can change them all to use this fixture

The fixture test_client won't work... I just realized that this fixture is overridden in the test file itself.

thehesiod · 2017-07-26T22:00:10Z

ya I think botocore may need transfer-encoding disabled as well. I'll try working on that today.

cecton

Small suggestions for readability improvement

cecton · 2017-07-27T09:05:56Z

tests/test_test_utils.py

+                resp = yield from client.request("GET", "/gzip_hello")
+                assert resp.status == 200
+                data = yield from resp.read()
+                assert data == _hello_world_gz


This test coroutine is duplicated at a lot of different places. Maybe you can make a helper outside the test and provide the client instance in argument. I think it would be more readable. What do you think?

async def _assertBody(client, url, body): resp = await ... assert resp.status == 200 data = await ... assert data == body

I've been avoiding too much refactoring because @asvetlov has frowned upon that in the past in PRs that implement new features. He's wanted these types of changes in multiple PRs. If he gives the go-ahead I can do these suggested changes.

cecton · 2017-07-27T09:10:15Z

tests/test_test_utils.py



-def _create_example_app():
+_hello_world_str = "Hello, world"


At this rate you can even generate a random string. Like:

uuid.uuid4().hex

I would actually even put the str, bytes and gzip inside the app instance:

app = web.Application() app["body_str"] = uuid.uuid4().hex app["body_bytes"] = app["body_str"].encode('utf-8') app["body_gz"] = gzip.compress(app["body_bytes"])

thehesiod · 2017-07-27T16:35:50Z

@fafhrd91 @asvetlov are there any aiohttp examples of client/server gzip transfer encoding? I don't see any tests or examples in the web.

fafhrd91 · 2017-07-27T17:02:35Z

what do you mean by example? implementation is shared between client and server,
http_parser.DeflateBuffer and http_writer.PayloadWriter for reading and for writing

asvetlov · 2017-07-29T15:21:56Z

Thanks!

lock · 2019-10-28T20:03:43Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

Fix a variable-shadowing bug causing `ThreadedResolver.resolve` to return the resolved IP as the "hostname" in each record, which prevented validation of HTTPS connections. Fixes aio-libs#2110.

Fix a variable-shadowing bug causing `ThreadedResolver.resolve` to return the resolved IP as the "hostname" in each record, which prevented validation of HTTPS connections. Fixes #2110.

thehesiod added 7 commits May 22, 2017 15:11

Merge remote-tracking branch 'aio-libs/master'

a647b47

Merge branch 'aio-libs/master'

dd56884

Merge branch 'aio-libs/master'

5d681f6

Merge branch 'aio-libs/master'

2b90858

Merge branch 'aio-libs/master'

b16fafe

Merge branch 'aio-libs/master'

d78d618

work on feature to disable automatic response body decompression

72fe4c4

thehesiod changed the title ~~option to disable response body decompression~~ [WIP] option to disable response body decompression Jul 17, 2017

thehesiod changed the title ~~[WIP] option to disable response body decompression~~ [WIP] option to disable automatic response body decompression Jul 17, 2017

thehesiod added 4 commits July 17, 2017 17:02

add a missing folder to clean

201cfc3

bugfix

115e414

rename

5559504

add unittests

109c229

thehesiod changed the title ~~[WIP] option to disable automatic response body decompression~~ option to disable automatic response body decompression Jul 18, 2017

thehesiod mentioned this pull request Jul 18, 2017

add support for compressed responses aio-libs/aiobotocore#298

Closed

flake fixes

5f025a4

thehesiod changed the title ~~option to disable automatic response body decompression~~ option to disable automatic client response body decompression Jul 18, 2017

thehesiod added 4 commits July 17, 2017 18:47

another pep fix

406db0a

fix unittest

7f451ef

use constants

1e482da

fix spelling

22c8ba7

asvetlov added 2 commits July 26, 2017 20:03

Update client_reference.rst

2f57548

Pin version to 2.3

Merge branch 'master' into thehesiod/disable_decompression

979585b

cecton reviewed Jul 26, 2017

View reviewed changes

fix unittests

15030ba

cecton reviewed Jul 27, 2017

View reviewed changes

fix test

7a9240a

pep

75f7814

asvetlov merged commit bb646d1 into aio-libs:master Jul 29, 2017

thehesiod deleted the thehesiod/disable_decompression branch August 1, 2017 22:58

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Uh oh!

Conversation

thehesiod commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thehesiod commented Jul 18, 2017

Uh oh!

codecov-io commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fafhrd91 commented Jul 26, 2017

Uh oh!

asvetlov commented Jul 26, 2017

Uh oh!

fafhrd91 commented Jul 26, 2017

Uh oh!

asvetlov commented Jul 26, 2017

Uh oh!

asvetlov commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thehesiod commented Jul 26, 2017

Uh oh!

cecton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thehesiod Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thehesiod commented Jul 27, 2017

Uh oh!

fafhrd91 commented Jul 27, 2017

Uh oh!

asvetlov commented Jul 29, 2017

Uh oh!

lock bot commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thehesiod commented Jul 17, 2017 •

edited

Loading

codecov-io commented Jul 18, 2017 •

edited

Loading

thehesiod Jul 27, 2017 •

edited

Loading