New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle nested fields_for by adding indexes to record_name #21431

Merged
merged 1 commit into from Sep 26, 2015

Conversation

Projects
None yet
7 participants
@ojab
Contributor

ojab commented Aug 30, 2015

Fixes #15332

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Aug 30, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ojab ojab changed the title from Handle nested form_for by adding indexes to record_name to Handle nested fields_for by adding indexes to record_name Aug 30, 2015

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Aug 30, 2015

Member

Can you please update your commit message to better express what the issue this is fixing is, why this fix works, and what the use case is? Even with the issue linked I'm having trouble following, and we like to keep as much context in the commits as possible.

Member

sgrif commented Aug 30, 2015

Can you please update your commit message to better express what the issue this is fixing is, why this fix works, and what the use case is? Even with the issue linked I'm having trouble following, and we like to keep as much context in the commits as possible.

@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab Aug 30, 2015

Contributor

Updated commit message, hope that the issue subject is more clear now.

Contributor

ojab commented Aug 30, 2015

Updated commit message, hope that the issue subject is more clear now.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 30, 2015

Member

I still think this is not the correct usage. With only one nested attribute the name format is: post[comments_attributes][0][body] I'd expect with who the format to be post[post_attributes][0][comments_attributes][0][body]

Member

rafaelfranca commented Aug 30, 2015

I still think this is not the correct usage. With only one nested attribute the name format is: post[comments_attributes][0][body] I'd expect with who the format to be post[post_attributes][0][comments_attributes][0][body]

@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab Aug 30, 2015

Contributor

Both variants (foos[foo[0]][bar[1]][id] & foos[foo][0][bar][1]][id]) producing identical params hash, but yeah, second is more readable.

Pushed fixed version.

Contributor

ojab commented Aug 30, 2015

Both variants (foos[foo[0]][bar[1]][id] & foos[foo][0][bar][1]][id]) producing identical params hash, but yeah, second is more readable.

Pushed fixed version.

@egilburg

View changes

Show outdated Hide outdated actionview/lib/action_view/helpers/form_helper.rb
@@ -1604,6 +1604,9 @@ def fields_for(record_name, record_object = nil, fields_options = {}, &block)
when String, Symbol
if nested_attributes_association?(record_name)
return fields_for_with_nested_attributes(record_name, record_object, fields_options, block)
elsif !defined?(@auto_index) && !options[:index] && record_name.to_s.end_with?('[]')
record_name = record_name.to_s.sub(/(.*)\[\]$/, "[\\1][#{record_object.id}]")

This comment has been minimized.

@egilburg

egilburg Aug 30, 2015

Contributor

perhaps do this line in the below if nested block to keep all string building in one place

@egilburg

egilburg Aug 30, 2015

Contributor

perhaps do this line in the below if nested block to keep all string building in one place

This comment has been minimized.

@ojab

ojab Aug 30, 2015

Contributor

Actually the whole part is unneeded, pushed another one version.

@ojab

ojab Aug 30, 2015

Contributor

Actually the whole part is unneeded, pushed another one version.

Handle nested fields_for by adding indexes to record_name
In case of the form with nested fields_for, i. e.

<%= form_for :foos, url: root_path do |f| %>
    <% @foos.each do |foo| %>
        <%= f.fields_for 'foo[]', foo do |f2| %>
            <%= f2.text_field :id %>
            <% foo.bars.each do |bar| %>
                <%= f2.fields_for 'bar[]', bar do |b| %>
                    <%= b.text_field :id %>
                <% end %>
            <% end %>
        <% end %>
     <% end %>
    <%= f.submit %>
<% end %>

rails doesn't add index for 'foo' in the inner fields_for block, so field names
in the outer fields_for looks like "foos[foo][#{foo_index}][id]" and in the
inner "foos[foo[]][bar][#{bar_index}][id]". Submitting of such form leads to an
error like:
>ActionController::BadRequest (Invalid request parameters: expected Array
>(got Rack::QueryParser::Params) for param `foo'):

This commit adds indexes for the foos in the inner blocks, so field names
become "foos[foo][#{foo_index}][bar][#{bar_index}][id]" and submitting of such
form works fine as expected.

Fixes #15332
@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab Sep 13, 2015

Contributor

ping?

Contributor

ojab commented Sep 13, 2015

ping?

@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab Sep 13, 2015

Contributor

Also I've run a benchmark for the else case, i. e.

      form_for(:posts) do |f|
        f.fields_for('post[]', @post) do |f2|
          f2.text_field(:id)
        end
      end

but results are inconclusive, I'm getting very close results with large stddev:

Calculating -------------------------------------
                orig   235.000  i/100ms
                gsub   247.000  i/100ms
              substr   251.000  i/100ms
-------------------------------------------------
                orig      2.530k (±12.7%) i/s -     49.820k
                gsub      2.664k (±13.4%) i/s -     50.882k
              substr      2.609k (±12.2%) i/s -     49.196k

orig -- rails without patch, gsub -- rails with this PR applied, substr -- record_name is formed using "#{object_name}[#{record_name.to_s[0..-3]}][#{record_object.id}]" without gsub.
So AFAIU performance change is negligible and gsub is as good as substring.

Contributor

ojab commented Sep 13, 2015

Also I've run a benchmark for the else case, i. e.

      form_for(:posts) do |f|
        f.fields_for('post[]', @post) do |f2|
          f2.text_field(:id)
        end
      end

but results are inconclusive, I'm getting very close results with large stddev:

Calculating -------------------------------------
                orig   235.000  i/100ms
                gsub   247.000  i/100ms
              substr   251.000  i/100ms
-------------------------------------------------
                orig      2.530k (±12.7%) i/s -     49.820k
                gsub      2.664k (±13.4%) i/s -     50.882k
              substr      2.609k (±12.2%) i/s -     49.196k

orig -- rails without patch, gsub -- rails with this PR applied, substr -- record_name is formed using "#{object_name}[#{record_name.to_s[0..-3]}][#{record_object.id}]" without gsub.
So AFAIU performance change is negligible and gsub is as good as substring.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 21, 2015

Member

r? @rafaelfranca I feel like you have a better handle on this than I do.

Member

sgrif commented Sep 21, 2015

r? @rafaelfranca I feel like you have a better handle on this than I do.

@rails-bot rails-bot assigned rafaelfranca and unassigned sgrif Sep 21, 2015

rafaelfranca added a commit that referenced this pull request Sep 26, 2015

Merge pull request #21431 from ojab/master
Handle nested fields_for by adding indexes to record_name

@rafaelfranca rafaelfranca merged commit 5b64835 into rails:master Sep 26, 2015

@paratiger

This comment has been minimized.

Show comment
Hide comment
@paratiger

paratiger May 20, 2016

Hi,

I've come across a situation that is effected by this change and wanted to check that this is correct

I use fields_for to add non active record objects that are stored in a serialized field, for example

class Page < ActiveRecord::Base
  serialize :content_blocks, Array
end
class ContentBlock
...
end

Now using f.fields_for "content_blocks[]", ContentBlock.new do |ff| fails as it doesn't respond to id

I can get around this by passing an empty index index: '' to give me page[content_blocks][][title] and defining id as empty string on ContentBlock but should the method rescue when the record_object doesn't respond to id

paratiger commented May 20, 2016

Hi,

I've come across a situation that is effected by this change and wanted to check that this is correct

I use fields_for to add non active record objects that are stored in a serialized field, for example

class Page < ActiveRecord::Base
  serialize :content_blocks, Array
end
class ContentBlock
...
end

Now using f.fields_for "content_blocks[]", ContentBlock.new do |ff| fails as it doesn't respond to id

I can get around this by passing an empty index index: '' to give me page[content_blocks][][title] and defining id as empty string on ContentBlock but should the method rescue when the record_object doesn't respond to id

@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab May 21, 2016

Contributor

I'm not quite sure what exactly you want to archive. My testcase (using rails-4.2.6) produces input field w/ ContentBlock.new.to_s as id, i. e. something like:

<input value="some content" name="page[content_blocks][#<ContentBlock:0x007fbcf6bfc790>][block_content]" id="page_content_blocks_#<ContentBlock:0x007fbcf6bfc790>_block_content" type="text">

and I doubt that it is what you want.
Can you upload a testcase that reproduces your issue?

Contributor

ojab commented May 21, 2016

I'm not quite sure what exactly you want to archive. My testcase (using rails-4.2.6) produces input field w/ ContentBlock.new.to_s as id, i. e. something like:

<input value="some content" name="page[content_blocks][#<ContentBlock:0x007fbcf6bfc790>][block_content]" id="page_content_blocks_#<ContentBlock:0x007fbcf6bfc790>_block_content" type="text">

and I doubt that it is what you want.
Can you upload a testcase that reproduces your issue?

@rocketblr

This comment has been minimized.

Show comment
Hide comment
@rocketblr

rocketblr Aug 23, 2016

Hi @ojab
To be honest I'm not sure if it was a bug or a feature, but this pull request broke the possibility of using fields_for with arrays, because they don't respond to .id method. We use the following approach in our project:

<%= form_for :bulk do |t| %>
  <%= t.fields_for 'people[]', [] do |p| %>
    First Name: <%= p.text_field :first_name %>
    Last Name: <%= p.text_field :last_name %>
  <% end %>
<% end %>

And params looks like:

"bulk" => {
  "people" => [
    {"first_name" => "Michael", "last_name" => "Jordan"},
    {"first_name" => "Steve", "last_name" => "Jobs"},
    {"first_name" => "Barack", "last_name" => "Obama"}
  ]
}

It works perfect in Rails 4.2.5, but fails in Rails 5.

Again I don't know if this behavior of fields_for was a bug or a feature, but it looks like there is no way to get params as array using fields_for now.

Hi @ojab
To be honest I'm not sure if it was a bug or a feature, but this pull request broke the possibility of using fields_for with arrays, because they don't respond to .id method. We use the following approach in our project:

<%= form_for :bulk do |t| %>
  <%= t.fields_for 'people[]', [] do |p| %>
    First Name: <%= p.text_field :first_name %>
    Last Name: <%= p.text_field :last_name %>
  <% end %>
<% end %>

And params looks like:

"bulk" => {
  "people" => [
    {"first_name" => "Michael", "last_name" => "Jordan"},
    {"first_name" => "Steve", "last_name" => "Jobs"},
    {"first_name" => "Barack", "last_name" => "Obama"}
  ]
}

It works perfect in Rails 4.2.5, but fails in Rails 5.

Again I don't know if this behavior of fields_for was a bug or a feature, but it looks like there is no way to get params as array using fields_for now.

@ojab

This comment has been minimized.

Show comment
Hide comment
@ojab

ojab Aug 23, 2016

Contributor

As far as I understand, this form is supposed to create a form w/ fields for one Person and other people's fields are created via AJAX or something similar, isn't it?
If so, I believe that you can use t.fields_for :people, index: nil instead.

Contributor

ojab commented Aug 23, 2016

As far as I understand, this form is supposed to create a form w/ fields for one Person and other people's fields are created via AJAX or something similar, isn't it?
If so, I believe that you can use t.fields_for :people, index: nil instead.

@rocketblr

This comment has been minimized.

Show comment
Hide comment
@rocketblr

rocketblr Aug 23, 2016

I swear I've tried that. My bad. Working as expected. Thanks! Sorry!

I swear I've tried that. My bad. Working as expected. Thanks! Sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment