fix(dedupe): scope content-ordered creation to the reimport path

Drop the import-path creation sort: the customer-visible flip is a reimport
phenomenon. On plain imports the intra-report winner is chosen once (first in
file) and is pre-existing - and therefore permanently stable - for every later
import/reimport, so sorting there only changed the one-time initial winner
while breaking order-coupled fixtures (JIRA VCR cassettes replay recorded
issue interactions positionally, so changing import creation order made
test_keep_sync_push_finding_then_reimport_with_no_push request an issue a
fourth time against a cassette with three recordings).

The reimporter keeps creating findings in deduplication_ordering_key order,
and the Fortify order-independence repro now exercises the reimport path it
protects.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: Maffooch

dojo/finding/deduplication.py

@@ -546,16 +546,16 @@ def find_candidates_for_reimport_legacy(test, findings, service=None):
 
 def deduplication_ordering_key(finding):
     """
-    Stable, content-derived sort key used by the importers to decide the order
+    Stable, content-derived sort key used by the reimporter to decide the order
     findings from one report are created (and therefore get their ids) in.
 
     Deduplication itself always picks the lowest-id finding as the canonical
-    "original". Because the importers sort a report's findings by this key
-    BEFORE saving them, "lowest id" within one import equals "canonical by
-    content", so the winner among findings that collide on the deduplication
-    key is reproducible across re-scans regardless of the order the scanner
-    exports its findings in. Findings from earlier imports always have smaller
-    ids, so an already-established original never flips.
+    "original". Because the reimporter sorts a report's findings by this key
+    BEFORE saving them, "lowest id" among findings created by one reimport
+    equals "canonical by content", so the winner among findings that collide
+    on the deduplication key is reproducible across re-scans regardless of the
+    order the scanner exports its findings in. Findings from earlier imports
+    always have smaller ids, so an already-established original never flips.
 
     id is the final tiebreak and is only reached when two findings are
     identical across every content field in the key (in which case the choice
@@ -583,7 +583,7 @@ def _is_candidate_older(new_finding, candidate):
     # independently from concurrent dedupe batches (and from the `dedupe`
     # management command over pre-existing findings), so it has to be globally
     # antisymmetric — for any pair, exactly one side may see the other as
-    # "older". Content-stable winner selection is achieved by the importers
+    # "older". Content-stable winner selection is achieved by the reimporter
     # creating a report's findings in deduplication_ordering_key order instead.
     is_older = candidate.id < new_finding.id
     if not is_older:

dojo/importers/default_importer.py

@@ -7,7 +7,6 @@
 
 from dojo.celery_dispatch import dojo_dispatch_task
 from dojo.finding import helper as finding_helper
-from dojo.finding.deduplication import deduplication_ordering_key
 from dojo.importers.base_importer import BaseImporter, Parser
 from dojo.importers.base_location_manager import LocationHandler
 from dojo.importers.options import ImporterOptions
@@ -204,66 +203,54 @@ def _process_findings_internal(
         logger.debug("starting import of %i parsed findings.", len(parsed_findings) if parsed_findings else 0)
         group_names_to_findings_dict = {}
 
-        # Pre-sanitize and filter by minimum severity. Findings are also fully
-        # prepared here (fields/overrides/hash_code) so they can be sorted by a
-        # stable content key below.
+        # Pre-sanitize and filter by minimum severity
         cleaned_findings = []
         for raw_finding in parsed_findings or []:
             sanitized = self.sanitize_severity(raw_finding)
             if Finding.SEVERITIES[sanitized.severity] > Finding.SEVERITIES[self.minimum_severity]:
                 logger.debug("skipping finding due to minimum severity filter (finding=%s severity=%s min=%s)", sanitized.title, sanitized.severity, self.minimum_severity)
                 continue
+            cleaned_findings.append(sanitized)
+
+        for idx, unsaved_finding in enumerate(cleaned_findings):
+            is_final_finding = idx == len(cleaned_findings) - 1
+
             # Some parsers provide "mitigated" field but do not set timezone (because they are probably not available in the report)
             # Finding.mitigated is DateTimeField and it requires timezone
-            if sanitized.mitigated and not sanitized.mitigated.tzinfo:
-                sanitized.mitigated = sanitized.mitigated.replace(tzinfo=self.now.tzinfo)
+            if unsaved_finding.mitigated and not unsaved_finding.mitigated.tzinfo:
+                unsaved_finding.mitigated = unsaved_finding.mitigated.replace(tzinfo=self.now.tzinfo)
             # Set some explicit fields on the finding
-            sanitized.test = self.test
-            sanitized.reporter = self.user
-            sanitized.last_reviewed_by = self.user
-            sanitized.last_reviewed = self.now
-            logger.debug("process_parsed_finding: unique_id_from_tool: %s, hash_code: %s, active from report: %s, verified from report: %s", sanitized.unique_id_from_tool, sanitized.hash_code, sanitized.active, sanitized.verified)
-            # indicates an override. Otherwise, do not change the value of sanitized.active
+            unsaved_finding.test = self.test
+            unsaved_finding.reporter = self.user
+            unsaved_finding.last_reviewed_by = self.user
+            unsaved_finding.last_reviewed = self.now
+            logger.debug("process_parsed_finding: unique_id_from_tool: %s, hash_code: %s, active from report: %s, verified from report: %s", unsaved_finding.unique_id_from_tool, unsaved_finding.hash_code, unsaved_finding.active, unsaved_finding.verified)
+            # indicates an override. Otherwise, do not change the value of unsaved_finding.active
             if self.active is not None:
-                sanitized.active = self.active
+                unsaved_finding.active = self.active
             # indicates an override. Otherwise, do not change the value of verified
             if self.verified is not None:
-                sanitized.verified = self.verified
+                unsaved_finding.verified = self.verified
             # scan_date was provided, override value from parser
             if self.scan_date_override:
-                sanitized.date = self.scan_date.date()
+                unsaved_finding.date = self.scan_date.date()
             if self.service is not None:
-                sanitized.service = self.service
+                unsaved_finding.service = self.service
 
             # Parsers shouldn't use the tags field, and use unsaved_tags instead.
             # Merge any tags set by parser into unsaved_tags
-            tags_from_parser = sanitized.tags if isinstance(sanitized.tags, list) else []
-            unsaved_tags_from_parser = sanitized.unsaved_tags if isinstance(sanitized.unsaved_tags, list) else []
+            tags_from_parser = unsaved_finding.tags if isinstance(unsaved_finding.tags, list) else []
+            unsaved_tags_from_parser = unsaved_finding.unsaved_tags if isinstance(unsaved_finding.unsaved_tags, list) else []
             merged_tags = unsaved_tags_from_parser + tags_from_parser
             if merged_tags:
-                sanitized.unsaved_tags = merged_tags
-            sanitized.tags = None
-            prepared = self.process_cve(sanitized)
+                unsaved_finding.unsaved_tags = merged_tags
+            unsaved_finding.tags = None
+            finding = self.process_cve(unsaved_finding)
             # Calculate hash_code before saving based on unsaved_endpoints/unsaved_locations and unsaved_vulnerability_ids
-            prepared.set_hash_code(True)
-            cleaned_findings.append(prepared)
-
-        # Create findings in stable content-key order so their ids follow content, not
-        # the scanner's export order. Deduplication picks the lowest id among findings
-        # that collide on the dedupe key, so this makes the surviving "original" for
-        # intra-report collisions reproducible across re-scans regardless of the order
-        # the scanner emitted its findings. Unsaved findings have no id, so the id
-        # tiebreak is inert here and byte-identical findings keep their relative
-        # (immaterial) order via Python's stable sort.
-        cleaned_findings.sort(key=deduplication_ordering_key)
-
-        for idx, finding in enumerate(cleaned_findings):
-            is_final_finding = idx == len(cleaned_findings) - 1
+            finding.set_hash_code(True)
 
-            # Findings are already prepared (fields/overrides/hash_code) and globally
-            # sorted above. postprocessing will be done after processing related
-            # fields like locations, vulnerability ids, etc.
-            finding.save_no_options()
+            # postprocessing will be done after processing related fields like locations, vulnerability ids, etc.
+            unsaved_finding.save_no_options()
 
             # Determine how the finding should be grouped
             finding_will_be_grouped = self.process_finding_groups(
@@ -282,8 +269,8 @@ def _process_findings_internal(
                 findings_with_parser_tags.append((finding, [cleaned_tags]))
             # Process any files
             self.process_files(finding)
-            # Process vulnerability IDs (mutates the finding in place)
-            self.store_vulnerability_ids(finding)
+            # Process vulnerability IDs
+            finding = self.store_vulnerability_ids(finding)
             # Categorize this finding as a new one
             new_findings.append(finding)
             # all data is already saved on the finding, we only need to trigger post processing in batches

unittests/test_deduplication_logic.py

@@ -19,6 +19,7 @@
     set_duplicate,
 )
 from dojo.importers.default_importer import DefaultImporter
+from dojo.importers.default_reimporter import DefaultReImporter
 from dojo.models import (
     Development_Environment,
     Endpoint,
@@ -31,6 +32,7 @@
     Test,
     Test_Import,
     Test_Import_Finding_Action,
+    Test_Type,
     User,
     copy_model_util,
 )
@@ -654,14 +656,17 @@ def test_deduplication_ordering_key_is_order_independent(self):
 
     def _dedupe_fortify_repro_scan(self, fpr_filename, engagement_name):
         """
-        Import one corrected Fortify repro .fpr into its own (isolated) engagement
-        under a shared product via the real importer (so findings are created in
-        stable content-key order), using unique_id_from_tool deduplication, and
-        return the single surviving non-duplicate finding.
-
-        The two fixtures contain the same pair of findings - which share a
-        unique_id_from_tool but have different content (different rule/title/line) -
-        in opposite document order.
+        Reimport one corrected Fortify repro .fpr into its own (isolated)
+        engagement under a shared product via the real reimporter (so findings
+        are created in stable content-key order), using unique_id_from_tool
+        deduplication, and return the single surviving non-duplicate finding.
+
+        The customer-reported flip is a reimport phenomenon: the surviving
+        "original" among findings that collide on the dedupe key changed with
+        the scanner's export order between re-scans. The two fixtures contain
+        the same pair of findings - which share a unique_id_from_tool but have
+        different content (different rule/title/line) - in opposite document
+        order.
         """
         product_type, _ = Product_Type.objects.get_or_create(name="Fortify UID Repro PT")
         product, _ = Product.objects.get_or_create(
@@ -675,38 +680,42 @@ def _dedupe_fortify_repro_scan(self, fpr_filename, engagement_name):
             target_end=timezone.now().date(),
             deduplication_on_engagement=True,  # isolate each scan's dedupe scope
         )
+        test_type, _ = Test_Type.objects.get_or_create(name="Fortify Scan")
+        test = Test.objects.create(
+            engagement=engagement,
+            test_type=test_type,
+            scan_type="Fortify Scan",
+            target_start=timezone.now(),
+            target_end=timezone.now(),
+        )
         admin = User.objects.get(username="admin")
-        environment, _ = Development_Environment.objects.get_or_create(name="Development")
         with (get_unit_tests_scans_path("fortify") / fpr_filename).open(encoding="utf-8") as scan:
-            importer = DefaultImporter(
-                scan=scan,
-                scan_type="Fortify Scan",
-                engagement=engagement,
+            reimporter = DefaultReImporter(
+                test=test,
                 user=admin,
                 lead=admin,
-                environment=environment,
-                scan_date=timezone.now(),
-                min_severity="Info",
+                scan_date=None,
+                minimum_severity="Info",
                 active=True,
                 verified=True,
-                push_to_jira=False,
-                close_old_findings=False,
+                force_sync=True,
+                scan_type="Fortify Scan",
             )
-            test, _, _len_new, _, _, _, _ = importer.process_scan(scan)
+            reimporter.process_scan(scan)
 
         # Deduplicate explicitly so the test does not depend on the async/eager
-        # configuration of import post-processing. Dedupe is idempotent, so this
+        # configuration of reimport post-processing. Dedupe is idempotent, so this
         # is safe even when post-processing already deduplicated the batch.
         finding_ids = list(Finding.objects.filter(test=test).values_list("id", flat=True))
         dedupe_batch_of_findings(get_finding_models_for_deduplication(finding_ids))
 
         findings = list(Finding.objects.filter(test=test).order_by("id"))
-        # The importer must have created the findings in stable content-key order,
+        # The reimporter must have created the findings in stable content-key order,
         # i.e. id order equals deduplication_ordering_key order.
         self.assertEqual(
             [f.id for f in findings],
             [f.id for f in sorted(findings, key=deduplication_ordering_key)],
-            "importer did not create findings in deduplication_ordering_key order",
+            "reimporter did not create findings in deduplication_ordering_key order",
         )
         non_duplicates = [f for f in findings if not f.duplicate]
         # unique_id_from_tool dedupe collapses the shared-uid pair to one original