Delete associated files when file attachments get completely deleted.

Review Request #13142 — Created July 13, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

Currently, when file attachments get completely deleted from the database we
keep their associated files lying around. There's no good reason to do this,
when the file attachment gets deleted we have no way of accessing these files
anymore.

This change deletes associated files from the filesystem when file
attachments get deleted. Any files created by the MimetypeHandler will be
deleted, and then the main file of the file attachment will be deleted.

  • Tested deleting draft .png file attachments that have thumbnail files,
    saw that they were deleted.
  • Tested deleting draft .txt files.
  • Tested deleting published files, saw that their files weren't deleted.
  • Tested deleting a draft revision of a file attachment that has
    been published before (has a history), saw that only the files for
    the draft revision were deleted.
Summary ID
Delete associated files when file attachments get completely deleted.
9807aee29beba5ed20f5297ddf9336733c665959
Description From Last Updated

no newline at end of file Column: 46 Error code: W292

reviewbotreviewbot

Can you add -> None so this will opt into type hints? You'll also need an annotations future import here …

chipx86chipx86

Since we don't use this outside of typing, you should import typing.TYPE_CHECKING and then do this import in an if …

chipx86chipx86

I think we should override FileAttachment.delete to call delete_associated_files before performing the main deletion functionality. We wouldn't need to worry …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

maubin
david
  1. Ship It!
  2. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/apps.py (Diff revision 3)
     
     
    Show all issues

    Can you add -> None so this will opt into type hints?

    You'll also need an annotations future import here and in other new files.

  3. Show all issues

    Since we don't use this outside of typing, you should import typing.TYPE_CHECKING and then do this import in an if TYPE_CHECKING: block (which is basically treated as a 4th import group).

    1. I need it when connecting the signal handler to the signal: pre_delete.connect(_on_file_attachment_deleted, sender=FileAttachment) so I'll keep the import as is.

  4. reviewboard/attachments/signal_handlers.py (Diff revision 3)
     
     
     
    Show all issues

    I think we should override FileAttachment.delete to call delete_associated_files before performing the main deletion functionality.

    We wouldn't need to worry about the order of signal handling or anything in that case, and can keep all deletion logic in one place.

    1. I was originally going to go that route, but saw in the Django docs that overridden model methods are not called on bulk operations. So this logic wouldn't run when we do a cascade delete, for example. Does this matter for us? If not then I'll go ahead and override the method, but if so then this is the only workaround that Django suggests.

    2. We do cascade delete file attachments when users and local sites are deleted, so there would be a problem there.

    3. That's a fair point. I hadn't considered that aspect.

      Okay, let's keep the signal approach.

    4. Sounds good, thanks.

  5. 
      
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (f6f2070)