This project is archived and is in readonly mode.
accepts_nested_attributes_for causes stack level too deep
Reported by Theodore Mills | April 28th, 2009 @ 06:41 PM | in 2.x
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
Comments and changes to this ticket
-
Theodore Mills April 28th, 2009 @ 06:43 PM
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
-
Denis September 29th, 2009 @ 10:11 PM
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
-
Bryan Stearns October 1st, 2009 @ 02:59 AM
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 hasbelongs_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'supdated_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'supdated_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 fromelsif autosave
toelsif 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 LineItem saves itself, then the generated
-
Lawrence Pit October 1st, 2009 @ 04:20 AM
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
-
Lawrence Pit October 1st, 2009 @ 04:37 AM
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.
-
Bryan Stearns October 1st, 2009 @ 08:19 AM
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.
-
Eloy Duran October 1st, 2009 @ 09:00 AM
- Assigned user set to Eloy Duran
-
Lawrence Pit October 1st, 2009 @ 09:27 AM
Bryan, applied your patch again, confirmed that without my patch your test will go into a stack overflow.
+1, if I may :)
-
Bryan Stearns October 1st, 2009 @ 04:04 PM
- Tag set to patch
OK, then! Thanks, Lawrence - Hi Eloy. Was I supposed to add a "patch" tag? Oh, look, here's one.
-
Bryan Stearns October 9th, 2009 @ 12:23 AM
- Tag cleared.
Hm. I'm now back to working on the problem this patch was supposed to fix, and it's not fixing it (I'm still seeing the stack overflow). I'm trying to figure out what's different, and will update the patch once I figure it out. (I'm removing the patch tag in the meantime.)
-
Bryan Stearns October 12th, 2009 @ 02:20 AM
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?)
-
Eloy Duran October 12th, 2009 @ 08:34 AM
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.
-
Eloy Duran December 17th, 2009 @ 03:01 PM
@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
-
Eloy Duran December 28th, 2009 @ 08:51 PM
- State changed from new to resolved
The :inverse_of patches have been pushed to 2.3-stable, closing for now.
-
Repository January 8th, 2010 @ 11:24 PM
(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... -
Repository January 8th, 2010 @ 11:28 PM
(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... -
Wil Gieseler July 7th, 2010 @ 06:44 AM
- Importance changed from to
Hi,
I'm not sure if this is the right place to ask, but I can't find anywhere else.
I am using Rails 2.3.8 and this problem still seems to occur. Were these commits ever merged into a released version of Rails?
-
Todd Eichel September 25th, 2010 @ 09:49 PM
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.
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>
People watching this ticket
Attachments
Referenced by
- 2578 accepts_nested_attributes_for causes stack level too deep (from [5193fe9dd730f9bbb72db055f37625fe9558b6ca]) Exclude...
- 2578 accepts_nested_attributes_for causes stack level too deep (from [eb22c248de8a867d3ffef09ff8575409918c9011]) Exclude...
- 3353 accepts_nested_attributes, validates_presence_of - saves associated model We're executing after_save callback by touching associate...