Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions lib/http/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,17 @@ def build_request(verb, uri, opts = {}) # rubocop:disable Style/OptionHash
body = make_request_body(opts, headers)
proxy = opts.proxy

HTTP::Request.new(
req = HTTP::Request.new(
:verb => verb,
:uri => uri,
:headers => headers,
:proxy => proxy,
:body => body,
:auto_deflate => opts.feature(:auto_deflate)
:body => body
)

opts.features.inject(req) do |request, (_name, feature)|
feature.wrap_request(request)
end
end

# @!method persistent?
Expand Down Expand Up @@ -78,10 +81,13 @@ def perform(req, options)
:proxy_headers => @connection.proxy_response_headers,
:connection => @connection,
:encoding => options.encoding,
:auto_inflate => options.feature(:auto_inflate),
:uri => req.uri
)

res = options.features.inject(res) do |response, (_name, feature)|
feature.wrap_response(response)
end

@connection.finish_response if req.verb == :head
@state = :clean

Expand Down
8 changes: 8 additions & 0 deletions lib/http/feature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,13 @@ class Feature
def initialize(opts = {}) # rubocop:disable Style/OptionHash
@opts = opts
end

def wrap_request(request)
request
end

def wrap_response(response)
response
end
end
end
21 changes: 18 additions & 3 deletions lib/http/features/auto_deflate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
require "zlib"
require "tempfile"

require "http/request/body"

module HTTP
module Features
class AutoDeflate < Feature
Expand All @@ -16,6 +18,19 @@ def initialize(*)
raise Error, "Only gzip and deflate methods are supported" unless %w[gzip deflate].include?(@method)
end

def wrap_request(request)
return request unless method

Request.new(
:version => request.version,
:verb => request.verb,
:uri => request.uri,
:headers => request.headers,
:proxy => request.proxy,
:body => deflated_body(request.body)
)
end

def deflated_body(body)
case method
when "gzip"
Expand All @@ -25,9 +40,9 @@ def deflated_body(body)
end
end

class CompressedBody
def initialize(body)
@body = body
class CompressedBody < HTTP::Request::Body
def initialize(uncompressed_body)
@body = uncompressed_body
@compressed = nil
end

Expand Down
20 changes: 14 additions & 6 deletions lib/http/features/auto_inflate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@
module HTTP
module Features
class AutoInflate < Feature
def stream_for(connection, response)
if %w[deflate gzip x-gzip].include?(response.headers[:content_encoding])
Response::Inflater.new(connection)
else
connection
end
def wrap_response(response)
return response unless %w[deflate gzip x-gzip].include?(response.headers[:content_encoding])
Response.new(
:status => response.status,
:version => response.version,
:headers => response.headers,
:proxy_headers => response.proxy_headers,
:connection => response.connection,
:body => stream_for(response.connection)
)
end

def stream_for(connection)
Response::Body.new(Response::Inflater.new(connection))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would make more sense for the Inflater to wrap the Response::Body object instead of Connection? That would be symmetrical to CompressedBody wrapping the Request::Body object. And it makes more sense to me that Response::Body object reads the response body as is, and then the Inflater wrapper changes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. In fact, I think the entire Inflater methods could be moved into this class, similar to how AutoDeflate does it. But, again, I wanted this PR to remain low-impact.

Personally, I'm more interested in being able to add more "features" like these as plugins, like logging/instrumentation or caching. I was reading through the PR history and debating how to monkeypatch it for my own use, when I noticed that with a few minor changes to how the AutoInflate/Deflate features were implemented, then #use could become much more powerful, and enable hooks that I need without moneypatching.

I'd like for this to get merged, and then perhaps used as a jumping-off point for further refactorings and more features. There's also some ergonomic low-hanging-fruit here too (like implementing a Request#merge(opts) method, rather than each feature implementation having to reconstruct the hash by hand. For example, in this file L8-15 could be replaced with response.merge(body: stream_for(response.connection)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes total sense. I do like the gradual approach to refactoring, with keeping the same functionality 👍

end
end
end
Expand Down
5 changes: 1 addition & 4 deletions lib/http/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ def initialize(opts)
raise(UnsupportedSchemeError, "unknown scheme: #{scheme}") unless SCHEMES.include?(@scheme)

@proxy = opts[:proxy] || {}
@body = Request::Body.new(opts[:body])
@deflate = opts[:auto_deflate]
@body = (body = opts[:body]).is_a?(Request::Body) ? body : Request::Body.new(body)
@version = opts[:version] || "1.1"
@headers = HTTP::Headers.coerce(opts[:headers] || {})

Expand All @@ -106,15 +105,13 @@ def redirect(uri, verb = @verb)
:headers => headers,
:proxy => proxy,
:body => body.source,
:auto_deflate => @deflate,
:version => version
)
end

# Stream the request to a socket
def stream(socket)
include_proxy_headers if using_proxy? && !@uri.https?
body = @deflate ? @deflate.deflated_body(self.body) : self.body
Request::Writer.new(socket, body, headers, headline).stream
end

Expand Down
22 changes: 7 additions & 15 deletions lib/http/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ class Response
# @return [Status]
attr_reader :status

# @return [String]
attr_reader :version

# @return [Body]
attr_reader :body

Expand All @@ -46,14 +49,13 @@ def initialize(opts)
@headers = HTTP::Headers.coerce(opts[:headers] || {})
@proxy_headers = HTTP::Headers.coerce(opts[:proxy_headers] || {})

if opts.include?(:connection)
if opts.include?(:body)
@body = opts.fetch(:body)
else
connection = opts.fetch(:connection)
encoding = opts[:encoding] || charset || Encoding::BINARY
stream = body_stream_for(connection, opts)

@body = Response::Body.new(stream, :encoding => encoding)
else
@body = opts.fetch(:body)
@body = Response::Body.new(connection, :encoding => encoding)
end
end

Expand Down Expand Up @@ -160,15 +162,5 @@ def parse(as = nil)
def inspect
"#<#{self.class}/#{@version} #{code} #{reason} #{headers.to_h.inspect}>"
end

private

def body_stream_for(connection, opts)
if opts[:auto_inflate]
opts[:auto_inflate].stream_for(connection, self)
else
connection
end
end
end
end
38 changes: 17 additions & 21 deletions spec/lib/http/features/auto_inflate_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

RSpec.describe HTTP::Features::AutoInflate do
subject { HTTP::Features::AutoInflate.new }
subject(:feature) { HTTP::Features::AutoInflate.new }
let(:connection) { double }
let(:headers) { {} }
let(:response) do
Expand All @@ -13,56 +13,52 @@
)
end

describe "stream_for" do
describe "wrap_response" do
subject(:result) { feature.wrap_response(response) }

context "when there is no Content-Encoding header" do
it "returns connection" do
stream = subject.stream_for(connection, response)
expect(stream).to eq(connection)
it "returns original request" do
expect(result).to be response
end
end

context "for identity Content-Encoding header" do
let(:headers) { {:content_encoding => "not-supported"} }
let(:headers) { {:content_encoding => "identity"} }

it "returns connection" do
stream = subject.stream_for(connection, response)
expect(stream).to eq(connection)
it "returns original request" do
expect(result).to be response
end
end

context "for unknown Content-Encoding header" do
let(:headers) { {:content_encoding => "not-supported"} }

it "returns connection" do
stream = subject.stream_for(connection, response)
expect(stream).to eq(connection)
it "returns original request" do
expect(result).to be response
end
end

context "for deflate Content-Encoding header" do
let(:headers) { {:content_encoding => "deflate"} }

it "returns HTTP::Response::Inflater instance - connection wrapper" do
stream = subject.stream_for(connection, response)
expect(stream).to be_instance_of HTTP::Response::Inflater
it "returns a HTTP::Response wrapping the inflated response body" do
expect(result.body).to be_instance_of HTTP::Response::Body
end
end

context "for gzip Content-Encoding header" do
let(:headers) { {:content_encoding => "gzip"} }

it "returns HTTP::Response::Inflater instance - connection wrapper" do
stream = subject.stream_for(connection, response)
expect(stream).to be_instance_of HTTP::Response::Inflater
it "returns a HTTP::Response wrapping the inflated response body" do
expect(result.body).to be_instance_of HTTP::Response::Body
end
end

context "for x-gzip Content-Encoding header" do
let(:headers) { {:content_encoding => "x-gzip"} }

it "returns HTTP::Response::Inflater instance - connection wrapper" do
stream = subject.stream_for(connection, response)
expect(stream).to be_instance_of HTTP::Response::Inflater
it "returns a HTTP::Response wrapping the inflated response body" do
expect(result.body).to be_instance_of HTTP::Response::Body
end
end
end
Expand Down