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

I've also attached is a patch for 3-0-stable (the original patch is for master)

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.

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?

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 are right, this is a seperate issue or should be addressed in #4386.