Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Don't convert params if the request isn't HTML - fixes #5341

  • Loading branch information...
commit 7a80b69e00f68e673c6ceb5cc684aa9196ed3d9f 1 parent ffc861c
Andrew White pixeltrix authored
24 actionpack/lib/action_controller/test_case.rb
View
@@ -147,17 +147,23 @@ def assign_parameters(routes, controller_path, action, parameters = {})
extra_keys = routes.extra_keys(parameters)
non_path_parameters = get? ? query_parameters : request_parameters
parameters.each do |key, value|
- if value.is_a? Fixnum
- value = value.to_s
- elsif value.is_a? Array
- value = Result.new(value.map { |v| v.is_a?(String) ? v.dup : v })
- elsif value.is_a? String
+ if value.is_a?(Array) && (value.frozen? || value.any?(&:frozen?))
+ value = value.map{ |v| v.duplicable? ? v.dup : v }
+ elsif value.is_a?(Hash) && (value.frozen? || value.any?{ |k,v| v.frozen? })
+ value = Hash[value.map{ |k,v| [k, v.duplicable? ? v.dup : v] }]
+ elsif value.frozen? && value.duplicable?
value = value.dup
end
if extra_keys.include?(key.to_sym)
non_path_parameters[key] = value
else
+ if value.is_a?(Array)
+ value = Result.new(value.map(&:to_param))
+ else
+ value = value.to_param
+ end
+
path_parameters[key.to_s] = value
end
end
@@ -453,7 +459,7 @@ def process(action, http_method = 'GET', *args)
# Ensure that numbers and symbols passed as params are converted to
# proper params, as is the case when engaging rack.
- parameters = paramify_values(parameters)
+ parameters = paramify_values(parameters) if html_format?(parameters)
@request.recycle!
@response.recycle!
@@ -546,6 +552,12 @@ def build_request_uri(action, parameters)
@request.env["QUERY_STRING"] = query_string || ""
end
end
+
+ def html_format?(parameters)
+ return true unless parameters.is_a?(Hash)
+ format = Mime[parameters[:format]]
+ format.nil? || format.html?
+ end
end
# When the request.remote_addr remains the default for testing, which is 0.0.0.0, the exception is simply raised inline
42 actionpack/test/controller/test_case_test.rb
View
@@ -45,6 +45,10 @@ def test_uri
render :text => request.fullpath
end
+ def test_format
+ render :text => request.format
+ end
+
def test_query_string
render :text => request.query_string
end
@@ -579,14 +583,34 @@ def test_params_passing_with_fixnums
)
end
+ def test_params_passing_with_fixnums_when_not_html_request
+ get :test_params, :format => 'json', :count => 999
+ parsed_params = eval(@response.body)
+ assert_equal(
+ {'controller' => 'test_case_test/test', 'action' => 'test_params',
+ 'format' => 'json', 'count' => 999 },
+ parsed_params
+ )
+ end
+
+ def test_params_passing_path_parameter_is_string_when_not_html_request
+ get :test_params, :format => 'json', :id => 1
+ parsed_params = eval(@response.body)
+ assert_equal(
+ {'controller' => 'test_case_test/test', 'action' => 'test_params',
+ 'format' => 'json', 'id' => '1' },
+ parsed_params
+ )
+ end
+
def test_params_passing_with_frozen_values
assert_nothing_raised do
- get :test_params, :frozen => 'icy'.freeze, :frozens => ['icy'.freeze].freeze
+ get :test_params, :frozen => 'icy'.freeze, :frozens => ['icy'.freeze].freeze, :deepfreeze => { :frozen => 'icy'.freeze }.freeze
Aaron Patterson Owner

Is this like a thing, that people do, that we need to support? Also, the above code only checks for frozenness to a certain depth, e.g. we could have n-depth nested hashes, where maybe only the n-th deep value is frozen.

Steve Klabnik Collaborator

FREEZE ALL THE THINGS!!!!1 (even things that are already frozen. Several times.)

http://en.wikipedia.org/wiki/Zero_one_infinity_rule

Andrew White Owner

The test case was added in #185 because ruby 1.9 freezes string keys in hash constants, all I did here was check hashes to the same depth. I think it's unlikely that anyone will be using an hash constant inside an arbitrarily deep hash but we could change it to use deep_dup on master (deep_dup wasn't backported to 3-2-stable).

All Rubies (1.8/1.9) dup.freeze key strings in all hashes:

irb(main):001:0> s = 'abc'
=> "abc"
irb(main):002:0> h = {}
=> {}
irb(main):003:0> h[s] = true
=> true
irb(main):004:0> h.keys.first.frozen?
=> true
irb(main):005:0> s << 'd'
=> "abcd"
irb(main):006:0> h['abc']
=> true
irb(main):007:0> h.keys
=> ["abc"]
irb(main):008:0> {'a' => true}.keys.first.frozen?
=> true

Aaron Patterson Owner

I assume we're duping these because somewhere we're mutating them. Shouldn't we just stop the mutations? I'm trying to understand why we need this.

Andrew White Owner

The params are mutated when they are forcibly encoding to UTF8:
https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/http/parameters.rb#L47-64

Aaron Patterson Owner

Ah interesting. Thanks for helping me understand!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
parsed_params = eval(@response.body)
assert_equal(
{'controller' => 'test_case_test/test', 'action' => 'test_params',
- 'frozen' => 'icy', 'frozens' => ['icy']},
+ 'frozen' => 'icy', 'frozens' => ['icy'], 'deepfreeze' => { 'frozen' => 'icy' }},
parsed_params
)
end
@@ -687,6 +711,20 @@ def test_request_protocol_is_reset_after_request
assert_equal "http://", @response.body
end
+ def test_request_format
+ get :test_format, :format => 'html'
+ assert_equal 'text/html', @response.body
+
+ get :test_format, :format => 'json'
+ assert_equal 'application/json', @response.body
+
+ get :test_format, :format => 'xml'
+ assert_equal 'application/xml', @response.body
+
+ get :test_format
+ assert_equal 'text/html', @response.body
+ end
+
def test_should_have_knowledge_of_client_side_cookie_state_even_if_they_are_not_set
cookies['foo'] = 'bar'
get :no_op
Aaron Patterson

Is this like a thing, that people do, that we need to support? Also, the above code only checks for frozenness to a certain depth, e.g. we could have n-depth nested hashes, where maybe only the n-th deep value is frozen.

Steve Klabnik

FREEZE ALL THE THINGS!!!!1 (even things that are already frozen. Several times.)

http://en.wikipedia.org/wiki/Zero_one_infinity_rule

Andrew White

The test case was added in #185 because ruby 1.9 freezes string keys in hash constants, all I did here was check hashes to the same depth. I think it's unlikely that anyone will be using an hash constant inside an arbitrarily deep hash but we could change it to use deep_dup on master (deep_dup wasn't backported to 3-2-stable).

thedarkone

All Rubies (1.8/1.9) dup.freeze key strings in all hashes:

irb(main):001:0> s = 'abc'
=> "abc"
irb(main):002:0> h = {}
=> {}
irb(main):003:0> h[s] = true
=> true
irb(main):004:0> h.keys.first.frozen?
=> true
irb(main):005:0> s << 'd'
=> "abcd"
irb(main):006:0> h['abc']
=> true
irb(main):007:0> h.keys
=> ["abc"]
irb(main):008:0> {'a' => true}.keys.first.frozen?
=> true

Aaron Patterson

I assume we're duping these because somewhere we're mutating them. Shouldn't we just stop the mutations? I'm trying to understand why we need this.

Aaron Patterson

Ah interesting. Thanks for helping me understand!

Please sign in to comment.
Something went wrong with that request. Please try again.