accepts_nested_attributes_for causes stack level too deep

Here's a simple example. The after_save callback in the associated model causes a stack level too deep exception. There's needs to be an :if or :unless option in accepts_nested_attributes_for so you can skip it when necessary.

class Invoice < ActiveRecord::Base
  has_many :line_items
  accepts_nested_attributes_for :line_items
  
  def paid?
  ...
  end
end

class LineItem < ActiveRecord::Base
  belongs_to :invoice  
  after_save :mark_as_paid

  def mark_as_paid
    invoice.update_attribute :paid_at, Time.now if invoice.paid?
  end
end

A workaround that I was able to use in a similar situation was to use ActiveRecord::Dirty#changed? to short-circuit the recursion. So in your case:

  def mark_as_paid
    return unless changed?
    invoice.update_attribute :paid_at, Time.now if invoice.paid?
  end

I think my problem (which I asked about on rails-core) is the same as this: I even used an Invoice/LineItem example! My Invoice has a before_save that sums LineItems' amounts, my LineItem has belongs_to :invoice, :touch => true, and I get the same stack overflow when I try to save a LineItem:

class LineItem < ActiveRecord::Base
  # t.integer :amount
  # t.integer :invoice_id
  belongs_to :invoice, :touch => true
end

class Invoice < ActiveRecord::Base
  # t.integer :total
  # t.timestamps
  has_many :line_items
  accepts_nested_attributes_for :line_items
  before_save :summarize

  def summarize
    self.total = line_items.map(&:amount).sum
  end
end

With these models, in script/console:

>> Invoice.create
=> #<Invoice id: 1, total: 0, created_at: "2009-09-30 05:04:41", updated_at:
"2009-09-30 05:04:41">
>> LineItem.create(:invoice_id => 1, :amount => 10)

This fails:

• The LineItem saves itself, then the generated belongs_to_touch_after_save_or_destroy_for_invoice causes the invoice's updated_at to be changed, then the invoice to be saved.
• Saving the invoice fires its before_save callback, which sums up its total, which is then saved.
• Because accepts_nested_models_for turns on autosave for the association, the invoice then saves its lineitems.
• Saving the lineitems triggers belongs_to_touch_after_save_or_destroy_for_invoice again, and the cycle repeats until the stack overflows.

I was hoping there's a way to break this cycle. I don't want to get rid of my use of :touch; it not only simplifies doing this sort of summarization in the database, it also makes it easy to come up with cache keys for UI fragments (I can just use the parent model's updated_at time).

So, I focused on why the lineitems were being saved again after the parent model was touched; this led me to AutosaveAssociation#save_collection_association, which iterates over the loaded records and saves them, apparently even if they're not new or dirty (the save(false) call on line 295). This surprised me, so I tried hacking my version to change line 294 from elsif autosave to elsif autosave && record.changed? -- and my problem went away. Unfortunately, I now find that this change causes a dozen existing ActiveRecord tests to fail, though I'm not sure why. (This change is attached below as nested_plus_touch__save_collection_association.diff anyway.)

Lawrence Pit responded to my original message, suggesting that it would be better to modify the method that produces the list of records that save_collection_association will iterate over. (His suggestion also causes a bunch of test failures, but I've attached it as a separate independent patch as nested_plus_touch__associated_records_to_validate_or_save.diff anyway.)

I'm going to continue looking into this, but I'd appreciate more suggestions on where to look!

The suggestion was from the top of my head ;-) The following works a lot better, though still 2 failing tests (actually it's 1 test)... but I'm thinking the test is wrong.

   def associated_records_to_validate_or_save(association, new_record, autosave)
      if new_record
        association
      elsif autosave
        association.target.select { |record| record.new_record? || record.changed? || record.marked_for_destruction? }
      else
        association.target.select { |record| record.new_record? }
      end
    end

The test was indeed faulty in my opinion. Attached is a patch with all tests passing.

What remains is to add a test that mimics your original scenario.

Thanks, Lawrence! OK, here's your patch, combined with a test that looks for the stack overflow; only the new test fails without your patch, and passes with it.

Bryan, applied your patch again, confirmed that without my patch your test will go into a stack overflow.

+1, if I may :)

Aha: In my situation, the child model wants to validate_presence_of the parent reference, but the parent reference isn't included in the hash used to create the child. This is discussed in #1943, with a fix (for Rails 3.0? I'm not sure) in #2815.

In my case, I'd tried to work around it using a before_validation_on_create callback on the parent, (like Ari Epstein suggested in a #2815 comment)[https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets..., but the 'late' setting of the parent reference is resurrecting the stack overflow problem for me.

From reading #2815, it sounds like I should be able to include :inverse_of => :invoice in Invoice's has_many :line_items declaration, but that doesn't seem to help - I'd expect to see something looking at the inverse in nested_attributes.rb's ActiveRecord::NestedAttributes::ClassMethods#assign_nested_attributes_for_collection_association (so that it could include the inverse reference in the child's original attribute hash), but I don't see that.

(I'm not sure whether this is the right tree to be barking up... Eloy?)

Aha: In my situation, the child model wants to validate_presence_of the parent reference, but the parent reference isn't included in the hash used to create the child.

Yes that's true, but that's because we are calling #build on the association which will set the appropriate foreign keys. Although it doesn't do this at the same time in all cases, which can make some situations hard. Passing in the instance from nested attributes would mean we'd have to duplicate this logic to set the appropriate field. Where field is the parent association on the child. I'd rather see #build on all associations work in a way where the parent is available before save.

From reading #2815, it sounds like I should be able to include :inverse_of => :invoice in Invoice's has_many :line_items declaration

I couldn't figure out if you mentioned which Rails you're using, but be advised that this unfortunately only works in master, ie Rails 3.0pre.

@Bryan: After some discussion the :inverse_of patches have been backported. It would be great if you could verify that they fix the problem you had. All the patches are in: http://github.com/Fingertips/rails/tree/2-3-stable

Thanks

(from [5193fe9dd730f9bbb72db055f37625fe9558b6ca]) Exclude unchanged records from the collection being considered for autosave. [#2578 state:resolved]

Signed-off-by: Eloy Duran eloy.de.enige@gmail.com
http://github.com/rails/rails/commit/5193fe9dd730f9bbb72db055f37625...

(from [eb22c248de8a867d3ffef09ff8575409918c9011]) Exclude unchanged records from the collection being considered for autosave. [#2578 state:resolved]

Signed-off-by: Eloy Duran eloy.de.enige@gmail.com
http://github.com/rails/rails/commit/eb22c248de8a867d3ffef09ff85754...

I have the same observation as Wil Gieseler. It seems like this section of the codebase got some work in the 2.3.6 release, but the problem in this ticket still occurs. References:

http://github.com/rails/rails/commit/eb22c248de8a867d3ffef09ff85754...
http://github.com/rails/rails/commit/a5696e36c6b49381e84184fcc2f164...
http://github.com/rails/rails/blob/v2.3.6/activerecord/lib/active_r...
http://github.com/rails/rails/commits/v2.3.6/activerecord/lib/activ...

Anyone know what happened? My AR-fu is not strong enough to figure out what is going on in those updates.

Automatic cleanup of spam.

louis vuitton watches