This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

converts hashes in arrays of unfiltered params to unpermitted params [c…

…loses #185, closes #159]
  • Loading branch information...
1 parent b53893b commit 62c0efbf9eb7e23a29761bbe21bc55b56ae74502 @fxn fxn committed Dec 21, 2013
Showing with 17 additions and 3 deletions.
  1. +10 −3 lib/action_controller/parameters.rb
  2. +7 −0 test/parameters_permit_test.rb
@@ -111,11 +111,18 @@ def convert_value(value)
private
def convert_hashes_to_parameters(key, value)
- if value.is_a?(Parameters) || !value.is_a?(Hash)
+ converted = convert_value_to_parameters(value)
+ self[key] = converted unless converted.equal?(value)
+ converted
+ end
+
+ def convert_value_to_parameters(value)
+ if value.is_a?(Array)
+ value.map { |_| convert_value_to_parameters(_) }
+ elsif value.is_a?(Parameters) || !value.is_a?(Hash)
value
else
- # Convert to Parameters on first access
- self[key] = self.class.new(value)
+ self.class.new(value)
end
end
@@ -333,4 +333,11 @@ def assert_filtered_out(params, key)
assert_nil params[:properties]["1"]
assert_equal "prop0", params[:properties]["0"]
end
+
+ test 'hashes in array values get wrapped' do
+ params = ActionController::Parameters.new(foo: [{}, {}])
+ params[:foo].each do |hash|
+ assert !hash.permitted?
+ end
+ end
end

15 comments on commit 62c0efb

Is this worthy of a version bump?

Owner

fxn replied Dec 21, 2013

Yep, just pushed 0.2.2 to RubyGems.

That seems like it could be expensive though to do the conversion every time then do a comparison… or is the thought that the size of params is just too small for this to really matter?

Owner

fxn replied Dec 21, 2013

The implementation is based on two things:

  • We do not recursively convert values in params up front, only on demand.
  • Paramaters are typically processed once.

In your use case, params[:user] triggers wrapping the array, that is returned, and no other pass is done over it because it is passed to AR.

Paramaters are typically processed once.

Yeah, you're right. It's going to be two (or three) different edge cases combined where one would run into a performance issue with this. :)

Probably more than sufficient. Thanks for the fix.

Owner

fxn replied Dec 21, 2013

Did you realize that the conversion was already being done every time? I just refactored and extracted a new method to be able to recurse on arrays.

Did you realize that the conversion was already being done every time?

For non-hashes? Yes, I guess so… but hashes are only converted once though do to the value.is_a?(Parameter) logic. One could argue arrays of hashes also deserve the "once and only once" treatment… but then you'd have to have an internal array to keep track of which keys you'd converted already, etc… probably not worth the trouble.

Just wanted to make sure it was at least thought about. :)

Ok, I guess deep hashes themselves are still only processed [converted to Params] once… it's just the array mapping that happens over and over. shrugs

Consider my thought retracted. :) An extra map for someone referencing a param several times doesn't sound like a world under.

Owner

fxn replied Dec 21, 2013

Yeah, we could internally remember which keys have been already processed and avoid calling that method altogether, not just for arrays. Off the top of my head I believe that would eliminate the is_a?(Parameters) checks. That's a good idea.

Well, there might be an edge case to that… if you are converting a Hash it actually already does a full traversal thanks to the IndifferentAccess stuff… not sure if it does that via the [] method or .each… so you could have a newly inited Params object where all the keys were already converted (thanks to IndifferentAccess) yet you're still running them thru convert in order to flip that bit in the array.

If one of the design goals was not to do recursive converts I'd say that goal fails 99% of time. The original from Rack is a raw hash, right?

N/M, I think params are already indifferent when Parameters gets them… otherwise this problem wouldn't' have reared it's head.

Owner

fxn replied Dec 21, 2013

I have tried to keep track of the keys whose value has already been converted to avoid a double loop in the typical use case permit + update_attributes, and the overhead this introduces does not seem to compensate the benefits in my benchmarks. It is not clear.

Owner

fxn replied Dec 21, 2013

Arrived to this compromise rails/rails@273045d there we track only arrays, thus not introducing overhead for the rest of value types, and avoiding repeated looping.

Please sign in to comment.