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

[lighthouse-import]

_Imported from Lighthouse._ Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/4386
Created by Jens - 2011-02-22 08:32:33 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Ryan Bigg - 2010-04-14 03:22:59 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Andrew White - 2010-04-14 07:46:03 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Jens - 2010-04-15 05:44:12 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Andrew White - 2010-04-15 09:17:42 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Jens - 2010-04-16 05:23:20 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Jens - 2010-04-16 05:27:30 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by guilherme - 2010-07-29 18:24:34 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Neeraj Singh - 2010-08-11 13:54:46 UTC

@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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Kane - 2010-08-11 16:06:14 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Neeraj Singh - 2010-08-11 16:32:28 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Kane - 2010-08-11 21:32:29 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Ellis Berner - 2010-11-09 15:57:46 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by masciugo - 2010-12-16 16:50:10 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Golly - 2010-12-19 08:18:40 UTC

Probably related to #6191

[lighthouse-import]

_Imported from Lighthouse._
Comment by Santiago Pastorino - 2011-02-04 21:42:59 UTC

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

[lighthouse-import]

_Imported from Lighthouse._
Comment by Andrew White - 2011-02-09 03:31:18 UTC

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.

[lighthouse-import]

_Imported from Lighthouse._
Comment by Brian Buchalter - 2011-04-24 13:54:03 UTC

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

[tamaloa]

This still is present (in 3.0.11 at least).

Maybe at least make this behaviour clear in the documentation?

[wpp]

Hi,

I had the exact same problem today. Also had to move the before_destroy before the association definitions. (3.1.0)

[Gawyn]

Exactly the same problem in 3.2.8: I also had to move the hook before the association.

[collimarco]

Same problem in 3.2.6. I had to move before_destroy before associations.

[collimarco]

It is even worse with Observers. How can I access to record associations (within an Observer) in a before_destroy callback?

[fxn]

Associations just set a callback, callbacks run in order, maybe the docs could be more explicit about it?

[steveklabnik]

@fxn I am pretty sure that I fixed the docs for this. I can't remember the ticket though.

[fxn]

@steveklabnik awesome

[steveklabnik]

Actually, I can't find it. I'll submit something to docrails now.

[steveklabnik]

Done in https://github.com/lifo/docrails/commit/79cbfc8

[fxn]

:heart:

[nextofsearch]

Same problem in 3.2.12. :(

[fxn]

@nextofsearch it is not exactly a problem, please read the last comments.

[nextofsearch]

@fxn I was misunderstood. Thanks!

[ndbroadbent]

Could we please just give before_destroy callbacks a higher precedence than :dependent => :destroy associations?

I feel like the before prefix should mean that it prepends the callback, so it runs before everything else.

[wingice]

I thought a 'before_destroy_validation' would be better.

[tboyko]

@ndbroadbent That's my thought too, though I would not be surprised if there are cases where before_destroy ideally runs after dependent: :destroy has taken place. I'm interested in hearing about these cases if they exist. If they don't, then a change would be nice (perhaps in Rails 5).

[funkenstrahlen]

Please do something about this. This is years old and very very hard to debug!

Associated dependencies should _never_ get destroyed before all before_destroy callbacks got executed.

[fxn]

@i42n please read comments above, there is nothing to fix. AR cannot assume a set of callbacks should run before another set of callbacks. The contract is they run in order, just declare callbacks and associations in order if order is important (as explained above).

[smudge]

AR cannot assume a set of callbacks should run before another set of callbacks.

That's not exactly true. It assumes before_event callbacks run before event and after_event callbacks run after event. Callback types are clearly grouped by type & event, run in the order of the events, and only then within their individual groupings do they run in the order they were defined in the model code.

So why can't before_destroy run before dependents are destroyed? It seems like those are two separate callback types which could be separately grouped just like any other callback.

[kuldeepaggarwal]

I think we can use prepend: true option in before_destroy callback as illustrated in the example....

[fxn]

@Smudge I mean, the point is that :dependent runs via a before_destroy callback. See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/builder/association.rb#L129-L136.

The contract is easy: define callbacks in the order you want them to be executed. I think that is enough.

[smudge]

@fxn Technically, yes, you are correct. My issue isn't with whether or not it is internally consistent. The behavior is predictable, once you understand what is happening. My issue is that it introduces a non-obvious (and potentially destructive) gotcha. So, at the very least, something about the developer experience could be improved.

I mean, aside from a brief mention in the docs, nothing about before_destroy indicates that it won't run before any and all destructive operations. And nothing about :dependent indicates that it is built on before_destroy. Sure, you can simply re-order your code, which means putting callbacks before association declarations. However, this is a departure from most style guides. And even then, it's not obvious at a glance why the declarations are ordered that way.

For now, I'll probably just start putting prepend: true on any before_destroy and call it a day. But it's not ideal.

[fxn]

But how can Rails know which is your desired order?

If we do it the other way around, then a different ticket is going to be created by someone expecting the current order. (That was an hypothetical experiment, obviously we cannot do that).

There's no a priori correct order, so the natural one is the one implemented.

Also, there is inheritance etc.

I believe we just have to document this better if needed.

[smudge]

I have trouble believing that the current order would be preferred _more often_ than the hypothetical order, but I do understand that removing the gotcha would require a breaking change.

The workaround, in the hypothetical experiment, would be to explicitly create a before_destroy which destroys dependents (or deletes/nullifies, etc), and make sure it gets ordered before the other before_destroy callbacks. From a grokability standpoint, this would make the behavior far more obvious than with implicit callbacks defined by the :dependent option.

Personally, I had no idea that :dependent used before_destroy internally (IMO this is just an implementation detail, not a feature), but in lieu of making a breaking change, it would make sense to me if this implementation detail were exposed in a more obvious manner. Maybe by making before_destroy: { dependent: :destroy } an alias for dependent: :destroy, so that at least it's more clear at a glance what is happening. That would also allow for the possibility of after_destroy: { dependent: :destroy }, which could be a desired ordering as well.

[matt-hwy1]

I have to agree that having before_destroy run _after_ the child objects are nullified/destroyed is unintuitive behavior. I was just bitten by this and it seems intuitive that before_destroy should run before any destructive actions occur to alter the state of the database within the transaction.

[AndyObtiva]

We got bit by this on our project as well. Expected before_destroy to happen before any kind of destruction on the active record and all its dependent destroy active records. That wasn't the case. One hour and a half of troubleshooting later, I found out the cause. I think we can save many Rails developers trouble in the future by having this _adhere to the principle of least surprise_ (aka, before_destroy happens before any kind of destruction on the main active record or dependent ones), or alternatively if that is too difficult given some API constraints, then add a before_dependent_destroy.

Thanks. I've been a happy camper with Rails and appreciate the Rails folks continually striving to meet developer needs.

[sgrif]

@AndyObtiva Please feel free to open a new issue with a reproduction script. You're unlikely to get much response on a 4 year old closed issue.

[AndyObtiva]

Thanks a lot for the response. I'll see if I can contribute a proposed Pull Request fix in the process of opening the new issue as well. Regards. -Andy

[gamov]

Just got bitten by this...

[AndyObtiva]

Just refreshed pull-request for Rails 5: https://github.com/rails/rails/pull/22520

[joshuapinter]

Just got bit by this on Rails 3.2.22.5. Yikes. This is not what you would expect the behaviour to be. Is there a desire to fix this?

[joshuapinter]

Just found two solutions:

1) Order the before_destroy callback before the association with dependent: :destroy.

2) Use prepend: true on the before_destroy callback so it gets put at the "front" of the callbacks.

See here for more details:

• https://stackoverflow.com/questions/31343453/ruby-on-rails-association-gets-deleted-before-before-destroy
• https://github.com/rails/rails/issues/3458