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

Closed
jaylevitt opened this Issue Oct 28, 2011 · 35 comments

Comments

Projects
None yet
@jaylevitt
Contributor

jaylevitt commented Oct 28, 2011

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

This comment has been minimized.

Show comment
Hide comment
@jaylevitt

jaylevitt Oct 28, 2011

Contributor

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.

Contributor

jaylevitt commented Oct 28, 2011

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

This comment has been minimized.

Show comment
Hide comment
@ganeshkumar

ganeshkumar Dec 14, 2011

Contributor

me too facing the same problem.....

Contributor

ganeshkumar commented Dec 14, 2011

me too facing the same problem.....

@mitijain123

This comment has been minimized.

Show comment
Hide comment
@mitijain123

mitijain123 Dec 15, 2011

Contributor

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

Contributor

mitijain123 commented Dec 15, 2011

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

This comment has been minimized.

Show comment
Hide comment
@raghunadhd

raghunadhd Dec 15, 2011

Contributor

Good one @mitijain123

Contributor

raghunadhd commented Dec 15, 2011

Good one @mitijain123

@jaylevitt

This comment has been minimized.

Show comment
Hide comment
@jaylevitt

jaylevitt Dec 15, 2011

Contributor

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

Contributor

jaylevitt commented Dec 15, 2011

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

@mitijain123

This comment has been minimized.

Show comment
Hide comment
@mitijain123

mitijain123 Dec 15, 2011

Contributor

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

Contributor

mitijain123 commented Dec 15, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@jaylevitt

jaylevitt Dec 16, 2011

Contributor

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

Contributor

jaylevitt commented Dec 16, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@aka47

aka47 Feb 1, 2012

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.

aka47 commented Feb 1, 2012

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

This comment has been minimized.

Show comment
Hide comment
@tamaloa

tamaloa Feb 2, 2012

Maybe at least make this behaviour clear in the documentation?

tamaloa commented Feb 2, 2012

Maybe at least make this behaviour clear in the documentation?

@pyrat

This comment has been minimized.

Show comment
Hide comment
@pyrat

pyrat Feb 10, 2012

Contributor

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

Contributor

pyrat commented Feb 10, 2012

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

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 4, 2012

Contributor

Is this still an issue?

Contributor

isaacsanders commented May 4, 2012

Is this still an issue?

@jaylevitt

This comment has been minimized.

Show comment
Hide comment
@jaylevitt

jaylevitt May 5, 2012

Contributor
Contributor

jaylevitt commented May 5, 2012

@scomma

This comment has been minimized.

Show comment
Hide comment
@scomma

scomma May 31, 2012

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.

scomma commented May 31, 2012

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.

@rogercampos

This comment has been minimized.

Show comment
Hide comment
@rogercampos

rogercampos Jun 20, 2012

Just bitten by this too. +1 to a less error-prone solution for this issue, independent of the macro definition order.

Just bitten by this too. +1 to a less error-prone solution for this issue, independent of the macro definition order.

@kliuless

This comment has been minimized.

Show comment
Hide comment
@kliuless

kliuless Jul 24, 2012

@pyrat are you sure it was working in 3.0.10? I just did a test and it's broken there too.

@pyrat are you sure it was working in 3.0.10? I just did a test and it's broken there too.

@hackhowtofaq

This comment has been minimized.

Show comment
Hide comment
@hackhowtofaq

hackhowtofaq Jul 27, 2012

Just bitten by this too in Rails 3.2.3 +1 For a better solution, or at least indicate this in documentation.

Just bitten by this too in Rails 3.2.3 +1 For a better solution, or at least indicate this in documentation.

@gstark

This comment has been minimized.

Show comment
Hide comment
@gstark

gstark Aug 7, 2012

Just bit me too. I agree that this should, at least, be in the documentation.

gstark commented Aug 7, 2012

Just bit me too. I agree that this should, at least, be in the documentation.

@aless

This comment has been minimized.

Show comment
Hide comment
@aless

aless Aug 30, 2012

Same problem here with 3.2.2. Is there any workaround for observers?

aless commented Aug 30, 2012

Same problem here with 3.2.2. Is there any workaround for observers?

@mtaylor

This comment has been minimized.

Show comment
Hide comment
@mtaylor

mtaylor Sep 11, 2012

We have also ran into this issue. If this is expected behaviour then it should be clearly highlighted in the documentation.

mtaylor commented Sep 11, 2012

We have also ran into this issue. If this is expected behaviour then it should be clearly highlighted in the documentation.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 15, 2012

Member

I've just pushed lifo/docrails#ecaf72877, which adds documentation for this behavior.

Changing this behavior might mess with existing apps, and so we'd need a migration path. Since it's not exactly a bug, and I've written documentation, I'm giving this a close. If someone would like to make them work in a more declarative fashion, please implement it and submit a pull request, or ping rails-core to see if it's even a good idea; changing this may subtly break a lot of apps.

Thanks!

Member

steveklabnik commented Sep 15, 2012

I've just pushed lifo/docrails#ecaf72877, which adds documentation for this behavior.

Changing this behavior might mess with existing apps, and so we'd need a migration path. Since it's not exactly a bug, and I've written documentation, I'm giving this a close. If someone would like to make them work in a more declarative fashion, please implement it and submit a pull request, or ping rails-core to see if it's even a good idea; changing this may subtly break a lot of apps.

Thanks!

@njakobsen

This comment has been minimized.

Show comment
Hide comment
@njakobsen

njakobsen Dec 9, 2012

Contributor

Just got bitten by this too. It is definitely counter intuitive, especially if your models are defined in an acts_as module and you want your code to look like

class MyClass
  acts_as_foo    
  before_destroy :bar
end

@steveklabnik Seeing as how it subtly broke a lot of apps by changing to the way it is right now, I think "how do you think it should work" should be the criteria for deciding on the behaviour. I'll poke around in there and see if there's a way to do it that makes sense, and doesn't require us to document why this doesn't work the way you expect it to work anymore.

Contributor

njakobsen commented Dec 9, 2012

Just got bitten by this too. It is definitely counter intuitive, especially if your models are defined in an acts_as module and you want your code to look like

class MyClass
  acts_as_foo    
  before_destroy :bar
end

@steveklabnik Seeing as how it subtly broke a lot of apps by changing to the way it is right now, I think "how do you think it should work" should be the criteria for deciding on the behaviour. I'll poke around in there and see if there's a way to do it that makes sense, and doesn't require us to document why this doesn't work the way you expect it to work anymore.

@Silex

This comment has been minimized.

Show comment
Hide comment
@Silex

Silex Jan 10, 2013

Biten by this too. I agree with @jaylevitt, this is a bug and not a "documentation problem". Declarations orders shouldn't matter.

Silex commented Jan 10, 2013

Biten by this too. I agree with @jaylevitt, this is a bug and not a "documentation problem". Declarations orders shouldn't matter.

@earnold

This comment has been minimized.

Show comment
Hide comment
@earnold

earnold Mar 19, 2013

Contributor

@steveklabnik I feel like this should also be mentioned in the documentation for ActiveRecord::Observer

Right now, I don't think there is any way for before_destroy methods in observers to see whether or not the model-in-question had children.

Contributor

earnold commented Mar 19, 2013

@steveklabnik I feel like this should also be mentioned in the documentation for ActiveRecord::Observer

Right now, I don't think there is any way for before_destroy methods in observers to see whether or not the model-in-question had children.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 19, 2013

Member

Go ahead and send a pull request to rails/rails-observers , since Observers are no longer in Rails.

Member

steveklabnik commented Mar 19, 2013

Go ahead and send a pull request to rails/rails-observers , since Observers are no longer in Rails.

@earnold earnold referenced this issue in rails/rails-observers Mar 19, 2013

Merged

Update README #3

@matpowel

This comment has been minimized.

Show comment
Hide comment
@matpowel

matpowel Mar 25, 2013

The but problem with this is issue is that it is not consistent, for example we are seeing the problem when running in POW but not in Webrick. I'm unsure if we're seeing it in production (Passenger + REE) but it seems that we are.

A number have people have referenced the order of the has_many and before_destroy but it doesn't seem to make any difference for us, only the hosting environment which is not comforting. We're going to look into it a little more in an empty Rails project and will report back.

The but problem with this is issue is that it is not consistent, for example we are seeing the problem when running in POW but not in Webrick. I'm unsure if we're seeing it in production (Passenger + REE) but it seems that we are.

A number have people have referenced the order of the has_many and before_destroy but it doesn't seem to make any difference for us, only the hosting environment which is not comforting. We're going to look into it a little more in an empty Rails project and will report back.

daviddavis added a commit to Katello/katello that referenced this issue Apr 23, 2013

jlsherrill added a commit to jlsherrill/katello that referenced this issue Apr 23, 2013

daviddavis added a commit to Katello/katello that referenced this issue Apr 27, 2013

daviddavis added a commit to Katello/katello that referenced this issue Apr 27, 2013

Merge pull request #2030 from daviddavis/temp_1366666322
Moving before_destroy callbacks because of rails/rails#3458

daviddavis added a commit to Katello/katello that referenced this issue Apr 30, 2013

@AnwarShah

This comment has been minimized.

Show comment
Hide comment
@AnwarShah

AnwarShah Feb 8, 2016

Just bitten by this too. This should be mentioned in the documentation at least. Very frustrating!

Just bitten by this too. This should be mentioned in the documentation at least. Very frustrating!

damianlegawiec added a commit to spark-solutions/spree that referenced this issue Jun 15, 2016

Fixed: don't destroy dependent associations if we cannot destroy the …
…Product/Variant

Previously : ensure_no_line_items  callback could stop deleting the
Product/Variant but associations (with dependent: :destroy) were
deleted whatsoever, as this is a rails bug (see
rails/rails#3458)

damianlegawiec added a commit to spark-solutions/spree that referenced this issue Jun 15, 2016

Fixed: don't destroy dependent associations if we cannot destroy the …
…Product/Variant

Previously : ensure_no_line_items  callback could stop deleting the
Product/Variant but associations (with dependent: :destroy) were
deleted whatsoever, as this is a rails bug (see
rails/rails#3458)

damianlegawiec added a commit to spark-solutions/spree that referenced this issue Jun 15, 2016

Fixed: don't destroy dependent associations if we cannot destroy the …
…Product/Variant

Previously : ensure_no_line_items  callback could stop deleting the
Product/Variant but associations (with dependent: :destroy) were
deleted whatsoever, as this is a rails bug (see
rails/rails#3458)

damianlegawiec added a commit to spark-solutions/spree that referenced this issue Jun 16, 2016

Fixed: don't destroy dependent associations if we cannot destroy the …
…Country

Previously :ensure_not_default  callback could stop deleting the
Country but associations (with dependent: :destroy) were
deleted whatsoever, as this is a rails bug (see
rails/rails#3458)
@r0qs

This comment has been minimized.

Show comment
Hide comment
@r0qs

r0qs Aug 23, 2016

Is this still an issue?

r0qs commented Aug 23, 2016

Is this still an issue?

@kesha-antonov

This comment has been minimized.

Show comment
Hide comment
@kesha-antonov

kesha-antonov Nov 6, 2016

Contributor

Same

Contributor

kesha-antonov commented Nov 6, 2016

Same

@lucasocon

This comment has been minimized.

Show comment
Hide comment

Same

@pierrea

This comment has been minimized.

Show comment
Hide comment
@pierrea

pierrea Feb 3, 2017

Still having the same issue here too

pierrea commented Feb 3, 2017

Still having the same issue here too

@andreiglingeanu

This comment has been minimized.

Show comment
Hide comment
@andreiglingeanu

andreiglingeanu Feb 13, 2017

Yeah, I just fixed it by swapping positions for before_destroy and dependent: :destroy. This fixed the problem.

Yeah, I just fixed it by swapping positions for before_destroy and dependent: :destroy. This fixed the problem.

@xinranxiao

This comment has been minimized.

Show comment
Hide comment
@xinranxiao

xinranxiao Mar 29, 2017

Another approach (basically built for this):

(using the original example)

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

  before_destroy :check, prepend: true

  def check
    # this will run before the dependent: destroy callback.
    return self.grandchildren.still_there.empty?
  end
end

prepend: true will cause the callback to run before the dependent destroy on the children.

Reference: http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html

Another approach (basically built for this):

(using the original example)

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

  before_destroy :check, prepend: true

  def check
    # this will run before the dependent: destroy callback.
    return self.grandchildren.still_there.empty?
  end
end

prepend: true will cause the callback to run before the dependent destroy on the children.

Reference: http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html

@vishalzambre

This comment has been minimized.

Show comment
Hide comment
@vishalzambre

vishalzambre Apr 3, 2017

Contributor

@xinranxiao Here if you want to restrict to delete record need to throw exception to abort
throw(:abort) if you return false it'd not work as expected.

Contributor

vishalzambre commented Apr 3, 2017

@xinranxiao Here if you want to restrict to delete record need to throw exception to abort
throw(:abort) if you return false it'd not work as expected.

@xinranxiao

This comment has been minimized.

Show comment
Hide comment
@xinranxiao

xinranxiao Apr 3, 2017

^that is only the case if using Rails 5 with ActiveSupport.halt_callback_chains_on_return_false = false set right?

^that is only the case if using Rails 5 with ActiveSupport.halt_callback_chains_on_return_false = false set right?

@HassanTC

This comment has been minimized.

Show comment
Hide comment
@HassanTC

HassanTC Aug 6, 2017

i solved this problem by using a funny hack,
you just need to put the before_destroy line
before the association line
and it will run before_destroy before deleting the associations

HassanTC commented Aug 6, 2017

i solved this problem by using a funny hack,
you just need to put the before_destroy line
before the association line
and it will run before_destroy before deleting the associations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment