This project is archived and is in readonly mode.

#2172 ✓duplicate
David Yip

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

Reported by David Yip | March 8th, 2009 @ 10:56 AM | in 2.x

When a belongs_to association called in a nested model form construction context evaluates to nil, new_record? is invoked on nil, which causes an error to be raised.

Here is an example. Consider the following models:


class Author < ActiveRecord::Base
end

class Post < ActiveRecord::Base
  belongs_to :author

  accepts_nested_attributes_for :author
end

Suppose we have in a PostsController


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

This is (to my understanding) a common Rails controller pattern.

Finally, in the view for PostsController#new:


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

As of commit 5c87e9adddc22703a3dbbb785e32fafe0e91ce78, invoking new triggers the code path that results in the behavior described above.

This may also occur with has_one associations; I haven't explicitly tried those, but reasoning through the code brings me to a similar conclusion.

Attached are a first cut at a patch to solve this issue, an automated test to describe the problem, and a small application that demonstrates the issue.

Comments and changes to this ticket

  • David Yip

    David Yip March 8th, 2009 @ 11:02 AM

    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.

  • David Yip

    David Yip March 8th, 2009 @ 11:05 AM

    • Title changed from “Nested model forms error on loading data from one-to-one associations that evaluate to nil” to “Nested model forms error on loading data from belongs_to associations that evaluate to nil”
  • Eloy Duran

    Eloy Duran March 8th, 2009 @ 01:03 PM

    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?

  • David Yip

    David Yip March 8th, 2009 @ 08:06 PM

    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 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.

  • Eloy Duran

    Eloy Duran March 8th, 2009 @ 10:08 PM

    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.

  • David Yip

    David Yip March 9th, 2009 @ 04:10 AM

    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 :)

  • Eloy Duran

    Eloy Duran March 9th, 2009 @ 10:02 AM

    • Assigned user changed from “Eloy Duran” to “Michael Koziarski”
    • State changed from “new” to “open”

    Very nice, thanks a lot!

    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 :)

    I have refactored it a little bit, which now also makes it return nil instead of an empty string, your test still passes so it should be good enough.

    I also moved the test down in the file to where the other tests related to nested attributes are and renamed the method to match that of the others.

    +1

  • Eloy Duran
  • Pratik

    Pratik March 9th, 2009 @ 11:28 AM

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

  • José Valim

    José Valim March 9th, 2009 @ 11:42 AM

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

  • Eloy Duran

    Eloy Duran March 9th, 2009 @ 12:47 PM

    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.

  • Eloy Duran

    Eloy Duran March 9th, 2009 @ 12:51 PM

    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.

  • Pratik

    Pratik March 9th, 2009 @ 12:55 PM

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

  • Eloy Duran

    Eloy Duran March 9th, 2009 @ 01:39 PM

    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?

  • David Yip

    David Yip March 9th, 2009 @ 04:02 PM

    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?

  • Eloy Duran

    Eloy Duran March 9th, 2009 @ 08:14 PM

    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.

  • José Valim

    José Valim March 19th, 2009 @ 11:53 AM

    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.

  • Ivan Vanderbyl

    Ivan Vanderbyl March 19th, 2009 @ 11:08 PM

    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
    
  • Eloy Duran

    Eloy Duran March 29th, 2009 @ 12:37 PM

    • Assigned user changed from “Michael Koziarski” to “Eloy Duran”

    Please see #2365 for a possible solution on building new associated records if they don't exist yet.

  • José Valim

    José Valim August 8th, 2009 @ 12:44 PM

    • State changed from “open” to “duplicate”

    Marking as duplicated. As Eloy said, try #2365 and let your comments there, so we can get it applied.

  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 09:45 PM

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages