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

[jaylevitt]

Reposting #670 since the lighthouse importer auto-closed it, even though it was still open on lighthouse...

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 before_destroy 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 were 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
  scope :still_there, :conditions => ...
end

[jaylevitt]

Per discussion on #670, the problem is that :dependent => :destroy is in fact implemented as a before_destroy callback, and since callbacks are executed in the order they're defined, the :dependent callback is run before the :check callback. The solution seems to be either force the :dependent callback to run last, or make it its own callback.

[ganeshkumar]

me too facing the same problem.....

[mitijain123]

hey it is not a issue in rails... since previously we are doing everything in transaction for dependent destroy, but now in Rails 3.1.3 we are building the before_destroy method internally we need to rearrange the code.

so your code should be:

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

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

This will do the trick..... :)

[raghunadhd]

Good one @mitijain123

[jaylevitt]

@mitjain123: I respectfully disagree.. I think macros should try as hard as possible to act as declarative as they look.

[mitijain123]

@jaylevitt did you tried this?

here is the rails code which does that.

def configure_dependency
        if options[:dependent]
          unless options[:dependent].in?([:destroy, :delete_all, :nullify, :restrict])
            raise ArgumentError, "The :dependent option expects either :destroy, :delete_all, " \
                                 ":nullify or :restrict (#{options[:dependent].inspect})"
          end

          send("define_#{options[:dependent]}_dependency_method")
          model.before_destroy dependency_method_name
        end
      end

so this is creating a method: before_destroy :X_method

and if X_method is defined before any other before_destroy that will be executed.

may this makes you more clear.

[jaylevitt]

@mitjain123: I'm saying class macros like before_destroy are declarative, and order shouldn't matter. I understand that it does matter because of the internal implementation, and I understand the workaround. I'm saying there shouldn't need to be a workaround.

[aka47]

the workaround, to move the before_destroy callback before the has_many, actually only works half way!!
It does not destroy the "parent", but it still destroy the children.

[tamaloa]

Maybe at least make this behaviour clear in the documentation?

[pyrat]

This is a regression as before_destroy works fine in this manner in 3.0.10 !

[isaacsanders]

Is this still an issue?

[jaylevitt]

@isaacsanders: yes.

[scomma]

Just want to chime in that I got bitten by this. Can't they be moved to a different chain that's executed after user-defined before_destroys? It's not very obvious that dependent: :destroy calls before_destroy internally and that the order of its declaration matters.