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]: #394

Author: seanpdoyle

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,50 +89,52 @@ 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
       # Makes a request to the remote service.
       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)

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

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