From d937a1175f10586b892842348c1d6ecaa47aad2e Mon Sep 17 00:00:00 2001
From: Sean Griffin <sean@thoughtbot.com>
Date: Fri, 24 Jul 2015 09:13:20 -0600
Subject: [PATCH] `destroy` shouldn't raise when child associations fail to
 save

Deep down in the association internals, we're calling `destroy!` rather
than `destroy` when handling things like `dependent` or autosave
association callbacks. Unfortunately, due to the structure of the code
(e.g. it uses callbacks for everything), it's nearly impossible to pass
whether to call `destroy` or `destroy!` down to where we actually need
it.

As such, we have to do some legwork to handle this. Since the callbacks
are what actually raise the exception, we need to rescue it in
`ActiveRecord::Callbacks`, rather than `ActiveRecord::Persistence` where
it matters. (As an aside, if this code wasn't so callback heavy, it
would handling this would likely be as simple as changing `destroy` to
call `destroy!` instead of the other way around).

Since we don't want to lose the exception when `destroy!` is called (in
particular, we don't want the value of the `record` field to change to
the parent class), we have to do some additional legwork to hold onto it
where we can use it.

Again, all of this is ugly and there is definitely a better way to do
this. However, barring a much more significant re-architecting for what
I consider to be a reletively minor improvement, I'm willing to take
this small hit to the flow of this code (begrudgingly).
---
 activerecord/CHANGELOG.md                     |  7 +++++
 activerecord/lib/active_record/callbacks.rb   |  3 ++
 activerecord/lib/active_record/persistence.rb |  9 +++++-
 .../has_many_associations_test.rb             | 30 +++++++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 8676f95d6d..99d9101347 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Don't raise an error if an association failed to destroy when `destroy` was
+    called on the parent (as opposed to `destroy!`).
+
+    Fixes #20991.
+
+    *Sean Griffin*
+
 *   ActiveRecord::RecordNotFound modified to store model name, primary_key and
     id of the caller model. It allows the catcher of this exception to make
     a better decision to what to do with it. For example consider this simple
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index 19f0dca5a6..c7c769b283 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/activerecord/lib/active_record/callbacks.rb
@@ -290,6 +290,9 @@ module ClassMethods
 
     def destroy #:nodoc:
       _run_destroy_callbacks { super }
+    rescue RecordNotDestroyed => e
+      @_association_destroy_exception = e
+      false
     end
 
     def touch(*) #:nodoc:
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 0a6e4ac0bd..09c36d7b4d 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -193,7 +193,7 @@ def destroy
     # and #destroy! raises ActiveRecord::RecordNotDestroyed.
     # See ActiveRecord::Callbacks for further details.
     def destroy!
-      destroy || raise(RecordNotDestroyed.new("Failed to destroy the record", self))
+      destroy || _raise_record_not_destroyed
     end
 
     # Returns an instance of the specified +klass+ with the attributes of the
@@ -548,5 +548,12 @@ def _create_record(attribute_names = self.attribute_names)
     def verify_readonly_attribute(name)
       raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
     end
+
+    def _raise_record_not_destroyed
+      @_association_destroy_exception ||= nil
+      raise @_association_destroy_exception || RecordNotDestroyed.new("Failed to destroy the record", self)
+    ensure
+      @_association_destroy_exception = nil
+    end
   end
 end
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index c2ef59d0f7..21e2ee3b11 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -2278,4 +2278,34 @@ def test_association_force_reload_with_only_true_is_deprecated
 
     assert_deprecated { company.clients_of_firm(true) }
   end
+
+  class AuthorWithErrorDestroyingAssociation < ActiveRecord::Base
+    self.table_name = "authors"
+    has_many :posts_with_error_destroying,
+      class_name: "PostWithErrorDestroying",
+      foreign_key: :author_id,
+      dependent: :destroy
+  end
+
+  class PostWithErrorDestroying < ActiveRecord::Base
+    self.table_name = "posts"
+    self.inheritance_column = nil
+    before_destroy -> { throw :abort }
+  end
+
+  def test_destroy_does_not_raise_when_association_errors_on_destroy
+    assert_no_difference "AuthorWithErrorDestroyingAssociation.count" do
+      author = AuthorWithErrorDestroyingAssociation.first
+
+      assert_not author.destroy
+    end
+  end
+
+  def test_destroy_with_bang_bubbles_errors_from_associations
+    error = assert_raises ActiveRecord::RecordNotDestroyed do
+      AuthorWithErrorDestroyingAssociation.first.destroy!
+    end
+
+    assert_instance_of PostWithErrorDestroying, error.record
+  end
 end
-- 
GitLab