From b73dc53f34bea6b77cba047130f48955f2afe202 Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Sun, 5 May 2019 11:08:07 -0500 Subject: [PATCH 1/7] Update Rubocop and run auto-fixes In an attempt to do some house keeping this pushed the rubocop version up by ten and fixes everything that is correctable from an auto-fix standpoint. These changes should be safe if we assume that rubocop's auto-fix changes are safe. --- Gemfile | 2 +- http.gemspec | 8 ++++---- lib/http/chainable.rb | 2 ++ lib/http/client.rb | 10 +++------- lib/http/headers.rb | 3 +++ lib/http/mime_type/json.rb | 1 + lib/http/options.rb | 3 ++- lib/http/redirector.rb | 1 + lib/http/request/body.rb | 1 + lib/http/request/writer.rb | 1 + lib/http/response/body.rb | 3 ++- lib/http/response/status.rb | 1 + lib/http/timeout/global.rb | 4 +--- lib/http/timeout/per_operation.rb | 1 + spec/lib/http/client_spec.rb | 2 +- spec/lib/http/request/writer_spec.rb | 2 +- spec/lib/http/request_spec.rb | 2 +- spec/lib/http_spec.rb | 2 +- spec/support/black_hole.rb | 2 +- spec/support/dummy_server/servlet.rb | 2 +- spec/support/ssl_helper.rb | 2 +- 21 files changed, 31 insertions(+), 24 deletions(-) diff --git a/Gemfile b/Gemfile index 4cff89f5..94125230 100644 --- a/Gemfile +++ b/Gemfile @@ -28,7 +28,7 @@ group :test do gem "rspec", "~> 3.0" gem "rspec-its" - gem "rubocop", "= 0.49.1" + gem "rubocop", "= 0.59.2" gem "yardstick" end diff --git a/http.gemspec b/http.gemspec index bff3c71d..f7f022f6 100644 --- a/http.gemspec +++ b/http.gemspec @@ -1,6 +1,6 @@ # frozen_string_literal: true -lib = File.expand_path("../lib", __FILE__) +lib = File.expand_path("lib", __dir__) $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) require "http/version" @@ -27,10 +27,10 @@ Gem::Specification.new do |gem| gem.required_ruby_version = ">= 2.3" - gem.add_runtime_dependency "http-parser", "~> 1.2.0" - gem.add_runtime_dependency "http-form_data", "~> 2.0" - gem.add_runtime_dependency "http-cookie", "~> 1.0" gem.add_runtime_dependency "addressable", "~> 2.3" + gem.add_runtime_dependency "http-cookie", "~> 1.0" + gem.add_runtime_dependency "http-form_data", "~> 2.0" + gem.add_runtime_dependency "http-parser", "~> 1.2.0" gem.add_development_dependency "bundler", "~> 2.0" diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 8eeb7716..b19c9132 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -101,6 +101,7 @@ def timeout(options) %i[global read write connect].each do |k| next unless options.key? k + options["#{k}_timeout".to_sym] = options.delete k end @@ -144,6 +145,7 @@ def persistent(host, timeout: 5) options = {:keep_alive_timeout => timeout} p_client = branch default_options.merge(options).with_persistent host return p_client unless block_given? + yield p_client ensure p_client.close if p_client diff --git a/lib/http/client.rb b/lib/http/client.rb index f6f73e22..ef27bbde 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -93,7 +93,7 @@ def perform(req, options) @state = :clean res - rescue + rescue StandardError close raise end @@ -128,15 +128,11 @@ def verify_connection!(uri) def make_request_uri(uri, opts) uri = uri.to_s - if default_options.persistent? && uri !~ HTTP_OR_HTTPS_RE - uri = "#{default_options.persistent}#{uri}" - end + uri = "#{default_options.persistent}#{uri}" if default_options.persistent? && uri !~ HTTP_OR_HTTPS_RE uri = HTTP::URI.parse uri - if opts.params && !opts.params.empty? - uri.query_values = uri.query_values(Array).to_a.concat(opts.params.to_a) - end + uri.query_values = uri.query_values(Array).to_a.concat(opts.params.to_a) if opts.params && !opts.params.empty? # Some proxies (seen on WEBRick) fail if URL has # empty path (e.g. `http://example.com`) while it's RFC-complaint: diff --git a/lib/http/headers.rb b/lib/http/headers.rb index 0cccd80e..3c820d8c 100644 --- a/lib/http/headers.rb +++ b/lib/http/headers.rb @@ -118,6 +118,7 @@ def keys # @return [Boolean] def ==(other) return false unless other.respond_to? :to_a + @pile == other.to_a end @@ -127,6 +128,7 @@ def ==(other) # @return [Headers] self-reference def each return to_enum(__method__) unless block_given? + @pile.each { |arr| yield(arr) } self end @@ -218,6 +220,7 @@ def normalize_header(name) def validate_value(value) v = value.to_s return v unless v.include?("\n") + raise HeaderError, "Invalid HTTP header field value: #{v.inspect}" end end diff --git a/lib/http/mime_type/json.rb b/lib/http/mime_type/json.rb index 0dd4df4e..a5bac75d 100644 --- a/lib/http/mime_type/json.rb +++ b/lib/http/mime_type/json.rb @@ -10,6 +10,7 @@ class JSON < Adapter # Encodes object to JSON def encode(obj) return obj.to_json if obj.respond_to?(:to_json) + ::JSON.dump obj end diff --git a/lib/http/options.rb b/lib/http/options.rb index 4e4837e2..3337a5fc 100644 --- a/lib/http/options.rb +++ b/lib/http/options.rb @@ -18,8 +18,9 @@ class << self attr_accessor :default_socket_class, :default_ssl_socket_class, :default_timeout_class attr_reader :available_features - def new(options = {}) # rubocop:disable Style/OptionHash + def new(options = {}) return options if options.is_a?(self) + super end diff --git a/lib/http/redirector.rb b/lib/http/redirector.rb index 08791a11..b4fb9abf 100644 --- a/lib/http/redirector.rb +++ b/lib/http/redirector.rb @@ -89,6 +89,7 @@ def redirect_to(uri) if UNSAFE_VERBS.include?(verb) && STRICT_SENSITIVE_CODES.include?(code) raise StateError, "can't follow #{@response.status} redirect" if @strict + verb = :get end diff --git a/lib/http/request/body.rb b/lib/http/request/body.rb index 614df12d..14af30b8 100644 --- a/lib/http/request/body.rb +++ b/lib/http/request/body.rb @@ -19,6 +19,7 @@ def size @source.bytesize elsif @source.respond_to?(:read) raise RequestError, "IO object must respond to #size" unless @source.respond_to?(:size) + @source.size elsif @source.nil? 0 diff --git a/lib/http/request/writer.rb b/lib/http/request/writer.rb index 06823654..c90c11e7 100644 --- a/lib/http/request/writer.rb +++ b/lib/http/request/writer.rb @@ -108,6 +108,7 @@ def write(data) until data.empty? length = @socket.write(data) break unless data.bytesize > length + data = data.byteslice(length..-1) end rescue Errno::EPIPE diff --git a/lib/http/response/body.rb b/lib/http/response/body.rb index 8acc368b..43969585 100644 --- a/lib/http/response/body.rb +++ b/lib/http/response/body.rb @@ -52,7 +52,7 @@ def to_s @contents << chunk.force_encoding(@encoding) chunk.clear # deallocate string end - rescue + rescue StandardError @contents = nil raise end @@ -64,6 +64,7 @@ def to_s # Assert that the body is actively being streamed def stream! raise StateError, "body has already been consumed" if @streaming == false + @streaming = true end diff --git a/lib/http/response/status.rb b/lib/http/response/status.rb index 1479bb12..23878627 100644 --- a/lib/http/response/status.rb +++ b/lib/http/response/status.rb @@ -141,6 +141,7 @@ def #{symbol}? # def bad_request? def __setobj__(obj) raise TypeError, "Expected #{obj.inspect} to respond to #to_i" unless obj.respond_to? :to_i + @code = obj.to_i end diff --git a/lib/http/timeout/global.rb b/lib/http/timeout/global.rb index 817f70cb..1078d889 100644 --- a/lib/http/timeout/global.rb +++ b/lib/http/timeout/global.rb @@ -121,9 +121,7 @@ def reset_timer def log_time @time_left -= (Time.now - @started) - if @time_left <= 0 - raise TimeoutError, "Timed out after using the allocated #{@timeout} seconds" - end + raise TimeoutError, "Timed out after using the allocated #{@timeout} seconds" if @time_left <= 0 reset_timer end diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 7d47a4bd..a5e594fb 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -66,6 +66,7 @@ def readpartial(size, buffer = nil) return result if result != :wait_readable raise TimeoutError, "Read timed out after #{@read_timeout} seconds" if timeout + # marking the socket for timeout. Why is this not being raised immediately? # it seems there is some race-condition on the network level between calling # #read_nonblock and #wait_readable, in which #read_nonblock signalizes waiting diff --git a/spec/lib/http/client_spec.rb b/spec/lib/http/client_spec.rb index 3c1491b7..4ba473b3 100644 --- a/spec/lib/http/client_spec.rb +++ b/spec/lib/http/client_spec.rb @@ -1,5 +1,5 @@ -# frozen_string_literal: true # coding: utf-8 +# frozen_string_literal: true require "support/http_handling_shared" require "support/dummy_server" diff --git a/spec/lib/http/request/writer_spec.rb b/spec/lib/http/request/writer_spec.rb index 80a67809..1dc999a1 100644 --- a/spec/lib/http/request/writer_spec.rb +++ b/spec/lib/http/request/writer_spec.rb @@ -1,5 +1,5 @@ -# frozen_string_literal: true # coding: utf-8 +# frozen_string_literal: true RSpec.describe HTTP::Request::Writer do let(:io) { StringIO.new } diff --git a/spec/lib/http/request_spec.rb b/spec/lib/http/request_spec.rb index 854195d3..b3dc10ed 100644 --- a/spec/lib/http/request_spec.rb +++ b/spec/lib/http/request_spec.rb @@ -1,5 +1,5 @@ -# frozen_string_literal: true # coding: utf-8 +# frozen_string_literal: true RSpec.describe HTTP::Request do let(:proxy) { {} } diff --git a/spec/lib/http_spec.rb b/spec/lib/http_spec.rb index da0c17e4..5e188e2b 100644 --- a/spec/lib/http_spec.rb +++ b/spec/lib/http_spec.rb @@ -1,5 +1,5 @@ -# frozen_string_literal: true # encoding: utf-8 +# frozen_string_literal: true require "json" diff --git a/spec/support/black_hole.rb b/spec/support/black_hole.rb index 7399a8d8..8c4c4281 100644 --- a/spec/support/black_hole.rb +++ b/spec/support/black_hole.rb @@ -2,7 +2,7 @@ module BlackHole class << self - def method_missing(*) # rubocop: disable Style/MethodMissing + def method_missing(*) self end diff --git a/spec/support/dummy_server/servlet.rb b/spec/support/dummy_server/servlet.rb index 7b0bb955..8b1386d2 100644 --- a/spec/support/dummy_server/servlet.rb +++ b/spec/support/dummy_server/servlet.rb @@ -1,5 +1,5 @@ -# frozen_string_literal: true # encoding: UTF-8 +# frozen_string_literal: true class DummyServer < WEBrick::HTTPServer # rubocop:disable Metrics/ClassLength diff --git a/spec/support/ssl_helper.rb b/spec/support/ssl_helper.rb index 21e40667..01926719 100644 --- a/spec/support/ssl_helper.rb +++ b/spec/support/ssl_helper.rb @@ -5,7 +5,7 @@ require "certificate_authority" module SSLHelper - CERTS_PATH = Pathname.new File.expand_path("../../../tmp/certs", __FILE__) + CERTS_PATH = Pathname.new File.expand_path("../../tmp/certs", __dir__) class RootCertificate < ::CertificateAuthority::Certificate EXTENSIONS = {"keyUsage" => {"usage" => %w[critical keyCertSign]}}.freeze From dab0fbea1ed0f0c114eab8812c466d45501a4520 Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Sun, 5 May 2019 11:10:18 -0500 Subject: [PATCH 2/7] Disable cop around black hole This black hole method should not call up to super. That is the purpose of a black hole object. --- spec/support/black_hole.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/support/black_hole.rb b/spec/support/black_hole.rb index 8c4c4281..0326fd16 100644 --- a/spec/support/black_hole.rb +++ b/spec/support/black_hole.rb @@ -2,9 +2,11 @@ module BlackHole class << self + # rubocop:disable Style/MethodMissingSuper def method_missing(*) self end + # rubocop:enable Style/MethodMissingSuper def respond_to_missing?(*) true From 92686f2e4d3d612d0ede56ba036f8f0ee9353acb Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Sun, 5 May 2019 11:11:04 -0500 Subject: [PATCH 3/7] Do not take certain changes to maintain backwards compatibility Do not take the Unfreeze string cop because it is only supported in Ruby 2.3 and above. --- .rubocop.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 36631e0a..d5288577 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -61,6 +61,9 @@ Metrics/ParameterLists: Performance/RegexpMatch: Enabled: false +Performance/UnfreezeString: + Enabled: false + ## Style ####################################################################### Style/CollectionMethods: @@ -110,3 +113,6 @@ Style/TrivialAccessors: Style/YodaCondition: Enabled: false + +Style/FormatStringToken: + EnforcedStyle: unannotated From fb6d66ea3f2080e4af10d857b5b879a8e34af10e Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Sun, 5 May 2019 11:12:27 -0500 Subject: [PATCH 4/7] Fix class_eval to be friendlier This came up in Style::EvalWithLocation. It makes backtraces easier to follow. --- lib/http/mime_type/adapter.rb | 2 +- lib/http/response/status.rb | 2 +- spec/lib/http/response/status_spec.rb | 6 +++--- spec/support/dummy_server/servlet.rb | 2 +- spec/support/ssl_helper.rb | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/http/mime_type/adapter.rb b/lib/http/mime_type/adapter.rb index fb5b104c..82efb451 100644 --- a/lib/http/mime_type/adapter.rb +++ b/lib/http/mime_type/adapter.rb @@ -15,7 +15,7 @@ class << self end %w[encode decode].each do |operation| - class_eval <<-RUBY, __FILE__, __LINE__ + class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{operation}(*) fail Error, "\#{self.class} does not supports ##{operation}" end diff --git a/lib/http/response/status.rb b/lib/http/response/status.rb index 23878627..c5c2e525 100644 --- a/lib/http/response/status.rb +++ b/lib/http/response/status.rb @@ -132,7 +132,7 @@ def inspect end SYMBOLS.each do |code, symbol| - class_eval <<-RUBY, __FILE__, __LINE__ + class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{symbol}? # def bad_request? #{code} == code # 400 == code end # end diff --git a/spec/lib/http/response/status_spec.rb b/spec/lib/http/response/status_spec.rb index 509912bd..6930ab97 100644 --- a/spec/lib/http/response/status_spec.rb +++ b/spec/lib/http/response/status_spec.rb @@ -26,7 +26,7 @@ end described_class::REASONS.each do |code, reason| - class_eval <<-RUBY + class_eval <<-RUBY, __FILE__, __LINE__ + 1 context 'with well-known code: #{code}' do let(:code) { #{code} } it { is_expected.to eq #{reason.inspect} } @@ -165,7 +165,7 @@ end described_class::SYMBOLS.each do |code, symbol| - class_eval <<-RUBY + class_eval <<-RUBY, __FILE__, __LINE__ + 1 context 'with well-known code: #{code}' do let(:code) { #{code} } it { is_expected.to be #{symbol.inspect} } @@ -193,7 +193,7 @@ end described_class::SYMBOLS.each do |code, symbol| - class_eval <<-RUBY + class_eval <<-RUBY, __FILE__, __LINE__ + 1 describe '##{symbol}?' do subject { status.#{symbol}? } diff --git a/spec/support/dummy_server/servlet.rb b/spec/support/dummy_server/servlet.rb index 8b1386d2..bc59ac25 100644 --- a/spec/support/dummy_server/servlet.rb +++ b/spec/support/dummy_server/servlet.rb @@ -18,7 +18,7 @@ def self.handlers end %w[get post head].each do |method| - class_eval <<-RUBY, __FILE__, __LINE__ + class_eval <<-RUBY, __FILE__, __LINE__ + 1 def self.#{method}(path, &block) handlers["#{method}:\#{path}"] = block end diff --git a/spec/support/ssl_helper.rb b/spec/support/ssl_helper.rb index 01926719..b475b766 100644 --- a/spec/support/ssl_helper.rb +++ b/spec/support/ssl_helper.rb @@ -90,7 +90,7 @@ def client_params end %w[server client].each do |side| - class_eval <<-RUBY, __FILE__, __LINE__ + class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{side}_cert @#{side}_cert ||= ChildCertificate.new ca end From 4ceb3ee0575a1ea80425f067ef83f12d197f80fc Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Sun, 5 May 2019 11:13:39 -0500 Subject: [PATCH 5/7] Selectively disabled cops for methods and classes Rubocop now requires that cops are scoped tighter. This reformats it so that only the specific method or class has the cop disabled. --- lib/http/options.rb | 4 +--- spec/support/black_hole.rb | 4 +--- spec/support/dummy_server/servlet.rb | 3 +-- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/http/options.rb b/lib/http/options.rb index 3337a5fc..20805abc 100644 --- a/lib/http/options.rb +++ b/lib/http/options.rb @@ -1,14 +1,12 @@ # frozen_string_literal: true -# rubocop:disable Metrics/ClassLength - require "http/headers" require "openssl" require "socket" require "http/uri" module HTTP - class Options + class Options # rubocop:disable Metrics/ClassLength @default_socket_class = TCPSocket @default_ssl_socket_class = OpenSSL::SSL::SSLSocket @default_timeout_class = HTTP::Timeout::Null diff --git a/spec/support/black_hole.rb b/spec/support/black_hole.rb index 0326fd16..c5d61dd1 100644 --- a/spec/support/black_hole.rb +++ b/spec/support/black_hole.rb @@ -2,11 +2,9 @@ module BlackHole class << self - # rubocop:disable Style/MethodMissingSuper - def method_missing(*) + def method_missing(*) # rubocop:disable Style/MethodMissingSuper self end - # rubocop:enable Style/MethodMissingSuper def respond_to_missing?(*) true diff --git a/spec/support/dummy_server/servlet.rb b/spec/support/dummy_server/servlet.rb index bc59ac25..39eccc82 100644 --- a/spec/support/dummy_server/servlet.rb +++ b/spec/support/dummy_server/servlet.rb @@ -2,8 +2,7 @@ # frozen_string_literal: true class DummyServer < WEBrick::HTTPServer - # rubocop:disable Metrics/ClassLength - class Servlet < WEBrick::HTTPServlet::AbstractServlet + class Servlet < WEBrick::HTTPServlet::AbstractServlet # rubocop:disable Metrics/ClassLength def self.sockets @sockets ||= [] end From a590ebe9e08850a4f97c5d0c559ec4e77811bfa5 Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Tue, 7 May 2019 13:47:07 -0500 Subject: [PATCH 6/7] Enforce implicit on Style/RescueStandardError Per ixti's code review, the maintainers prefer this enforced style so I am changing to the requested style. --- .rubocop.yml | 3 +++ lib/http/client.rb | 2 +- lib/http/response/body.rb | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index d5288577..cbf08415 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -116,3 +116,6 @@ Style/YodaCondition: Style/FormatStringToken: EnforcedStyle: unannotated + +Style/RescueStandardError: + EnforcedStyle: implicit diff --git a/lib/http/client.rb b/lib/http/client.rb index ef27bbde..e9a8980f 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -93,7 +93,7 @@ def perform(req, options) @state = :clean res - rescue StandardError + rescue close raise end diff --git a/lib/http/response/body.rb b/lib/http/response/body.rb index 43969585..0997a49f 100644 --- a/lib/http/response/body.rb +++ b/lib/http/response/body.rb @@ -52,7 +52,7 @@ def to_s @contents << chunk.force_encoding(@encoding) chunk.clear # deallocate string end - rescue StandardError + rescue @contents = nil raise end From f838be47cad096b341102dfca6f98af72cb682ea Mon Sep 17 00:00:00 2001 From: Nate Holland Date: Tue, 7 May 2019 13:50:29 -0500 Subject: [PATCH 7/7] Simplify code to ternary This code could easily be collapsed to a ternary per ixti's request so that is what I am doing. --- lib/http/options.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/http/options.rb b/lib/http/options.rb index 20805abc..1bf166e3 100644 --- a/lib/http/options.rb +++ b/lib/http/options.rb @@ -17,9 +17,7 @@ class << self attr_reader :available_features def new(options = {}) - return options if options.is_a?(self) - - super + options.is_a?(self) ? options : super end def defined_options