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 for Gzip/Inflate #375

Merged
merged 4 commits into from
Dec 25, 2016
Merged

Support for Gzip/Inflate #375

merged 4 commits into from
Dec 25, 2016

Conversation

Bonias
Copy link
Contributor

@Bonias Bonias commented Sep 20, 2016

Hi,

I implemented gzip/inflate decompression. It resolves #113.

Before:

HTTP.headers('Accept-Encoding' => 'gzip').get("https://github.com").to_s[0,12]
 => "\u001F\x8B\b\u0000\u0000\u0000\u0000\u0000\u0000\u0003\xED]"

After:

HTTP.headers('Accept-Encoding' => 'gzip').get("https://github.com").to_s[0,12]
=> "\n\n\n<!DOCTYPE"

Please, let me know if I need to change something here.

@ixti
Copy link
Member

ixti commented Sep 20, 2016

Awesome! Thanks! Couple of notes though...

This should not be auto-enabled. User should explicitly tell to use inflater on incoming data. Probably something like this:

HTTP.use(:auto_inflate).headers("Accept-Encoding" => "gzip").get("https://github.com")

Sometimes one may want not to inflate body and keep it as is. Also it would make sense to add deflate support as well. Second thing is we should NEVER modify response headers, regardless if body was auto-inflated or not.

I see inflater to be an IO wrapper. Or filter. So that it would be possible to re-use for example apart from HTTP gem.

@ixti ixti self-assigned this Sep 20, 2016
@tarcieri
Copy link
Member

This should not be auto-enabled.

👍 on that. Compression over TLS can lead to compression oracles (ala TIME, CRIME, BREACH)

@Bonias
Copy link
Contributor Author

Bonias commented Sep 21, 2016

Second thing is we should NEVER modify response headers, regardless if body was auto-inflated or not

Are you sure about that? The body is no longer encoded, so Content-Encoding header can be misleading. Also it is what Net::HTTP is doing (https://github.com/ruby/ruby/blob/trunk/lib/net/http/response.rb#L257) now. In fact, this is the reason why I implemented it in such way. But I will change it if you certain of that.

@ixti
Copy link
Member

ixti commented Sep 21, 2016

@Bonias Hm. Good call, but I think Net::HTTP is doing that wrong.

@britishtea
Copy link
Contributor

britishtea commented Sep 21, 2016

I think it's worth considering to enable it by default, even going as far as setting Accept-Encoding by default unless:

  • Accept-Encoding is already set
  • Cache-Control is set and contains no-transform
  • the scheme is not "http"

To make it possible for clients to retrieve the compressed body, perhaps Response::Body#to_s can take an optional parameter to disable automatic inflating?

I had a stab at implementing this a while ago, but got stuck on writing proper tests and kind of forgot. Deflate seemed to come in two flavours.

Can be used this way:

    # body will be automatically decoded when Content-Encoding is set to gzip or deflate
    HTTP.use(:auto_inflate).headers("Accept-Encoding" => "gzip").get("https://github.com").to_s

    # request body will be automatically encoded with gzip. Content-Encoding header will be automatically set to gzip
    HTTP.use(:auto_deflate).post("https://github.com", body: "Some body").to_s

    # both features can be used at the same time
    HTTP.use(:auto_inflate, :auto_deflate).headers("Accept-Encoding" => "gzip").post("https://github.com", body: "Some body")

    # deflate encoding can be used to encode body
    HTTP.use(auto_deflate: { method: :deflate }).headers("Accept-Encoding" => "gzip").post("https://github.com", body: "Some body")
@Bonias
Copy link
Contributor Author

Bonias commented Sep 26, 2016

I updated PR. We can turn decoding on by setting use(:auto_inflate):

HTTP.use(:auto_inflate).headers("Accept-Encoding" => "gzip").get("https://github.com").to_s

We can turn encoding on (gzip by default):

HTTP.use(:auto_deflate).post("https://github.com", body: "Some body").to_s

Or both features at once:

HTTP.use(:auto_inflate, :auto_deflate).headers("Accept-Encoding" => "gzip").post("https://github.com", body: "Some body")

We can also use deflate instead of gzip:

HTTP.use(auto_deflate: { method: :deflate }).post("https://github.com", body: "Some body")

Unfortunately rubocop is reporting 3 offenses. I'm not sure how to fix them, but maybe I'll handle that later, when you will not have any other remarks to this PR.

What do you think?

@ixti
Copy link
Member

ixti commented Sep 26, 2016

So far looks good. Have some strange feelings, but can't formulate them yet -will take another deeper look later today or tomorrow.

@Overload119
Copy link

Ship it!

@tarcieri
Copy link
Member

tarcieri commented Oct 2, 2016

This looks good to me.

Regarding the RuboCop offenses, I think we should just disable ClassLength checks on the offending classes.

Re: MethodLength, I'm fine turning that up to 25 globally.

@ixti
Copy link
Member

ixti commented Oct 3, 2016

Sorry got lots of things to do this week. Will provide my input tomorrow.

Increase Metrics/MethodLength to 25
Disable Metrics/ClassLength on HTTP:Options and DummyServer::Servlet

httprb#375 (comment)
@Bonias
Copy link
Contributor Author

Bonias commented Oct 7, 2016

I've changed rubocop config for now.

It seems `Zlib::GzipWriter.wrap` behave differently with JRuby and ruby MRI.
See jruby/jruby#4249.
@Bonias
Copy link
Contributor Author

Bonias commented Oct 26, 2016

I missed JRuby failed tests before. They are fixed now.

@Bonias
Copy link
Contributor Author

Bonias commented Dec 23, 2016

Hi guys. Is there anything else what is blocking this PR?

@tarcieri
Copy link
Member

It looks ok to me. I'm down to merge it if @ixti is

@ixti ixti merged commit 27822cf into httprb:master Dec 25, 2016
@ixti
Copy link
Member

ixti commented Dec 25, 2016

Sorry, completely overlooked this one. Thanks again for all your hard work! Merged!

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Feb 5, 2017
Upstream changes (from CHANGES.md):

## 2.2.0 (2017-02-03)

* [#375](httprb/http#375)
  Add support for automatic Gzip/Inflate
  ([@Bonias])

* [#390](httprb/http#390)
  Add REPORT to the list of valid HTTP verbs
  ([@ixti])


## 2.1.0 (2016-11-08)

* [#370](httprb/http#370)
  Add Headers#include?
  ([@ixti])

* [#364](httprb/http#364)
  Add HTTP::Response#connection
  ([@janko-m])

* [#362](httprb/http#362)
  connect_ssl uses connect_timeout (Closes #359)
  ([@TiagoCardoso1983])
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.

Support for Gzip/Inflate
5 participants