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

option to disable automatic client response body decompression #2110

Merged

Conversation

thehesiod
Copy link
Contributor

@thehesiod thehesiod commented Jul 17, 2017

enhancement for #1992

@thehesiod thehesiod changed the title option to disable response body decompression [WIP] option to disable response body decompression Jul 17, 2017
@thehesiod thehesiod changed the title [WIP] option to disable response body decompression [WIP] option to disable automatic response body decompression Jul 17, 2017
@thehesiod thehesiod changed the title [WIP] option to disable automatic response body decompression option to disable automatic response body decompression Jul 18, 2017
@thehesiod
Copy link
Contributor Author

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

@thehesiod thehesiod changed the title option to disable automatic response body decompression option to disable automatic client response body decompression Jul 18, 2017
@codecov-io
Copy link

codecov-io commented Jul 18, 2017

Codecov Report

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

Impacted file tree graph

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

@fafhrd91
Copy link
Member

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

@asvetlov
Copy link
Member

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

@fafhrd91
Copy link
Member

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

@asvetlov
Copy link
Member

Makes sense.

@asvetlov
Copy link
Member

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

def test_auto_gzip_decompress():
with loop_context() as loop:
app = _create_example_app()
with _TestClient(app, loop=loop) as client:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thehesiod
Copy link
Contributor Author

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

Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestions for readability improvement

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@thehesiod thehesiod Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -14,11 +15,20 @@
teardown_test_loop, unittest_run_loop)


def _create_example_app():
_hello_world_str = "Hello, world"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
Copy link
Member

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 asvetlov merged commit bb646d1 into aio-libs:master Jul 29, 2017
@asvetlov
Copy link
Member

Thanks!

@thehesiod thehesiod deleted the thehesiod/disable_decompression branch August 1, 2017 22:58
@lock
Copy link

lock bot commented Oct 28, 2019

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.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
djmitche added a commit to djmitche/aiohttp that referenced this pull request Oct 24, 2020
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.
djmitche added a commit to djmitche/aiohttp that referenced this pull request Oct 24, 2020
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.
asvetlov pushed a commit that referenced this pull request Oct 24, 2020
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.
aio-libs-github-bot bot pushed a commit that referenced this pull request Oct 24, 2020
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants