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

Better handling of Accept-Encoding / Content-Encoding decompression (fixes #562) #729

Merged
merged 2 commits into from
Jun 29, 2021

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented May 25, 2021

Summary

Better handling of Accept-Encoding / Content-Encoding decompression:

  1. By default, let Net::HTTP always do decompression, if it can.
  2. Add support to decompress "br" (Brotli) and "compress" (LZW) if the required gems are present. These are not supported by Net::HTTP
  3. Add :skip_decompression option to disable both Net::HTTP and HTTParty decompression (i.e. 1 and 2 above)
  4. Add "HTTP Compression" section to docs/README.md

This fixes #562 and reverts some Accept-Encoding behavior in v0.18.x back to how it was in v0.17.3 and prior.

More Background

PR #678, released in v0.18.0, introduced undesirable behavior which broke my app in production.

  • In v0.17.3 and prior, you could set the Accept-Encoding header (e.g. gzip, etc.) in a request, and Net::HTTP would automatically handle decompression the response body in most cases.
  • In v0.18.0+, as a result of PR Support gzip/deflate transfer encoding when explicit headers are set. #678, when setting Accept-Encoding, Net::HTTP no longer performs automatic decompression. This means your response.body will be a gzipped byte-string, and JSON.parse(request.body) will fail. In my case, it actually failed with a process segfault because I use a C-based native JSON parsing lib called Oj.

The Way Forward

The "happy path" for the 99.9% majority of users to party on is to have Net::HTTP auto-decompress whenever it can, even if you set the Accept-Encoding header. The 0.1% of purist users out there can use the new :skip_decompression option which disables auto-decompression in all cases, meaning you'll always get the raw response and the Content-Encoding header so you can roll your own decompression.

This change is fully backward compatible with versions prior to v0.18.0 (i.e. it fixes the 0.17.3 --> 0.18.0 breakage), and mostly compatible with v0.18.0+, with the caveat that users using Accept-Encoding may get an uncompressed response when they previous got a compressed one. It should be released as a minor bump 0.19.0

@johnnyshields johnnyshields changed the title Fix automatic response decompression (fixes #562) WIP: Fix automatic response decompression (fixes #562) May 25, 2021
@johnnyshields johnnyshields changed the title WIP: Fix automatic response decompression (fixes #562) WIP: Fix automatic response decompression + add :skip_decompression option (fixes #562) May 25, 2021
1. Net::HTTP should always decompress, if possible.
2. HTTParty should decompress "br" (Brotli) and "compress" (LZW) if the required gems are present.
3. Add :skip_decompression option to disable both Net::HTTP and HTTParty decompression.
4. Add "HTTP Compression" section to docs/README.md
@johnnyshields
Copy link
Contributor Author

@jnunemaker appreciate if you can review this.

@johnnyshields johnnyshields changed the title WIP: Fix automatic response decompression + add :skip_decompression option (fixes #562) Better handling of Accept-Encoding / Content-Encoding decompression (fixes #562) May 26, 2021
@jnunemaker
Copy link
Owner

I think what you are saying makes sense to me. I don't know my way around this area much. But I'm inclined to merge because you have been so thorough and obviously have more experience than me here.

That said, any ideas why the ssl certificate spec is failing? It does not on master but does on this branch. I even tried locally and got the same result.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Jun 15, 2021

@jnunemaker there should be nothing in this PR which modifies the default behavior. which spec specifically is failing? Could it be related to the fixes in this PR: #702

@jnunemaker
Copy link
Owner

@johnnyshields hmm, I don't think so because master is passing on GitHub and my local machine and that PR isn't merged.

Fix issue with mock
@johnnyshields
Copy link
Contributor Author

@jnunemaker I fixed the issue. It was an issue with RSpec mocks, not with the actual SSL functionality.

@johnnyshields
Copy link
Contributor Author

@jnunemaker any update on this?

@jnunemaker jnunemaker merged commit fb7c403 into jnunemaker:master Jun 29, 2021
@jnunemaker
Copy link
Owner

@johnnyshields thanks again!

@johnnyshields
Copy link
Contributor Author

@jnunemaker thank you. Can you cut this as a new release 0.19.0?

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.

GZip content not automatically being deflated
2 participants