Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Nested Attributes: use accepts_nested_attributes_for without fields_for #86

Closed
wants to merge 1 commit into from

3 participants

@HurricaneJames

Occasionally, I want forms that that do not use fields_for and array of some association. For example, when I do not want the meaningless fields_attributes[0][attribute] syntax, but instead something more parseable like fields_attributes[name][attribute]. Since the model ignores the key values anyway, this works just fine without strong_parameters. However, I notice that strong_parameters goes out of its way to assume everybody will be using fields_for.

lib/action_controller/parameters.rb line 183-84:

   # fields_for on an array of records uses numeric hash keys.
   elsif value.is_a?(Hash) && value.keys.all? { |k| k =~ /\A-?\d+\z/ }

Was there some reason for doing the extra hash keys check? I didn't see one, but I also spent less than 10 minutes looking at the code. If there was not some reason to check for numeric hash keys, this patch will allow generic attribute index keys to work.

@HurricaneJames HurricaneJames not everybody uses fields_for. if you have accepts_nested_attributes …
…with an index other than numeric keys then there are problem. this commit removes the unnecessary requirement that a hash have numeric keys
7808c57
@carlosantoniodasilva

Please make sure you add some tests for it, thanks.

@fxn
Owner

Not sure I follow... the method each_element has three cases. Regular hashes are yielded in the last one.

We do not certainly want to accept arbitrary keys, strong parameters must be super strict in what it whitelists, if your code uses custom keys then you need to declare them.

@HurricaneJames

Maybe I don't fully understand what the gem code is doing here. I found the line after about 30 minutes with a debugger trying to figure out why MyModel.build(my_params) did not build any of the nested resources (yet MyModel.build(params[:my]) works just fine.

In the specific case that created the problem this afternoon (though I have a couple of apps that are going to be difficult to upgrade) params looks something like this using form builder:

user_entry => {
    entry_field_responses_attributes => {
        0 => { response => abc, entry_field_id => 3 },
        1 => { response => def, entry_field_id => 2 }
    }
}

in my performance test code this gets re-encoded to look more like

user_entry => {
    entry_field_responses_attributes => {
        first_name => { response => abc, entry_field_id => 3 },
        last_name => { response => def, entry_field_id => 2 }
    }
}

This is done because JMeter can be really difficult to setup for large, deeply nested forms and saying first_name is easier than remembering that entry_field_id=3 maps to first_name. This has never been a problem with the entry_field_responses_attributes= method generated by accepted_nested_attributes_for :entry_field_responses because it ignores the indices and just looks at the values.

I assumed my solution did not break anything because the hash_filter method still runs element.permit on everything. Though, admittedly, I really should have run some tests before doing a pull request.

Now that it looks like I was being stupid, I guess I need to spend a couple hours seeing if it is possible to make strong_parameters play nicely with accepts_nested_attributes_for or if the fields_for convention of numeric indices is a necessary evil when using strong_parameters.

@HurricaneJames

Oh, and I should point out that is is impossible to white list the attributes because they are user defined in the database. "first_name" was just an example, it could very easily have been named "control_parameter_xyz_of_the_fifth_gate_beta."

@fxn
Owner

If you cannot whitelist the attributes because they are dynamic, then you have two options: one is to generate the declarations dynamically somehow, and the other one is not to use strong params in that call, which is probably the one that makes more sense. If you don't really know in advance which are the parameters that are going to come, you cannot whitelist them, which is the whole purpose of permit.

@fxn
Owner

Yeah, I think we can close this one, by design strong params does not support unknown hash keys.

@fxn fxn closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 21, 2013
  1. @HurricaneJames

    not everybody uses fields_for. if you have accepts_nested_attributes …

    HurricaneJames authored
    …with an index other than numeric keys then there are problem. this commit removes the unnecessary requirement that a hash have numeric keys
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 2 deletions.
  1. +3 −2 lib/action_controller/parameters.rb
View
5 lib/action_controller/parameters.rb
@@ -180,8 +180,9 @@ def hash_filter(params, filter)
def each_element(value)
if value.is_a?(Array)
value.map { |el| yield el }.compact
- # fields_for on an array of records uses numeric hash keys.
- elsif value.is_a?(Hash) && value.keys.all? { |k| k =~ /\A-?\d+\z/ }
+ # fields_for on an array of records uses numeric hash keys.
+ # however, some people don't use fields_for because they want more meaningful params than something[0]
+ elsif value.is_a?(Hash)
hash = value.class.new
value.each { |k,v| hash[k] = yield v }
hash
Something went wrong with that request. Please try again.