Rails Ruby on Rails

Comments and changes to this ticket

• 

  Ryan Bigg April 14th, 2010 @ 04:22 AM

  Could you perhaps create another method that you can call BEFORE calling destroy on the Foo record?

• 

  Andrew White April 14th, 2010 @ 08:46 AM

  Another option is to override the destroy method, e.g:

  class Order < ActiveRecord::Base
    has_many :items
  
    def destroy
      ok_to_destroy? ? super : self
    end
  
    private
      def ok_to_destroy?
        errors.clear
        errors.add(:items, "Can't destroy order as items have been processed") if items.processed_any?
        errors.empty?
      end
    end
  end
  
  class Item < ActiveRecord::Base
    belongs_to :order
    named_scope :processed, :conditions => { :processed => true }
  end
  

• You flagged this item as spam.
  

  Jens April 15th, 2010 @ 06:44 AM

  Thank you for the hints!

  Creating another method and manually calling this before destroying children is IMO exactly what :before_destroy should be for. Right? Also, I would have to insert this in a dozen places where complex dependencies exist, so this is not really a solution.

  Overriding "destroy" can be a solution if I do not accidentally touch Rails internals (as in overriding the "initialize" method, which can have numerous side effects).

  But I still regard this as a bug: before_destroy should either be renamed, or be executed before anything ist destroyed, including child objects.

• 

  Andrew White April 15th, 2010 @ 10:17 AM

  The problem is that the child records are deleted using a before_destroy callback as well and the callbacks are executed in the order that they're added. This can't be changed to after_destroy because if foreign keys are being used in the database it will cause an error if they're not cascading deletes and the child records won't be found to have their destroy methods called if cascading deletes are enabled.

• You flagged this item as spam.
  

  Jens April 16th, 2010 @ 06:23 AM

  Then this issue is maybe more general than I thought. Perhaps we need a way to order the callbacks? Something like

  @@@ ruby Class Foo < AR::Base
  # adds :check to the beginning of the before_destroy chain before_destroy :check, :order => :first # default, adds :check to the end of the before_destroy chain before_destroy :check, :order => :last ... end
  

  
  Same for all other callbacks.
  
  
  
  
  
  
  
  IMHO this is absolutely necessary if Rails also uses these callbacks internally, since  the callbacks give the impression that the user has complete control over them, which is not true.
  
  
  
  
  
  
  
  Alternatively (and maybe better), the child deletion procedure (and other internal routines which use before / after callbacks) need to be rewritten to be executed after all before callbacks, or before all after callbacks, respectively, since this is what the user expects according to the naming of these procedures and their documentation, which does not mention that pre-defined callbacks already exist internally.
  

• You flagged this item as spam.
  

  Jens April 16th, 2010 @ 06:27 AM

  Argh, formatting messed up. (Why? preview worked..)

  Then this issue is maybe more general than I thought. Perhaps we need a way to order the callbacks? Something like

  Class Foo < AR::Base
     # adds :check to the beginning of the before_destroy chain
     before_destroy :check, :order => :first
     # default, adds :check to the end of the before_destroy chain
     before_destroy :check, :order => :last
     ...
  end
  

  Same for all other callbacks.

  IMHO this is absolutely necessary if Rails also uses these callbacks internally, since the callbacks give the impression that the user has complete control over them, which is not true.

  Alternatively, the child deletion procedure (and other internal routines which use before / after callbacks) need to be rewritten to be executed after all before callbacks, or before all after callbacks, respectively, since this is what the user expects (IMHO) according to the naming of these procedures and their documentation.

• You flagged this item as spam.
  

  guilherme July 29th, 2010 @ 07:24 PM

  I agree with you Jens.
  What you think about the callbacks implementation could be a stack(LIFO) ? so the :dependent => :destroy will be executed after all before_destroy callbacks, what i think is the expected behavior.

• 

  Neeraj Singh August 11th, 2010 @ 02:54 PM

  • Importance changed from “” to “Low”

  @Jen

  Can you try with rails edge. I am not able to reproduce this problem.

  class Car < ActiveRecord::Base
    has_many :brakes, :dependent => :destroy
    before_destroy :check
  
    def check
      false
    end
  
    def self.lab
      Car.delete_all
      Brake.delete_all
  
      car = Car.create(:name => 'honda')
      car.brakes.create(:name => 'b1')
      car.reload
      car.destroy
      puts Car.count #=> 1 if check returns false. 0 if check returns true
      puts Brake.count #=> 1 if check returns false. 0 if check returns true
    end
  
  end
  

• You flagged this item as spam.
  

  Kane August 11th, 2010 @ 05:06 PM

  i encountered this too in 3.0.0.rc
  to workaround this issue i didnt use the dependent option and instead created an before_destroy callback which destroys all associations for me. i put it after all other before_destroy callbacks.

  As Andrew White pointed out, doing the destroy of the associations in a after_destroy callback collides with fk contraints.

  so the destroy of the associated objects should happen after the before_destroy callbacks but before the destroy.

  @Neeraj Singh

  you dont cover the described behaviour.
  your example just shows that the children are not destroyed if the callback chain is interrupted cause all changes are rolled back.

  the problem is as Jens described that in the before callbacks the children are already all gone. this happens because the before_destroy callback registered by the has_many is called before the other callback.you could simply fix this by altering the order, and register the other callbacks first, but i like to declare the associations first.

  class Car < ActiveRecord::Base
    has_many :brakes, :dependent => :destroy
    before_destroy :check
  
    def check
      false unless brakes.empty?
    end
  
    def self.lab
      Car.delete_all
      Brake.delete_all
  
      car = Car.create(:name => 'honda')
      car.brakes.create(:name => 'b1')
      car.reload
      car.destroy
      puts Car.count #=>  0 
      puts Brake.count #=> 0
    end
  

  I know this example is kinda stupid but it shows the problem.
  In check brakes is already empty.

• 

  Neeraj Singh August 11th, 2010 @ 05:32 PM

  The order of callbacks matter. Checkout #2765 in which extra record was getting created because of order of callbacks.

• You flagged this item as spam.
  

  Kane August 11th, 2010 @ 10:32 PM

  yea i know as i said the problem can be avoided by altering the order.
  but the issue here is that this is not so obvious for the user and whether the actual behaviour is the right one.

• You flagged this item as spam.
  

  Ellis Berner November 9th, 2010 @ 03:57 PM

  I agree. The before_destroy callback needs to be before the dependent destroy callback.

• You flagged this item as spam.
  

  masciugo December 16th, 2010 @ 04:50 PM

  I had same problem,

  the solution was easy and scary at the same time. I just moved before_destroy definition before association definition

  I had never supposed such order was significant, and actually i'd prefer not but so it seems

  here the same situation
  http://blog.ireneros.com/rails-model-callbacks-and-associations-order

• You flagged this item as spam.
  

  Golly December 19th, 2010 @ 08:18 AM

  Probably related to #6191

• 

  Santiago Pastorino February 4th, 2011 @ 09:42 PM

  Can someone check if this is still an issue after we pushed a fix for #6191 ?

• 

  Andrew White February 9th, 2011 @ 03:31 AM

  • State changed from “new” to “open”

  It's still there - it can't be easily fixed since both callbacks in the same chain. As masciugo points out you can work around the problem by moving the before_destroy call before the association definition. Perhaps a more longterm solution is to add a destroy/delete validation callback. Obviously this would be a different validation process - no point in validating uniqueness on something you're about to destroy.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:03 AM

  louis vuitton watches

• You flagged this item as spam.
  

  Brian Buchalter April 24th, 2011 @ 02:54 PM

  I've recently encountered this problem as well. Still not fixed? Seems like a core piece of functionality...