Respect the dependent: :destroy_async when replacing the association

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Respect the `dependent: :destroy_async` when replacing the association.
+
+    *Maciej Małecki*
+
 *   Move the forcing of clear text encoding to the `ActiveRecord::Encryption::Encryptor`.
 
     Fixes #42699.

activerecord/lib/active_record/associations/has_many_association.rb

@@ -27,9 +27,7 @@ def handle_dependency
           load_target.each { |t| t.destroyed_by_association = reflection }
           destroy_all
         when :destroy_async
-          load_target.each do |t|
-            t.destroyed_by_association = reflection
-          end
+          load_target.each { |t| t.destroyed_by_association = reflection }
 
           unless target.empty?
             association_class = target.first.class
@@ -117,8 +115,25 @@ def delete_or_nullify_all_records(method)
 
         # Deletes the records according to the <tt>:dependent</tt> option.
         def delete_records(records, method)
-          if method == :destroy
+          case method
+          when :destroy
             records.each(&:destroy!)
+            update_counter(-records.length) unless reflection.inverse_updates_counter_cache?
+          when :destroy_async
+            association_class = records.first.class
+            primary_key_column = association_class.primary_key.to_sym
+            ids = records.collect { |assoc| assoc.public_send(primary_key_column) }
+
+            enqueue_destroy_association(
+              owner_model_name: owner.class.to_s,
+              owner_id: owner.id,
+              association_class: reflection.klass.to_s,
+              association_ids: ids,
+              association_primary_key_column: primary_key_column,
+              ensuring_owner_was_method: options.fetch(:ensuring_owner_was, nil),
+              ensuring_owner_destroyed: false
+            )
+
             update_counter(-records.length) unless reflection.inverse_updates_counter_cache?
           else
             scope = self.scope.where(reflection.klass.primary_key => records)

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -105,7 +105,7 @@ def build_record(attributes)
 
         def remove_records(existing_records, records, method)
           super
-          delete_through_records(records)
+          delete_through_records(records, method)
         end
 
         def target_reflection_has_associated_record?
@@ -142,13 +142,28 @@ def delete_records(records, method)
               scope.each(&:_run_destroy_callbacks)
               count = scope.delete_all
             end
+          when :destroy_async
+            count = scope.count
+            association_class = scope.klass
+            primary_key_column = association_class.primary_key.to_sym
+            ids = scope.collect { |r| r.public_send(primary_key_column) }
+
+            enqueue_destroy_association(
+              owner_model_name: owner.class.to_s,
+              owner_id: owner.id,
+              association_class: association_class.to_s,
+              association_ids: ids,
+              association_primary_key_column: primary_key_column,
+              ensuring_owner_was_method: options.fetch(:ensuring_owner_was, nil),
+              ensuring_owner_destroyed: false
+            )
           when :nullify
             count = scope.update_all(source_reflection.foreign_key => nil)
           else
             count = scope.delete_all
           end
 
-          delete_through_records(records)
+          delete_through_records(records, method)
 
           if source_reflection.options[:counter_cache] && method != :destroy
             counter = source_reflection.counter_cache_column
@@ -196,7 +211,7 @@ def through_records_for(record)
           end
         end
 
-        def delete_through_records(records)
+        def delete_through_records(records, method)
           records.each do |record|
             through_records = through_records_for(record)
 

activerecord/lib/active_record/associations/has_one_association.rb

@@ -61,7 +61,7 @@ def replace(record, save = true)
             save &&= owner.persisted?
 
             transaction_if(save) do
-              remove_target!(options[:dependent]) if target && !target.destroyed? && assigning_another_record
+              remove!(target, options[:dependent]) if target && !target.destroyed? && assigning_another_record
 
               if record
                 set_owner_attributes(record)
@@ -87,25 +87,42 @@ def set_new_record(record)
           replace(record, false)
         end
 
-        def remove_target!(method)
+        def remove!(record, method)
           case method
           when :delete
-            target.delete
+            record.delete
           when :destroy
-            target.destroyed_by_association = reflection
-            if target.persisted?
-              target.destroy
+            record.destroyed_by_association = reflection
+            if record.persisted?
+              record.destroy
+            end
+          when :destroy_async
+            record.destroyed_by_association = reflection
+
+            if record.persisted?
+              primary_key_column = record.class.primary_key.to_sym
+              id = record.public_send(primary_key_column)
+
+              enqueue_destroy_association(
+                owner_model_name: owner.class.to_s,
+                owner_id: owner.id,
+                association_class: reflection.klass.to_s,
+                association_ids: [id],
+                association_primary_key_column: primary_key_column,
+                ensuring_owner_was_method: options.fetch(:ensuring_owner_was, nil),
+                ensuring_owner_destroyed: false
+              )
             end
           else
-            nullify_owner_attributes(target)
-            remove_inverse_instance(target)
+            nullify_owner_attributes(record)
+            remove_inverse_instance(record)
 
-            if target.persisted? && owner.persisted? && !target.save
-              set_owner_attributes(target)
+            if record.persisted? && owner.persisted? && !record.save
+              set_owner_attributes(record)
               raise RecordNotSaved.new(
                 "Failed to remove the existing associated #{reflection.name}. " \
                 "The record failed to save after its foreign key was set to nil.",
-                target
+                record
               )
             end
           end

activerecord/lib/active_record/destroy_association_async_job.rb

@@ -4,23 +4,32 @@ module ActiveRecord
   class DestroyAssociationAsyncError < StandardError
   end
 
-  # Job to destroy the records associated with a destroyed record in background.
   class DestroyAssociationAsyncJob < ActiveJob::Base
     queue_as { ActiveRecord.queues[:destroy] }
 
     discard_on ActiveJob::DeserializationError
 
     def perform(
-      owner_model_name: nil, owner_id: nil,
-      association_class: nil, association_ids: nil, association_primary_key_column: nil,
-      ensuring_owner_was_method: nil
-    )
+          owner_model_name: nil,
+          owner_id: nil,
+          association_class: nil,
+          association_ids: nil,
+          association_primary_key_column: nil,
+          ensuring_owner_was_method: nil,
+          ensuring_owner_destroyed: true
+        )
       association_model = association_class.constantize
-      owner_class = owner_model_name.constantize
-      owner = owner_class.find_by(owner_class.primary_key.to_sym => owner_id)
 
-      if !owner_destroyed?(owner, ensuring_owner_was_method)
-        raise DestroyAssociationAsyncError, "owner record not destroyed"
+      # Handle the case when the `has_many` association is replaced with a new
+      # set. In such situation, respect the `dependent: :destroy_async`, and
+      # delete removed records in the background.
+      if ensuring_owner_destroyed
+        owner_class = owner_model_name.constantize
+        owner = owner_class.find_by(owner_class.primary_key.to_sym => owner_id)
+
+        if !owner_destroyed?(owner, ensuring_owner_was_method)
+          raise DestroyAssociationAsyncError, "owner record not destroyed"
+        end
       end
 
       association_model.where(association_primary_key_column => association_ids).find_each do |r|

activerecord/test/activejob/destroy_association_async_test.rb

@@ -38,10 +38,11 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     end
   ensure
     Tag.delete_all
+    Tagging.delete_all
     BookDestroyAsync.delete_all
   end
 
-  test "destroying a scoped has_many through only deletes within the scope deleted" do
+  test "destroying a scoped has_many :through only deletes within the scope deleted" do
     tag = Tag.create!(name: "Der be treasure")
     tag2 = Tag.create!(name: "Der be rum")
     parent = BookDestroyAsyncWithScopedTags.create!
@@ -65,6 +66,29 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     BookDestroyAsyncWithScopedTags.delete_all
   end
 
+  test "replacing a has_many :through deletes missing records using a job" do
+    tag = Tag.create!(name: "Der be treasure")
+    tag2 = Tag.create!(name: "Der be rum")
+    book = BookDestroyAsync.create!(tags: [tag, tag2])
+
+    tag3 = Tag.create!(name: "Der be boat")
+    book.tags = [tag, tag3]
+    book.save!
+
+    assert_difference -> { Tagging.count }, -1 do
+      perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+    end
+
+    assert tag.reload
+    assert tag2.reload
+    assert tag3.reload
+    assert book.tags == [tag, tag3]
+  ensure
+    Tag.delete_all
+    Tagging.delete_all
+    BookDestroyAsync.delete_all
+  end
+
   test "enqueues the has_many through to be deleted with custom primary key" do
     dl_keyed_has_many = DlKeyedHasManyThrough.create!
     dl_keyed_has_many2 = DlKeyedHasManyThrough.create!
@@ -129,6 +153,25 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     BookDestroyAsync.delete_all
   end
 
+  test "has_one replacement" do
+    content = Content.create!(title: "hello")
+    book = BookDestroyAsync.create!(name: "Arr, matey!", content: content)
+
+    book.content = Content.new(title: "hi")
+    book.save!
+
+    assert_difference -> { Content.count }, -1 do
+      perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+    end
+
+    assert_raises ActiveRecord::RecordNotFound do
+      content.reload
+    end
+  ensure
+    Content.delete_all
+    BookDestroyAsync.delete_all
+  end
+
 
   test "enqueues has_one to be deleted with custom primary key" do
     child = DlKeyedHasOne.create!
@@ -162,6 +205,30 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     BookDestroyAsync.delete_all
   end
 
+  test "has_many replacement" do
+    essay = EssayDestroyAsync.create!(name: "Der be treasure")
+    essay2 = EssayDestroyAsync.create!(name: "Der be rum")
+    book = BookDestroyAsync.create!(name: "Arr, matey!", essays: [essay, essay2])
+
+    essay3 = EssayDestroyAsync.create!(name: "Der be boat")
+    book.essays = [essay, essay3, EssayDestroyAsync.new(name: "Der be coat")]
+    book.save!
+
+    assert_difference -> { EssayDestroyAsync.count }, -1 do
+      perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+    end
+
+    assert_raises ActiveRecord::RecordNotFound do
+      essay2.reload
+    end
+    assert essay.reload
+    assert essay3.reload
+    assert EssayDestroyAsync.find_by!(book_id: book.id, name: "Der be coat")
+  ensure
+    EssayDestroyAsync.delete_all
+    BookDestroyAsync.delete_all
+  end
+
   test "has_many with STI parent class destroys all children class records" do
     book = BookDestroyAsync.create!
     LongEssayDestroyAsync.create!(book: book)
@@ -189,6 +256,25 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     DestroyAsyncParent.delete_all
   end
 
+  test "enqueues the has_many to be deleted with custom primary key on replacement" do
+    dl_keyed_has_many = DlKeyedHasMany.new
+    parent = DestroyAsyncParent.create!(dl_keyed_has_many: [dl_keyed_has_many])
+
+    parent.dl_keyed_has_many = []
+    parent.save!
+
+    assert_difference -> { DlKeyedHasMany.count }, -1 do
+      perform_enqueued_jobs only: ActiveRecord::DestroyAssociationAsyncJob
+    end
+
+    assert_raises ActiveRecord::RecordNotFound do
+      dl_keyed_has_many.reload
+    end
+  ensure
+    DlKeyedHasMany.delete_all
+    DestroyAsyncParent.delete_all
+  end
+
   test "not enqueue the job if transaction is not committed" do
     dl_keyed_has_many = DlKeyedHasMany.new
     parent = DestroyAsyncParent.create!
@@ -228,6 +314,7 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
     end
   ensure
     Tag.delete_all
+    Tagging.delete_all
     DestroyAsyncParentSoftDelete.delete_all
   end
 
@@ -301,5 +388,6 @@ class DestroyAssociationAsyncTest < ActiveRecord::TestCase
   end
 ensure
   Tag.delete_all
+  Tagging.delete_all
   BookDestroyAsync.delete_all
 end