From 36ff4a5c3e13323d22f2a80a6d9f0e591ca41395 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 20:26:26 -0600 Subject: [PATCH 01/10] Refactor net_http and net_http_persistent adapters to use #connection --- lib/faraday/adapter.rb | 13 +++++ lib/faraday/adapter/net_http.rb | 54 +++++++++---------- lib/faraday/adapter/net_http_persistent.rb | 2 +- .../adapter/net_http_persistent_spec.rb | 8 +-- spec/faraday/adapter/net_http_spec.rb | 4 +- 5 files changed, 46 insertions(+), 35 deletions(-) diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index 6a3bb7d5c..dd547df84 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -46,6 +46,19 @@ def initialize(_app = nil, opts = {}, &block) @config_block = block end + # Yields an adapter's configured connection. Depends on #build_connection + # being defined on this adapter. + # + # @param env [Faraday::Env, Hash] The env object for a faraday request. + # + # @return An HTTP client connection for this adapter. + def connection(env) + conn = build_connection(env) + return conn unless block_given? + + yield conn + end + def call(env) env.clear_body if env.needs_body? env.response = Response.new diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index d5c0061d0..05d776c11 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -40,16 +40,26 @@ def initialize(app = nil, opts = {}, &block) super(app, opts, &block) end - def call(env) - super - with_net_http_connection(env) do |http| - if (env[:url].scheme == 'https') && env[:ssl] - configure_ssl(http, env[:ssl]) - end + def build_connection(env) + klass = if (proxy = env[:request][:proxy]) + Net::HTTP::Proxy(proxy[:uri].hostname, proxy[:uri].port, + proxy[:user], proxy[:password]) + else + Net::HTTP + end + port = env[:url].port || (env[:url].scheme == 'https' ? 443 : 80) + klass.new(env[:url].hostname, port).tap do |http| + configure_ssl(http, env[:ssl]) configure_request(http, env[:request]) + http.use_ssl = env[:url].scheme == 'https' + end + end + def call(env) + super + http_response = connection(env) do |http| begin - http_response = perform_request(http, env) + perform_request(http, env) rescue *NET_HTTP_EXCEPTIONS => e if defined?(OpenSSL) && e.is_a?(OpenSSL::SSL::SSLError) raise Faraday::SSLError, e @@ -57,13 +67,13 @@ def call(env) raise Faraday::ConnectionFailed, e end + end - save_response(env, http_response.code.to_i, - http_response.body || '', nil, - http_response.message) do |response_headers| - http_response.each_header do |key, value| - response_headers[key] = value - end + save_response(env, http_response.code.to_i, + http_response.body || '', nil, + http_response.message) do |response_headers| + http_response.each_header do |key, value| + response_headers[key] = value end end @@ -134,23 +144,9 @@ def request_via_request_method(http, env, &block) end end - def with_net_http_connection(env) - yield net_http_connection(env) - end - - def net_http_connection(env) - klass = if (proxy = env[:request][:proxy]) - Net::HTTP::Proxy(proxy[:uri].hostname, proxy[:uri].port, - proxy[:user], proxy[:password]) - else - Net::HTTP - end - port = env[:url].port || (env[:url].scheme == 'https' ? 443 : 80) - klass.new(env[:url].hostname, port) - end - def configure_ssl(http, ssl) - http.use_ssl = true + return unless ssl + http.verify_mode = ssl_verify_mode(ssl) http.cert_store = ssl_cert_store(ssl) diff --git a/lib/faraday/adapter/net_http_persistent.rb b/lib/faraday/adapter/net_http_persistent.rb index 1cf1a295d..1b1b3e4e4 100644 --- a/lib/faraday/adapter/net_http_persistent.rb +++ b/lib/faraday/adapter/net_http_persistent.rb @@ -8,7 +8,7 @@ class NetHttpPersistent < NetHttp private - def net_http_connection(env) + def build_connection(env) @cached_connection ||= if Net::HTTP::Persistent.instance_method(:initialize) .parameters.first == %i[key name] diff --git a/spec/faraday/adapter/net_http_persistent_spec.rb b/spec/faraday/adapter/net_http_persistent_spec.rb index 19b54d7a2..cdf85a134 100644 --- a/spec/faraday/adapter/net_http_persistent_spec.rb +++ b/spec/faraday/adapter/net_http_persistent_spec.rb @@ -12,7 +12,7 @@ http.idle_timeout = 123 end - http = adapter.send(:net_http_connection, url: url, request: {}) + http = adapter.send(:connection, url: url, request: {}) adapter.send(:configure_request, http, {}) expect(http.idle_timeout).to eq(123) @@ -23,7 +23,7 @@ adapter = described_class.new - http = adapter.send(:net_http_connection, url: url, request: {}) + http = adapter.send(:connection, url: url, request: {}) adapter.send(:configure_request, http, {}) # `max_retries=` is only present in Ruby 2.5 @@ -35,7 +35,7 @@ adapter = described_class.new(nil, pool_size: 5) - http = adapter.send(:net_http_connection, url: url, request: {}) + http = adapter.send(:connection, url: url, request: {}) # `pool` is only present in net_http_persistent >= 3.0 expect(http.pool.size).to eq(5) if http.respond_to?(:pool) @@ -47,7 +47,7 @@ adapter = described_class.new(nil) - http = adapter.send(:net_http_connection, url: url, request: {}) + http = adapter.send(:connection, url: url, request: {}) adapter.send(:configure_ssl, http, min_version: :TLS1_2) # `min_version` is only present in net_http_persistent >= 3.1 (UNRELEASED) diff --git a/spec/faraday/adapter/net_http_spec.rb b/spec/faraday/adapter/net_http_spec.rb index 2197ab180..c24586cb3 100644 --- a/spec/faraday/adapter/net_http_spec.rb +++ b/spec/faraday/adapter/net_http_spec.rb @@ -8,14 +8,16 @@ context 'checking http' do let(:url) { URI('http://example.com') } let(:adapter) { described_class.new } - let(:http) { adapter.send(:net_http_connection, url: url, request: {}) } + let(:http) { adapter.send(:connection, url: url, request: {}) } it { expect(http.port).to eq(80) } + it 'sets max_retries to 0' do adapter.send(:configure_request, http, {}) expect(http.max_retries).to eq(0) if http.respond_to?(:max_retries=) end + it 'supports write_timeout' do adapter.send(:configure_request, http, write_timeout: 10) From 86d3cf2e8b3d3ccbc0aa07479bc5a29b17389127 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 20:38:20 -0600 Subject: [PATCH 02/10] Refactor excon adapter to use #connection --- lib/faraday/adapter/excon.rb | 21 ++++++++++----------- spec/faraday/adapter/excon_spec.rb | 2 +- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index b0ea0d7d7..0c1ef2afb 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -6,15 +6,19 @@ class Adapter class Excon < Faraday::Adapter dependency 'excon' + def build_connection(env) + opts = opts_from_env(env) + ::Excon.new(env[:url].to_s, opts.merge(@connection_options)) + end + def call(env) super - opts = opts_from_env(env) - conn = create_connection(env, opts) - - resp = conn.request(method: env[:method].to_s.upcase, - headers: env[:request_headers], - body: read_body(env)) + resp = connection(env) do |http| + http.request(method: env[:method].to_s.upcase, + headers: env[:request_headers], + body: read_body(env)) + end req = env[:request] if req&.stream_response? @@ -36,11 +40,6 @@ def call(env) raise Faraday::TimeoutError, e end - # @return [Excon] - def create_connection(env, opts) - ::Excon.new(env[:url].to_s, opts.merge(@connection_options)) - end - # TODO: support streaming requests def read_body(env) env[:body].respond_to?(:read) ? env[:body].read : env[:body] diff --git a/spec/faraday/adapter/excon_spec.rb b/spec/faraday/adapter/excon_spec.rb index fee13d052..c6d16ae2f 100644 --- a/spec/faraday/adapter/excon_spec.rb +++ b/spec/faraday/adapter/excon_spec.rb @@ -10,7 +10,7 @@ adapter = described_class.new(nil, debug_request: true) - conn = adapter.create_connection({ url: url }, {}) + conn = adapter.build_connection(url: url) expect(conn.data[:debug_request]).to be_truthy end From bfdb66ab710a02b5077d3ceaa008f915d880efd5 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 19 Sep 2019 20:44:18 -0600 Subject: [PATCH 03/10] Refactor httpclient adapter to use #connection --- lib/faraday/adapter/httpclient.rb | 67 +++++++++++++------------ spec/faraday/adapter/httpclient_spec.rb | 4 +- 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/lib/faraday/adapter/httpclient.rb b/lib/faraday/adapter/httpclient.rb index e0ac23c7e..43d98fb9d 100644 --- a/lib/faraday/adapter/httpclient.rb +++ b/lib/faraday/adapter/httpclient.rb @@ -6,51 +6,54 @@ class Adapter class HTTPClient < Faraday::Adapter dependency 'httpclient' - # @return [HTTPClient] - def client - @client ||= ::HTTPClient.new - end - - def call(env) - super - - # enable compression - client.transparent_gzip_decompression = true + def build_connection(env) + @client ||= ::HTTPClient.new.tap do |cli| + # enable compression + cli.transparent_gzip_decompression = true + end if (req = env[:request]) if (proxy = req[:proxy]) - configure_proxy proxy + configure_proxy @client, proxy end if (bind = req[:bind]) - configure_socket bind + configure_socket @client, bind end - configure_timeouts req + configure_timeouts @client, req end if env[:url].scheme == 'https' && (ssl = env[:ssl]) - configure_ssl ssl + configure_ssl @client, ssl end - configure_client + configure_client @client + + @client + end + + def call(env) + super # TODO: Don't stream yet. # https://github.com/nahi/httpclient/pull/90 env[:body] = env[:body].read if env[:body].respond_to? :read - resp = client.request env[:method], env[:url], + connection(env) do |http| + resp = http.request env[:method], env[:url], body: env[:body], header: env[:request_headers] - if (req = env[:request]).stream_response? - warn "Streaming downloads for #{self.class.name} " \ - 'are not yet implemented.' - req.on_data.call(resp.body, resp.body.bytesize) - end - save_response env, resp.status, resp.body, resp.headers, resp.reason + if (req = env[:request]).stream_response? + warn "Streaming downloads for #{self.class.name} " \ + 'are not yet implemented.' + req.on_data.call(resp.body, resp.body.bytesize) + end + save_response env, resp.status, resp.body, resp.headers, resp.reason - @app.call env + @app.call env + end rescue ::HTTPClient::TimeoutError, Errno::ETIMEDOUT raise Faraday::TimeoutError, $ERROR_INFO rescue ::HTTPClient::BadResponseError => e @@ -71,7 +74,7 @@ def call(env) end # @param bind [Hash] - def configure_socket(bind) + def configure_socket(client, bind) client.socket_local.host = bind[:host] client.socket_local.port = bind[:port] end @@ -79,7 +82,7 @@ def configure_socket(bind) # Configure proxy URI and any user credentials. # # @param proxy [Hash] - def configure_proxy(proxy) + def configure_proxy(client, proxy) client.proxy = proxy[:uri] return unless proxy[:user] && proxy[:password] @@ -87,7 +90,7 @@ def configure_proxy(proxy) end # @param ssl [Hash] - def configure_ssl(ssl) + def configure_ssl(client, ssl) ssl_config = client.ssl_config ssl_config.verify_mode = ssl_verify_mode(ssl) ssl_config.cert_store = ssl_cert_store(ssl) @@ -100,23 +103,23 @@ def configure_ssl(ssl) end # @param req [Hash] - def configure_timeouts(req) - configure_timeout(req) if req[:timeout] - configure_open_timeout(req) if req[:open_timeout] + def configure_timeouts(client, req) + configure_timeout(client, req) if req[:timeout] + configure_open_timeout(client, req) if req[:open_timeout] end - def configure_timeout(req) + def configure_timeout(client, req) client.connect_timeout = req[:timeout] client.receive_timeout = req[:timeout] client.send_timeout = req[:timeout] end - def configure_open_timeout(req) + def configure_open_timeout(client, req) client.connect_timeout = req[:open_timeout] client.send_timeout = req[:open_timeout] end - def configure_client + def configure_client(client) @config_block&.call(client) end diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index d3fb5f0e5..43dd8a0d4 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -12,9 +12,7 @@ client.ssl_config.timeout = 25 end - client = adapter.client - adapter.configure_client - + client = adapter.build_connection(url: URI.parse('https://example.com')) expect(client.keep_alive_timeout).to eq(20) expect(client.ssl_config.timeout).to eq(25) end From bfe360a0a7000618dbdf837246448e0e9a65be9a Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 20 Sep 2019 04:25:02 -0600 Subject: [PATCH 04/10] Refactor patron adapter to use #connection --- lib/faraday/adapter/patron.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/faraday/adapter/patron.rb b/lib/faraday/adapter/patron.rb index 3020dcf4b..3df857c2b 100644 --- a/lib/faraday/adapter/patron.rb +++ b/lib/faraday/adapter/patron.rb @@ -6,11 +6,7 @@ class Adapter class Patron < Faraday::Adapter dependency 'patron' - def call(env) - super - # TODO: support streaming requests - env[:body] = env[:body].read if env[:body].respond_to? :read - + def build_connection(env) session = ::Patron::Session.new @config_block&.call(session) if (env[:url].scheme == 'https') && env[:ssl] @@ -33,7 +29,15 @@ def call(env) end end - response = begin + session + end + + def call(env) + super + # TODO: support streaming requests + env[:body] = env[:body].read if env[:body].respond_to? :read + + response = connection(env) do |session| data = env[:body] ? env[:body].to_s : nil session.request(env[:method], env[:url].to_s, env[:request_headers], data: data) From ba7b82d9a2211c4fdd840189e246dbd7a6e4c229 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 20 Sep 2019 05:32:14 -0600 Subject: [PATCH 05/10] lint: fix alignment and rescue stmt --- lib/faraday/adapter/patron.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/faraday/adapter/patron.rb b/lib/faraday/adapter/patron.rb index 3df857c2b..976334c15 100644 --- a/lib/faraday/adapter/patron.rb +++ b/lib/faraday/adapter/patron.rb @@ -38,11 +38,13 @@ def call(env) env[:body] = env[:body].read if env[:body].respond_to? :read response = connection(env) do |session| - data = env[:body] ? env[:body].to_s : nil - session.request(env[:method], env[:url].to_s, - env[:request_headers], data: data) - rescue Errno::ECONNREFUSED, ::Patron::ConnectionFailed - raise Faraday::ConnectionFailed, $ERROR_INFO + begin + data = env[:body] ? env[:body].to_s : nil + session.request(env[:method], env[:url].to_s, + env[:request_headers], data: data) + rescue Errno::ECONNREFUSED, ::Patron::ConnectionFailed + raise Faraday::ConnectionFailed, $ERROR_INFO + end end if (req = env[:request]).stream_response? From f8615d5255534a85240f35ece7e3aff09e9ae051 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 20 Sep 2019 07:30:21 -0600 Subject: [PATCH 06/10] net_http and net_http_persistent share _some_ config code --- lib/faraday/adapter/net_http.rb | 16 +++++++++++----- lib/faraday/adapter/net_http_persistent.rb | 4 +++- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/faraday/adapter/net_http.rb b/lib/faraday/adapter/net_http.rb index 05d776c11..209fab765 100644 --- a/lib/faraday/adapter/net_http.rb +++ b/lib/faraday/adapter/net_http.rb @@ -41,6 +41,16 @@ def initialize(app = nil, opts = {}, &block) end def build_connection(env) + net_http_connection(env).tap do |http| + if http.respond_to?(:use_ssl=) + http.use_ssl = env[:url].scheme == 'https' + end + configure_ssl(http, env[:ssl]) + configure_request(http, env[:request]) + end + end + + def net_http_connection(env) klass = if (proxy = env[:request][:proxy]) Net::HTTP::Proxy(proxy[:uri].hostname, proxy[:uri].port, proxy[:user], proxy[:password]) @@ -48,11 +58,7 @@ def build_connection(env) Net::HTTP end port = env[:url].port || (env[:url].scheme == 'https' ? 443 : 80) - klass.new(env[:url].hostname, port).tap do |http| - configure_ssl(http, env[:ssl]) - configure_request(http, env[:request]) - http.use_ssl = env[:url].scheme == 'https' - end + klass.new(env[:url].hostname, port) end def call(env) diff --git a/lib/faraday/adapter/net_http_persistent.rb b/lib/faraday/adapter/net_http_persistent.rb index 1b1b3e4e4..2465ad93c 100644 --- a/lib/faraday/adapter/net_http_persistent.rb +++ b/lib/faraday/adapter/net_http_persistent.rb @@ -8,7 +8,7 @@ class NetHttpPersistent < NetHttp private - def build_connection(env) + def net_http_connection(env) @cached_connection ||= if Net::HTTP::Persistent.instance_method(:initialize) .parameters.first == %i[key name] @@ -73,6 +73,8 @@ def perform_request(http, env) }.freeze def configure_ssl(http, ssl) + return unless ssl + http_set(http, :verify_mode, ssl_verify_mode(ssl)) http_set(http, :cert_store, ssl_cert_store(ssl)) From 83d933a61e802c897d92740dc43b831d947bd9de Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 14 Oct 2019 14:35:14 -0600 Subject: [PATCH 07/10] fix bad merge --- lib/faraday/adapter/excon.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/faraday/adapter/excon.rb b/lib/faraday/adapter/excon.rb index d59160b3f..e1b7eefc4 100644 --- a/lib/faraday/adapter/excon.rb +++ b/lib/faraday/adapter/excon.rb @@ -14,9 +14,6 @@ def build_connection(env) def call(env) super - opts = opts_from_env(env) - conn = create_connection(env, opts) - req_opts = { method: env[:method].to_s.upcase, headers: env[:request_headers], @@ -31,7 +28,7 @@ def call(env) end end - resp = conn.request(req_opts) + resp = connection(env) { |http| http.request(req_opts) } save_response(env, resp.status.to_i, resp.body, resp.headers, resp.reason_phrase) From 0294fed33c91efa624582bcaa58f047e695184c6 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 17 Oct 2019 10:23:28 -0600 Subject: [PATCH 08/10] Forgot to git-add these --- spec/faraday/adapter/httpclient_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index 4993466b2..15155fc1d 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -11,6 +11,7 @@ it_behaves_like 'an adapter' + it 'allows to provide adapter specific configs' do adapter = described_class.new do |client| client.keep_alive_timeout = 20 @@ -27,13 +28,13 @@ let(:env) { { request: request } } let(:options) { {} } let(:adapter) { Faraday::Adapter::HTTPClient.new } - let(:client) { adapter.client } + let(:client) { adapter.connection({url: URI.parse('https://example.com')}) } it 'configures timeout' do assert_default_timeouts! request.timeout = 5 - adapter.configure_timeouts(request) + adapter.configure_timeouts(client, request) expect(client.connect_timeout).to eq(5) expect(client.send_timeout).to eq(5) @@ -44,7 +45,7 @@ assert_default_timeouts! request.open_timeout = 1 - adapter.configure_timeouts(request) + adapter.configure_timeouts(client, request) expect(client.connect_timeout).to eq(1) expect(client.send_timeout).to eq(HTTPCLIENT_WRITE) @@ -57,7 +58,7 @@ request.open_timeout = 1 request.write_timeout = 10 request.read_timeout = 5 - adapter.configure_timeouts(request) + adapter.configure_timeouts(client, request) expect(client.connect_timeout).to eq(1) expect(client.send_timeout).to eq(10) From b1b617e53f28ba50ddcec7149d25c4a63f190c74 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 17 Oct 2019 10:24:03 -0600 Subject: [PATCH 09/10] Correct documentation to show #connection returns the block return value. This is so that it returns an HTTP response object from the client lib. --- lib/faraday/adapter.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/faraday/adapter.rb b/lib/faraday/adapter.rb index 1af7df604..c16227512 100644 --- a/lib/faraday/adapter.rb +++ b/lib/faraday/adapter.rb @@ -46,12 +46,13 @@ def initialize(_app = nil, opts = {}, &block) @config_block = block end - # Yields an adapter's configured connection. Depends on #build_connection - # being defined on this adapter. + # Yields or returns an adapter's configured connection. Depends on + # #build_connection being defined on this adapter. # # @param env [Faraday::Env, Hash] The env object for a faraday request. # - # @return An HTTP client connection for this adapter. + # @return The return value of the given block, or the HTTP connection object + # if no block is given. def connection(env) conn = build_connection(env) return conn unless block_given? From c82135056b7c786e0e9d84040007de9495c52883 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 17 Oct 2019 10:26:12 -0600 Subject: [PATCH 10/10] lint: Layout/EmptyLines, Style/BracesAroundHashParameters --- spec/faraday/adapter/httpclient_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/faraday/adapter/httpclient_spec.rb b/spec/faraday/adapter/httpclient_spec.rb index 15155fc1d..c9e805012 100644 --- a/spec/faraday/adapter/httpclient_spec.rb +++ b/spec/faraday/adapter/httpclient_spec.rb @@ -11,7 +11,6 @@ it_behaves_like 'an adapter' - it 'allows to provide adapter specific configs' do adapter = described_class.new do |client| client.keep_alive_timeout = 20 @@ -28,7 +27,7 @@ let(:env) { { request: request } } let(:options) { {} } let(:adapter) { Faraday::Adapter::HTTPClient.new } - let(:client) { adapter.connection({url: URI.parse('https://example.com')}) } + let(:client) { adapter.connection(url: URI.parse('https://example.com')) } it 'configures timeout' do assert_default_timeouts!