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

Not possible to do requests with pre-compressed data #619

Closed
mwfrojdman opened this issue Nov 3, 2015 · 5 comments
Closed

Not possible to do requests with pre-compressed data #619

mwfrojdman opened this issue Nov 3, 2015 · 5 comments
Labels
Milestone

Comments

@mwfrojdman
Copy link
Contributor

Currently aiohttp always compresses the request data if the Content-Encoding header is set. If the data is already compressed, aiohttp will compress it a second time. Example:

url = 'http://some.host/'
deflated_data = zlib.compress(b'mydata')
headers = {'Content-Encoding': 'deflate'}
yield from aiohttp.request('POST', url, data=deflated_data, headers=headers)

Looking at client_reqrep, self.update_content_encoding() is called in ClientRequest.init() on line 88. The method first checks if the Content-Encoding header is set, and if it is, self.compress is set to that value and chunking turned on. Otherwise, self.compress (originally set in init() from the constructor's argument with the same name) is used.

Later in send(), request.add_compression_filter(self.compress) is called on line 460 if self.compress is set. This leads to the double compression.

Suggestion: Only do compression of data in aiohttp if init() is called with the compress argument. If the Content-Encoding header is set, leave it to the user to supply compressed data.

I ran into this user when looking at the raven-python's aiohttp transport (used to send exceptions & al to Sentry at https://github.com/getsentry/raven-aiohttp/blob/c35ca66379f00d58cb8404b51f845f0e754c91b5/raven_aiohttp.py#L41 which gets the data already compressed and Content-Encoding header set.

@asvetlov
Copy link
Member

asvetlov commented Nov 3, 2015

Well, we may assign ClientRequest.compress to False if Content-Encoding header is set and ClientSession.request() has compress=False parameter.
compress=True enforces compression, compress=None respects Content-Encoding.
The proposal keeps backward compatibility I believe.
The single line need to be changed: instead of https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_reqrep.py#L234 we can add something like:

if self.compress is not False:
    self.compress = enc

Is that satisfies your needs?
If yes -- would you provide a Pull Request?
PR requires test(s) and documentation update.

@mwfrojdman
Copy link
Contributor Author

That would work. Though I'm also thinking that providing two ways in the API to instruct aiohttp to compress data makes its use more inconsistent among users: post(url, data=data, compress='deflate') does the exact same thing as post(url, data=data, headers={'Content-Encoding': 'deflate'}.

Backwards compatibility may be an issue though, so I'm preparing a pull req with the change you suggested.

@mwfrojdman
Copy link
Contributor Author

Created pull req: #621

@asvetlov asvetlov added this to the 0.19 milestone Nov 4, 2015
@asvetlov
Copy link
Member

asvetlov commented Nov 4, 2015

Done by #621

@lock
Copy link

lock bot commented Oct 29, 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 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants