Improve how params, headers & options are handled in the request phase

Old behavior:

  # init connection with some defaults:
  conn = Faraday.new :params => {...}, :request => {...}

  conn.get('/') do |request|
    request.params = {...}   # params got merged with defaults
    request.options = {...}  # options got deep-merged with defaults
  end

New behavior:

  conn.get('/', {...}) do |request|
    request.params.update(...)  # merge with existing params
    request.params = {...}      # replace all existing params

    request.options[:proxy][:user] = "..." # add extra request options:
    request.options = {...}                # replace all existing options
  end

Pros of the new behavior are consistency, ability to completely override
parameters per-request. Cons are breaking backwards-compatibility.

lib/faraday/connection.rb

@@ -11,8 +11,8 @@ class Connection
     METHODS = Set.new [:get, :post, :put, :delete, :head, :patch, :options]
     METHODS_WITH_BODIES = Set.new [:post, :put, :patch, :options]
 
-    attr_accessor :params, :headers, :parallel_manager
-    attr_reader   :url_prefix, :builder, :options, :ssl
+    attr_accessor :parallel_manager
+    attr_reader   :params, :headers, :url_prefix, :builder, :options, :ssl
 
     # :url
     # :params
@@ -30,12 +30,6 @@ def initialize(url = nil, options = {})
       @ssl              = options[:ssl]     || {}
       @parallel_manager = options[:parallel]
 
-      @proxy = nil
-      proxy(options.fetch(:proxy) { ENV['http_proxy'] })
-
-      @params.update options[:params]   if options[:params]
-      @headers.update options[:headers] if options[:headers]
-
       @builder = options[:builder] || begin
         # pass an empty block to Builder so it doesn't assume default middleware
         block = block_given?? Proc.new {|b| } : nil
@@ -47,9 +41,22 @@ def initialize(url = nil, options = {})
       @params.update options[:params]   if options[:params]
       @headers.update options[:headers] if options[:headers]
 
+      @proxy = nil
+      proxy(options.fetch(:proxy) { ENV['http_proxy'] })
+
       yield self if block_given?
     end
 
+    # Public: Replace default query parameters.
+    def params=(hash)
+      @params.replace hash
+    end
+
+    # Public: Replace default request headers.
+    def headers=(hash)
+      @headers.replace hash
+    end
+
     extend Forwardable
     def_delegators :builder, :build, :use, :request, :response, :adapter
 
@@ -77,7 +84,7 @@ def app
       class_eval <<-RUBY, __FILE__, __LINE__ + 1
         def #{method}(url = nil, params = nil, headers = nil)
           run_request(:#{method}, url, nil, headers) { |request|
-            request.params = params if params
+            request.params.update(params) if params
             yield request if block_given?
           }
         end
@@ -149,7 +156,7 @@ def url_prefix=(url)
       uri = @url_prefix = self.class.URI(url)
       self.path_prefix = uri.path
 
-      @params.merge_query(uri.query)
+      params.merge_query(uri.query)
       uri.query = nil
 
       if uri.user && uri.password
@@ -174,7 +181,7 @@ def run_request(method, url, body, headers)
         raise ArgumentError, "unknown http method: #{method}"
       end
 
-      request = Request.create(method) do |req|
+      request = build_request(method) do |req|
         req.url(url)                if url
         req.headers.update(headers) if headers
         req.body = body             if body
@@ -185,6 +192,18 @@ def run_request(method, url, body, headers)
       self.app.call(env)
     end
 
+    # Internal: Creates and configures the request object.
+    #
+    # Returns the new Request.
+    def build_request(method)
+      Request.create(method) do |req|
+        req.params  = self.params.dup
+        req.headers = self.headers.dup
+        req.options = self.options.merge(:proxy => self.proxy)
+        yield req if block_given?
+      end
+    end
+
     # Takes a relative url for a request and combines it with the defaults
     # set on the connection instance.
     #
@@ -197,18 +216,32 @@ def run_request(method, url, body, headers)
     #   conn.build_url("nigiri", :page => 2) # => https://sushi.com/api/nigiri?token=abc&page=2
     #
     def build_url(url, extra_params = nil)
+      uri = build_exclusive_url(url)
+
+      query_values = self.params.dup.merge_query(uri.query)
+      query_values.update extra_params if extra_params
+      uri.query = query_values.empty? ? nil : query_values.to_query
+
+      uri
+    end
+
+    # Internal: Build an absolute URL based on url_prefix.
+    #
+    # url    - A String or URI-like object
+    # params - A Faraday::Utils::ParamsHash to replace the query values
+    #          of the resulting url (default: nil).
+    #
+    # Returns the resulting URI instance.
+    def build_exclusive_url(url, params = nil)
       url = nil if url.respond_to?(:empty?) and url.empty?
       base = url_prefix
       if url and base.path and base.path !~ /\/$/
         base = base.dup
         base.path = base.path + '/'  # ensure trailing slash
       end
       uri = url ? base + url : base
-
-      params = @params.dup.merge_query(uri.query)
-      params.update extra_params if extra_params
-      uri.query = params.empty? ? nil : params.to_query
-
+      uri.query = params.to_query if params
+      uri.query = nil if uri.query and uri.query.empty?
       uri
     end
 

lib/faraday/request.rb

@@ -9,7 +9,7 @@ module Faraday
   #     req.body = 'abc'
   #   end
   #
-  class Request < Struct.new(:path, :params, :headers, :body, :options)
+  class Request < Struct.new(:method, :path, :params, :headers, :body, :options)
     extend AutoloadHelper
     extend MiddlewareRegistry
 
@@ -28,24 +28,38 @@ class Request < Struct.new(:path, :params, :headers, :body, :options)
       :basic_auth  => :BasicAuthentication,
       :token_auth  => :TokenAuthentication
 
-    attr_reader :method
-
     def self.create(request_method)
       new(request_method).tap do |request|
         yield request if block_given?
       end
     end
 
-    def initialize(request_method)
-      @method = request_method
-      self.params  = {}
-      self.headers = {}
-      self.options = {}
+    # Public: Replace params, preserving the existing hash type
+    def params=(hash)
+      if params then params.replace hash
+      else super
+      end
     end
 
-    def url(path, params = {})
-      self.path   = path
-      self.params = params
+    # Public: Replace request headers, preserving the existing hash type
+    def headers=(hash)
+      if headers then headers.replace hash
+      else super
+      end
+    end
+
+    def url(path, params = nil)
+      if path.respond_to? :query
+        if query = path.query
+          path = path.dup
+          path.query = nil
+        end
+      else
+        path, query = path.split('?', 2)
+      end
+      self.path = path
+      self.params.merge_query query
+      self.params.update(params) if params
     end
 
     def [](key)
@@ -73,17 +87,12 @@ def []=(key, value)
     #     :password   - Proxy server password
     # :ssl - Hash of options for configuring SSL requests.
     def to_env(connection)
-      env_params  = connection.params.merge(params)
-      env_headers = connection.headers.merge(headers)
-      request_options = Utils.deep_merge(connection.options, options)
-      Utils.deep_merge!(request_options, :proxy => connection.proxy)
-
       { :method           => method,
         :body             => body,
-        :url              => connection.build_url(path, env_params),
-        :request_headers  => env_headers,
+        :url              => connection.build_exclusive_url(path, params),
+        :request_headers  => headers,
         :parallel_manager => connection.parallel_manager,
-        :request          => request_options,
+        :request          => options,
         :ssl              => connection.ssl}
     end
   end

test/connection_test.rb

@@ -175,26 +175,6 @@ def test_build_url_bracketizes_nested_params_in_query
     assert_equal "a%5Bb%5D=c", uri.query
   end
 
-  def test_build_url_mashes_default_and_given_params_together
-    conn = Faraday::Connection.new 'http://sushi.com/api?token=abc', :params => {'format' => 'json'}
-    url = conn.build_url("nigiri?page=1", :limit => 5)
-    assert_equal %w[format=json limit=5 page=1 token=abc], url.query.split('&').sort
-  end
-
-  def test_build_url_overrides_default_params_with_given_params
-    conn = Faraday::Connection.new 'http://sushi.com/api?token=abc', :params => {'format' => 'json'}
-    url = conn.build_url("nigiri?page=1", :limit => 5, :token => 'def', :format => 'xml')
-    assert_equal %w[format=xml limit=5 page=1 token=def], url.query.split('&').sort
-  end
-
-  def test_default_params_hash_has_indifferent_access
-    conn = Faraday::Connection.new :params => {'format' => 'json'}
-    assert conn.params.has_key?(:format)
-    conn.params[:format] = 'xml'
-    url = conn.build_url("")
-    assert_equal %w[format=xml], url.query.split('&').sort
-  end
-
   def test_build_url_parses_url
     conn = Faraday::Connection.new
     uri = conn.build_url("http://sushi.com/sake.html")
@@ -309,3 +289,81 @@ def test_init_with_block_yields_connection
     assert_equal '/omnom', conn.path_prefix
   end
 end
+
+class TestRequestParams < Faraday::TestCase
+  def create_connection(*args)
+    @conn = Faraday::Connection.new(*args) do |conn|
+      yield conn if block_given?
+      class << conn
+        undef app
+        def app() lambda { |env| env } end
+      end
+    end
+  end
+
+  def get(*args)
+    env = @conn.get(*args) do |req|
+      yield req if block_given?
+    end
+    env[:url].query
+  end
+
+  def assert_query_equal(expected, query)
+    assert_equal expected, query.split('&').sort
+  end
+
+  def test_merges_connection_and_request_params
+    create_connection 'http://a.co/?token=abc', :params => {'format' => 'json'}
+    query = get '?page=1', :limit => 5
+    assert_query_equal %w[format=json limit=5 page=1 token=abc], query
+  end
+
+  def test_overrides_connection_params
+    create_connection 'http://a.co/?a=a&b=b&c=c', :params => {:a => 'A'} do |conn|
+      conn.params[:b] = 'B'
+      assert_equal 'c', conn.params[:c]
+    end
+    assert_query_equal %w[a=A b=B c=c], get
+  end
+
+  def test_all_overrides_connection_params
+    create_connection 'http://a.co/?a=a', :params => {:c => 'c'} do |conn|
+      conn.params = {'b' => 'b'}
+    end
+    assert_query_equal %w[b=b], get
+  end
+
+  def test_overrides_request_params
+    create_connection
+    query = get '?p=1&a=a', :p => 2
+    assert_query_equal %w[a=a p=2], query
+  end
+
+  def test_overrides_request_params_block
+    create_connection
+    query = get '?p=1&a=a', :p => 2 do |req|
+      req.params[:p] = 3
+    end
+    assert_query_equal %w[a=a p=3], query
+  end
+
+  def test_overrides_request_params_block_url
+    create_connection
+    query = get nil, :p => 2 do |req|
+      req.url '?p=1&a=a', 'p' => 3
+    end
+    assert_query_equal %w[a=a p=3], query
+  end
+
+  def test_overrides_all_request_params
+    create_connection :params => {:c => 'c'}
+    query = get '?p=1&a=a', :p => 2 do |req|
+      assert_equal 'a', req.params[:a]
+      assert_equal 'c', req.params['c']
+      assert_equal 2, req.params['p']
+      req.params = {:b => 'b'}
+      assert_equal 'b', req.params['b']
+    end
+    assert_query_equal %w[b=b], query
+  end
+end

test/env_test.rb

@@ -50,7 +50,7 @@ def test_per_request_options
     env = make_env do |req|
       req.options[:timeout] = 10
       req.options[:custom] = true
-      req.options[:oauth] = {:consumer_secret => 'xyz'}
+      req.options[:oauth][:consumer_secret] = 'xyz'
     end
     assert_equal 10, env[:request][:timeout]
     assert_equal 5, env[:request][:open_timeout]
@@ -73,7 +73,7 @@ def test_request_create_stores_proxy_options
   private
 
   def make_env(method = :get, connection = @conn, &block)
-    request = Faraday::Request.create(method, &block)
+    request = connection.build_request(method, &block)
     request.to_env(connection)
   end
 end