Merge pull request #27248 from stefanmb/master

Idempotent option for after_commit :destroy callback

4 files changed, 67 insertions, 5 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 8e2c039575..07176334e4 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Emulate db trigger behaviour for after_commit :destroy, :update
+
+    Race conditions can occur when an ActiveRecord is destroyed
+    twice or destroyed and updated. The callbacks should only be
+    triggered once, similar to a SQL database trigger.
+
+    *Stefan Budeanu*
+
 *   Moved DecimalWithoutScale, Text, and UnsignedInteger from Active Model to Active Record
 
     *Iain Beeston*
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 8e13ee3564..60d8e95b21 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -181,7 +181,11 @@ module ActiveRecord
       _raise_readonly_record_error if readonly?
       destroy_associations
       self.class.connection.add_transaction_record(self)
-      destroy_row if persisted?
+      @_trigger_destroy_callback = if persisted?
+        destroy_row > 0
+      else
+        true
+      end
       @destroyed = true
       freeze
     end
@@ -519,6 +523,7 @@ module ActiveRecord
           raise ActiveRecord::StaleObjectError.new(self, "touch")
         end
 
+        @_trigger_update_callback = result
         result
       else
         true
@@ -550,10 +555,13 @@ module ActiveRecord
     def _update_record(attribute_names = self.attribute_names)
       attributes_values = arel_attributes_with_values_for_update(attribute_names)
       if attributes_values.empty?
-        0
+        rows_affected = 0
+        @_trigger_update_callback = true
       else
-        self.class.unscoped._update_record attributes_values, id, id_in_database
+        rows_affected = self.class.unscoped._update_record attributes_values, id, id_in_database
+        @_trigger_update_callback = rows_affected > 0
       end
+      rows_affected
     end
 
     # Creates a record with values matching those of the instance attributes
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index af3fc88282..ce939c8b97 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -461,9 +461,10 @@ module ActiveRecord
           when :create
             transaction_record_state(:new_record)
           when :destroy
-            destroyed?
+            defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback
           when :update
-            !(transaction_record_state(:new_record) || destroyed?)
+            !(transaction_record_state(:new_record) || destroyed?) &&
+              (defined?(@_trigger_update_callback) && @_trigger_update_callback)
           end
         end
       end
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index dba100f5c9..391bbe8877 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -449,6 +449,51 @@ class CallbacksOnMultipleActionsTest < ActiveRecord::TestCase
   end
 end
 
+class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
+  class TopicWithHistory < ActiveRecord::Base
+    self.table_name = :topics
+
+    def self.clear_history
+      @@history = []
+    end
+
+    def self.history
+      @@history ||= []
+    end
+  end
+
+  class TopicWithCallbacksOnDestroy < TopicWithHistory
+    after_commit(on: :destroy) { |record| record.class.history << :destroy }
+  end
+
+  class TopicWithCallbacksOnUpdate < TopicWithHistory
+    after_commit(on: :update) { |record| record.class.history << :update }
+  end
+
+  def test_trigger_once_on_multiple_deletions
+    TopicWithCallbacksOnDestroy.clear_history
+    topic = TopicWithCallbacksOnDestroy.new
+    topic.save
+    topic_clone = TopicWithCallbacksOnDestroy.find(topic.id)
+    topic.destroy
+    topic_clone.destroy
+
+    assert_equal [:destroy], TopicWithCallbacksOnDestroy.history
+  end
+
+  def test_trigger_on_update_where_row_was_deleted
+    TopicWithCallbacksOnUpdate.clear_history
+    topic = TopicWithCallbacksOnUpdate.new
+    topic.save
+    topic_clone = TopicWithCallbacksOnUpdate.find(topic.id)
+    topic.destroy
+    topic_clone.author_name = "Test Author"
+    topic_clone.save
+
+    assert_equal [], TopicWithCallbacksOnUpdate.history
+  end
+end
+
 class TransactionEnrollmentCallbacksTest < ActiveRecord::TestCase
   class TopicWithoutTransactionalEnrollmentCallbacks < ActiveRecord::Base
     self.table_name = :topics