Nested model forms error on loading data from belongs_to associations that evaluate to nil

Here's a manual test in the form of a small Rails application that exercises the full stack in a scenario similar to the one in the problem description. To exercise it, go to /posts.

I agree we should fix this error people run into. But I wonder if simply using a regular fields_for is what the user expects. For instance, how about dynamic defaults that might be set when a model is first instantiated?

Rather I would opt for not yielding a form builder at all if the association is nil. This would be more consequent with the collection variant as well. As in, if there's no associated record then don't yield a new form builder.

So in this case the user knows clearly that he/she should initialize an instance in order to get the form builder, but a user can also choose not to have a form builder if there's no association. Thus leaving the control in the controller where it should be.

class PostsController < ApplicationController
  ...
  def new
    @post = Post.new
    @post.build_author
  end
  ...
end

Thoughts?

Hmm, let me make sure I understand the proposal.

In this case, if post.author is nil, then the render result for

<% form_for @post do |f| %>
  <% f.fields_for :author do |a| %>
    <%= a.label :name, 'Author name' %>
    <%= a.text_field :name %>
  <% end %>
  <%= f.submit %>
<% end %>

would be equivalent to the render result of

<% form_for @post do |f| %>
  <%= f.submit %>
<% end %>

?

I think this would be fine, provided (of course) that such behavior is documented on fields_for.

It does make controller code for complex nested model schemas a bit more tedious to produce; I'm currently working on an application that would end up looking like

class PostsController < ApplicationController
  def new
   @post = Post.new
   @post.build_author
   @post.author.build_foo
   @post.author.build_bar
   ...
end

But I don't think this extra verbosity is necessarily a problem. As a counterpoint, though, there was a comment on #1930 autosave should validate associations even if master is invalid by pete_lacey (http://rails.lighthouseapp.com/p...) in which he mentioned "27 deeply nested calls to accepts_nested_attributes_for", and I can see how this could get troublesome. I'll let others who are actually going that far chime in on that.

Yes, you got it right. And it should definitely be mentioned in the documentation.

About the extra work necessary in the controller; you might want to shove that into the Post model if a Post should have those associations anyway.

class Post < ActiveRecord::Base
  after_initialize :build_author
end

class Author < ActiveRecord::Base
  after_initialize :build_foo
  after_initialize :build_bar
end

class PostsController < ApplicationController
  def new
   @post = Post.new
   ...
end

Obviously if you only use these models in one controller it won't clean up the amount of lines of code. But the controller or model are more appropriate places for this so one has more control. Especially in those cases like Pete's, the model is definitely where you'd want to solve this imo.

Yeah, there's definitely times where the model is a more appropriate location for this sort of initialization code.

Here's another patch that implements the "don't yield a builder" behavior. It also adds some documentation for that behavior. Returning an empty string from fields_for_with_nested_attributes feels dirty for some reason I can't qualify, but it does communicate the intent and passes all Action Pack tests, so I'll leave elegance to those who are more familiar with this part of the Rails codebase :)

not sure I like the idea of silently eating the f.fields_for block when the association is nil.

I agree with Pratik. Why not see if the :author is an association on Post and call build_author?

not sure I like the idea of silently eating the f.fields_for block when the association is nil.

Like discussed in #rails-contrib, this is not silently eating it (there is even extra documentation about it). See my earlier comment:

Rather I would opt for not yielding a form builder at all if the association is nil. This would be more consequent with the collection variant as well. As in, if there's no associated record then don't yield a new form builder. So in this case the user knows clearly that he/she should initialize an instance in order to get the form builder, but a user can also choose not to have a form builder if there's no association. Thus leaving the control in the controller where it should be.

I agree with Pratik. Why not see if the :author is an association on Post and call build_author?

First of all because, of the reason I mentioned earlier; it's inconsistent with collection associations.

And second, it would mean that we would have to add a bunch of AR specific code into the form helper, which is not something I'd like to do. Right now only new_record? is being used, which works on DM as well.

I wouldn't really worry about DM. That should be a separate concern altogether.

After discussing this with Manfred, we came to the conclusion that it might be best to raise informative exceptions. Because I personally, and others with big forms, haven't needed this kind of magic yet, so I don't feel strongly about it. What I do feel though, is that building records is not a concern of the view.

Anyways, currently form_for only fails with the default whiny_nil message. So we were thinking exceptions along the lines of:

• "ArgumentError: The object found by or passed to form_for is nil."
• "ArgumentError: The object found by or passed to fields_for is nil."
• "ArgumentError: The object found by or passed to fields_for is nil, if you're rendering a form for a nested model make sure you build one first."

Thoughts?

I think exceptions would be fine; I personally haven't had a need for using forms on schemas that don't match what is expected by the form.

However, form_for doesn't currently raise an exception it finds is nil; it only occurs in the explicit object case. For example, given @post = nil,

<% form_for :post do |p| %>
  <%= p.text_field :title %>
<% end %>

currently renders a form with a text field having name post[title]. Is this behavior that should stay around, or should form_for be modified to require an object?

currently renders a form with a text field having name post[title]. Is this behavior that should stay around, or should form_for be modified to require an object?

Hmm, you're right. I personally don't really understand why that's supported at all, or have had a need for it, so let's see what Michael has to say about this.

What if we raise an error on fields_for just if the parent form object is not nil?

That would cover both cases:

1. It would work without objects (@post=nil and @author=nil).

2. It would raise an error when @post is not nil but @post.author is nil, as proposed previously.

In my case this fixed the problem, simply not calling new_record? on nil and handling it as though it was an existing record.

module ActionView
  module Helpers
    class FormBuilder
      def fields_for_nested_model(name, object, args, block)
        if object && object.new_record?
          @template.fields_for(name, object, *args, &block)
        else
          @template.fields_for(name, object, *args) do |builder|
            @template.concat builder.hidden_field(:id)
            block.call(builder)
          end
        end
      end
    end
  end
end

gucci watches