Refactoring the code to not add methods to all models

Only models that need to use the `:destroy_async` feature
need those methods so it is better to not add to all models.

activerecord/lib/active_record/after_commit_job_enqueue.rb

@@ -1,27 +0,0 @@
-# typed: false
-# frozen_string_literal: true
-module ActiveRecord
-  module AfterCommitJobEnqueue
-    extend ActiveSupport::Concern
-
-    included do |base|
-      base.after_commit(:enqueue_waiting_jobs)
-    end
-
-    def enqueue_transactional_job(job, options)
-      waiting_jobs.push([job, options])
-    end
-
-    private
-
-    def waiting_jobs
-      @waiting_jobs ||= []
-    end
-
-    def enqueue_waiting_jobs
-      waiting_jobs.uniq.each do |job|
-        job[0]&.perform_later(**job[1])
-      end
-    end
-  end
-end

activerecord/lib/active_record/associations/association.rb

@@ -331,9 +331,11 @@ module ActiveRecord
         end
 
         def enqueue_destroy_association(options)
-          job_class = owner.class
+          job_class = owner.class.destroy_association_async_job
 
-          @owner.enqueue_transactional_job(job_class.destroy_association_async_job, options)
+          if job_class
+            owner._after_commit_jobs.push([job_class, options])
+          end
         end
 
         def inversable?(record)

activerecord/lib/active_record/associations/builder/association.rb

@@ -76,6 +76,7 @@ module ActiveRecord::Associations::Builder # :nodoc:
       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 @@ module ActiveRecord::Associations::Builder # :nodoc:
 
     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

activerecord/lib/active_record/base.rb

@@ -11,7 +11,6 @@ require "active_record/relation/delegation"
 require "active_record/attributes"
 require "active_record/type_caster"
 require "active_record/database_configurations"
-require "active_record/after_commit_job_enqueue"
 
 module ActiveRecord #:nodoc:
   # = Active Record
@@ -311,7 +310,6 @@ module ActiveRecord #:nodoc:
     include SecureToken
     include SignedId
     include Suppressor
-    include AfterCommitJobEnqueue
   end
 
   ActiveSupport.run_load_hooks(:active_record, Base)

activerecord/test/activejob/destroy_association_async_test.rb

@@ -20,6 +20,8 @@ require "models/dl_keyed_has_many"
 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
 
 
@@ -146,22 +174,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!
-    parent.run_callbacks(:destroy)
-    parent.send(:enqueue_waiting_jobs)
-
-    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
@@ -172,9 +204,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent.save!
 
     parent.run_callbacks(:destroy)
-    parent.send(:enqueue_waiting_jobs)
+    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
@@ -184,6 +216,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
@@ -193,9 +228,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent.save!
 
     parent.run_callbacks(:destroy)
-    parent.send(:enqueue_waiting_jobs)
+    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
@@ -205,6 +240,9 @@ 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
@@ -212,9 +250,9 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent = DestroyAsyncParentSoftDelete.create!
     belongs.destroy_async_parent_soft_delete = parent
     belongs.save!
-    belongs.run_callbacks(:destroy)
-    belongs.send(:enqueue_waiting_jobs)
 
+    belongs.run_callbacks(:destroy)
+    belongs.run_callbacks(:commit)
 
     assert_raises ActiveRecord::DestroyAssociationAsyncError do
       perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
@@ -225,6 +263,9 @@ 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
@@ -232,6 +273,8 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     parent.destroy
 
     assert_no_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+  ensure
+    DestroyAsyncParent.delete_all
   end
 
   test "Rollback prevents jobs from being enqueued" do
@@ -246,4 +289,7 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     end
     assert_no_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
   end
+ensure
+  Tag.delete_all
+  BookDestroyAsync.delete_all
 end

activerecord/test/schema/schema.rb

@@ -305,7 +305,7 @@ ActiveRecord::Schema.define do
 
   create_table :destroy_async_parent_soft_deletes, force: true do |t|
     t.integer :tags_count, default: 0
-    t.boolean :deleted, default: false
+    t.boolean :deleted
   end
 
   create_table :dl_keyed_belongs_tos, force: true, id: false do |t|
@@ -316,7 +316,7 @@ ActiveRecord::Schema.define do
   create_table :dl_keyed_belongs_to_soft_deletes, force: true do |t|
     t.references :destroy_async_parent_soft_delete,
       index: { name: :soft_del_parent }
-    t.boolean :deleted, default: false
+    t.boolean :deleted
   end
 
   create_table :dl_keyed_has_ones, force: true, id: false do |t|