This project is archived and is in readonly mode.

#2578 ✓resolved
Theodore Mills

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

    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

    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

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

  • Lawrence Pit

    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

    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

    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

    Eloy Duran October 1st, 2009 @ 09:00 AM

    • Assigned user set to “Eloy Duran”
  • Lawrence Pit

    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

    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

    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

    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

    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

    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

    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

    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

    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

    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

    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.

  • Ryan Bigg

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

    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