refactor: scope prepare_duplicates_for_delete to full object, not per-engagement

Adds product= and product_type= parameters so the entire deletion scope
is handled in one call, avoiding unnecessary reconfiguration of findings
that are about to be deleted anyway. Uses subqueries instead of
materializing ID sets, and chunks the originals loop with prefetch to
bound memory. Reverts finding_delete to use ORM .delete() for single
finding cascade deletes.

Author: valentijnscholten

dojo/finding/helper.py

@@ -1,6 +1,7 @@
 import logging
 from contextlib import suppress
 from datetime import datetime
+from itertools import batched
 from time import strftime
 
 from django.conf import settings
@@ -563,8 +564,7 @@ def finding_delete(instance, **kwargs):
     duplicate_cluster = instance.original_finding.all()
     if duplicate_cluster:
         if settings.DUPLICATE_CLUSTER_CASCADE_DELETE:
-            # Delete the entire duplicate cluster efficiently via bulk_delete_findings
-            bulk_delete_findings(duplicate_cluster)
+            duplicate_cluster.order_by("-id").delete()
         else:
             reconfigure_duplicate_cluster(instance, duplicate_cluster)
     else:
@@ -615,53 +615,65 @@ def reconfigure_duplicate_cluster(original, cluster_outside):
         cluster_outside.exclude(id=new_original.id).update(duplicate_finding=new_original)
 
 
-def prepare_duplicates_for_delete(test=None, engagement=None):
-    logger.debug("prepare duplicates for delete, test: %s, engagement: %s", test.id if test else None, engagement.id if engagement else None)
-    if test is None and engagement is None:
-        logger.warning("nothing to prepare as test and engagement are None")
+def prepare_duplicates_for_delete(test=None, engagement=None, product=None, product_type=None):
+    logger.debug(
+        "prepare duplicates for delete, test: %s, engagement: %s, product: %s, product_type: %s",
+        test.id if test else None,
+        engagement.id if engagement else None,
+        product.id if product else None,
+        product_type.id if product_type else None,
+    )
+    if test is None and engagement is None and product is None and product_type is None:
+        logger.warning("nothing to prepare as no scope object provided")
         return
 
     # should not be needed in normal healthy instances.
     # but in that case it's a cheap count query and we might as well run it to be safe
     fix_loop_duplicates()
 
-    # Build scope filter
-    scope_filter = {}
-    if engagement:
-        scope_filter["test__engagement"] = engagement
-    if test:
-        scope_filter["test"] = test
+    # Build scope as a subquery — never materialized into Python memory
+    if product_type:
+        scope_filter = {"test__engagement__product__prod_type": product_type}
+    elif product:
+        scope_filter = {"test__engagement__product": product}
+    elif engagement:
+        scope_filter = {"test__engagement": engagement}
+    else:
+        scope_filter = {"test": test}
 
-    scope_finding_ids = set(
-        Finding.objects.filter(**scope_filter).values_list("id", flat=True),
-    )
-    if not scope_finding_ids:
+    scope_ids_subquery = Finding.objects.filter(**scope_filter).values_list("id", flat=True)
+
+    if not scope_ids_subquery.exists():
         logger.debug("no findings in scope, nothing to prepare")
         return
 
     # Bulk-reset inside-scope duplicates: single UPDATE instead of per-original mass_model_updater.
-    # Clears the duplicate_finding FK so Django's Collector won't trip over dangling references
-    # when deleting findings in this scope.
+    # Clears the duplicate_finding FK so cascade_delete won't trip over dangling self-references.
     inside_reset_count = Finding.objects.filter(
         duplicate=True,
-        duplicate_finding_id__in=scope_finding_ids,
-        id__in=scope_finding_ids,
+        duplicate_finding_id__in=scope_ids_subquery,
+        id__in=scope_ids_subquery,
     ).update(duplicate_finding=None, duplicate=False)
     logger.debug("bulk-reset %d inside-scope duplicates", inside_reset_count)
 
     # Reconfigure outside-scope duplicates: still per-original because each cluster
     # needs a new original chosen, status copied, and found_by updated.
-    # Pre-filter to only originals that have at least one duplicate outside scope,
-    # avoiding a per-original .exists() check.
-    originals_with_outside_dupes = Finding.objects.filter(
-        id__in=scope_finding_ids,
-        original_finding__in=Finding.objects.exclude(id__in=scope_finding_ids),
-    ).distinct().prefetch_related("original_finding")
-
-    for original in originals_with_outside_dupes:
-        # Inside-scope duplicates were already unlinked by the bulk UPDATE above,
-        # so original_finding.all() now only contains outside-scope duplicates.
-        reconfigure_duplicate_cluster(original, original.original_finding.all())
+    # Chunked with prefetch_related to bound memory while avoiding N+1 queries.
+    originals_ids = (
+        Finding.objects.filter(
+            id__in=scope_ids_subquery,
+            original_finding__in=Finding.objects.exclude(id__in=scope_ids_subquery),
+        )
+        .distinct()
+        .values_list("id", flat=True)
+        .iterator(chunk_size=500)
+    )
+
+    for chunk_ids in batched(originals_ids, 500):
+        for original in Finding.objects.filter(id__in=chunk_ids).prefetch_related("original_finding"):
+            # Inside-scope duplicates were already unlinked by the bulk UPDATE above,
+            # so original_finding.all() now only contains outside-scope duplicates.
+            reconfigure_duplicate_cluster(original, original.original_finding.all())
 
 
 @receiver(pre_delete, sender=Test)

dojo/utils.py

@@ -69,6 +69,7 @@
     Languages,
     Notifications,
     Product,
+    Product_Type,
     System_Settings,
     Test,
     Test_Type,
@@ -2094,11 +2095,12 @@ def async_delete_crawl_task(obj, model_list, **kwargs):
         # Step 2: Prepare duplicate clusters (must happen before any deletion)
         # When CASCADE_DELETE=True, reconfigure_duplicate_cluster skips deletion —
         # we handle that below by expanding scope to include outside duplicates.
-        if isinstance(obj, Engagement):
-            prepare_duplicates_for_delete(engagement=obj)
+        if isinstance(obj, Product_Type):
+            prepare_duplicates_for_delete(product_type=obj)
         elif isinstance(obj, Product):
-            for engagement in obj.engagement_set.all():
-                prepare_duplicates_for_delete(engagement=engagement)
+            prepare_duplicates_for_delete(product=obj)
+        elif isinstance(obj, Engagement):
+            prepare_duplicates_for_delete(engagement=obj)
         elif isinstance(obj, Test):
             prepare_duplicates_for_delete(test=obj)
 

unittests/test_prepare_duplicates_for_delete.py

@@ -224,20 +224,23 @@ def test_mixed_inside_and_outside_duplicates(self):
         self.assertIsNone(outside_dupe.duplicate_finding)
 
     @override_settings(DUPLICATE_CLUSTER_CASCADE_DELETE=True)
-    def test_cascade_delete_setting(self):
-        """When DUPLICATE_CLUSTER_CASCADE_DELETE=True, outside duplicates are deleted."""
+    def test_cascade_delete_skips_outside_reconfigure(self):
+        """When DUPLICATE_CLUSTER_CASCADE_DELETE=True, outside duplicates are left untouched.
+
+        The caller (async_delete_crawl_task) handles deletion of outside-scope
+        duplicates separately via bulk_delete_findings.
+        """
         original = self._create_finding(self.test1, "Original")
         outside_dupe = self._create_finding(self.test2, "Outside Dupe")
         self._make_duplicate(outside_dupe, original)
-        outside_dupe_id = outside_dupe.id
 
         with impersonate(self.testuser):
             prepare_duplicates_for_delete(test=self.test1)
 
-        self.assertFalse(
-            Finding.objects.filter(id=outside_dupe_id).exists(),
-            "Outside duplicate should be cascade-deleted",
-        )
+        outside_dupe.refresh_from_db()
+        # Outside dupe is still a duplicate — not reconfigured or deleted
+        self.assertTrue(outside_dupe.duplicate)
+        self.assertEqual(outside_dupe.duplicate_finding_id, original.id)
 
     def test_multiple_originals(self):
         """Multiple originals in the same test each get their clusters handled."""