Implement stricter timeout options parsing

Closes #754.

Author: sferik

CHANGELOG.md

@@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **BREAKING** Stricter timeout options parsing: `.timeout()` with a Hash now
+  rejects unknown keys, non-numeric values, string keys, and empty hashes.
+  `HTTP.timeout(global: N)` is no longer accepted; use `HTTP.timeout(N)` (#754)
 - Bumped min llhttp dependency version
 - **BREAKING** Handle responses in the reverse order from the requests (#776)
 

lib/http/chainable.rb

@@ -168,16 +168,16 @@ def build_request(verb, uri, opts = {})
     # @api public
     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, {}]
+                       when Numeric then [HTTP::Timeout::Global, { global_timeout: options }]
+                       when Hash
+                         [HTTP::Timeout::PerOperation, HTTP::Timeout::PerOperation.normalize_options(options)]
+                       when :null then [HTTP::Timeout::Null, {}]
                        else raise ArgumentError,
-                                  "Use `.timeout(global_timeout_in_seconds)` " \
-                                  "or `.timeout(connect: x, write: y, read: z)`."
+                                  "Use `.timeout(:null)`, " \
+                                  "`.timeout(global_timeout_in_seconds)` or " \
+                                  "`.timeout(connect: x, write: y, read: z)`."
                        end
 
-      normalize_timeout_keys!(options)
-
       branch default_options.merge(
         timeout_class:   klass,
         timeout_options: options

lib/http/chainable/helpers.rb

@@ -27,18 +27,5 @@ def build_proxy_hash(proxy)
       end
       result
     end
-
-    # Normalize timeout option keys to include _timeout suffix
-    #
-    # @param [Hash] options timeout options to normalize
-    # @return [void]
-    # @api private
-    def normalize_timeout_keys!(options)
-      %i[global read write connect].each do |k|
-        next unless options.key? k
-
-        options[:"#{k}_timeout"] = options.delete k
-      end
-    end
   end
 end

lib/http/timeout/per_operation.rb

@@ -15,6 +15,55 @@ class PerOperation < Null
       # Default read timeout in seconds
       READ_TIMEOUT = 0.25
 
+      # Mapping of shorthand option keys to their full forms
+      KEYS = %i[read write connect].to_h { |k| [k, :"#{k}_timeout"] }.freeze
+
+      # Normalize and validate timeout options
+      #
+      # @example
+      #   PerOperation.normalize_options(read: 5, write: 3)
+      #
+      # @param [Hash] options timeout options with short or long keys
+      # @return [Hash] normalized options with long keys
+      # @raise [ArgumentError] if options are invalid
+      # @api public
+      def self.normalize_options(options)
+        remaining  = options.dup
+        normalized = {} #: Hash[Symbol, Numeric]
+
+        KEYS.each do |short, long|
+          next if !remaining.key?(short) && !remaining.key?(long)
+
+          normalized[long] = resolve_timeout_value!(remaining, short, long)
+        end
+
+        raise ArgumentError, "unknown timeout options: #{remaining.keys.join(', ')}" unless remaining.empty?
+        raise ArgumentError, "no timeout options given" if normalized.empty?
+
+        normalized
+      end
+
+      # Resolve a single timeout value from the options hash
+      #
+      # @example
+      #   resolve_timeout_value!({read: 5}, :read, :read_timeout)
+      #
+      # @param [Hash] options mutable options hash (keys are deleted as consumed)
+      # @param [Symbol] short short key name (e.g. :read)
+      # @param [Symbol] long long key name (e.g. :read_timeout)
+      # @return [Numeric] the timeout value
+      # @raise [ArgumentError] if both forms given or value is not numeric
+      # @api private
+      private_class_method def self.resolve_timeout_value!(options, short, long)
+        raise ArgumentError, "can't pass both #{short} and #{long}" if options.key?(short) && options.key?(long)
+
+        value = options.delete(options.key?(long) ? long : short)
+
+        raise ArgumentError, "#{long} must be numeric" unless value.is_a?(Numeric)
+
+        value
+      end
+
       # Initializes per-operation timeout with options
       #
       # @example

sig/http.rbs

@@ -50,7 +50,6 @@ module HTTP
 
     def branch: (Options options) -> Client
     def build_proxy_hash: (Array[untyped] proxy) -> Hash[Symbol, untyped]
-    def normalize_timeout_keys!: (Hash[untyped, untyped] options) -> void
   end
 
   class Client
@@ -975,6 +974,13 @@ module HTTP
       READ_TIMEOUT: Numeric
       WRITE_TIMEOUT: Numeric
       CONNECT_TIMEOUT: Numeric
+      KEYS: Hash[Symbol, Symbol]
+
+      def self.normalize_options: (Hash[Symbol, untyped] options) -> Hash[Symbol, Numeric]
+
+      private
+
+      def self.resolve_timeout_value!: (Hash[Symbol, untyped] options, Symbol short, Symbol long) -> Numeric
 
       attr_reader read_timeout: untyped
       attr_reader write_timeout: untyped

test/http/timeout/per_operation_test.rb

@@ -4,6 +4,64 @@
 
 describe HTTP::Timeout::PerOperation do
   cover "HTTP::Timeout::PerOperation*"
+  describe ".normalize_options" do
+    it "normalizes short read key to long form" do
+      assert_equal({ read_timeout: 5 }, HTTP::Timeout::PerOperation.normalize_options(read: 5))
+    end
+
+    it "normalizes short write key to long form" do
+      assert_equal({ write_timeout: 3 }, HTTP::Timeout::PerOperation.normalize_options(write: 3))
+    end
+
+    it "normalizes short connect key to long form" do
+      assert_equal({ connect_timeout: 1 }, HTTP::Timeout::PerOperation.normalize_options(connect: 1))
+    end
+
+    it "passes through long form keys" do
+      assert_equal({ read_timeout: 5 }, HTTP::Timeout::PerOperation.normalize_options(read_timeout: 5))
+    end
+
+    it "normalizes all keys together" do
+      result = HTTP::Timeout::PerOperation.normalize_options(read: 1, write: 2, connect: 3)
+
+      assert_equal({ read_timeout: 1, write_timeout: 2, connect_timeout: 3 }, result)
+    end
+
+    it "accepts float values" do
+      assert_equal({ read_timeout: 1.5 }, HTTP::Timeout::PerOperation.normalize_options(read: 1.5))
+    end
+
+    it "handles frozen hashes" do
+      result = HTTP::Timeout::PerOperation.normalize_options({ read: 5 }.freeze)
+
+      assert_equal({ read_timeout: 5 }, result)
+    end
+
+    it "raises when both short and long form of same key given" do
+      assert_raises(ArgumentError) do
+        HTTP::Timeout::PerOperation.normalize_options(read: 1, read_timeout: 2)
+      end
+    end
+
+    it "raises for non-numeric values" do
+      assert_raises(ArgumentError) do
+        HTTP::Timeout::PerOperation.normalize_options(read: "5")
+      end
+    end
+
+    it "raises for unknown keys" do
+      assert_raises(ArgumentError) do
+        HTTP::Timeout::PerOperation.normalize_options(timeout: 5)
+      end
+    end
+
+    it "raises for empty hash" do
+      assert_raises(ArgumentError) do
+        HTTP::Timeout::PerOperation.normalize_options({})
+      end
+    end
+  end
+
   let(:timeout) { HTTP::Timeout::PerOperation.new(connect_timeout: 1, read_timeout: 1, write_timeout: 1) }
 
   let(:io) { fake(wait_readable: true, wait_writable: true) }

test/http_test.rb

@@ -418,6 +418,22 @@
       end
     end
 
+    context "specifying per operation timeouts with long form keys" do
+      let(:client) { HTTP.timeout read_timeout: 123 }
+
+      it "sets given timeout options" do
+        assert_equal({ read_timeout: 123 }, client.default_options.timeout_options)
+      end
+    end
+
+    context "specifying all per operation timeouts" do
+      let(:client) { HTTP.timeout read: 1, write: 2, connect: 3 }
+
+      it "sets all timeout options" do
+        assert_equal({ read_timeout: 1, write_timeout: 2, connect_timeout: 3 }, client.default_options.timeout_options)
+      end
+    end
+
     context "specifying per operation timeouts as frozen hash" do
       let(:frozen_options) { { read: 123 }.freeze }
       let(:client) { HTTP.timeout(frozen_options) }
@@ -427,6 +443,42 @@
       end
     end
 
+    context "with empty hash" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout({}) }
+      end
+    end
+
+    context "with unknown timeout key" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout(timeout: 2) }
+      end
+    end
+
+    context "with both short and long form of same key" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout(read: 2, read_timeout: 2) }
+      end
+    end
+
+    context "with non-numeric timeout value" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout(read: "2") }
+      end
+    end
+
+    context "with string keys" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout("read" => 2) }
+      end
+    end
+
+    context "with global key as hash" do
+      it "raises ArgumentError" do
+        assert_raises(ArgumentError) { HTTP.timeout(global: 161) }
+      end
+    end
+
     context "specifying a global timeout" do
       let(:client) { HTTP.timeout 123 }
 
@@ -439,6 +491,14 @@
       end
     end
 
+    context "specifying a float global timeout" do
+      let(:client) { HTTP.timeout 2.5 }
+
+      it "sets given timeout option" do
+        assert_equal({ global_timeout: 2.5 }, client.default_options.timeout_options)
+      end
+    end
+
     context "with unsupported options" do
       it "raises ArgumentError" do
         assert_raises(ArgumentError) { HTTP.timeout("invalid") }