Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  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
  

• You flagged this item as spam.
  

  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
  

• You flagged this item as spam.
  

  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!

  • nested_plus_touch__save_collection_association.diff 1 KB
  • nested_plus_touch__associated_records_to_validate_or_save.diff 1.5 KB

• You flagged this item as spam.
  

  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
  

• You flagged this item as spam.
  

  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.

  • autosave_changes_only.diff 4.1 KB

• You flagged this item as spam.
  

  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.

  • autosave_changes_with_test.diff 5.8 KB
• 

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

  • Assigned user set to “Eloy Duran”

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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?

• You flagged this item as spam.
  

  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 October 9th, 2010 @ 09:53 PM

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:13 AM

  louis vuitton watches