Page MenuHomePhorge

Destroy file attachments when file is deleted
AcceptedPublic

Authored by Dylsss on Sep 12 2022, 13:25.
Tags
Referenced Files
F2055672: D25051.id1391.diff
Wed, Mar 27, 23:43
Unknown Object (File)
Wed, Mar 27, 15:14
Unknown Object (File)
Mon, Mar 25, 19:35
Unknown Object (File)
Sun, Mar 24, 05:36
Unknown Object (File)
Sun, Mar 24, 01:18
Unknown Object (File)
Wed, Mar 20, 09:51
Unknown Object (File)
Feb 23 2024, 16:07
Unknown Object (File)
Feb 23 2024, 16:04

Details

Summary

Adds new file attachment deletion logic in the PhabricatorFile object from the file side and destruction engine extension from the object side and SQL patch for existing leftover attachments from deleted files.

Closes T15110

Test Plan

Upload file, delete file. No Unknown Object in Referenced Files curtain.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25051
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 83
Build 83: arc lint + arc unit

Unit TestsFailed

TimeTest
1,645 msPhabricatorLibraryTestCase::testEverythingImplemented
EXCEPTION (Exception): Source file "/opt/phabricator/src/applications/files/engineextension/PhabricatorFilesAttachmentsDestructionEngineExtension.php" failed to load. #0 /opt/arcanist/src/init/lib/PhutilBootloader.php(211): PhutilBootloader->executeInclude('/opt/phabricato...') #1 /opt/arcanist/src/symbols/PhutilSymbolLoader.php(423): PhutilBootloader->loadLibrarySource('phorge', 'applications/fi...')
945 msPhabricatorLibraryTestCase::testLibraryMap
Assertion failed, expected 'true' (at PhutilLibraryTestCase.php:51): The library map is out of date. Rebuild it with `arc liberate`. These entries differ: class.PhabricatorFileAttachmentDestructionEngineExtension, class.PhabricatorFileAttachmentsDestructionEngineExtension, xmap.PhabricatorFileAttachmentDestructionEngineExtension, xmap.PhabricatorFileAttachmentsDestructionEngineExtension.
465 msPhabricatorLibraryTestCase::testMethodVisibility
EXCEPTION (Exception): Source file "/opt/phabricator/src/applications/files/engineextension/PhabricatorFilesAttachmentsDestructionEngineExtension.php" failed to load. #0 /opt/arcanist/src/init/lib/PhutilBootloader.php(211): PhutilBootloader->executeInclude('/opt/phabricato...') #1 /opt/arcanist/src/symbols/PhutilSymbolLoader.php(423): PhutilBootloader->loadLibrarySource('phorge', 'applications/fi...')
881 msPhabricatorCelerityTestCase::testCelerityMaps
1 assertion passed.
908 msPhabricatorConduitTestCase::testConduitMethods
1 assertion passed.
View Full Test Results (3 Failed · 4 Passed)

Event Timeline

Dylsss requested review of this revision.Sep 12 2022, 13:25
This revision is now accepted and ready to land.Jan 28 2023, 10:40

Do you want an hand to land this accepted revision?

Dylsss planned changes to this revision.Mar 22 2023, 23:21

I'm very curious about this improvement.

Feel free to share a comment about your current concerns on this change. I ask this since it's indicated as "planned changes".

Organised attachment deletion from the file side into the File object and deletion from the object side into the destruction engine extension. Also updated the SQL patch date in the file name.

This revision is now accepted and ready to land.Sep 17 2023, 22:59

Commit fixup/update author details

resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)

I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run.

src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php
4

I might not have enough coffee yet, but what and when is triggering this extension?

src/applications/files/storage/PhabricatorFile.php
1713 ↗(On Diff #1390)

I don't like using mpull for the side-effect of the method invoked.

src/applications/files/storage/PhabricatorFile.php
1713 ↗(On Diff #1390)

I agree, also because it returns an array of [null, null, null, null] which is not useful to read. A simple foreach() may be better

Ekubischta added inline comments.
resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)

Agreed!

resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)

No there isn't any garbage collector for this specific issue (it's a bug originally from upstream so it's not intended for lingering file attachments to exist in the first place).

I agree it's a destructive migration, although I don't see any other way to get rid of the orphaned records (which will clean up the UI spam in the Referenced Files curtain). I've ran the query a couple times locally, and as far as I can tell, it deletes the intended records which reference a file PHID which does not exist.

Alternatively we could just change the UI logic to not show attachments that reference non-existent PHIDs, and leave the records left in the table, although I'd prefer not leaving a bunch of broken records using space/reducing efficiency.

src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php
4

PhabricatorFileAttachmentQuery seems to eagerly load the object relation, however since the DestructionEngine extensions run after the object is already deleted, this fails and nothing is returned in the query. Changed to LiskDAO loadAllWhere, which also seems to use less queries in general (due to not eagerly loading), so changed the query in PhabricatorFile as well. Changed mpull to foreach per suggestion.

resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)
resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)

I'm not sure a garbage collector would be any different to a migration, you'd still be running the same query but in just in different ways.

I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run.

You are concerned for performance reasons because it doesn't limit the results. Right?

And instead a Phorge garbage collector would delete them a few at a time, right?

In this case, I completely agree. Do we have a cute example?

I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run.

You are concerned for performance reasons because it doesn't limit the results. Right?

I'm more worried about accidentally deleting something that shouldn't be deleted, although the performance reason is also valid.

There are implementation examples - they extend PhabricatorGarbageCollector:

resources/sql/autopatches/20230917.fileattachment.01.delete.sql
1 ↗(On Diff #1390)

It's probably because I'm not comfortable enough with SQL, that I feel it would be safer to run the same query using PHP (but much slower):

foreach ($load-all-file-attachments as $attachment) {
  $have_file = $load_file_with_phid($filePHID);
  if ($have_file) {
    $attachment->delete();
  }
}

Running that periodically as a GC will also help(/hide) in any other cases where we generate bad attachments.