Delete related objects when deleting a review request.

Review Request #13735 — Created April 16, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

We never really designed Review Board to delete data, and we (at least
I) also didn't fully understand how foreign keys and deletion worked
when we first designed many aspects of our database schema. The result
of this is that when a review request was deleted, we'd leave a bunch of
related things in the database, now completely orphaned. The biggest
offender here was file attachments, which not only use up space in the
database, but can also use up a lot of space on the filesystem.

This change adds a new signal handler for pre_delete on
ReviewRequest. In this handler, we explicitly delete change
descriptions, file attachments (both active and inactive), screenshots
(both active and inactive), and the diffset history.

A future change will add a management command to clean up any existing
orphaned objects.

  • Ran unit tests.
  • Individually disabled lines in the signal handler and verified that
    without this, we were leaving these objects in the DB, and with it,
    they get deleted.
Summary ID
Delete related objects when deleting a review request.
We never really designed Review Board to delete data, and we (at least I) also didn't fully understand how foreign keys and deletion worked when we first designed many aspects of our database schema. The result of this is that when a review request was deleted, we'd leave a bunch of related things in the database, now completely orphaned. The biggest offender here was file attachments, which not only use up space in the database, but can also use up a lot of space on the filesystem. This change adds a new signal handler for `pre_delete` on `ReviewRequest`. In this handler, we explicitly delete change descriptions, file attachments (both active and inactive), screenshots (both active and inactive), and the diffset history. A future change will add a management command to clean up any existing orphaned objects. Testing Done: - Ran unit tests. - Individually disabled lines in the signal handler and verified that without this, we were leaving these objects in the DB, and with it, they get deleted.
b528d0f1f78ec9c045d9378be3dda83d7e03ef10
Description From Last Updated

You could pull out instance.file_attachments_count and instance.inactive_file_attachments_count into variables since we access them again later on.

maubinmaubin

I know it'd be a lot of additional work, but given that we're deleting content, it'd be really nice to …

chipx86chipx86
maubin
  1. 
      
  2. reviewboard/reviews/signal_handlers.py (Diff revision 1)
     
     
     
    Show all issues

    You could pull out instance.file_attachments_count and instance.inactive_file_attachments_count into variables since we access them again later on.

    1. It's just one extra dot access, and this is not a performance bottleneck since it happens extremely rarely. I'd rather keep the code simpler.

  3. 
      
maubin
  1. Ship It!
  2. 
      
chipx86
  1. Looks great!

  2. Show all issues

    I know it'd be a lot of additional work, but given that we're deleting content, it'd be really nice to use assertQueries here and in other tests, ensure we can see exactly what operations are being executed.

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (c43cb1e)