This project is archived and is in readonly mode.

#4386 open
Jens

dependent => :destroy deletes children before "before_destroy" is executed

Reported by Jens | April 13th, 2010 @ 09:42 PM

Problem: Upon destroying an ActiveRecord::Base object, the "before_destroy" method - which should trigger a transaction rollback if returning false - is only exceuted AFTER all child objects have been destroyed via ":dependent => :destroy".

However, this prevents the before_destroy method from seeing those same child objects, in case it needs them to determine whether the destruction should be successful.

Expected behaviour:
before_destroy should be called before any objects are destroyed, even child records. The before_destroy context should see the original state of the application as if "destroy" was never called. It should be executed within the "destroy" transaction, however, so that any changes it makes can be rolled back.

class Foo < AR::Base
 has_many :children, :dependent => :destroy
 has_many :grandchildren, :through => :children

 before_destroy :check
 def check
  # will always be true since all grandchildren have already been destroyed at this stage
  return self.grandchildren.still_there.empty?
 end
end

class Child < AR::Base
 has_many :grandchildren
 belongs_to :foo
end

class Grandchild < AR::Base
 belongs_to :child
 named_scope :still_there, :conditions => ...
end

Comments and changes to this ticket

  • Ryan Bigg

    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

    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
    
  • Jens

    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

    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.

  • Jens

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

    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.

  • guilherme

    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

    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
    
  • Kane

    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

    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.

  • Kane

    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.

  • Ellis Berner

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

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

  • masciugo

    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

  • Golly
  • Santiago Pastorino

    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

    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.

  • bingbing
  • Brian Buchalter

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

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>