Add HTTP::URI::InvalidError and raise ArgumentError for nil/empty URIs

Define HTTP::URI::InvalidError and raise it from URI.parse for malformed
URIs, moving error handling to the URI layer where it belongs. Raise
ArgumentError for nil and empty string URIs, since those are programming
mistakes rather than parse failures.

Closes #723.

Author: sferik

CHANGELOG.md

@@ -38,9 +38,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Instrumentation feature now correctly starts a new span for each retry
   attempt, fixing `NoMethodError` with `ActiveSupport::Notifications` when
   using `.retriable` with the instrumentation feature (#826)
-- Raise `HTTP::Request::InvalidURIError` for invalid URIs (nil, empty string,
-  missing scheme, malformed) instead of confusing `UnsupportedSchemeError` or
-  `Addressable::URI::InvalidURIError` (#565)
+- Raise `HTTP::URI::InvalidError` for malformed or schemeless URIs and
+  `ArgumentError` for nil or empty URIs, instead of confusing
+  `UnsupportedSchemeError` or `Addressable::URI::InvalidURIError` (#565)
 - Strip `Authorization` header when following redirects to a different origin
   (scheme, host, or port) to prevent credential leakage (#770)
 - AutoInflate now preserves the response charset encoding instead of

lib/http/request.rb

@@ -26,9 +26,6 @@ class UnsupportedMethodError < RequestError; end
     # The scheme of given URI was not understood
     class UnsupportedSchemeError < RequestError; end
 
-    # The URI given was not valid
-    class InvalidURIError < RequestError; end
-
     # Default User-Agent header value
     USER_AGENT = "http.rb/#{HTTP::VERSION}".freeze
 
@@ -335,18 +332,19 @@ def default_host_header_value
     # @return [void]
     # @api private
     def parse_uri!(uri)
+      raise ArgumentError, "uri is nil" if uri.nil?
+      raise ArgumentError, "uri is empty" if uri.is_a?(String) && uri.empty?
+
       @uri = @uri_normalizer.call(uri)
       @scheme = @uri.scheme.to_s.downcase.to_sym if @uri.scheme
-    rescue TypeError, Addressable::URI::InvalidURIError
-      raise InvalidURIError, "invalid URI: #{uri.inspect}"
     end
 
     # Validate HTTP method and URI scheme
     # @return [void]
     # @api private
     def validate_method_and_scheme!
       raise(UnsupportedMethodError, "unknown method: #{verb}") unless METHODS.include?(@verb)
-      raise(InvalidURIError, "invalid URI: #{@uri}") unless @scheme
+      raise(HTTP::URI::InvalidError, "invalid URI: #{@uri}") unless @scheme
       raise(UnsupportedSchemeError, "unknown scheme: #{scheme}") unless SCHEMES.include?(@scheme)
     end
 

lib/http/uri.rb

@@ -7,6 +7,9 @@ module HTTP
   class URI
     extend Forwardable
 
+    # The URI given was not valid
+    class InvalidError < HTTP::RequestError; end
+
     def_delegators :@uri, :scheme, :normalized_scheme, :scheme=
     def_delegators :@uri, :user, :normalized_user, :user=
     def_delegators :@uri, :password, :normalized_password, :password=
@@ -77,6 +80,8 @@ def self.parse(uri)
       return uri if uri.is_a?(self)
 
       new(Addressable::URI.parse(uri))
+    rescue TypeError, Addressable::URI::InvalidURIError
+      raise InvalidError, "invalid URI: #{uri.inspect}"
     end
 
     # Encodes key/value pairs as application/x-www-form-urlencoded

sig/http.rbs

@@ -562,6 +562,9 @@ module HTTP
   class URI
     extend Forwardable
 
+    class InvalidError < RequestError
+    end
+
     @uri: Addressable::URI
     @hash: Integer
 
@@ -661,9 +664,6 @@ module HTTP
     class UnsupportedSchemeError < RequestError
     end
 
-    class InvalidURIError < RequestError
-    end
-
     USER_AGENT: String
     METHODS: Array[verb]
     SCHEMES: Array[Symbol]

test/http/request_test.rb

@@ -21,29 +21,29 @@
     assert_kind_of HTTP::Headers, request.headers
   end
 
-  it "raises InvalidURIError for URI without scheme" do
-    err = assert_raises(HTTP::Request::InvalidURIError) do
+  it "raises InvalidError for URI without scheme" do
+    err = assert_raises(HTTP::URI::InvalidError) do
       HTTP::Request.new(verb: :get, uri: "example.com/")
     end
     assert_match(/invalid URI/, err.message)
   end
 
-  it "raises InvalidURIError for nil URI" do
-    err = assert_raises(HTTP::Request::InvalidURIError) do
+  it "raises ArgumentError for nil URI" do
+    err = assert_raises(ArgumentError) do
       HTTP::Request.new(verb: :get, uri: nil)
     end
-    assert_match(/invalid URI/, err.message)
+    assert_equal "uri is nil", err.message
   end
 
-  it "raises InvalidURIError for empty string URI" do
-    err = assert_raises(HTTP::Request::InvalidURIError) do
+  it "raises ArgumentError for empty string URI" do
+    err = assert_raises(ArgumentError) do
       HTTP::Request.new(verb: :get, uri: "")
     end
-    assert_match(/invalid URI/, err.message)
+    assert_equal "uri is empty", err.message
   end
 
-  it "raises InvalidURIError for malformed URI" do
-    err = assert_raises(HTTP::Request::InvalidURIError) do
+  it "raises InvalidError for malformed URI" do
+    err = assert_raises(HTTP::URI::InvalidError) do
       HTTP::Request.new(verb: :get, uri: ":")
     end
     assert_match(/invalid URI/, err.message)

test/http/uri_test.rb

@@ -138,6 +138,20 @@
       # is_a?(self) returns true for subclasses; instance_of? does not
       assert_same sub_uri, HTTP::URI.parse(sub_uri)
     end
+
+    it "raises InvalidError for nil" do
+      err = assert_raises(HTTP::URI::InvalidError) do
+        HTTP::URI.parse(nil)
+      end
+      assert_equal "invalid URI: nil", err.message
+    end
+
+    it "raises InvalidError for malformed URI" do
+      err = assert_raises(HTTP::URI::InvalidError) do
+        HTTP::URI.parse(":")
+      end
+      assert_equal 'invalid URI: ":"', err.message
+    end
   end
 
   describe ".form_encode" do