Skip to content
Browse files

adds a regression test for the strong params converted arrays cache

This is a regression test for 29844dd.
  • Loading branch information...
1 parent 1ecada2 commit f712f89961b64b61064d99d33dbbfd1903b189d4 @fxn fxn committed Jun 7, 2014
Showing with 18 additions and 1 deletion.
  1. +18 −1 actionpack/test/controller/parameters/parameters_permit_test.rb
View
19 actionpack/test/controller/parameters/parameters_permit_test.rb
@@ -167,9 +167,26 @@ def assert_filtered_out(params, key)
end
end
+ # Strong params has an optimization to avoid looping every time you read
+ # a key whose value is an array and building a new object. We check that
+ # optimization here.
test 'arrays are converted at most once' do
params = ActionController::Parameters.new(foo: [{}])
- assert params[:foo].equal?(params[:foo])
+ assert_same params[:foo], params[:foo]
+ end
+
+ # Strong params has an internal cache to avoid duplicated loops in the most
+ # common usage pattern. See the docs of the method `coverted_array`.
+ #
+ # This test checks that if we push a hash to an array (in-place modification)
+ # the cache does not get fooled, the hash is still wrapped as strong params,
+ # and not permitted.
+ test 'mutated arrays are detected' do
+ params = ActionController::Parameters.new(users: [{id: 1}])
+
+ permitted = params.permit(users: [:id])
+ permitted[:users] << {injected: 1}
+ assert_not permitted[:users].last.permitted?
@zenspider
zenspider added a note Jun 7, 2014

You do know that there are way more assertions than assert and assert_not, right? Why don't you use them to your advantage? You like not knowing how something failed?

https://github.com/seattlerb/minitest/blob/master/lib/minitest/assertions.rb#L618

@fxn
Ruby on Rails member
fxn added a note Jun 7, 2014

Oh, thanks, I do generally prefer to use the most specific assertion available in the API.

When I wrote the one you patched I didn't remember there is assert_same. I like that assertion and that's why I have kept it from your patch.

Not a fan of refute_predicate though. I personally believe

assert_not permitted[:users].last.permitted?
assert !permitted[:users].last.permitted?

read better over

refute_predicate permitted[:users].last, :permitted?
assert_not_predicate permitted[:users].last, :permitted?

We agree that the error message is better in the latter approach, but for me the test reads better with the former. And for me reading the test is more important than optimizing for its failure. If it ever fails I check the line number and see what happens, it is my trade-off.

Just explaining why I wrote it that way, not impliying others should or should not use the complete API.

@phoet
phoet added a note Jun 17, 2014

rspec users will facepalm here...

@fxn
Ruby on Rails member
fxn added a note Jun 17, 2014

Oh yes, in RSpec you test that invoking a method your object does not respond to, passing as argument the return value of a call to a method that does not exist. You can't compare!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
test "fetch doesnt raise ParameterMissing exception if there is a default" do

0 comments on commit f712f89

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