From a779b1a08bb73be2b50d42ae69b3946de98e5af4 Mon Sep 17 00:00:00 2001
From: Brian Durand <bbdurand@gmail.com>
Date: Thu, 3 May 2018 17:27:20 -0700
Subject: Fix logic on disabling commit callbacks

Commit callbacks are intentionally disabled when errors occur when calling the callback chain in order to reset the internal record state. However, the implicit order of operations on the logic for checking if callbacks are disabled is wrong. The result is that callbacks can be unexpectedly when errors occur in transactions.
---
 activerecord/CHANGELOG.md                            |  4 ++++
 activerecord/lib/active_record/transactions.rb       |  2 +-
 .../test/cases/transaction_callbacks_test.rb         | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

(limited to 'activerecord')

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 36a3d59784..dda7d19915 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Fix logic on disabling commit callbacks so they are not called unexpectedly when errors occur.
+
+    *Brian Durand*
+
 *   Ensure `Associations::CollectionAssociation#size` and `Associations::CollectionAssociation#empty?`
     use loaded association ids if present.
 
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 97cba5d1c7..be4f41050e 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -340,7 +340,7 @@ module ActiveRecord
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
     def committed!(should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && destroyed? || persisted?
+      if should_run_callbacks && (destroyed? || persisted?)
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
       end
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index e89ac53732..05941c75ac 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -367,6 +367,26 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     assert_match(/:on conditions for after_commit and after_rollback callbacks have to be one of \[:create, :destroy, :update\]/, e.message)
   end
 
+  def test_after_commit_chain_not_called_on_errors
+    record_1 = TopicWithCallbacks.create!
+    record_2 = TopicWithCallbacks.create!
+    record_3 = TopicWithCallbacks.create!
+    callbacks = []
+    record_1.after_commit_block { raise }
+    record_2.after_commit_block { callbacks << record_2.id }
+    record_3.after_commit_block { callbacks << record_3.id }
+    begin
+      TopicWithCallbacks.transaction do
+        record_1.save!
+        record_2.save!
+        record_3.save!
+      end
+    rescue
+      # From record_1.after_commit
+    end
+    assert_equal [], callbacks
+  end
+
   def test_saving_a_record_with_a_belongs_to_that_specifies_touching_the_parent_should_call_callbacks_on_the_parent_object
     pet   = Pet.first
     owner = pet.owner
-- 
cgit v1.2.3