Merge pull request #13042 from brianstorti/fix-dependent-destroy-12812

Raise `RecordNotDestroyed` when children can't be replaced

5 files changed, 25 insertions, 1 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 1114cdb94d..bce288aefb 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Raise `ActiveRecord::RecordNotDestroyed` when a replaced child marked with `dependent: destroy` fails to be destroyed.
+
+    Fix #12812
+
+    *Brian Thomas Storti*
+
 *   Fix validation on uniqueness of empty association.
 
     *Evgeny Li*
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 0a23109b9b..72e0891702 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -108,7 +108,7 @@ module ActiveRecord
         # Deletes the records according to the <tt>:dependent</tt> option.
         def delete_records(records, method)
           if method == :destroy
-            records.each { |r| r.destroy }
+            records.each(&:destroy!)
             update_counter(-records.length) unless inverse_updates_counter_cache?
           else
             if records == :all
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index a025d49fa3..45bc974025 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1770,4 +1770,15 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [bulb1], car.bulbs
     assert_equal [bulb1, bulb2], car.all_bulbs.sort_by(&:id)
   end
+
+  test "raises RecordNotDestroyed when replaced child can't be destroyed" do
+    car = Car.create!
+    original_child = FailedBulb.create!(car: car)
+
+    assert_raise(ActiveRecord::RecordNotDestroyed) do
+      car.failed_bulbs = [FailedBulb.create!]
+    end
+
+    assert_equal [original_child], car.reload.failed_bulbs
+  end
 end
diff --git a/activerecord/test/models/bulb.rb b/activerecord/test/models/bulb.rb
index 4361188e21..831a0d5387 100644
--- a/activerecord/test/models/bulb.rb
+++ b/activerecord/test/models/bulb.rb
@@ -43,3 +43,9 @@ class FunkyBulb < Bulb
     raise "before_destroy was called"
   end
 end
+
+class FailedBulb < Bulb
+  before_destroy do
+    false
+  end
+end
diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb
index 8f3b70a7c6..c4a15a79e2 100644
--- a/activerecord/test/models/car.rb
+++ b/activerecord/test/models/car.rb
@@ -2,6 +2,7 @@ class Car < ActiveRecord::Base
   has_many :bulbs
   has_many :all_bulbs, -> { unscope where: :name }, class_name: "Bulb"
   has_many :funky_bulbs, class_name: 'FunkyBulb', dependent: :destroy
+  has_many :failed_bulbs, class_name: 'FailedBulb', dependent: :destroy
   has_many :foo_bulbs, -> { where(:name => 'foo') }, :class_name => "Bulb"
 
   has_one :bulb