#6562 HABTM join tables cleared on destroy even if a before_destroy says don't - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#6562 ✓committed

HABTM join tables cleared on destroy even if a before_destroy says don't

Reported by Murray Steele | March 11th, 2011 @ 12:37 PM | in 3.0.6

Given the following models:


class Disesase < ActiveRecord::Base
  has_and_belongs_to_many :fleas
  before_destroy :not_if_fleas_still_carry_it

  protected
  def not_if_fleas_still_carry_it
    raise ActiveRecord::Rollback unless fleas.empty?
  end
end

class Flea < ActiveRecord::Base
  has_and_belongs_to_many :diseases
end

Then if we run the following code


plague = Disease.create
plague.fleas << Flea.create
plague.fleas << Flea.create

plague.fleas.reload.empty? #=> false

plague.destroy
plague.destroyed? # => false

plague.fleas.reload.empty? #=> true :(

e.g. a before_destroy that halts destruction of a model instance doesn't halt the clearing of the habtm join table for that instance. The current implementation will only halt the clearing of the join table if the before_destroy throws an exception other than ActiveRecord::Rollback which is not the standard way of doing things.

This is mildly related to #6191 which fixed it so that the join table wasn't cleared before before_destroy callbacks were run. The attached patch moves the clearing into an after_destroy instead of redefining the destroy method.

Comments and changes to this ticket

Santiago Pastorino March 11th, 2011 @ 02:27 PM
- Assigned user set to “Aaron Patterson”
- Importance changed from “” to “Low”
You flagged this item as spam.
Murray Steele March 11th, 2011 @ 02:55 PM
I've also attached is a patch for 3-0-stable (the original patch is for master)
You flagged this item as spam.
Murray Steele March 11th, 2011 @ 02:56 PM
- no changes were found...
Aaron Patterson March 22nd, 2011 @ 10:14 PM
- State changed from “new” to “needs-more-info”
It seems this patch changes the current behavior such that the exception isn't re-raised. Can you possibly provide an implementation that doesn't change the existing test?

I'm OK to change the behavior of the existing test, but not in a bugfix release.
You flagged this item as spam.
Kane March 23rd, 2011 @ 01:00 AM
It seems this patch changes the current behavior such that the exception isn't re-raised.

isnt this the expceted behaviour if ActiveRecord::Rollback is raised?

ActiveRecord::Transactions::ClassMethods.transaction uses this exception to distinguish a deliberate rollback from other exceptional situations. Normally, raising an exception will cause the transaction method to rollback the database transaction and pass on the exception. But if you raise an ActiveRecord::Rollback exception, then the database transaction will be rolled back, without passing on the exception.

Also im concerned with the order of destruction.
Destroying the associated objects after the destroy violates foreign key constraints as stated in #4386.
We seem to have a major inconsistency between has_many and has_and_belongs_to_many. In has_many the dependant destroy is hooked as a before_callback

rails / activerecord / lib / active_record / associations / builder / has_many.rb

23 model.before_destroy dependency_method_name

and has_and_belongs_to_many runs it afterwards

rails / activerecord / lib / active_record / associations / builder / has_and_belongs_to_many.rb

23 super # super

24 #{name}.clear # posts.clear

I thing the goal should be to do the destruction of the associated objects after the before_destroy callbacks of the model and before the actual destroy, so before_destroy callbacks dont see any change and fk constraints can be honored.
You flagged this item as spam.
Murray Steele March 23rd, 2011 @ 12:32 PM
Attached reworked versions of both patches.

It seems this patch changes the current behavior such that the exception isn't re-raised. Can you possibly provide an implementation that doesn't change the existing test?

I'm OK to change the behavior of the existing test, but not in a bugfix release.

Aaron - I've redone the first commit in both patches to not change the behaviour of the Lesson object so it's before_destroy still raises a LessonError and added a (for this test only) before_destroy to Student that raises an ActiveRecord::Rollback and then tested both cases. I think that should address your concerns, although adding a one-time before_destroy callback to Student is ugly - I'm happy to change that to something better if I can without adding a new model.

I thing the goal should be to do the destruction of the associated objects after the before_destroy callbacks of the model and before the actual destroy, so before_destroy callbacks dont see any change and fk constraints can be honored.

Kane - I think this would be a worthy goal, but I don't know how to tackle it. I'll have a think about how we might ensure our before_destroy is always the last before_destroy. However, should wanting to fix foreign key constraints hold up fixing this bug? Maybe it's a separate issue, or we just need to make sure that whatever fixes #4386 also addresses what we've done here? Aaron?
- 3-0-stable_make_habtm_join_tabl.diff 4.6 KB
- make_habtm_join_table_destructi.diff 4.4 KB
You flagged this item as spam.
Murray Steele March 23rd, 2011 @ 12:34 PM
ps. I removed the old patches so we don't get confused (as they had the same names). I hope this isn't bad form.
You flagged this item as spam.
Kane March 23rd, 2011 @ 02:24 PM
You are right, this is a seperate issue or should be addressed in #4386.
Aaron Patterson March 23rd, 2011 @ 09:30 PM
- State changed from “needs-more-info” to “open”
@Murray awesome, thank you. I think the foreign key fix should be dealt with in the other ticket. :-)
Aaron Patterson March 23rd, 2011 @ 09:49 PM
- State changed from “open” to “committed”
- Milestone set to “3.0.6”
Applied and pushed. Thanks a bunch!