From 539409c5f8df9f45c76149770a3d8a44fbd33d74 Mon Sep 17 00:00:00 2001 From: Paul Sadauskas Date: Thu, 7 Jun 2018 17:05:34 -0600 Subject: [PATCH 1/2] Refactor AutoInflate/Deflate into generic features This makes HTTP::Client, Request and Response unaware of the implementation details behind inflating and deflating stream bodies. Instead, the client iterates over all enabled features on request and response, calling `#wrap_request` or `#wrap_response`, which is expected to return the Request or Response as-is if nothing should be done, or returns a new Request or Response object after having done whatever transformation the feature requires. --- lib/http/client.rb | 14 +++++--- lib/http/feature.rb | 8 +++++ lib/http/features/auto_deflate.rb | 18 ++++++++++ lib/http/features/auto_inflate.rb | 20 +++++++---- lib/http/request.rb | 3 -- lib/http/response.rb | 22 ++++-------- spec/lib/http/features/auto_inflate_spec.rb | 38 +++++++++------------ 7 files changed, 74 insertions(+), 49 deletions(-) diff --git a/lib/http/client.rb b/lib/http/client.rb index 40d309c9..c7ef28ea 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -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? @@ -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 diff --git a/lib/http/feature.rb b/lib/http/feature.rb index 1cffbd87..a7e9e757 100644 --- a/lib/http/feature.rb +++ b/lib/http/feature.rb @@ -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 diff --git a/lib/http/features/auto_deflate.rb b/lib/http/features/auto_deflate.rb index 12b170fa..c5af118d 100644 --- a/lib/http/features/auto_deflate.rb +++ b/lib/http/features/auto_deflate.rb @@ -16,6 +16,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" @@ -36,6 +49,11 @@ def size @compressed.size end + def read(*a) + compress_all! unless @compressed + @compressed.read(*a) + end + def each(&block) return to_enum __method__ unless block diff --git a/lib/http/features/auto_inflate.rb b/lib/http/features/auto_inflate.rb index 5c099dff..1f6ce9f8 100644 --- a/lib/http/features/auto_inflate.rb +++ b/lib/http/features/auto_inflate.rb @@ -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)) end end end diff --git a/lib/http/request.rb b/lib/http/request.rb index 5a4ce713..2fd5dfa7 100644 --- a/lib/http/request.rb +++ b/lib/http/request.rb @@ -87,7 +87,6 @@ def initialize(opts) @proxy = opts[:proxy] || {} @body = Request::Body.new(opts[:body]) - @deflate = opts[:auto_deflate] @version = opts[:version] || "1.1" @headers = HTTP::Headers.coerce(opts[:headers] || {}) @@ -106,7 +105,6 @@ def redirect(uri, verb = @verb) :headers => headers, :proxy => proxy, :body => body.source, - :auto_deflate => @deflate, :version => version ) end @@ -114,7 +112,6 @@ def redirect(uri, verb = @verb) # 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 diff --git a/lib/http/response.rb b/lib/http/response.rb index 3fe2624a..bec0754b 100644 --- a/lib/http/response.rb +++ b/lib/http/response.rb @@ -20,6 +20,9 @@ class Response # @return [Status] attr_reader :status + # @return [String] + attr_reader :version + # @return [Body] attr_reader :body @@ -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 @@ -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 diff --git a/spec/lib/http/features/auto_inflate_spec.rb b/spec/lib/http/features/auto_inflate_spec.rb index 1d21e953..ea764ab4 100644 --- a/spec/lib/http/features/auto_inflate_spec.rb +++ b/spec/lib/http/features/auto_inflate_spec.rb @@ -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 @@ -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 From 5ff08ae8256c7fab360abfb4561fa7f07d97da5f Mon Sep 17 00:00:00 2001 From: Paul Sadauskas Date: Mon, 11 Jun 2018 13:48:06 -0600 Subject: [PATCH 2/2] CompressedBody works just like a regular Request::Body --- lib/http/features/auto_deflate.rb | 13 +++++-------- lib/http/request.rb | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/http/features/auto_deflate.rb b/lib/http/features/auto_deflate.rb index c5af118d..4db92c85 100644 --- a/lib/http/features/auto_deflate.rb +++ b/lib/http/features/auto_deflate.rb @@ -3,6 +3,8 @@ require "zlib" require "tempfile" +require "http/request/body" + module HTTP module Features class AutoDeflate < Feature @@ -38,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 @@ -49,11 +51,6 @@ def size @compressed.size end - def read(*a) - compress_all! unless @compressed - @compressed.read(*a) - end - def each(&block) return to_enum __method__ unless block diff --git a/lib/http/request.rb b/lib/http/request.rb index 2fd5dfa7..23081999 100644 --- a/lib/http/request.rb +++ b/lib/http/request.rb @@ -86,7 +86,7 @@ def initialize(opts) raise(UnsupportedSchemeError, "unknown scheme: #{scheme}") unless SCHEMES.include?(@scheme) @proxy = opts[:proxy] || {} - @body = Request::Body.new(opts[:body]) + @body = (body = opts[:body]).is_a?(Request::Body) ? body : Request::Body.new(body) @version = opts[:version] || "1.1" @headers = HTTP::Headers.coerce(opts[:headers] || {})