e4e1b62 broke `to_param` handling:

- There was an issue inside controller tests where order params were not respected, the reason
  was because we were calling `Hash#to_query` which sorts the results lexicographically.
  1e4e1b62 fixed that issue by not using `to_query` but instead a utility function provided by rack.
- However with the fix came another issue where it's now no longer possible to do this

  ```
   post :foo, params: { user: User.first }

   # Prior to the patch the controller will receive { "user" => "1" }
   # Whereas now you get { "user": "#<User: ...>" }
  ```

  The fix in this PR is to modify `Hash#to_query` to sort only when it
  doesn't contain an array structure that looks something like "bar[]"

  Ref https://github.com/rails/rails/pull/33341#issuecomment-404039396
This commit is contained in:
Edouard CHIN 2018-07-10 22:34:02 -04:00
parent d4ea114bd6
commit 48b6bacbc5
4 changed files with 35 additions and 5 deletions

View File

@ -109,7 +109,7 @@ module ActionController
when :xml
data = non_path_parameters.to_xml
when :url_encoded_form
data = Rack::Utils.build_nested_query(non_path_parameters)
data = non_path_parameters.to_query
else
@custom_param_parsers[content_mime_type.symbol] = ->(_) { non_path_parameters }
data = non_path_parameters.to_query

View File

@ -220,7 +220,7 @@ XML
params = Hash[:page, { name: "page name" }, "some key", 123]
post :render_raw_post, params: params.dup
assert_equal Rack::Utils.build_nested_query(params), @response.body
assert_equal params.to_query, @response.body
end
def test_params_round_trip
@ -231,12 +231,25 @@ XML
assert_equal params.merge(controller_info), JSON.parse(@response.body)
end
def test_handle_to_params
klass = Class.new do
def to_param
"bar"
end
end
post :test_params, params: { foo: klass.new }
assert_equal JSON.parse(@response.body)["foo"], "bar"
end
def test_body_stream
params = Hash[:page, { name: "page name" }, "some key", 123]
post :render_body, params: params.dup
assert_equal Rack::Utils.build_nested_query(params), @response.body
assert_equal params.to_query, @response.body
end
def test_document_body_and_params_with_post

View File

@ -75,11 +75,14 @@ class Hash
#
# This method is also aliased as +to_param+.
def to_query(namespace = nil)
collect do |key, value|
query = collect do |key, value|
unless (value.is_a?(Hash) || value.is_a?(Array)) && value.empty?
value.to_query(namespace ? "#{namespace}[#{key}]" : key)
end
end.compact.sort! * "&"
end.compact
query.sort! unless namespace.to_s.include?("[]")
query.join("&")
end
alias_method :to_param, :to_query

View File

@ -77,6 +77,20 @@ class ToQueryTest < ActiveSupport::TestCase
assert_equal "name=Nakshay&type=human", hash.to_query
end
def test_hash_not_sorted_lexicographically_for_nested_structure
params = {
"foo" => {
"contents" => [
{ "name" => "gorby", "id" => "123" },
{ "name" => "puff", "d" => "true" }
]
}
}
expected = "foo[contents][][name]=gorby&foo[contents][][id]=123&foo[contents][][name]=puff&foo[contents][][d]=true"
assert_equal expected, URI.decode(params.to_query)
end
private
def assert_query_equal(expected, actual)
assert_equal expected.split("&"), actual.to_query.split("&")