From f547dcb62794fcf081870594de9fb07a61ddbf41 Mon Sep 17 00:00:00 2001
From: Murray Steele <muz@h-lame.com>
Date: Fri, 11 Mar 2011 11:41:30 +0000
Subject: [PATCH 1/2] Failing test case to show that habtm join table contents are removed when a model is destroyed but the destruction is blocked by a before_destroy.

---
 .../test/cases/habtm_destroy_order_test.rb         |   34 ++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/activerecord/test/cases/habtm_destroy_order_test.rb b/activerecord/test/cases/habtm_destroy_order_test.rb
index 1559839..f2b91d9 100644
--- a/activerecord/test/cases/habtm_destroy_order_test.rb
+++ b/activerecord/test/cases/habtm_destroy_order_test.rb
@@ -13,5 +13,39 @@ class HabtmDestroyOrderTest < ActiveRecord::TestCase
         sicp.destroy
       end
     end
+    assert !sicp.destroyed?
+  end
+
+  test "not destroying a student with lessons leaves student<=>lesson association intact" do
+    # test a normal before_destroy doesn't destroy the habtm joins
+    begin
+      sicp = Lesson.new(:name => "SICP")
+      ben = Student.new(:name => "Ben Bitdiddle")
+      # add a before destroy to student
+      Student.class_eval do
+        before_destroy do
+          raise ActiveRecord::Rollback unless lessons.empty?
+        end
+      end
+      ben.lessons << sicp
+      ben.save!
+      ben.destroy
+      assert !ben.reload.lessons.empty?
+    ensure
+      # get rid of it so Student is still like it was
+      Student.reset_callbacks(:destroy)
+    end
+  end
+
+  test "not destroying a lesson with students leaves student<=>lesson association intact" do
+    # test a more aggressive before_destroy  doesn't destroy the habtm joins and still throws the exception
+    sicp = Lesson.new(:name => "SICP")
+    ben = Student.new(:name => "Ben Bitdiddle")
+    sicp.students << ben
+    sicp.save!
+    assert_raises LessonError do
+      sicp.destroy
+    end
+    assert !sicp.reload.students.empty?
   end
 end
-- 
1.7.3.5


From c3f155a8013c127697a617a041f0d94c551b3ac1 Mon Sep 17 00:00:00 2001
From: Murray Steele <muz@h-lame.com>
Date: Fri, 11 Mar 2011 12:02:49 +0000
Subject: [PATCH 2/2] Make clearing of HABTM join table contents happen in an after_destory callback.

The old method of redefining destroy meant that clearing the HABTM join table would happen as long as the call to destroy succeeded.  Which meant if there was a before_destroy that stopped the instance being destroyed using normal means (returning false, raising ActiveRecord::Rollback) rather than exceptional means the join table would be cleared even though the instance wasn't destroyed.  Doing it in an after_destroy hook avoids this and has the advantage of happening inside the DB transaction too.
---
 activerecord/lib/active_record/associations.rb |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 30164e3..3d36eed 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1411,16 +1411,7 @@ module ActiveRecord
         reflection = create_has_and_belongs_to_many_reflection(association_id, options, &extension)
         collection_accessor_methods(reflection, HasAndBelongsToManyAssociation)
 
-        # Don't use a before_destroy callback since users' before_destroy
-        # callbacks will be executed after the association is wiped out.
-        include Module.new {
-          class_eval <<-RUBY, __FILE__, __LINE__ + 1
-            def destroy                     # def destroy
-              super                         #   super
-              #{reflection.name}.clear      #   posts.clear
-            end                             # end
-          RUBY
-        }
+        configure_after_destroy_method_for_has_and_belongs_to_many(reflection)
 
         add_association_callbacks(reflection.name, options)
       end
@@ -1714,6 +1705,17 @@ module ActiveRecord
           end
         end
 
+        def configure_after_destroy_method_for_has_and_belongs_to_many(reflection)
+          method_name = :"has_and_belongs_to_many_after_destroy_for_#{reflection.name}"
+          class_eval <<-eoruby, __FILE__, __LINE__ + 1
+            def #{method_name}
+              association = #{reflection.name}
+              association.delete_all if association
+            end
+          eoruby
+          after_destroy method_name
+        end
+
         def delete_all_has_many_dependencies(record, reflection_name, association_class, dependent_conditions)
           association_class.delete_all(dependent_conditions)
         end
-- 
1.7.3.5