From 2b7c0ec92f6c5662d1fe332725b29df11721f98d Mon Sep 17 00:00:00 2001
From: Dylan F <git@dylanfarrar.com>
Date: Sat, 10 Aug 2024 22:15:10 +0200
Subject: [PATCH] Destroy file attachments when file is deleted, or object is
 deleted

Summary:
Adds file attachment deletion logics:

- PhabricatorFile: delete the attachment if file is deleted
- destruction engine extension: delete attachment if object is deleted
- SQL patch: delete existing leftover attachments from deleted files

To apply the cleanup, as usual, run:

    ./bin/storage upgrade

This cleanup may take some time, proportionally to the size of these tables:

    phabricator_file.file
    phabricator_file.file_attachment

Just as an indication: the storage upgrade, in a Phorge with `file` count 1.3M rows and `file_attachment` consisting in 9K rows, it may delete 170K rows in less than 1 second on average hardware.

Closes T15110

Test Plan:
Apply the patch, run `./bin/storage/upgrade`:

- no "Unknown Object" in any "Referenced Files" curtain of any object.

Have phd daemon running.

Upload file, attach the file to a task, delete the file from the web interface:

- no "Unknown Object" in "Referenced Files" curtain of that task.
- the query `SELECT * FROM file_attachment WHERE filePHID = '<file phid>'` returns empty result

Upload file, attach the file to a task, delete the task from the `./bin/remove destroy` workflow:

- the query `SELECT * FROM file_attachment WHERE objectPHID = '<task phid>'` returns empty result

Reviewers: O1 Blessed Committers, avivey, valerio.bozzolan

Reviewed By: O1 Blessed Committers, avivey, valerio.bozzolan

Subscribers: Ekubischta, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Tags: #files

Maniphest Tasks: T15110

Differential Revision: https://we.phorge.it/D25051
---
 .../20230917.fileattachment.01.delete.sql     |  6 +++++
 src/__phutil_library_map__.php                |  2 ++
 ...leAttachmentDestructionEngineExtension.php | 22 +++++++++++++++++++
 .../files/storage/PhabricatorFile.php         |  8 +++++++
 4 files changed, 38 insertions(+)
 create mode 100644 resources/sql/autopatches/20230917.fileattachment.01.delete.sql
 create mode 100644 src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php

diff --git a/resources/sql/autopatches/20230917.fileattachment.01.delete.sql b/resources/sql/autopatches/20230917.fileattachment.01.delete.sql
new file mode 100644
index 0000000000..74adf25f52
--- /dev/null
+++ b/resources/sql/autopatches/20230917.fileattachment.01.delete.sql
@@ -0,0 +1,6 @@
+USE {$NAMESPACE}_file;
+  DELETE FROM file_attachment
+  WHERE NOT EXISTS
+  (SELECT *
+    FROM file
+    WHERE phid=file_attachment.filePHID)
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 396bb1a308..5c7e655cd1 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3453,6 +3453,7 @@ phutil_register_library_map(array(
     'PhabricatorFileAES256StorageFormat' => 'applications/files/format/PhabricatorFileAES256StorageFormat.php',
     'PhabricatorFileAltTextTransaction' => 'applications/files/xaction/PhabricatorFileAltTextTransaction.php',
     'PhabricatorFileAttachment' => 'applications/files/storage/PhabricatorFileAttachment.php',
+    'PhabricatorFileAttachmentDestructionEngineExtension' => 'applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php',
     'PhabricatorFileAttachmentQuery' => 'applications/files/query/PhabricatorFileAttachmentQuery.php',
     'PhabricatorFileBundleLoader' => 'applications/files/query/PhabricatorFileBundleLoader.php',
     'PhabricatorFileChunk' => 'applications/files/storage/PhabricatorFileChunk.php',
@@ -9924,6 +9925,7 @@ phutil_register_library_map(array(
       'PhabricatorPolicyInterface',
       'PhabricatorExtendedPolicyInterface',
     ),
+    'PhabricatorFileAttachmentDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension',
     'PhabricatorFileAttachmentQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorFileBundleLoader' => 'Phobject',
     'PhabricatorFileChunk' => array(
diff --git a/src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php b/src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php
new file mode 100644
index 0000000000..6d62dfa601
--- /dev/null
+++ b/src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php
@@ -0,0 +1,22 @@
+<?php
+
+final class PhabricatorFileAttachmentDestructionEngineExtension
+  extends PhabricatorDestructionEngineExtension {
+
+  const EXTENSIONKEY = 'file.attachments';
+
+  public function getExtensionName() {
+    return pht('File Attachments');
+  }
+
+  public function destroyObject(
+    PhabricatorDestructionEngine $engine,
+    $object) {
+    $attachments = id(new PhabricatorFileAttachment())->loadAllWhere(
+      'objectPHID = %s',
+      $object->getPHID());
+    foreach ($attachments as $attachment) {
+      $attachment->delete();
+    }
+  }
+}
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 3918adcec2..ffa29f1919 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -1785,6 +1785,14 @@ final class PhabricatorFile extends PhabricatorFileDAO
     PhabricatorDestructionEngine $engine) {
 
     $this->openTransaction();
+
+      $attachments = id(new PhabricatorFileAttachment())->loadAllWhere(
+        'filePHID = %s',
+        $this->getPHID());
+      foreach ($attachments as $attachment) {
+        $attachment->delete();
+      }
+
       $this->delete();
     $this->saveTransaction();
   }