From 1ecada20d163ec1a3b0a3b6b51922da1dd7f089e Mon Sep 17 00:00:00 2001 From: Xavier Noria Date: Sat, 7 Jun 2014 13:04:40 +0200 Subject: Revert "Convert StrongParameters cache to a hash. This fixes an unbounded" We cannot cache keys because arrays are mutable. We rather want to cache the arrays. This behaviour is tailor-made for the usage pattern strongs params is designed for. In a forthcoming commit I am going to add a test that covers why we need to cache by value. Every strong params instance has a live span of a request, the cache goes away with the object. Since strong params have such a concrete intention, it would be interesting to see if there are actually any real-world use cases that are an actual leak, one that practically may matter. I am not convinced that the theoretical leak has any practical consequences, but if it can be shown there are, then I believe we should either get rid of the cache (which is an optimization), or else wipe it in the mutating API. This reverts commit e63be2769c039e4e9ada523a8497ce3206cc8a9b. --- actionpack/lib/action_controller/metal/strong_parameters.rb | 12 ++++++------ .../test/controller/parameters/parameters_permit_test.rb | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 0236af4a19..55954d0f37 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -130,7 +130,7 @@ module ActionController # looping in the common use case permit + mass-assignment. Defined in a # method to instantiate it only if needed. def converted_arrays - @converted_arrays ||= {} + @converted_arrays ||= Set.new end # Returns +true+ if the parameter is permitted, +false+ otherwise. @@ -333,15 +333,15 @@ module ActionController private def convert_hashes_to_parameters(key, value, assign_if_converted=true) - converted = convert_value_to_parameters(key, value) + converted = convert_value_to_parameters(value) self[key] = converted if assign_if_converted && !converted.equal?(value) converted end - def convert_value_to_parameters(key, value) - if value.is_a?(Array) && !converted_arrays.member?(key) - converted = value.map { |v| convert_value_to_parameters(nil, v) } - converted_arrays[key] = converted if key + def convert_value_to_parameters(value) + if value.is_a?(Array) && !converted_arrays.member?(value) + converted = value.map { |_| convert_value_to_parameters(_) } + converted_arrays << converted converted elsif value.is_a?(Parameters) || !value.is_a?(Hash) value diff --git a/actionpack/test/controller/parameters/parameters_permit_test.rb b/actionpack/test/controller/parameters/parameters_permit_test.rb index 1856ecd42b..33a91d72d9 100644 --- a/actionpack/test/controller/parameters/parameters_permit_test.rb +++ b/actionpack/test/controller/parameters/parameters_permit_test.rb @@ -169,7 +169,7 @@ class ParametersPermitTest < ActiveSupport::TestCase test 'arrays are converted at most once' do params = ActionController::Parameters.new(foo: [{}]) - assert_same params[:foo], params[:foo] + assert params[:foo].equal?(params[:foo]) end test "fetch doesnt raise ParameterMissing exception if there is a default" do -- cgit v1.2.3