From 9655bf3e24c79c3dd6a744da894c5a29e71f98ca Mon Sep 17 00:00:00 2001
From: Ted Johansson <ted@discourse.org>
Date: Mon, 6 May 2024 14:08:10 +0800
Subject: [PATCH] DEV: Delete upload references on draft cleanup (#26877)

In #22851 we added a dependent strategy for deleting upload references when a draft is destroyed. This, however, didn't catch all cases, because we still have some code that issues DELETE drafts queries directly to the database. Specifically in the weekly cleanup job handled by Draft#cleanup!.

This PR fixes that by turning the raw query into an ActiveRecord #destroy_all, which will invoke the dependent strategy that ultimately deletes the upload references. It also includes a post migration to clear orphaned upload references that are already in the database.
---
 app/models/draft.rb                           | 11 +++++-----
 ..._clear_orphaned_draft_upload_references.rb | 20 +++++++++++++++++++
 spec/models/draft_spec.rb                     |  7 +++++++
 3 files changed, 32 insertions(+), 6 deletions(-)
 create mode 100644 db/post_migrate/20240506035024_clear_orphaned_draft_upload_references.rb

diff --git a/app/models/draft.rb b/app/models/draft.rb
index 1f759d4e2cf..8e97aaf512a 100644
--- a/app/models/draft.rb
+++ b/app/models/draft.rb
@@ -237,19 +237,18 @@ class Draft < ActiveRecord::Base
   end
 
   def self.cleanup!
-    DB.exec(<<~SQL)
-      DELETE FROM drafts
-       WHERE sequence < (
+    Draft.where(<<~SQL).in_batches(of: 100).destroy_all
+      sequence < (
         SELECT MAX(s.sequence)
           FROM draft_sequences s
-         WHERE s.draft_key = drafts.draft_key
-           AND s.user_id = drafts.user_id
+          WHERE s.draft_key = drafts.draft_key
+          AND s.user_id = drafts.user_id
       )
     SQL
 
     # remove old drafts
     delete_drafts_older_than_n_days = SiteSetting.delete_drafts_older_than_n_days.days.ago
-    Draft.where("updated_at < ?", delete_drafts_older_than_n_days).destroy_all
+    Draft.where("updated_at < ?", delete_drafts_older_than_n_days).in_batches(of: 100).destroy_all
 
     UserStat.update_draft_count
   end
diff --git a/db/post_migrate/20240506035024_clear_orphaned_draft_upload_references.rb b/db/post_migrate/20240506035024_clear_orphaned_draft_upload_references.rb
new file mode 100644
index 00000000000..a349aa073c0
--- /dev/null
+++ b/db/post_migrate/20240506035024_clear_orphaned_draft_upload_references.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class ClearOrphanedDraftUploadReferences < ActiveRecord::Migration[7.0]
+  def up
+    execute <<~SQL
+      DELETE
+      FROM
+        "upload_references"
+      WHERE
+        "upload_references"."target_type" = 'Draft' AND
+        "upload_references"."target_id" NOT IN (
+          SELECT "drafts"."id" FROM "drafts"
+        )
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb
index daa0a205f1c..cf7ce63e247 100644
--- a/spec/models/draft_spec.rb
+++ b/spec/models/draft_spec.rb
@@ -133,6 +133,7 @@ RSpec.describe Draft do
     key = Draft::NEW_TOPIC
 
     Draft.set(user, key, 0, "draft")
+
     Draft.cleanup!
     expect(Draft.count).to eq 1
     expect(user.user_stat.draft_count).to eq(1)
@@ -142,10 +143,16 @@ RSpec.describe Draft do
     Draft.set(user, key, seq, "draft")
     DraftSequence.update_all("sequence = sequence + 1")
 
+    draft = Draft.first
+    draft.upload_references << UploadReference.create!(target: draft, upload: Fabricate(:upload))
+
+    expect(UploadReference.count).to eq(1)
+
     Draft.cleanup!
 
     expect(Draft.count).to eq 0
     expect(user.reload.user_stat.draft_count).to eq(0)
+    expect(UploadReference.count).to eq(0)
 
     Draft.set(Fabricate(:user), Draft::NEW_TOPIC, 0, "draft")