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

GZip content not automatically being deflated #562

Closed
KNejad opened this issue Nov 9, 2017 · 24 comments · Fixed by #729
Closed

GZip content not automatically being deflated #562

KNejad opened this issue Nov 9, 2017 · 24 comments · Fixed by #729

Comments

@KNejad
Copy link

KNejad commented Nov 9, 2017

When making a HTTParty GZipped request with Accept-Encoding set to gzip, deflate I would expect HTTParty to automatically un-GZip the response. What I am seeing is content which is still GZipped.

To reproduce

Create HTTParty get request with Accept-Encoding set to gzip, deflate as below

r = HTTParty.get("https://example.com", {headers: { "Accept-Encoding" => "gzip, deflate"}})
r.body => # Still GZipped content

Expected result

r = HTTParty.get("https://example.com", {headers: { "Accept-Encoding" => "gzip, deflate"}})
r.body => # Plain text content

This is related to post I created on campaignmonitor/createsend-ruby#58 where they are doing the above.

Edit: The response body has to be over 1KB for the compressions to kick in. Which is where the bug appears

@jnunemaker
Copy link
Owner

What version of ruby and httparty are you using? GZip should be handled by net/http. HTTParty removed its decompression logic because of this. Any of that trigger some thoughts on your end?

@jackross
Copy link

jackross commented Nov 14, 2017

Seeing exactly the same issue.

ruby 2.4.2p198
createsend (4.1.1)
httparty (0.15.6)

FYI, if you remove the header "Accept-Encoding"=>"gzip, deflate" from createsend's generated request headers and then perform the request, you'll get back an uncompressed response body.

i.e.

require 'createsend'

AUTH = { api_key: ENV['API_KEY'] }
client_id = ENV['CLIENT_ID']

client = CreateSend::Client.new AUTH, client_id

response = client.send(:get, 'campaigns')
response.body => # Still GZipped content

request = response.request
request.options.merge!(headers: request.options[:headers].tap { |h| h.delete "Accept-Encoding" })
request.perform.response.body => # Unzipped content

Could it be a Campaign Monitor problem, perhaps? Not well-versed enough in this stuff to know.

@KNejad
Copy link
Author

KNejad commented Nov 14, 2017

@jnunemaker I think @jackross is right. This seems to be an issue with createsend and not httparty.

Create Send probably shouldn't be handling the compression since net/http does it by itself.

For the record here is my version info:

ruby: 2.4.1
httparty: 0.15.6
createsend: 4.1.1

I will leave this issue open for you to determine if it should be closed since I am not entirely sure.

@liu33173847
Copy link

liu33173847 commented Nov 21, 2017

Hello @jackross,

My Ruby and Createsend Gem version as below:

ruby: 2.4.2p198 
httparty: 0.15.6
createsend: 4.1.1

I could not reproduce the issue due to identical response body received with the code you provided, here are my code and the output:

require 'createsend'

AUTH = { api_key: ENV['API_KEY'] }
client_id = ENV['CLIENT_ID']

client = CreateSend::Client.new AUTH, client_id

response = client.send(:get, 'lists')

p"response.body.encoding: #{response.body.encoding}" # UTF-8
p"response.body: #{response.body}" # [{\"ListID\":\"12345679\",\"Name\":\"test\"}]

request = response.request
request.options.merge!(headers: request.options[:headers].tap { |h| h.delete "Accept-Encoding" })
request.perform.response.body 

p"request.perform.response.body.encoding: #{request.perform.response.body.encoding}" # UTF-8
p"request.perform.response.body: #{request.perform.response.body}" # [{\"ListID\":\"12345679\",\"Name\":\"test\"}]

Can you please double check?
Thanks

@KNejad
Copy link
Author

KNejad commented Nov 21, 2017

@liu33173847 The response from Createsend has to be over 1KB. I forgot to mention that in my issue. I have updated it now.

@cmtiml
Copy link

cmtiml commented Nov 27, 2017

Hello @KNejad,

We have merged the change for removing "Accept-Encoding"=>"gzip, deflate" and released it now for version 4.1.2 (https://rubygems.org/gems/createsend).

Please kindly check and verify if it solved the issue.
Thank you very much for your solution to the issue.

@mattprivman
Copy link

Removing Accept-Encoding only disables gzip, which is why that fix for createsend worked.

I think this is still a problem with HTTParty. I can reproduce it on ruby 2.4 and httparty 0.15.6.

@jnunemaker
Copy link
Owner

jnunemaker commented Feb 5, 2018

I am definitely seeing this on 2.4 with the examples provided. When I add handle_deflation back it fixes the problem so I am inclined to do that. I'll try to get a failing spec and PR soon.

fyi @jaypatrickhoward
xref #541

@dvodvo
Copy link

dvodvo commented Jul 28, 2018

I am experienceing this with ruby 2.3 and httparty v 0.16.2. But I am lost a bit here... what is the recommended solution for deflation?

@BookOfGreg
Copy link

Still running into this issue. Not just with CreateSend, but also Azure APIM will not be un-zipped correctly when you send the request with 'Accept-Encoding' => 'gzip,deflate',.

@dvodvo If you DONT send the 'Accept-Encoding', header then it DOES work correctly and is gzipped.

@kellynabours
Copy link

Also just had this issue.
Using with emaildirect gem, which uses httparty with 'Accept-Encoding' => 'gzip,deflate'.
Ruby 2.3.4
Httparty 0.16.2 (also tried with 0.15.6, and 0.15.5).
emaildirect 2.0.0

@philipfong
Copy link

I'm running into this issue as well. When rolling back all the way to 0.14.0 I do not run into this problem. Httparty 0.16.3 and ruby 2.5.1

@TheSmartnik
Copy link
Collaborator

Okay. Here is what is happening.
If you don't specify Accept-Encoding net/http by defaults adds default header

gzip;q=1.0,deflate;q=0.6,identity;q=0.3

But if you do ruby assumes that you want to decode response yourself and does nothing.
HTTParty since version this fix follows the same idea.

It's counter intuitive, confusing and not user friendly.
However, I'm not sure what a correct behavior would be. I'm thinking that maybe when user adds gzip,deflate, we should decode_content anyway, but for other cases leave it as is.
@jnunemaker would love to hear your opinion on this

@jnunemaker
Copy link
Owner

@TheSmartnik Go with your gut. It isn't super clear to me how many people are having problems with how it currently works verse how many people would have problems if we changed it. Any sense how much of an impact this would be? I'm assuming a minor version change for sure. Is that right? I'm all for user friendly.

@BookOfGreg
Copy link

It could just be a judgement call of how many people are likely adding gzip/deflate headers manually (incorrectly) and how many are likely doing it intentionally.

Or not changing the behavior and finding another way of mitigating the incorrect usage, documentation doesn't help much as people only find docs when they're looking, so would a warning at runtime or something obviously visible in logs or debugger reach these people better?

@TheSmartnik
Copy link
Collaborator

@BookOfGreg Yeah, I was thinking about warning at first. However, if header is set intentionally, it would be annoying for users as they got no way to get rid of it

@dvodvo
Copy link

dvodvo commented Jan 25, 2019

Since I re-opened the issue, maybe I should chime in. I am not sure what correct behaviour would be either. However, the documentation aspect did leave me 'wanting'. Usually, the base usage has examples and those, to me, are the best starting points. If that were to illustrate in particular major gotchas, this type of issue would go away.

The number of major gotchas is what would limit the effectiveness of this tactic.

@jaypatrickhoward
Copy link

I'm no longer on the project where I was experiencing this issue, but, if I'm understanding the discussion correctly, here's my two cents:

Make it work "out of the box" for people who are using the headers intentionally and correctly. That is, no special flags need to be passed, etc.

Create a flag that can be passed to signal "workaround mode". This will be used by those who have to accommodate broken server behavior.

thomasleese added a commit to alphagov/gds-api-adapters that referenced this issue Aug 23, 2019
Since rest-client 2.1, the gzip behaviour has changed
rest-client/rest-client#597 such that it relies
on Net::HTTP rather than performing the decompression itself.

This test is currently failing because the body is not getting decoded
correctly, and I can't find a way of getting it to pass. It's blocking
us from adding new features so I thought it would be reasonable to
temporarily comment it out and I'll write up a card for Platform Health
to investigate putting it back.

I'm not sure if the test is actually useful in the first place because
we're testing HTTP behaviour of the Gem, not anything in our code.

I've tried the following things to get it to work:

- Manually adding an `Accept-Encoding` header (although it seems like
this definitely shouldn't work:
jnunemaker/httparty#562 (comment))
- Manually adding a `Content-Type` header.
- Generating the Gziped string with `GzipWriter`.

One thing I've noticed is that for some reason `decode_content` is
always set to false on the Net::HTTPResponse object
(https://ruby-doc.org/stdlib-2.6.3/libdoc/net/http/rdoc/Net/HTTPResponse.html).
@typhoon2099
Copy link

I've just been doing a bit of testing and found that if any headers are supplied then the Accept-Encoding headers are being removed. Tested using https://lumtest.com/echo.json which returns the headers sent with a request. If I specify no headers it sends the Accept-Encoding headers, if I specify any others in the HTTParty.get request (which I need to do to set a User-Agent), when the header is not included and the request is performed without any gzipping at all.

@johnnyshields
Copy link
Contributor

johnnyshields commented May 25, 2021

So, I'm one of those poor schmucks who was setting Accept-Encoding: gzip and upgrading to latest HTTParty broke my app in production.

As I understand it, here are the 3 cases. CAVEAT: I have NOT tested all these yet, just trying to first recap what I'm reading in the thread.

Here is a summary of the behavior as of HTTParty v0.18.1

Case 1: No headers set in HTTParty

  • Current Behavior: (CORRECT AS-IS) Net::HTTP will correctly set Accept-Encoding to "gzip;q=1.0,deflate;q=0.6,identity;q=0.3" and decompress the response.

Case 2: Accept-Encoding: "gzip" explicitly set in HTTParty

  • Current behavior: (NOT IDEAL) The HTTParty::Response#body is returned as a gzipped byte string. For me I think this crashed because it's not automatically deflating the response, and it tried to parse JSON and failed. (BAD)
  • Suggested Improvement: Use Net::Http's gzip handling logic and have HTTParty use only non-gzipped objects. There's no practical reason a user of HTTParty would want to deal with low-level gzip stuff; we could make it a non-default flag to return gzip.

Case 3: Other headers set in HTTParty

  • Current behavior: (CORRECT AS-IS) According to @typhoon2099 's comment, the Net::Http default Accept-Encoding header is not set. EDITED: Same as Case 1; Net::HTTP will correctly set Accept-Encoding to "gzip;q=1.0,deflate;q=0.6,identity;q=0.3" and decompress the response. (@typhoon2099 's comment is not correct.)

@johnnyshields
Copy link
Contributor

johnnyshields commented May 25, 2021

I've raised a PR to fix this issue: #729

jnunemaker added a commit that referenced this issue Jun 29, 2021
Better handling of Accept-Encoding / Content-Encoding decompression (fixes #562)
@zwang-zuora
Copy link

zwang-zuora commented Dec 2, 2022

I am facing the same issue on httparty 0.16.4. I found it could be workaround by adding a header Accept-encoding : gzip, deflate, br.
The header name can't be Accept-Encoding or accept-encoding.
The idea is to utilize a bug of Httparty prior to 0.18.0(not included)
https://github.com/jnunemaker/httparty/blob/v0.16.4/lib/httparty/request.rb#L218

      if @raw_request.respond_to?(:decode_content) && (headers_hash.key?('Accept-Encoding') || headers_hash.key?('accept-encoding'))
        # Using the '[]=' sets decode_content to false
        @raw_request['accept-encoding'] = @raw_request['accept-encoding']
      end

@johnnyshields
Copy link
Contributor

@Zhen-w you should upgrade to at least httparty 0.19.0 to get the proper fix for this. In order versions the behavior was plainly wrong/counterintuitive.

@zwang-zuora
Copy link

I tried, but it's a quite big effort for us to upgrade the lib since our infra lib depends on the version.
Anyway, I would raise the issue to our infra team. Thank you @johnnyshields .

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 a pull request may close this issue.