From 9252da96597fbffe2246704556524c4804239552 Mon Sep 17 00:00:00 2001
From: Ryuta Kamizono <kamipo@gmail.com>
Date: Wed, 10 Apr 2019 17:49:42 +0900
Subject: Don't call after_commit callbacks despite a record isn't saved

Regardless of a record isn't saved (e.g. validation is failed),
`after_commit` / `after_rollback` callbacks are invoked for now.

To fix the issue, this adds a record to the current transaction only
when a record is actually saved.

Fixes #29747.
Closes #29833.
---
 activerecord/CHANGELOG.md                          |  6 +++++
 activerecord/lib/active_record/transactions.rb     | 12 +++++++++-
 activerecord/test/cases/callbacks_test.rb          |  4 ----
 .../test/cases/transaction_callbacks_test.rb       | 26 ++++++++++++++++++++++
 4 files changed, 43 insertions(+), 5 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 1cf3f3352f..3b8767787f 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Don't call commit/rollback callbacks despite a record isn't saved.
+
+    Fixes #29747.
+
+    *Ryuta Kamizono*
+
 *   Fix circular `autosave: true` causes invalid records to be saved.
 
     Prior to the fix, when there was a circular series of `autosave: true`
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index a45d228298..333f1a5435 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -376,9 +376,19 @@ module ActiveRecord
     def with_transaction_returning_status
       status = nil
       self.class.transaction do
-        add_to_transaction
+        unless has_transactional_callbacks?
+          sync_with_transaction_state
+          set_transaction_state(self.class.connection.transaction_state)
+        end
+        remember_transaction_record_state
+
         status = yield
         raise ActiveRecord::Rollback unless status
+      ensure
+        if has_transactional_callbacks? &&
+            (@_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback)
+          self.class.connection.add_transaction_record(self)
+        end
       end
       status
     end
diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb
index 4d6a112af5..b4026078f1 100644
--- a/activerecord/test/cases/callbacks_test.rb
+++ b/activerecord/test/cases/callbacks_test.rb
@@ -458,10 +458,6 @@ class CallbacksTest < ActiveRecord::TestCase
       [ :before_validation, :object ],
       [ :before_validation, :block  ],
       [ :before_validation, :throwing_abort ],
-      [ :after_rollback,    :block  ],
-      [ :after_rollback,    :object ],
-      [ :after_rollback,    :proc   ],
-      [ :after_rollback,    :method ],
     ], david.history
   end
 
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index e88d20a453..cd73f6082e 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -111,6 +111,32 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     assert_equal [:after_commit], @first.history
   end
 
+  def test_dont_call_any_callbacks_after_transaction_commits_for_invalid_record
+    @first.after_commit_block { |r| r.history << :after_commit }
+    @first.after_rollback_block { |r| r.history << :after_rollback }
+
+    def @first.valid?(*)
+      false
+    end
+
+    assert_not @first.save
+    assert_equal [], @first.history
+  end
+
+  def test_dont_call_any_callbacks_after_explicit_transaction_commits_for_invalid_record
+    @first.after_commit_block { |r| r.history << :after_commit }
+    @first.after_rollback_block { |r| r.history << :after_rollback }
+
+    def @first.valid?(*)
+      false
+    end
+
+    @first.transaction do
+      assert_not @first.save
+    end
+    assert_equal [], @first.history
+  end
+
   def test_only_call_after_commit_on_save_after_transaction_commits_for_saving_record
     record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today)
     record.after_commit_block(:save) { |r| r.history << :after_save }
-- 
cgit v1.2.3