From 058d0da16a77f8d90f54a2aa2b7ca86a665985f7 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 8 Jun 2023 09:59:03 +0200 Subject: [PATCH 1/4] Reorganize chainable timeout Move option transformation into then block since we want to extent it. --- lib/http/chainable.rb | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 188f6c37..808ab08e 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -93,18 +93,21 @@ def build_request(*args) # @param [Numeric] global_timeout def timeout(options) klass, options = case options - when Numeric then [HTTP::Timeout::Global, {global: options}] - when Hash then [HTTP::Timeout::PerOperation, options.dup] - when :null then [HTTP::Timeout::Null, {}] - else raise ArgumentError, "Use `.timeout(global_timeout_in_seconds)` or `.timeout(connect: x, write: y, read: z)`." - - end + when Numeric then [HTTP::Timeout::Global, {global_timeout: options}] + when Hash + options = options.dup + %i[read write connect].each do |k| + next unless options.key? k - %i[global read write connect].each do |k| - next unless options.key? k + options["#{k}_timeout".to_sym] = options.delete k + end - options[:"#{k}_timeout"] = options.delete k - end + [HTTP::Timeout::PerOperation, options.dup] + when :null then [HTTP::Timeout::Null, {}] + else raise ArgumentError, "Use `.timeout(:null)`, " \ + "`.timeout(global_timeout_in_seconds)` or " \ + "`.timeout(connect: x, write: y, read: z)`." + end branch default_options.merge( timeout_class: klass, From a08972b47ac4a4528d07d2d4bdcede5282ff97cc Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 8 Jun 2023 10:34:25 +0200 Subject: [PATCH 2/4] Be stricter of allowed values for timeout --- lib/http/chainable.rb | 29 +++++++++++----- lib/http/timeout/per_operation.rb | 1 + spec/lib/http_spec.rb | 56 ++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 808ab08e..19fd7979 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -93,17 +93,30 @@ def build_request(*args) # @param [Numeric] global_timeout def timeout(options) klass, options = case options - when Numeric then [HTTP::Timeout::Global, {global_timeout: options}] + when Numeric then [HTTP::Timeout::Global, {:global_timeout => options}] when Hash - options = options.dup - %i[read write connect].each do |k| - next unless options.key? k + options = options.dup + %i[read write connect].each do |k| + next unless options.key? k - options["#{k}_timeout".to_sym] = options.delete k - end + if options.key?("#{k}_timeout".to_sym) + raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}" + end - [HTTP::Timeout::PerOperation, options.dup] - when :null then [HTTP::Timeout::Null, {}] + options["#{k}_timeout".to_sym] = options.delete k + end + + options.each do |key, value| + unless HTTP::Timeout::PerOperation::SETTINGS.member?(key) && value.is_a?(Numeric) + raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \ + "`.timeout(connect: x, write: y, read: z)`." + end + end + + raise ArgumentError, "at least one option" if options.empty? + + [HTTP::Timeout::PerOperation, options.dup] + when :null then [HTTP::Timeout::Null, {}] else raise ArgumentError, "Use `.timeout(:null)`, " \ "`.timeout(global_timeout_in_seconds)` or " \ "`.timeout(connect: x, write: y, read: z)`." diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 100ff66c..24eafc82 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -11,6 +11,7 @@ class PerOperation < Null WRITE_TIMEOUT = 0.25 READ_TIMEOUT = 0.25 + SETTINGS = Set.new(%i[read_timeout write_timeout connect_timeout]) def initialize(*args) super diff --git a/spec/lib/http_spec.rb b/spec/lib/http_spec.rb index 614ea9a7..4f9b7f71 100644 --- a/spec/lib/http_spec.rb +++ b/spec/lib/http_spec.rb @@ -335,8 +335,50 @@ end it "sets given timeout options" do + client = HTTP.timeout :read => 125 + expect(client.default_options.timeout_options). - to eq read_timeout: 123 + to eq read_timeout: 125 + end + + it "sets given timeout options" do + client = HTTP.timeout :read_timeout => 321 + + expect(client.default_options.timeout_options). + to eq :read_timeout => 321 + end + + it "sets all available options" do + client = HTTP.timeout :read => 123, :write => 12, :connect => 1 + + expect(client.default_options.timeout_options). + to eq(:connect_timeout => 1, :write_timeout => 12, :read_timeout => 123) + end + + it "raises an error is empty hash is passed" do + expect { HTTP.timeout({}) } + .to raise_error(ArgumentError) + end + + it "raises if an invalid key is passed" do + expect { HTTP.timeout({:timeout => 2}) } + .to raise_error(ArgumentError) + end + + it "raises if both read and read_timeout is passed" do + expect { HTTP.timeout({:read => 2, :read_timeout => 2}) } + .to raise_error(ArgumentError) + end + + it "raises if a string is passed as read timeout" do + expect { HTTP.timeout({:connect => 1, :read => "2"}) } + .to raise_error(ArgumentError) + end + + it "don't accept string keys" do + expect { HTTP.timeout({:connect => 1, "read" => 2}) } + .to raise_error(ArgumentError) +>>>>>>> 0b150cb (Be stricter of allowed values for timeout) end end @@ -362,6 +404,18 @@ expect(client.default_options.timeout_options). to eq global_timeout: 123 end + + it "accepts floats argument" do + client = HTTP.timeout 2.5 + + expect(client.default_options.timeout_options). + to eq(:global_timeout => 2.5) + end + + it "raises expect if a string is passed" do + expect { HTTP.timeout "1" } + .to raise_error(ArgumentError) + end end end From 875201af66ad9dee9edb940c09f8b747c005df3e Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 8 Jun 2023 10:57:42 +0200 Subject: [PATCH 3/4] Abstract timeout option parsing Rubocop told me the method started to be too complex. lib/http/chainable.rb:94:5: C: Metrics/PerceivedComplexity: Perceived complexity for timeout is too high. [10/8] def timeout(options) ... ^^^^^^^^^^^^^^^^^^^^ 80 files inspected, 1 offense detected --- lib/http/chainable.rb | 22 +-------------------- lib/http/timeout/per_operation.rb | 33 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 19fd7979..2a8c3e65 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -95,27 +95,7 @@ def timeout(options) klass, options = case options when Numeric then [HTTP::Timeout::Global, {:global_timeout => options}] when Hash - options = options.dup - %i[read write connect].each do |k| - next unless options.key? k - - if options.key?("#{k}_timeout".to_sym) - raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}" - end - - options["#{k}_timeout".to_sym] = options.delete k - end - - options.each do |key, value| - unless HTTP::Timeout::PerOperation::SETTINGS.member?(key) && value.is_a?(Numeric) - raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \ - "`.timeout(connect: x, write: y, read: z)`." - end - end - - raise ArgumentError, "at least one option" if options.empty? - - [HTTP::Timeout::PerOperation, options.dup] + [HTTP::Timeout::PerOperation, HTTP::Timeout::PerOperation.parse_options(options)] when :null then [HTTP::Timeout::Null, {}] else raise ArgumentError, "Use `.timeout(:null)`, " \ "`.timeout(global_timeout_in_seconds)` or " \ diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 24eafc82..589a9c8a 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -12,6 +12,39 @@ class PerOperation < Null READ_TIMEOUT = 0.25 SETTINGS = Set.new(%i[read_timeout write_timeout connect_timeout]) + + class << self + def parse_options(options) + options = options.dup.then { |opts| expand_names(opts) } + options.each do |key, value| + unless SETTINGS.member?(key) && value.is_a?(Numeric) + raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \ + "`.timeout(connect: x, write: y, read: z)`." + end + end + + raise ArgumentError, "at least one option" if options.empty? + + options + end + + private + + def expand_names(options) + %i[read write connect].each do |k| + next unless options.key? k + + if options.key?("#{k}_timeout".to_sym) + raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}" + end + + options["#{k}_timeout".to_sym] = options.delete k + end + + options + end + end + def initialize(*args) super From 648610c4cddfebfe08c6fbc27a123e2851115bd1 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 26 Jun 2023 15:38:18 +0200 Subject: [PATCH 4/4] normalize_options instead of parse_options rubocop offences be rubocop -a 15:37:07 lib/http/timeout/per_operation.rb:17:9: C: Metrics/AbcSize: Assignment Branch Condition size for normalize_options is too high. [<5, 23, 9> 25.2/17] def normalize_options(options) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/http/timeout/per_operation.rb:17:9: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for normalize_options is too high. [10/7] def normalize_options(options) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/http/timeout/per_operation.rb:17:9: C: Metrics/MethodLength: Method has too many lines. [11/10] def normalize_options(options) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/http/timeout/per_operation.rb:17:9: C: Metrics/PerceivedComplexity: Perceived complexity for normalize_options is too high. [10/8] def normalize_options(options) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 80/80 files |================================================================================================================ 100 =================================================================================================================>| Time: 00:00:00 80 files inspected, 4 offenses detected --- lib/http/chainable.rb | 2 +- lib/http/timeout/per_operation.rb | 38 +++++++++++-------------------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/lib/http/chainable.rb b/lib/http/chainable.rb index 2a8c3e65..42115c41 100644 --- a/lib/http/chainable.rb +++ b/lib/http/chainable.rb @@ -95,7 +95,7 @@ def timeout(options) klass, options = case options when Numeric then [HTTP::Timeout::Global, {:global_timeout => options}] when Hash - [HTTP::Timeout::PerOperation, HTTP::Timeout::PerOperation.parse_options(options)] + [HTTP::Timeout::PerOperation, HTTP::Timeout::PerOperation.normalize_options(options)] when :null then [HTTP::Timeout::Null, {}] else raise ArgumentError, "Use `.timeout(:null)`, " \ "`.timeout(global_timeout_in_seconds)` or " \ diff --git a/lib/http/timeout/per_operation.rb b/lib/http/timeout/per_operation.rb index 589a9c8a..a98d391a 100644 --- a/lib/http/timeout/per_operation.rb +++ b/lib/http/timeout/per_operation.rb @@ -11,37 +11,25 @@ class PerOperation < Null WRITE_TIMEOUT = 0.25 READ_TIMEOUT = 0.25 - SETTINGS = Set.new(%i[read_timeout write_timeout connect_timeout]) + KEYS = %i[read write connect].to_h { |k| [k, :"#{k}_timeout"] }.freeze class << self - def parse_options(options) - options = options.dup.then { |opts| expand_names(opts) } - options.each do |key, value| - unless SETTINGS.member?(key) && value.is_a?(Numeric) - raise ArgumentError, "invalid option #{key.inspect}, must be numeric " \ - "`.timeout(connect: x, write: y, read: z)`." - end - end - - raise ArgumentError, "at least one option" if options.empty? - - options - end + def normalize_options(options) + normalized = {} + original = options.dup - private + KEYS.each do |short, long| + next if !original.key?(short) && !original.key?(long) + raise ArgumentError, "can't pass both #{short} and #{long}" if original.key?(short) && original.key?(long) - def expand_names(options) - %i[read write connect].each do |k| - next unless options.key? k - - if options.key?("#{k}_timeout".to_sym) - raise ArgumentError, "can't pass both #{k} and #{"#{k}_timeout".to_sym}" - end - - options["#{k}_timeout".to_sym] = options.delete k + normalized[long] = original.key?(long) ? original.delete(long) : original.delete(short) + raise ArgumentError, "#{long} must be numeric" unless normalized[long].is_a?(Numeric) end - options + raise ArgumentError, "unknown timeout options: #{original.keys.join(', ')}" if original.size.positive? + raise ArgumentError, "no timeout options given" if normalized.empty? + + normalized end end