diff options
author | Edouard CHIN <edouard.chin@shopify.com> | 2018-07-10 22:34:02 -0400 |
---|---|---|
committer | Edouard CHIN <edouard.chin@shopify.com> | 2018-07-12 13:04:27 -0400 |
commit | 48b6bacbc534d4be3fa89bc19ea83c357a20e598 (patch) | |
tree | 6c3a8bb3abd570eebfd8733ec4600093a5120404 | |
parent | d4ea114bd611c608482375af94f49e2d54889202 (diff) | |
download | rails-48b6bacbc534d4be3fa89bc19ea83c357a20e598.tar.gz rails-48b6bacbc534d4be3fa89bc19ea83c357a20e598.tar.bz2 rails-48b6bacbc534d4be3fa89bc19ea83c357a20e598.zip |
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
4 files changed, 35 insertions, 5 deletions
diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index 33b0bcbefe..5d784ceb31 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -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 diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index 663832e9bb..2d0b6cd602 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -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 diff --git a/activesupport/lib/active_support/core_ext/object/to_query.rb b/activesupport/lib/active_support/core_ext/object/to_query.rb index abb461966a..bac6ff9c97 100644 --- a/activesupport/lib/active_support/core_ext/object/to_query.rb +++ b/activesupport/lib/active_support/core_ext/object/to_query.rb @@ -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 diff --git a/activesupport/test/core_ext/object/to_query_test.rb b/activesupport/test/core_ext/object/to_query_test.rb index 7593bcfa4d..b0b7ef0913 100644 --- a/activesupport/test/core_ext/object/to_query_test.rb +++ b/activesupport/test/core_ext/object/to_query_test.rb @@ -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("&") |