Merge pull request #41093 from gwincr11/action-job-transactions

Make destroy async transactional
rails · Feb 2, 2021 · b801005 · b801005
1 parent f5a87fe
commit b801005
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 14 deletions.
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
@@ -327,7 +327,11 @@ def skip_statement_cache?(scope)
         end
 
         def enqueue_destroy_association(options)
-          owner.class.destroy_association_async_job&.perform_later(**options)
+          job_class = owner.class.destroy_association_async_job
+
+          if job_class
+            owner._after_commit_jobs.push([job_class, options])
+          end
         end
 
         def inversable?(record)

diff --git a/activerecord/lib/active_record/associations/builder/association.rb b/activerecord/lib/active_record/associations/builder/association.rb
@@ -76,6 +76,7 @@ def self.define_callbacks(model, reflection)
       if dependent = reflection.options[:dependent]
         check_dependent_options(dependent, model)
         add_destroy_callbacks(model, reflection)
+        add_after_commit_jobs_callback(model, dependent)
       end
 
       Association.extensions.each do |extension|
@@ -132,11 +133,31 @@ def self.check_dependent_options(dependent, model)
 
     def self.add_destroy_callbacks(model, reflection)
       name = reflection.name
-      model.before_destroy lambda { |o| o.association(name).handle_dependency }
+      model.before_destroy(->(o) { o.association(name).handle_dependency })
+    end
+
+    def self.add_after_commit_jobs_callback(model, dependent)
+      if dependent == :destroy_async
+        mixin = model.generated_association_methods
+
+        unless mixin.method_defined?(:_after_commit_jobs)
+          model.after_commit(-> do
+            _after_commit_jobs.each do |job_class, job_arguments|
+              job_class.perform_later(**job_arguments)
+            end
+          end)
+
+          mixin.class_eval <<-CODE, __FILE__, __LINE__ + 1
+            def _after_commit_jobs
+              @_after_commit_jobs ||= []
+            end
+          CODE
+        end
+      end
     end
 
     private_class_method :build_scope, :macro, :valid_options, :validate_options, :define_extensions,
       :define_callbacks, :define_accessors, :define_readers, :define_writers, :define_validations,
-      :valid_dependent_options, :check_dependent_options, :add_destroy_callbacks
+      :valid_dependent_options, :check_dependent_options, :add_destroy_callbacks, :add_after_commit_jobs_callback
   end
 end
diff --git a/activerecord/test/activejob/destroy_association_async_test.rb b/activerecord/test/activejob/destroy_association_async_test.rb
@@ -20,6 +20,8 @@
 require "models/dl_keyed_has_many_through"
 
 class DestroyAssociationAsyncTest < ActiveRecord::TestCase
+  self.use_transactional_tests = false
+
   include ActiveJob::TestHelper
 
   test "destroying a record destroys the has_many :through records using a job" do
@@ -34,6 +36,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { Tag.count }, -2 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    Tag.delete_all
+    BookDestroyAsync.delete_all
   end
 
   test "destroying a scoped has_many through only deletes within the scope deleted" do
@@ -54,6 +59,10 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
       tag2.reload
     end
     assert tag.reload
+  ensure
+    Tag.delete_all
+    Tagging.delete_all
+    BookDestroyAsyncWithScopedTags.delete_all
   end
 
   test "enqueues the has_many through to be deleted with custom primary key" do
@@ -69,6 +78,10 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
    end
+  ensure
+    DlKeyedHasManyThrough.delete_all
+    DestroyAsyncParent.delete_all
+    DlKeyedJoin.delete_all
   end
 
   test "belongs to" do
@@ -81,6 +94,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { BookDestroyAsync.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    EssayDestroyAsync.delete_all
+    BookDestroyAsync.delete_all
   end
 
   test "enqueues belongs_to to be deleted with custom primary key" do
@@ -93,6 +109,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { DestroyAsyncParent.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    DlKeyedBelongsTo.delete_all
+    DestroyAsyncParent.delete_all
   end
 
   test "has_one" do
@@ -105,6 +124,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { Content.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    Content.delete_all
+    BookDestroyAsync.delete_all
   end
 
 
@@ -118,6 +140,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { DlKeyedHasOne.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    DlKeyedHasOne.delete_all
+    DestroyAsyncParent.delete_all
   end
 
 
@@ -132,6 +157,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { EssayDestroyAsync.count }, -2 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    EssayDestroyAsync.delete_all
+    BookDestroyAsync.delete_all
   end
 
   test "has_many with sti parent class destroys all children class records" do
@@ -156,24 +184,26 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { DlKeyedHasMany.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    DlKeyedHasMany.delete_all
+    DestroyAsyncParent.delete_all
   end
 
-  test "throw an error if the record is not actually deleted" do
+  test "not enqueue the job if transaction is not committed" do
     dl_keyed_has_many = DlKeyedHasMany.new
     parent = DestroyAsyncParent.create!
     parent.dl_keyed_has_many << [dl_keyed_has_many]
 
     parent.save!
-    DestroyAsyncParent.transaction do
-      parent.destroy
-      raise ActiveRecord::Rollback
-    end
-
-    assert_difference -> { DlKeyedHasMany.count }, 0 do
-      assert_raises ActiveRecord::DestroyAssociationAsyncError do
-        perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+    assert_no_enqueued_jobs do
+      DestroyAsyncParent.transaction do
+        parent.destroy
+        raise ActiveRecord::Rollback
       end
     end
+  ensure
+    DlKeyedHasMany.delete_all
+    DestroyAsyncParent.delete_all
   end
 
   test "has many ensures function for parent" do
@@ -184,8 +214,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent.save!
 
     parent.run_callbacks(:destroy)
+    parent.run_callbacks(:commit)
 
-    assert_difference -> { Tag.count }, -0 do
+    assert_no_difference -> { Tag.count } do
       assert_raises ActiveRecord::DestroyAssociationAsyncError do
         perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
       end
@@ -195,6 +226,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { Tag.count }, -2 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    Tag.delete_all
+    DestroyAsyncParentSoftDelete.delete_all
   end
 
   test "has one ensures function for parent" do
@@ -204,8 +238,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent.save!
 
     parent.run_callbacks(:destroy)
+    parent.run_callbacks(:commit)
 
-    assert_difference -> { DlKeyedHasOne.count }, -0 do
+    assert_no_difference -> { DlKeyedHasOne.count } do
       assert_raises ActiveRecord::DestroyAssociationAsyncError do
         perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
       end
@@ -215,14 +250,19 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     assert_difference -> { DlKeyedHasOne.count }, -1 do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     end
+  ensure
+    DlKeyedHasOne.delete_all
+    DestroyAsyncParentSoftDelete.delete_all
   end
 
   test "enqueues belongs_to to be deleted with ensuring function" do
     belongs = DlKeyedBelongsToSoftDelete.create!
     parent = DestroyAsyncParentSoftDelete.create!
     belongs.destroy_async_parent_soft_delete = parent
     belongs.save!
+
     belongs.run_callbacks(:destroy)
+    belongs.run_callbacks(:commit)
 
     assert_raises ActiveRecord::DestroyAssociationAsyncError do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
@@ -233,12 +273,33 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     belongs.destroy
     perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
     assert parent.reload.deleted?
+  ensure
+    DlKeyedBelongsToSoftDelete.delete_all
+    DestroyAsyncParentSoftDelete.delete_all
   end
 
   test "Don't enqueue with no relations" do
     parent = DestroyAsyncParent.create!
     parent.destroy
 
+    assert_no_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+  ensure
+    DestroyAsyncParent.delete_all
+  end
+
+  test "Rollback prevents jobs from being enqueued" do
+    tag = Tag.create!(name: "Der be treasure")
+    tag2 = Tag.create!(name: "Der be rum")
+    book = BookDestroyAsync.create!
+    book.tags << [tag, tag2]
+    book.save!
+    ActiveRecord::Base.transaction do
+      book.destroy
+      raise ActiveRecord::Rollback
+    end
     assert_no_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
   end
+ensure
+  Tag.delete_all
+  BookDestroyAsync.delete_all
 end