From 59f67e7ddaa986e0475b0f2d1ad7d8f6fead4b76 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Fri, 24 Oct 2025 14:54:33 -0400 Subject: [PATCH 1/2] Adjust `Connection#HTTP_FORMAT_HEADER_NAMES` space Move the `get: "Accept",` to a new line and horizontally align it with the rest of the keys. --- lib/active_resource/connection.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/active_resource/connection.rb b/lib/active_resource/connection.rb index 59d7f19957..ce684206ad 100644 --- a/lib/active_resource/connection.rb +++ b/lib/active_resource/connection.rb @@ -11,7 +11,8 @@ module ActiveResource # This class is used by ActiveResource::Base to interface with REST # services. class Connection - HTTP_FORMAT_HEADER_NAMES = { get: "Accept", + HTTP_FORMAT_HEADER_NAMES = { + get: "Accept", put: "Content-Type", post: "Content-Type", patch: "Content-Type", From 2ea5401a56a8e0060de4d34ec2eb693ff60b84c6 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Mon, 27 Jan 2025 23:36:55 -0500 Subject: [PATCH 2/2] Implement `Connection` in terms of `Net::HTTP` Request Proposal --- This commit changes the private `Connection#request` method to invoke [Net::HTTP#request][], rather than the individual verb methods (like [get][], [post][], etc.). Since the `Net::HTTP#request` method expects an instance (rather than its constituent parts), this commit also changes the `Connection#request` method to construct an instance of the HTTP method-appropriate class: * `GET` builds [Net::HTTP::Get][] * `PUT` builds [Net::HTTP::Put][] * `POST` builds [Net::HTTP::Post][] * `PATCH` builds [Net::HTTP::Patch][] * `DELETE` builds [Net::HTTP::Delete][] * `HEAD` builds [Net::HTTP::Head][] While the behavior changes aim to be minimal, the availability of a Net::HTTP request instance enables more future pre-request modifications (for example, to support for Files through `multipart/form-data` requests like outlined in [#394][]). Additional information --- Net::HTTP Request instances downcase all HTTP header names, but supports reading from values regardless of the key's casing: ```ruby request["Accept"] = "application/json" # => "application/json" request.each.to_h # => {"accept"=>"application/json"} request["Accept"] # => "application/json" request["accept"] # => "application/json" ``` Additionally, new Net::HTTP Request instances come with default headers: ``` accept-encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3 user-agent: Ruby host: 37s.sunrise.i:3000 ``` The `host:` key is inferred from the request instance's URI. For the sake of backwards compatibility, this commit clears all default headers prior to merging any provided or inferred headers (like `content-type` or `accept`). [Net::HTTP#request]: https://docs.ruby-lang.org/en/master/Net/HTTP.html#method-i-request [get]: https://docs.ruby-lang.org/en/master/Net/HTTP.html#method-c-get [post]: https://docs.ruby-lang.org/en/master/Net/HTTP.html#method-c-post [Net::HTTP::Get]: https://docs.ruby-lang.org/en/master/Net/HTTP/Get.html [Net::HTTP::Put]: https://docs.ruby-lang.org/en/master/Net/HTTP/Put.html [Net::HTTP::Post]: https://docs.ruby-lang.org/en/master/Net/HTTP/Post.html [Net::HTTP::Patch]: https://docs.ruby-lang.org/en/master/Net/HTTP/Patch.html [Net::HTTP::Delete]: https://docs.ruby-lang.org/en/master/Net/HTTP/Delete.html [Net::HTTP::Head]: https://docs.ruby-lang.org/en/master/Net/HTTP/Head.html [#394]: https://github.com/rails/activeresource/issues/394 --- lib/active_resource/connection.rb | 37 ++++++++++++++++++------- lib/active_resource/http_mock.rb | 45 ++++++++++++++++++------------- test/cases/connection_test.rb | 10 +++---- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/lib/active_resource/connection.rb b/lib/active_resource/connection.rb index ce684206ad..7a5e917e3b 100644 --- a/lib/active_resource/connection.rb +++ b/lib/active_resource/connection.rb @@ -19,6 +19,14 @@ class Connection delete: "Accept", head: "Accept" } + HTTP_METHODS = { + get: Net::HTTP::Get, + put: Net::HTTP::Put, + post: Net::HTTP::Post, + patch: Net::HTTP::Patch, + delete: Net::HTTP::Delete, + head: Net::HTTP::Head + }.freeze # :nodoc: attr_reader :site, :user, :password, :bearer_token, :auth_type, :timeout, :open_timeout, :read_timeout, :proxy, :ssl_options attr_accessor :format, :logger @@ -81,37 +89,37 @@ def auth_type=(auth_type) # Executes a GET request. # Used to get (find) resources. def get(path, headers = {}) - with_auth { request(:get, path, build_request_headers(headers, :get, self.site.merge(path))) } + with_auth { request(:get, path, headers) } end # Executes a DELETE request (see HTTP protocol documentation if unfamiliar). # Used to delete resources. def delete(path, headers = {}) - with_auth { request(:delete, path, build_request_headers(headers, :delete, self.site.merge(path))) } + with_auth { request(:delete, path, headers) } end # Executes a PATCH request (see HTTP protocol documentation if unfamiliar). # Used to update resources. def patch(path, body = "", headers = {}) - with_auth { request(:patch, path, body.to_s, build_request_headers(headers, :patch, self.site.merge(path))) } + with_auth { request(:patch, path, body.to_s, headers) } end # Executes a PUT request (see HTTP protocol documentation if unfamiliar). # Used to update resources. def put(path, body = "", headers = {}) - with_auth { request(:put, path, body.to_s, build_request_headers(headers, :put, self.site.merge(path))) } + with_auth { request(:put, path, body.to_s, headers) } end # Executes a POST request. # Used to create new resources. def post(path, body = "", headers = {}) - with_auth { request(:post, path, body.to_s, build_request_headers(headers, :post, self.site.merge(path))) } + with_auth { request(:post, path, body.to_s, headers) } end # Executes a HEAD request. # Used to obtain meta-information about resources, such as whether they exist and their size (via response headers). def head(path, headers = {}) - with_auth { request(:head, path, build_request_headers(headers, :head, self.site.merge(path))) } + with_auth { request(:head, path, headers) } end private @@ -119,12 +127,14 @@ def head(path, headers = {}) def request(method, path, *arguments) body, headers = arguments headers, body = body, nil if headers.nil? + request = build_request(method, path, headers) + request.body = body result = ActiveSupport::Notifications.instrument("request.active_resource") do |payload| payload[:method] = method - payload[:request_uri] = "#{site.scheme}://#{site.host}:#{site.port}#{path}" - payload[:headers] = headers + payload[:request_uri] = request.uri.to_s + payload[:headers] = request.each_capitalized.to_h payload[:body] = body - payload[:result] = http.send(method, path, *arguments) + payload[:result] = http.request(request) end handle_response(result) rescue Timeout::Error => e @@ -219,6 +229,15 @@ def default_header @default_header ||= {} end + def build_request(http_method, path, headers) + HTTP_METHODS[http_method].new(site.merge(path)).tap do |request| + headers = build_request_headers(headers, http_method, request.uri) + + request.each_name { |name| request.delete(name) } + headers.each_pair { |name, value| request[name] = value } + end + end + # Builds headers for request to remote service. def build_request_headers(headers, http_method, uri) authorization_header(http_method, uri).update(default_header).update(http_format_header(http_method)).update(headers.to_hash) diff --git a/lib/active_resource/http_mock.rb b/lib/active_resource/http_mock.rb index ed5f53caf6..bb832808d8 100644 --- a/lib/active_resource/http_mock.rb +++ b/lib/active_resource/http_mock.rb @@ -75,6 +75,8 @@ class InvalidRequestError < StandardError; end # :nodoc: # # When a block is passed to the mock, it ignores the +body+, +status+, and +response_headers+ arguments. class HttpMock + HTTP_METHODS = Connection::HTTP_METHODS.invert.freeze # :nodoc: + class Responder # :nodoc: def initialize(responses) @responses = responses @@ -270,33 +272,38 @@ def net_connection_disabled? methods.each do |method| # def post(path, body, headers, options = {}) # request = ActiveResource::Request.new(:post, path, body, headers, options) - # self.class.requests << request - # if response = self.class.responses.assoc(request) - # response = response[1] - # response = response.call(request) if response.respond_to?(:call) # - # Response.wrap(response) - # else - # raise InvalidRequestError.new("Could not find a response recorded for #{request.to_s} - Responses recorded are: - #{inspect_responses}") - # end + # process(request) # end module_eval <<-EOE, __FILE__, __LINE__ + 1 def #{method}(path, #{'body, ' if has_body}headers, options = {}) request = ActiveResource::Request.new(:#{method}, path, #{has_body ? 'body, ' : 'nil, '}headers, options) - self.class.requests << request - if response = self.class.responses.assoc(request) - response = response[1] - response = response.call(request) if response.respond_to?(:call) - - Response.wrap(response) - else - raise InvalidRequestError.new("Could not find a response recorded for \#{request.to_s} - Responses recorded are: \#{inspect_responses}") - end + + process(request) end EOE end end + def request(http) # :nodoc: + request = Request.new(HTTP_METHODS[http.class], http.path, nil, http.each_capitalized.to_h) + request.body = http.body if http.request_body_permitted? + + process(request) + end + + def process(request) # :nodoc: + self.class.requests << request + if response = self.class.responses.assoc(request) + response = response[1] + response = response.call(request) if response.respond_to?(:call) + + Response.wrap(response) + else + raise InvalidRequestError.new("Could not find a response recorded for #{request} - Responses recorded are: #{inspect_responses}") + end + end + def initialize(site) # :nodoc: @site = site end @@ -310,7 +317,7 @@ class Request attr_accessor :path, :method, :body, :headers def initialize(method, path, body = nil, headers = {}, options = {}) - @method, @path, @body, @headers, @options = method, path, body, headers, options + @method, @path, @body, @headers, @options = method, path, body, headers.transform_keys(&:downcase), options end def ==(req) @@ -339,7 +346,7 @@ def same_path?(req) def headers_match?(req) # Ignore format header on equality if it's not defined - format_header = ActiveResource::Connection::HTTP_FORMAT_HEADER_NAMES[method] + format_header = ActiveResource::Connection::HTTP_FORMAT_HEADER_NAMES[method].downcase if headers[format_header].present? || req.headers[format_header].blank? headers == req.headers else diff --git a/test/cases/connection_test.rb b/test/cases/connection_test.rb index 941b477698..9be07f4917 100644 --- a/test/cases/connection_test.rb +++ b/test/cases/connection_test.rb @@ -242,7 +242,7 @@ def test_delete_with_header def test_timeout @http = mock("new Net::HTTP") @conn.expects(:http).returns(@http) - @http.expects(:get).raises(Timeout::Error, "execution expired") + @http.expects(:request).raises(Timeout::Error, "execution expired") assert_raise(ActiveResource::TimeoutError) { @conn.get("/people_timeout.json") } end @@ -261,7 +261,7 @@ def test_accept_http_header @http = mock("new Net::HTTP") @conn.expects(:http).returns(@http) path = "/people/1.xml" - @http.expects(:get).with(path, { "Accept" => "application/xhtml+xml" }).returns(ActiveResource::Response.new(@matz, 200, "Content-Type" => "text/xhtml")) + @http.expects(:request).with { |get| get.path == path && get["Accept"] == "application/xhtml+xml" }.returns(ActiveResource::Response.new(@matz, 200, "Content-Type" => "text/xhtml")) assert_nothing_raised { @conn.get(path, "Accept" => "application/xhtml+xml") } end @@ -284,21 +284,21 @@ def test_ssl_options_get_applied_to_https_urls_without_explicitly_setting_ssl_op def test_ssl_error http = Net::HTTP.new("") @conn.expects(:http).returns(http) - http.expects(:get).raises(OpenSSL::SSL::SSLError, "Expired certificate") + http.expects(:request).raises(OpenSSL::SSL::SSLError, "Expired certificate") assert_raise(ActiveResource::SSLError) { @conn.get("/people/1.json") } end def test_handle_econnrefused http = Net::HTTP.new("") @conn.expects(:http).returns(http) - http.expects(:get).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") + http.expects(:request).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") assert_raise(ActiveResource::ConnectionRefusedError) { @conn.get("/people/1.json") } end def test_handle_econnrefused_with_backwards_compatible_error http = Net::HTTP.new("") @conn.expects(:http).returns(http) - http.expects(:get).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") + http.expects(:request).raises(Errno::ECONNREFUSED, "Failed to open TCP connection") assert_raise(Errno::ECONNREFUSED) { @conn.get("/people/1.json") } end