- 
                Notifications
    You must be signed in to change notification settings 
- Fork 363
          Implement Connection in terms of Net::HTTP Request
          #449
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dd3e0e9    to
    3f96385      
    Compare
  
            
          
                lib/active_resource/connection.rb
              
                Outdated
          
        
      | payload[:request_uri] = "#{site.scheme}://#{site.host}:#{site.port}#{path}" | ||
| payload[:headers] = headers | ||
| payload[:request_uri] = request.uri.to_s | ||
| payload[:headers] = request.each.to_h | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| payload[:headers] = request.each.to_h | |
| payload[:headers] = request.to_hash | 
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there is a different between the value returned by Net::HTTPHeader#to_hash and Net::HTTPHeader#each_capitalized`:
request.to_hash                # => {"accept"=>["application/json"]}
request. each_capitalized.to_h # => {"accept"=>"application/json"}Calling to_hash results in a Hash where all the values are wrapped in Arrays, whereas each_capitalized.to_h does not.
In my opinion, calling to_hash is more correct, in that Net HTTP knows how to transform multi-value headers to the underlying HTTP request.
Having said that, it probably represents a backwards incompatibile breaking change from the current behavior. We could merge this and include it in a release, and then replace each_capitalized.to_h with to_hash in a future major release. What do you think @rafaelfranca ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with current implementation. Let's keep as is
3f96385    to
    2c4e2b8      
    Compare
  
    Move the `get: "Accept",` to a new line and horizontally align it with the rest of the keys.
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 [rails#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 [rails#394]: rails#394
2c4e2b8    to
    2ea5401      
    Compare
  
    
Proposal
This commit changes the private
Connection#requestmethod to invoke Net::HTTP#request, rather than the individual verb methods (like get, post, etc.).Since the
Net::HTTP#requestmethod expects an instance (rather than its constituent parts), this commit also changes theConnection#requestmethod to construct an instance of the HTTP method-appropriate class:GETbuilds Net::HTTP::GetPUTbuilds Net::HTTP::PutPOSTbuilds Net::HTTP::PostPATCHbuilds Net::HTTP::PatchDELETEbuilds Net::HTTP::DeleteHEADbuilds Net::HTTP::HeadWhile 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-datarequests 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:
Additionally, new Net::HTTP Request instances come with default headers:
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-typeoraccept).