From a051e7c8b42c7ff9fe3ef644b34df3a57e17359c Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 21 Dec 2025 01:11:16 -0500 Subject: [PATCH] Assign `Net::HTTPRequest` to exceptions With the changes made in [2ea5401][], the underlying [Net::HTTPRequest][] instance is available to the connection throughout the lifetime of the request. With that instance available, exceptions raised during the request-response handshake can be constructed with the `Net::HTTPRequest` instance. Access to the `Net::HTTPRequest` from code that rescues from those exceptions is valuable: * for debugging purposes * for observability and logging * for retrying requests, etc. However, the exception classes are documented and therefore their constructors are part of the public API. Inserting a `request` positional argument is a breaking change. To account for that, this commit includes deprecations for subclasses that call `super` from within `#initialize` to instruct them on changing the signature to account for the `Net::HTTPRequest` instance. [2ea5401]: https://github.com/rails/activeresource/commit/2ea5401a56a8e0060de4d34ec2eb693ff60b84c6 [Net::HTTPRequest]: https://docs.ruby-lang.org/en/master/Net/HTTPRequest.html --- Gemfile | 1 + lib/active_resource/base.rb | 2 +- lib/active_resource/connection.rb | 42 ++++++++++++++-------------- lib/active_resource/exceptions.rb | 46 +++++++++++++++++++++++++++---- test/cases/connection_test.rb | 14 ++++++---- 5 files changed, 73 insertions(+), 32 deletions(-) diff --git a/Gemfile b/Gemfile index 473d9b60fd..215b3df34c 100644 --- a/Gemfile +++ b/Gemfile @@ -18,6 +18,7 @@ gem "rubocop-performance" gem "rubocop-rails" gem "rubocop-rails-omakase" +gem "minitest", "< 6" gem "minitest-bisect" gemspec diff --git a/lib/active_resource/base.rb b/lib/active_resource/base.rb index 10312bb6f5..741af32896 100644 --- a/lib/active_resource/base.rb +++ b/lib/active_resource/base.rb @@ -1509,7 +1509,7 @@ def save # of the before_* callbacks throw +:abort+ the action is # cancelled and save! raises ActiveResource::ResourceInvalid. def save! - save || raise(ResourceInvalid.new(self)) + save || raise(ResourceInvalid.new(nil, self)) end ## diff --git a/lib/active_resource/connection.rb b/lib/active_resource/connection.rb index 085ae453f6..c40ed7c070 100644 --- a/lib/active_resource/connection.rb +++ b/lib/active_resource/connection.rb @@ -136,52 +136,52 @@ def request(method, path, *arguments) payload[:request] = request payload[:result] = http.request(request) end - handle_response(result) + handle_response(request, result) rescue Timeout::Error => e - raise TimeoutError.new(e.message) + raise TimeoutError.new(request, e.message) rescue OpenSSL::SSL::SSLError => e - raise SSLError.new(e.message) + raise SSLError.new(request, e.message) rescue Errno::ECONNREFUSED => e - raise ConnectionRefusedError.new(e.message) + raise ConnectionRefusedError.new(request, e.message) end # Handles response and error codes from the remote service. - def handle_response(response) + def handle_response(request, response) case response.code.to_i when 301, 302, 303, 307 - raise(Redirection.new(response)) + raise(Redirection.new(request, response)) when 200...400 response when 400 - raise(BadRequest.new(response)) + raise(BadRequest.new(request, response)) when 401 - raise(UnauthorizedAccess.new(response)) + raise(UnauthorizedAccess.new(request, response)) when 402 - raise(PaymentRequired.new(response)) + raise(PaymentRequired.new(request, response)) when 403 - raise(ForbiddenAccess.new(response)) + raise(ForbiddenAccess.new(request, response)) when 404 - raise(ResourceNotFound.new(response)) + raise(ResourceNotFound.new(request, response)) when 405 - raise(MethodNotAllowed.new(response)) + raise(MethodNotAllowed.new(request, response)) when 409 - raise(ResourceConflict.new(response)) + raise(ResourceConflict.new(request, response)) when 410 - raise(ResourceGone.new(response)) + raise(ResourceGone.new(request, response)) when 412 - raise(PreconditionFailed.new(response)) + raise(PreconditionFailed.new(request, response)) when 422 - raise(ResourceInvalid.new(response)) + raise(ResourceInvalid.new(request, response)) when 429 - raise(TooManyRequests.new(response)) + raise(TooManyRequests.new(request, response)) when 451 - raise(UnavailableForLegalReasons.new(response)) + raise(UnavailableForLegalReasons.new(request, response)) when 401...500 - raise(ClientError.new(response)) + raise(ClientError.new(request, response)) when 500...600 - raise(ServerError.new(response)) + raise(ServerError.new(request, response)) else - raise(ConnectionError.new(response, "Unknown response code: #{response.code}")) + raise(ConnectionError.new(request, response, "Unknown response code: #{response.code}")) end end diff --git a/lib/active_resource/exceptions.rb b/lib/active_resource/exceptions.rb index ea35490b63..5281975090 100644 --- a/lib/active_resource/exceptions.rb +++ b/lib/active_resource/exceptions.rb @@ -2,9 +2,19 @@ module ActiveResource class ConnectionError < StandardError # :nodoc: - attr_reader :response + attr_reader :request, :response - def initialize(response, message = nil) + def initialize(request, response = nil, message = nil) + if request.is_a?(Net::HTTPResponse) && (response.is_a?(String) || response.nil?) + ActiveResource.deprecator.warn(<<~WARN) + ConnectionError subclasses must be constructed with a request. Call super with a Net::HTTPRequest instance as the first argument. + WARN + + message = response + response, request = request, nil + end + + @request = request @response = response @message = message end @@ -13,6 +23,7 @@ def to_s return @message if @message message = +"Failed." + message << " Request = #{request.method} #{request.uri}." if request.respond_to?(:method) && request.respond_to?(:uri) message << " Response code = #{response.code}." if response.respond_to?(:code) message << " Response message = #{response.message}." if response.respond_to?(:message) message @@ -21,7 +32,16 @@ def to_s # Raised when a Timeout::Error occurs. class TimeoutError < ConnectionError - def initialize(message) + def initialize(request, message = nil) + if request.is_a?(String) + ActiveResource.deprecator.warn(<<~WARN) + TimeoutError subclasses must be constructed with a request. Call super with a Net::HTTPRequest instance as the first argument. + WARN + + message, request = request, nil + end + + @request = request @message = message end def to_s; @message ; end @@ -29,14 +49,30 @@ def to_s; @message ; end # Raised when a OpenSSL::SSL::SSLError occurs. class SSLError < ConnectionError - def initialize(message) + def initialize(request, message = nil) + if request.is_a?(String) + ActiveResource.deprecator.warn(<<~WARN) + SSLError subclasses must be constructed with a request. Call super with a Net::HTTPRequest instance as the first argument. + WARN + + message, request = request, nil + end + + @request = request @message = message end def to_s; @message ; end end # Raised when a Errno::ECONNREFUSED occurs. - class ConnectionRefusedError < Errno::ECONNREFUSED; end + class ConnectionRefusedError < Errno::ECONNREFUSED + attr_reader :request + + def initialize(request, message) + @request = request + super(message) + end + end # 3xx Redirection class Redirection < ConnectionError # :nodoc: diff --git a/test/cases/connection_test.rb b/test/cases/connection_test.rb index 03f5d39f02..99417f0dba 100644 --- a/test/cases/connection_test.rb +++ b/test/cases/connection_test.rb @@ -243,7 +243,8 @@ def test_timeout @http = mock("new Net::HTTP") @conn.expects(:http).returns(@http) @http.expects(:request).raises(Timeout::Error, "execution expired") - assert_raise(ActiveResource::TimeoutError) { @conn.get("/people_timeout.json") } + error = assert_raise(ActiveResource::TimeoutError) { @conn.get("/people_timeout.json") } + assert_kind_of Net::HTTPRequest, error.request end def test_setting_timeout @@ -285,21 +286,24 @@ def test_ssl_error http = Net::HTTP.new("") @conn.expects(:http).returns(http) http.expects(:request).raises(OpenSSL::SSL::SSLError, "Expired certificate") - assert_raise(ActiveResource::SSLError) { @conn.get("/people/1.json") } + error = assert_raise(ActiveResource::SSLError) { @conn.get("/people/1.json") } + assert_kind_of Net::HTTPRequest, error.request end def test_handle_econnrefused http = Net::HTTP.new("") @conn.expects(:http).returns(http) http.expects(:request).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") - assert_raise(ActiveResource::ConnectionRefusedError) { @conn.get("/people/1.json") } + error = assert_raise(ActiveResource::ConnectionRefusedError) { @conn.get("/people/1.json") } + assert_kind_of Net::HTTPRequest, error.request end def test_handle_econnrefused_with_backwards_compatible_error http = Net::HTTP.new("") @conn.expects(:http).returns(http) http.expects(:request).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") - assert_raise(Errno::ECONNREFUSED) { @conn.get("/people/1.json") } + error = assert_raise(Errno::ECONNREFUSED) { @conn.get("/people/1.json") } + assert_kind_of Net::HTTPRequest, error.request end def test_auth_type_can_be_string @@ -378,7 +382,7 @@ def assert_redirect_raises(code) end def handle_response(response) - @conn.__send__(:handle_response, response) + @conn.__send__(:handle_response, nil, response) end def decode(response)