fix: Uploaded and immediately deleted images are not removed from storage (#4562)

* fix: Uploaded and immediately deleted images are not removed from storage upon permanant delete
closes #4557

* Move attachment deletion async

server/commands/documentPermanentDeleter.test.ts

@@ -1,9 +1,12 @@
 import { subDays } from "date-fns";
 import { Attachment, Document } from "@server/models";
+import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask";
 import { buildAttachment, buildDocument } from "@server/test/factories";
 import { setupTestDatabase } from "@server/test/support";
 import documentPermanentDeleter from "./documentPermanentDeleter";
 
+jest.mock("@server/queues/tasks/DeleteAttachmentTask");
+
 setupTestDatabase();
 
 describe("documentPermanentDeleter", () => {
@@ -47,11 +50,15 @@ describe("documentPermanentDeleter", () => {
       teamId: document.teamId,
       documentId: document.id,
     });
+    await buildAttachment({
+      teamId: document.teamId,
+      documentId: document.id,
+    });
     document.text = `![text](${attachment.redirectUrl})`;
     await document.save();
     const countDeletedDoc = await documentPermanentDeleter([document]);
     expect(countDeletedDoc).toEqual(1);
-    expect(await Attachment.count()).toEqual(0);
+    expect(DeleteAttachmentTask.schedule).toHaveBeenCalledTimes(2);
     expect(
       await Document.unscoped().count({
         paranoid: false,

server/commands/documentPermanentDeleter.ts

@@ -1,7 +1,9 @@
+import { uniq } from "lodash";
 import { QueryTypes } from "sequelize";
 import { sequelize } from "@server/database/sequelize";
 import Logger from "@server/logging/Logger";
 import { Document, Attachment } from "@server/models";
+import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask";
 import parseAttachmentIds from "@server/utils/parseAttachmentIds";
 
 export default async function documentPermanentDeleter(documents: Document[]) {
@@ -22,10 +24,31 @@ export default async function documentPermanentDeleter(documents: Document[]) {
   `;
 
   for (const document of documents) {
-    const attachmentIds = parseAttachmentIds(document.text);
+    // Find any attachments that are referenced in the text content
+    const attachmentIdsInText = parseAttachmentIds(document.text);
 
-    for (const attachmentId of attachmentIds) {
-      const [{ count }] = await sequelize.query(query, {
+    // Find any attachments that were originally uploaded to this document
+    const attachmentIdsForDocument = (
+      await Attachment.findAll({
+        attributes: ["id"],
+        where: {
+          teamId: document.teamId,
+          documentId: document.id,
+        },
+      })
+    ).map((attachment) => attachment.id);
+
+    const attachmentIds = uniq([
+      ...attachmentIdsInText,
+      ...attachmentIdsForDocument,
+    ]);
+
+    await Promise.all(
+      attachmentIds.map(async (attachmentId) => {
+        // Check if the attachment is referenced in any other documents – this
+        // is needed as it's easy to copy and paste content between documents.
+        // An uploaded attachment may end up referenced in multiple documents.
+        const [{ count }] = await sequelize.query<{ count: string }>(query, {
           type: QueryTypes.SELECT,
           replacements: {
             documentId: document.id,
@@ -34,22 +57,20 @@ export default async function documentPermanentDeleter(documents: Document[]) {
           },
         });
 
+        // If the attachment is not referenced in any other documents then
+        // delete it from the database and the storage provider.
         if (parseInt(count) === 0) {
-        const attachment = await Attachment.findOne({
-          where: {
+          Logger.info(
+            "commands",
+            `Attachment ${attachmentId} scheduled for deletion`
+          );
+          await DeleteAttachmentTask.schedule({
+            attachmentId,
             teamId: document.teamId,
-            id: attachmentId,
-          },
           });
-
-        if (attachment) {
-          await attachment.destroy();
-          Logger.info("commands", `Attachment ${attachmentId} deleted`);
-        } else {
-          Logger.info("commands", `Unknown attachment ${attachmentId} ignored`);
-        }
-      }
         }
+      })
+    );
   }
 
   return Document.scope("withDrafts").destroy({

server/models/Team.ts

@@ -333,6 +333,7 @@ class Team extends ParanoidModel {
       if (attachment) {
         await DeleteAttachmentTask.schedule({
           attachmentId: attachment.id,
+          teamId: model.id,
         });
       }
     }

server/models/User.ts

@@ -604,6 +604,7 @@ class User extends ParanoidModel {
       if (attachment) {
         await DeleteAttachmentTask.schedule({
           attachmentId: attachment.id,
+          teamId: model.id,
         });
       }
     }

server/queues/tasks/DeleteAttachmentTask.ts

@@ -2,12 +2,19 @@ import { Attachment } from "@server/models";
 import BaseTask from "./BaseTask";
 
 type Props = {
+  teamId: string;
   attachmentId: string;
 };
 
 export default class DeleteAttachmentTask extends BaseTask<Props> {
-  public async perform({ attachmentId }: Props) {
-    const attachment = await Attachment.findByPk(attachmentId);
+  public async perform({ attachmentId, teamId }: Props) {
+    const attachment = await Attachment.findOne({
+      where: {
+        teamId,
+        id: attachmentId,
+      },
+    });
+
     if (!attachment) {
       return;
     }