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

Support gzip/deflate transfer encoding when explicit headers are set. #678

Merged

Conversation

znep
Copy link
Contributor

@znep znep commented Nov 28, 2019

httparty relies on Net::HTTP's built in transparent support for gzip and deflate
transfer encoding, however that did not work if you specified your own explicit headers
because calling Net::HTTPHeader#initialize_http_header overwrites the work done in
Net::HTTPGenericRequest#initialize to set the default User-Agent, Accept,
and Accept-Encoding. This also removes the need to duplicate the logic in
Net::HTTP around not decoding the response if the caller has explicitly set
the Accept-Encoding for their own purposes.

This does introduce a slight incompatible change in behavior where
previously specifying any custom headers would omit the default headers,
while with the new behavior you can override the values however
can't omit them entirely.

While I'm here, also remove the test for HEAD requests added in
4797c76 because it was not properly
testing what it claimed to and the code it was trying to test was
removed in 6f6bf6b anyway.

httparty relies on Net::HTTP's built in transparent support for gzip and deflate
transfer encoding, however that did not work if you specified your own explicit headers
because calling Net::HTTPHeader#initialize_http_header overwrites the work done in
Net::HTTPGenericRequest#initialize to set the default User-Agent, Accept,
and Accept-Encoding.  This also removes the need to duplicate the logic in
Net::HTTP around not decoding the response if the caller has explicitly set
the Accept-Encoding for their own purposes.

This does introduce a slight incompatible change in behavior where
previously specifying any custom headers would omit the default headers,
while with the new behavior you can override the values however
can't omit them entirely.

While I'm here, also remove the test for HEAD requests added in
4797c76 because it was not properly
testing what it claimed to and the code it was trying to test was
removed in 6f6bf6b anyway.
@jnunemaker
Copy link
Owner

Overlooked this somehow. Thanks for the research and the fix. This looks ok to me. @TheSmartnik any opinions on this prior to me merging?

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.

None yet

2 participants