From aac88eb999fb48468f2ac1cdaaafe6a03057269f Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Mon, 10 Aug 2026 02:07:01 +0200 Subject: [PATCH 1/5] refactor(importers): track reimport finding buckets by id, not instance DefaultReImporter kept every original/new/reactivated/unchanged Finding instance alive for the whole run just so notify_scan_added() and update_import_history() could read a few scalar fields at the end -- on a large reimport that pins the full result set in memory. new_items, reactivated_items, unchanged_items and original_items now hold ids; the two consumers that need real rows (close_old_findings, the JIRA finding-group push) requery deliberately and in bounded chunks at the point they need them, sharing self.test instead of copying the parent chain per row. notify_scan_added also gains NOTIFICATION_SCAN_ADDED_MAX_FINDINGS (default 100) so a reimport touching thousands of findings no longer templates all of them into a single notification body. --- dojo/finding/helper.py | 19 ++---- dojo/importers/base_importer.py | 95 +++++++++++++++----------- dojo/importers/default_importer.py | 15 ++-- dojo/importers/default_reimporter.py | 93 +++++++++++++++++++------ dojo/notifications/settings.py | 7 ++ unittests/test_reimport_batch_flush.py | 5 +- 6 files changed, 153 insertions(+), 81 deletions(-) diff --git a/dojo/finding/helper.py b/dojo/finding/helper.py index 6e22606e779..6d65778433a 100644 --- a/dojo/finding/helper.py +++ b/dojo/finding/helper.py @@ -184,22 +184,17 @@ def update_finding_status(new_state_finding, user, changed_fields=None): new_state_finding.last_status_update = now -def filter_findings_by_existence(findings): +def filter_finding_ids_by_existence(finding_ids): """ - Return only findings that still exist in the database (by id). + Return the subset of the given ids that still exist in the database. Centralized helper used by importers to avoid FK violations during - bulk_create. + bulk_create -- e.g. a background async_dupe_delete task removing a finding + between when an importer collected it and when it writes a history record. """ - if not findings: - return [] - candidate_ids = [finding.id for finding in findings if getattr(finding, "id", None)] - if not candidate_ids: - return [] - existing_ids = set( - Finding.objects.filter(id__in=candidate_ids).values_list("id", flat=True), - ) - return [finding for finding in findings if finding.id in existing_ids] + if not finding_ids: + return set() + return set(Finding.objects.filter(id__in=finding_ids).values_list("id", flat=True)) def can_edit_mitigated_data(user): diff --git a/dojo/importers/base_importer.py b/dojo/importers/base_importer.py index d5cb727b8ee..126fa2f59cf 100644 --- a/dojo/importers/base_importer.py +++ b/dojo/importers/base_importer.py @@ -475,10 +475,10 @@ def apply_import_tags_for_batch(self, findings: list[Finding]) -> None: def update_import_history( self, - new_findings: list[Finding] | None = None, - closed_findings: list[Finding] | None = None, - reactivated_findings: list[Finding] | None = None, - untouched_findings: list[Finding] | None = None, + new_findings: list[int] | None = None, + closed_findings: list[int] | None = None, + reactivated_findings: list[int] | None = None, + untouched_findings: list[int] | None = None, ) -> Test_Import: """Creates a record of the import or reimport operation that has occurred.""" # Quick fail check to determine if we even wanted this @@ -541,23 +541,22 @@ def update_import_history( # In longer running imports it can happen that the async_dupe_delete task removes a finding before the history record is created # We filter out these findings here to avoid FK violations (IntegrityError) - all_findings = [] + all_finding_ids = [] for list_, _ in finding_action_mappings: - all_findings.extend(list_) - existing_findings = finding_helper.filter_findings_by_existence(all_findings) if all_findings else [] - existing_ids = {f.id for f in existing_findings} + all_finding_ids.extend(list_) + existing_ids = finding_helper.filter_finding_ids_by_existence(all_finding_ids) if all_finding_ids else set() # Collect all import history records using the validated IDs import_history_records = [] - for findings, action in finding_action_mappings: + for finding_ids, action in finding_action_mappings: import_history_records.extend( Test_Import_Finding_Action( test_import=test_import, - finding_id=finding.id, + finding_id=finding_id, action=action, ) - for finding in findings - if finding.id in existing_ids + for finding_id in finding_ids + if finding_id in existing_ids ) # Bulk create all at once and let Django handle batching internally. @@ -1201,6 +1200,13 @@ def notify_scan_added( findings_reactivated=None, findings_untouched=None, ): + """ + new_findings/findings_mitigated/findings_reactivated/findings_untouched are + ids, not instances (M1) -- nothing here needs a live Finding until the + notification is actually built, and then only a capped, ordered slice of it + (a reimport that touches thousands of findings should not template all of + them into an email/webhook body). + """ if findings_untouched is None: findings_untouched = [] if findings_reactivated is None: @@ -1212,48 +1218,55 @@ def notify_scan_added( logger.debug("Scan added notifications") # When deduplication has finished (synchronous mode, or async_wait after the - # join), the in-memory findings still carry their pre-dedup duplicate=False - # flag because deduplication runs on separately-fetched instances. Refresh the - # flag from the database and split each list into "real" and duplicate findings - # so the notification reflects post-dedup reality instead of counting/listing - # deduplicated findings as brand new. In plain async mode dedup has not run yet, - # so we leave the lists untouched (best-effort, historical behavior). - findings_new_duplicate: list[Finding] = [] - findings_reactivated_duplicate: list[Finding] = [] - findings_untouched_duplicate: list[Finding] = [] + # join), the ids collected during matching still reflect pre-dedup reality + # because deduplication runs on separately-fetched instances. Split each list + # of ids into "real" and duplicate ids from a fresh query so the notification + # reflects post-dedup reality instead of counting/listing deduplicated + # findings as brand new. In plain async mode dedup has not run yet, so we + # leave the lists untouched (best-effort, historical behavior). + findings_new_duplicate_ids: list[int] = [] + findings_reactivated_duplicate_ids: list[int] = [] + findings_untouched_duplicate_ids: list[int] = [] if getattr(self, "deduplication_complete", False): - all_ids = [f.id for f in (*new_findings, *findings_reactivated, *findings_untouched)] + all_ids = [*new_findings, *findings_reactivated, *findings_untouched] duplicate_ids = set() if all_ids: duplicate_ids = set( Finding.objects.filter(id__in=all_ids, duplicate=True).values_list("id", flat=True), ) - def _split(findings): - kept, duplicates = [], [] - for finding in findings: - if finding.id in duplicate_ids: - # refresh the in-memory flag so any template logic is correct - finding.duplicate = True - duplicates.append(finding) - else: - kept.append(finding) + def _split(ids): + kept = [i for i in ids if i not in duplicate_ids] + duplicates = [i for i in ids if i in duplicate_ids] return kept, duplicates - new_findings, findings_new_duplicate = _split(new_findings) - findings_reactivated, findings_reactivated_duplicate = _split(findings_reactivated) - findings_untouched, findings_untouched_duplicate = _split(findings_untouched) + new_findings, findings_new_duplicate_ids = _split(new_findings) + findings_reactivated, findings_reactivated_duplicate_ids = _split(findings_reactivated) + findings_untouched, findings_untouched_duplicate_ids = _split(findings_untouched) # Recompute the headline count to exclude findings that turned out to be # duplicates of an existing finding (they are not genuinely new activity). updated_count = len(new_findings) + len(findings_reactivated) + len(findings_mitigated) - new_findings = sorted(new_findings, key=lambda x: x.numerical_severity) - findings_mitigated = sorted(findings_mitigated, key=lambda x: x.numerical_severity) - findings_reactivated = sorted(findings_reactivated, key=lambda x: x.numerical_severity) - findings_untouched = sorted(findings_untouched, key=lambda x: x.numerical_severity) - findings_new_duplicate = sorted(findings_new_duplicate, key=lambda x: x.numerical_severity) - findings_reactivated_duplicate = sorted(findings_reactivated_duplicate, key=lambda x: x.numerical_severity) - findings_untouched_duplicate = sorted(findings_untouched_duplicate, key=lambda x: x.numerical_severity) + max_findings = settings.NOTIFICATION_SCAN_ADDED_MAX_FINDINGS + + def _hydrate(ids): + # duplicate is re-read fresh here, so unlike the old in-memory instances + # there is no separate write-back needed to keep template logic correct. + if not ids: + return [] + return list( + Finding.objects.filter(id__in=ids) + .only("id", "title", "severity", "numerical_severity", "duplicate") + .order_by("numerical_severity")[:max_findings], + ) + + new_findings = _hydrate(new_findings) + findings_mitigated = _hydrate(findings_mitigated) + findings_reactivated = _hydrate(findings_reactivated) + findings_untouched = _hydrate(findings_untouched) + findings_new_duplicate = _hydrate(findings_new_duplicate_ids) + findings_reactivated_duplicate = _hydrate(findings_reactivated_duplicate_ids) + findings_untouched_duplicate = _hydrate(findings_untouched_duplicate_ids) title = ( f"Created/Updated {updated_count} findings for {test.engagement.product}: {test.engagement.name}: {test}" diff --git a/dojo/importers/default_importer.py b/dojo/importers/default_importer.py index 98e504cf8bd..11bbc223330 100644 --- a/dojo/importers/default_importer.py +++ b/dojo/importers/default_importer.py @@ -134,12 +134,17 @@ def process_scan( self.save_without_resurrecting(self.test) if self.engagement_target_end_updated: self.save_without_resurrecting(self.test.engagement) + # update_import_history()/notify_scan_added() take ids (M1); this path still + # builds new_findings/closed_findings as instances internally, so adapt at + # the boundary rather than reshape process_findings()/close_old_findings(). + new_finding_ids = [f.id for f in new_findings] + closed_finding_ids = [f.id for f in closed_findings] # Create a test import history object to record the flags sent to the importer # This operation will return None if the user does not have the import history # feature enabled test_import_history = self.update_import_history( - new_findings=new_findings, - closed_findings=closed_findings, + new_findings=new_finding_ids, + closed_findings=closed_finding_ids, ) # In 'async_wait' mode, block until background deduplication has finished # so notifications and statistics reflect the deduplicated state. @@ -157,12 +162,12 @@ def process_scan( url=reverse("view_test", args=(self.test.id,)), url_api=reverse("test-detail", args=(self.test.id,)), ) - updated_count = len(new_findings) + len(closed_findings) + updated_count = len(new_finding_ids) + len(closed_finding_ids) self.notify_scan_added( self.test, updated_count, - new_findings=new_findings, - findings_mitigated=closed_findings, + new_findings=new_finding_ids, + findings_mitigated=closed_finding_ids, ) # Update the test progress to reflect that the import has completed logger.debug("IMPORT_SCAN: Updating Test progress") diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index 04daaf8bcce..778e095832c 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -24,6 +24,7 @@ DEDUPLICATION_EXECUTION_MODE_ASYNC_WAIT, Development_Environment, Finding, + Finding_Group, Notes, Test, Test_Import, @@ -121,7 +122,15 @@ def process_scan( ) = self.process_findings(parsed_findings, **kwargs) # Close any old findings in the processed list if the the user specified for that # to occur in the form that is then passed to the kwargs - closed_findings = self.close_old_findings(findings_to_mitigate, **kwargs) + # + # process_findings() tracks findings_to_mitigate as ids (M1), but + # close_old_findings() calls .save() on each one and reads .finding_group -- + # hydrate real instances for just this batch, chunked, right before use. + closed_findings = self.close_old_findings( + self._hydrate_findings_for_close_old(findings_to_mitigate), + **kwargs, + ) + closed_finding_ids = [f.id for f in closed_findings] # Update the timestamps of the test object by looking at the findings imported logger.debug("REIMPORT_SCAN: Updating test/engagement timestamps") # Update the timestamps of the test object by looking at the findings imported @@ -145,7 +154,7 @@ def process_scan( # feature enabled test_import_history = self.update_import_history( new_findings=new_findings, - closed_findings=closed_findings, + closed_findings=closed_finding_ids, reactivated_findings=reactivated_findings, untouched_findings=untouched_findings, ) @@ -155,14 +164,14 @@ def process_scan( # so notifications and statistics reflect the deduplicated state. self.wait_for_post_processing() updated_count = ( - len(closed_findings) + len(reactivated_findings) + len(new_findings) + len(closed_finding_ids) + len(reactivated_findings) + len(new_findings) ) self.notify_scan_added( self.test, updated_count, new_findings=new_findings, findings_reactivated=reactivated_findings, - findings_mitigated=closed_findings, + findings_mitigated=closed_finding_ids, findings_untouched=untouched_findings, ) # Update the test progress to reflect that the import has completed @@ -173,7 +182,7 @@ def process_scan( self.test, updated_count, len(new_findings), - len(closed_findings), + len(closed_finding_ids), len(reactivated_findings), len(untouched_findings), test_import_history, @@ -310,20 +319,27 @@ def _process_findings_internal( self, parsed_findings: list[Finding], **kwargs: dict, - ) -> tuple[list[Finding], list[Finding], list[Finding], list[Finding]]: + ) -> tuple[list[int], list[int], set[int], set[int]]: self.deduplication_algorithm = self.determine_deduplication_algorithm() original_findings = self.get_original_findings() if logger.isEnabledFor(logging.DEBUG): # Guarded twice over: rendering .query raises EmptyResultSet for a none() queryset # (a legitimate get_original_findings() override), and the original_items render - # builds (id, hash) tuples for every finding already in the test — millions on a - # large test — even when DEBUG logging is off, because f-strings always evaluate. + # used to build (id, hash) tuples for every finding already in the test — millions + # on a large test — even when DEBUG logging is off, because f-strings always evaluate. with contextlib.suppress(EmptyResultSet): logger.debug(f"original_findings_qyer: {original_findings.query}") - self.original_items = list(original_findings) + # Ids, not instances (M1): a 25M-finding reimport used to keep every original, + # new, reactivated and unchanged Finding *instance* alive in memory for the whole + # run, purely so notify_scan_added() and update_import_history() could read a few + # scalar fields off them at the very end. self.to_mitigate/untouched below are + # plain id-set arithmetic either way; the only consumers that ever needed real + # rows (close_old_findings, process_groups_for_all_findings) now requery + # deliberately, in bounded chunks, at the point they need them. + self.original_items = set(original_findings.values_list("id", flat=True)) if logger.isEnabledFor(logging.DEBUG): - logger.debug(f"original_items: {[(item.id, item.hash_code) for item in self.original_items]}") + logger.debug(f"original_items: {sorted(self.original_items)}") self.new_items = [] self.reactivated_items = [] self.unchanged_items = [] @@ -582,7 +598,7 @@ def _finalize_pending_new_finding( _finalize_specific_pending_new_finding (the on-demand case: a later finding in this report just matched against this one while it was still pending). """ - self.new_items.append(finding) + self.new_items.append(finding.id) new_findings_in_batch.append(finding) finding = self.finding_post_processing( finding, @@ -731,6 +747,34 @@ def _flush_post_processing_batch( if self.deduplication_execution_mode == DEDUPLICATION_EXECUTION_MODE_ASYNC_WAIT: self.record_post_processing_result(result) + def _hydrate_findings_for_close_old(self, finding_ids: set[int]) -> list[Finding]: + """ + Fetch real Finding instances for close_old_findings() (M1). + + process_findings() tracks the to-mitigate set as ids, not instances, but + close_old_findings() mutates and .save()s each one and reads .finding_group -- + it needs real rows, not a `.only()` slice. Chunked the same way + _sync_close_old_finding_status_fields() chunks its own refresh query, so a + 25M-finding reimport never materializes more than one chunk's worth at a time. + + finding_group is a @cached_property (self.finding_group_set.all().first()), + not a real FK, so it is prefetched rather than select_related'd. + + Every id here came from get_original_findings(), scoped to self.test -- so rather + than select_related("test__...") copying the parent chain onto each row, each + finding shares self.test the same way reimport matching's candidates do (see + ProReImporter._reimport_hash_candidate_index's "share self.test" comment): one + instance for the whole run instead of one lazy-loaded copy per finding closed. + """ + if not finding_ids: + return [] + findings: list[Finding] = [] + for chunk in batched(finding_ids, _CLOSE_OLD_FINDINGS_STATUS_FIELDS_CHUNK, strict=False): + for finding in Finding.objects.filter(id__in=chunk).prefetch_related("finding_group_set"): + finding.test = self.test + findings.append(finding) + return findings + def _sync_close_old_finding_status_fields(self, findings: list[Finding]) -> list[Finding]: """ Refresh false_p, risk_accepted, and out_of_scope from the DB for each finding. @@ -994,7 +1038,7 @@ def process_matched_special_status_finding( and existing_finding.out_of_scope == unsaved_finding.out_of_scope and existing_finding.risk_accepted == unsaved_finding.risk_accepted ): - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) return existing_finding, True # If the finding is risk accepted and inactive in Defectdojo we do not sync the status from the scanner # We also need to add the finding to 'unchanged_items' as otherwise it will get mitigated by the reimporter @@ -1002,7 +1046,7 @@ def process_matched_special_status_finding( # We however do not exit the loop as we do want to update the endpoints/locations (in case some # endpoints/locations were fixed) if existing_finding.risk_accepted and not existing_finding.active: - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) return existing_finding, False # The finding was not an exact match, so we need to add more details about from the # new finding to the existing. Return False here to make process further @@ -1026,7 +1070,7 @@ def process_matched_mitigated_finding( if unsaved_finding.is_mitigated: # The new finding is already mitigated, so nothing to change on the # the existing finding - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) # Look closer at the mitigation timestamp if unsaved_finding.mitigated: logger.debug(f"item mitigated time: {unsaved_finding.mitigated.timestamp()}") @@ -1113,7 +1157,7 @@ def process_matched_mitigated_finding( note.save() self.location_handler.record_reactivations_for_finding(existing_finding) existing_finding.notes.add(note) - self.reactivated_items.append(existing_finding) + self.reactivated_items.append(existing_finding.id) # The new finding is active while the existing on is mitigated. The existing finding needs to # be updated in some way # Return False here to make sure further processing happens @@ -1173,7 +1217,7 @@ def process_matched_active_finding( existing_finding.save_no_options() else: # if finding is the same but list of affected was changed, finding is marked as unchanged. This is a known issue - self.unchanged_items.append(existing_finding) + self.unchanged_items.append(existing_finding.id) # Set the component name and version on the existing finding if it is present # on the old finding, but not present on the existing finding (do not override) component_name = getattr(unsaved_finding, "component_name", None) @@ -1396,11 +1440,18 @@ def process_groups_for_all_findings( # We dont check if the finding jira sync is applicable quite yet until we can get in the loop # but this is a way to at least make it that far if self.findings_groups_enabled and (self.push_to_jira or getattr(self.jira_instance, "finding_jira_sync", False)): - for finding_group in { - finding.finding_group - for finding in self.reactivated_items + self.unchanged_items - if finding.finding_group is not None and not finding.is_mitigated - }: + # reactivated_items/unchanged_items are ids (M1), so the group membership this + # used to read straight off in-memory instances is resolved in one query instead. + finding_group_ids = ( + Finding.objects.filter( + id__in=self.reactivated_items + self.unchanged_items, + is_mitigated=False, + finding_group__isnull=False, + ) + .values_list("finding_group_id", flat=True) + .distinct() + ) + for finding_group in Finding_Group.objects.filter(id__in=finding_group_ids): # Check the push_to_jira flag again to potentially shorty circuit without checking for existing findings if self.push_to_jira or jira_services.is_keep_in_sync(finding_group, prefetched_jira_instance=self.jira_instance): jira_services.push(finding_group) diff --git a/dojo/notifications/settings.py b/dojo/notifications/settings.py index 4e634d84b4a..da35c0a7ce8 100644 --- a/dojo/notifications/settings.py +++ b/dojo/notifications/settings.py @@ -8,6 +8,12 @@ "DD_ALERT_REFRESH": (bool, True), "DD_DISABLE_ALERT_COUNTER": (bool, False), "DD_MAX_ALERTS_PER_USER": (int, 999), + # Caps how many findings notify_scan_added() lists per category (new/mitigated/ + # reactivated/untouched, and their *_duplicate variants) in a scan_added + # notification. A report that touches thousands of findings should not template + # all of them into an email/webhook body -- the notification's finding_count + # still reflects the true total, only the listed rows are capped. + "DD_NOTIFICATION_SCAN_ADDED_MAX_FINDINGS": (int, 100), } _ENV_TO_SETTING = { @@ -20,6 +26,7 @@ "DD_ALERT_REFRESH": "ALERT_REFRESH", "DD_DISABLE_ALERT_COUNTER": "DISABLE_ALERT_COUNTER", "DD_MAX_ALERTS_PER_USER": "MAX_ALERTS_PER_USER", + "DD_NOTIFICATION_SCAN_ADDED_MAX_FINDINGS": "NOTIFICATION_SCAN_ADDED_MAX_FINDINGS", } diff --git a/unittests/test_reimport_batch_flush.py b/unittests/test_reimport_batch_flush.py index 668e9a9d7d5..bd883740d70 100644 --- a/unittests/test_reimport_batch_flush.py +++ b/unittests/test_reimport_batch_flush.py @@ -124,9 +124,10 @@ def test_final_force_continue_still_dispatches_the_batch(self): # Premise: the parity finding matched instead of being created -- the test only # exercises the force_continue tail when exactly one new finding exists. self.assertEqual(2, Finding.objects.filter(test=self.test).count()) - self.assertIn(self.existing, reimporter.unchanged_items) - self.assertEqual([fresh], new_items) + # new_items/unchanged_items are ids, not instances (M1). + self.assertIn(self.existing.id, reimporter.unchanged_items) self.assertIsNotNone(fresh.pk) + self.assertEqual([fresh.pk], new_items) # The regression: with the boundary check inside the loop, the force_continue on # the final iteration skipped it and this batch was never dispatched at all. From 3b765b3c85ce739c7879ff45c0c3f0ecb2f6bcf7 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Wed, 12 Aug 2026 08:57:02 +0200 Subject: [PATCH 2/5] fix(importers): count and audit a reimport-matched finding closed inline process_matched_active_finding()'s two inline-closing branches (report re-marks a matched, previously-active finding as mitigated, or as risk- accepted/false-p/out-of-scope) never tracked the finding into any bucket, and never left an audit note -- both were only papered over by a latent double-processing bug: since the finding was never removed from to_mitigate's arithmetic, close_old_findings() picked it up too and (redundantly) re-mitigated it, which is where the "closed" accounting and the "Mitigated by ... re-upload." note actually came from. M1's fresh, honest re-hydrate in close_old_findings() (rather than reusing a stale original_items instance) surfaces this: it correctly recognizes the finding is already mitigated and skips it, so the redundant re-save no longer happens -- and with it, the accounting and the note disappear too. The stale-instance re-save also happened to stomp the finding's `verified` field back to its pre-reimport value, masking that the reimport itself had legitimately just applied an explicit verified=False. Track these ids in a new self.actively_closed_matches list (disjoint from close_old_findings()'s own output by construction) and fold them into closed_finding_ids at the call site, and leave the same audit note inline that close_old_findings() would have left. Updates test_import_veracode_reimport_veracode_active_verified_mitigated's verified assertions to the corrected value, and re-pins the query counts in test_importers_performance.py that this fix and the seam changes shifted (net effect on the empty-report reimport step: 3 fewer queries, from no longer redundantly re-processing already-closed findings). --- dojo/importers/default_reimporter.py | 24 ++++++++- unittests/test_import_reimport.py | 11 ++-- unittests/test_importers_performance.py | 68 ++++++++++++------------- 3 files changed, 64 insertions(+), 39 deletions(-) diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index 778e095832c..4d0686f34af 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -130,7 +130,11 @@ def process_scan( self._hydrate_findings_for_close_old(findings_to_mitigate), **kwargs, ) - closed_finding_ids = [f.id for f in closed_findings] + # actively_closed_matches: findings process_matched_active_finding closed inline + # (see its docstring) rather than via close_old_findings() -- disjoint from + # closed_findings by construction, since close_old_findings() no-ops on a finding + # that is already mitigated by the time it hydrates it. + closed_finding_ids = [f.id for f in closed_findings] + self.actively_closed_matches # Update the timestamps of the test object by looking at the findings imported logger.debug("REIMPORT_SCAN: Updating test/engagement timestamps") # Update the timestamps of the test object by looking at the findings imported @@ -343,6 +347,14 @@ def _process_findings_internal( self.new_items = [] self.reactivated_items = [] self.unchanged_items = [] + # Findings process_matched_active_finding closed inline (report re-marked a + # matched, previously-active finding as mitigated/risk-accepted/false-p/out-of- + # scope) rather than via close_old_findings(). Neither reactivated nor unchanged, + # so to_mitigate's arithmetic below still includes their ids -- close_old_findings() + # correctly no-ops on them (already mitigated by the time it hydrates them), which + # means it never reports them as closed either. Tracked here so process_scan() can + # still count them for update_import_history()/notify_scan_added(). + self.actively_closed_matches: list[int] = [] self.group_names_to_findings_dict = {} # New findings queued by process_finding_that_was_not_matched, drained (persisted # via persist_new_findings, then post-processed) once per matching batch -- see @@ -1200,6 +1212,11 @@ def process_matched_active_finding( existing_finding.verified = self.verified existing_finding = self.process_cve(existing_finding) existing_finding.save_no_options() + existing_finding.notes.create( + author=self.user, + entry=f"Mitigated by {self.test.test_type} re-upload.", + ) + self.actively_closed_matches.append(existing_finding.id) elif unsaved_finding.risk_accepted or unsaved_finding.false_p or unsaved_finding.out_of_scope: logger.debug("Reimported mitigated item matches a finding that is currently open, closing.") @@ -1215,6 +1232,11 @@ def process_matched_active_finding( existing_finding.verified = self.verified existing_finding = self.process_cve(existing_finding) existing_finding.save_no_options() + existing_finding.notes.create( + author=self.user, + entry=f"Mitigated by {self.test.test_type} re-upload.", + ) + self.actively_closed_matches.append(existing_finding.id) else: # if finding is the same but list of affected was changed, finding is marked as unchanged. This is a known issue self.unchanged_items.append(existing_finding.id) diff --git a/unittests/test_import_reimport.py b/unittests/test_import_reimport.py index 417fe0ea9ea..fcab49cc7a4 100644 --- a/unittests/test_import_reimport.py +++ b/unittests/test_import_reimport.py @@ -401,12 +401,15 @@ def test_import_veracode_reimport_veracode_active_verified_mitigated(self): findings = self.get_test_findings_api(test_id) self.log_finding_summary_json_api(findings) - # reimported count must match count in veracode report - findings = self.get_test_findings_api(test_id, verified=True) + # reimport_scan_with_params() defaults verified=False, and this reimport does not + # override it, so the finding this reimport matches-and-closes inline picks up that + # explicit False (see process_matched_active_finding's `if self.verified is not + # None:`) rather than keeping the True it had from the initial import. + findings = self.get_test_findings_api(test_id, verified=False) self.assert_finding_count_json(1, findings) - # inversely, we should see no findings with verified=False - findings = self.get_test_findings_api(test_id, verified=False) + # inversely, we should see no findings with verified=True + findings = self.get_test_findings_api(test_id, verified=True) self.assert_finding_count_json(0, findings) # reimporting the exact same scan shouldn't create any notes, but there will be a new mitigated note diff --git a/unittests/test_importers_performance.py b/unittests/test_importers_performance.py index 4d8d17ff0f0..98cf6277857 100644 --- a/unittests/test_importers_performance.py +++ b/unittests/test_importers_performance.py @@ -343,13 +343,13 @@ def test_import_reimport_reimport_performance_pghistory_async(self): configure_pghistory_triggers() self._import_reimport_performance( - expected_num_queries1=160, + expected_num_queries1=161, expected_num_async_tasks1=2, - expected_num_queries2=127, + expected_num_queries2=129, expected_num_async_tasks2=1, - expected_num_queries3=27, + expected_num_queries3=30, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -367,13 +367,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._import_reimport_performance( - expected_num_queries1=179, + expected_num_queries1=180, expected_num_async_tasks1=2, - expected_num_queries2=137, + expected_num_queries2=139, expected_num_async_tasks2=1, - expected_num_queries3=37, + expected_num_queries3=40, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -392,13 +392,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async_with_product_gr self.system_settings(enable_product_grade=True) self._import_reimport_performance( - expected_num_queries1=189, + expected_num_queries1=190, expected_num_async_tasks1=5, - expected_num_queries2=147, + expected_num_queries2=149, expected_num_async_tasks2=4, - expected_num_queries3=46, + expected_num_queries3=49, expected_num_async_tasks3=3, - expected_num_queries4=112, + expected_num_queries4=109, expected_num_async_tasks4=3, ) @@ -526,9 +526,9 @@ def test_deduplication_performance_pghistory_async(self): self.system_settings(enable_deduplication=True) self._deduplication_performance( - expected_num_queries1=96, + expected_num_queries1=97, expected_num_async_tasks1=2, - expected_num_queries2=74, + expected_num_queries2=75, expected_num_async_tasks2=2, check_duplicates=False, # Async mode - deduplication happens later ) @@ -547,9 +547,9 @@ def test_deduplication_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._deduplication_performance( - expected_num_queries1=115, + expected_num_queries1=116, expected_num_async_tasks1=2, - expected_num_queries2=97, + expected_num_queries2=98, expected_num_async_tasks2=2, ) @@ -580,9 +580,9 @@ def test_deduplication_performance_pghistory_async_wait(self): # returns instantly without executing dedup on the request's DB connection. with patch("celery.result.AsyncResult.get", return_value=None): self._deduplication_performance( - expected_num_queries1=97, + expected_num_queries1=98, expected_num_async_tasks1=2, - expected_num_queries2=75, + expected_num_queries2=76, expected_num_async_tasks2=2, dedup_mode="async_wait", check_duplicates=False, @@ -670,13 +670,13 @@ def test_import_reimport_reimport_performance_pghistory_async(self): configure_pghistory_triggers() self._import_reimport_performance( - expected_num_queries1=166, + expected_num_queries1=167, expected_num_async_tasks1=2, - expected_num_queries2=135, + expected_num_queries2=137, expected_num_async_tasks2=1, - expected_num_queries3=34, + expected_num_queries3=37, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -694,13 +694,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._import_reimport_performance( - expected_num_queries1=189, + expected_num_queries1=190, expected_num_async_tasks1=2, - expected_num_queries2=149, + expected_num_queries2=151, expected_num_async_tasks2=1, - expected_num_queries3=48, + expected_num_queries3=51, expected_num_async_tasks3=1, - expected_num_queries4=103, + expected_num_queries4=100, expected_num_async_tasks4=0, ) @@ -719,13 +719,13 @@ def test_import_reimport_reimport_performance_pghistory_no_async_with_product_gr self.system_settings(enable_product_grade=True) self._import_reimport_performance( - expected_num_queries1=202, + expected_num_queries1=203, expected_num_async_tasks1=5, - expected_num_queries2=162, + expected_num_queries2=164, expected_num_async_tasks2=4, - expected_num_queries3=57, + expected_num_queries3=60, expected_num_async_tasks3=3, - expected_num_queries4=115, + expected_num_queries4=112, expected_num_async_tasks4=3, ) @@ -826,9 +826,9 @@ def test_deduplication_performance_pghistory_async(self): self.system_settings(enable_deduplication=True) self._deduplication_performance( - expected_num_queries1=102, + expected_num_queries1=103, expected_num_async_tasks1=2, - expected_num_queries2=76, + expected_num_queries2=77, expected_num_async_tasks2=2, check_duplicates=False, # Async mode - deduplication happens later ) @@ -846,8 +846,8 @@ def test_deduplication_performance_pghistory_no_async(self): testuser.usercontactinfo.save() self._deduplication_performance( - expected_num_queries1=125, + expected_num_queries1=126, expected_num_async_tasks1=2, - expected_num_queries2=103, + expected_num_queries2=104, expected_num_async_tasks2=2, ) From c9d53c53dbf574a745d67a9b31680713ad999356 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Wed, 12 Aug 2026 22:35:55 +0200 Subject: [PATCH 3/5] fix(importers): capture a new finding's id after its write, not before _finalize_pending_new_finding() read finding.id immediately after persist_new_findings() returned, on the assumption that the write had already happened. That assumption only holds for the default per-finding save; a downstream edition is explicitly allowed to override persist_new_finding() and defer the write to its own batch boundary (see that method's docstring), and for such an edition the finding is still unsaved at this point. new_items ended up holding None for every deferred new finding instead of its real id. Move the read to _flush_post_processing_batch(), after any such buffer has been flushed and every finding in new_findings_in_batch is guaranteed to have a primary key -- the same point batch_findings_to_dispatch already reads .id from, a few lines down in the same method. --- dojo/importers/default_reimporter.py | 17 +++++++++++++---- unittests/test_reimporter_persist_seam.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index 4d0686f34af..e5c9da60251 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -603,14 +603,18 @@ def _finalize_pending_new_finding( finding_will_be_grouped: bool, ) -> None: """ - Run one now-persisted new finding's post-processing and queue it for dispatch. - - `finding` must already have a primary key (persist_new_findings() has run on it). + Queue one now-prepared new finding for post-processing and dispatch. + + `finding` is NOT guaranteed to have a primary key yet: persist_new_findings() has + run on it, but a downstream edition is allowed to override persist_new_finding() + to defer the actual write to its own batch boundary (see that method's docstring). + new_items is therefore populated later, from new_findings_in_batch, once + _flush_post_processing_batch() has flushed any such buffer and every finding in + the batch is guaranteed to have a real id -- not here, while it may still be None. Shared by _drain_pending_new_findings (the normal per-matching-batch case) and _finalize_specific_pending_new_finding (the on-demand case: a later finding in this report just matched against this one while it was still pending). """ - self.new_items.append(finding.id) new_findings_in_batch.append(finding) finding = self.finding_post_processing( finding, @@ -732,6 +736,11 @@ def _flush_post_processing_batch( # their original creation; re-running it on no-change reimports # would be ~8 wasted queries per batch. apply_inherited_tags_for_findings(new_findings_in_batch) + # Read here, not when each finding was queued: a downstream edition's + # persist_new_finding() may have deferred the actual write to this flush (see + # _finalize_pending_new_finding), so this is the first point every finding in + # new_findings_in_batch is guaranteed to have a real id. + self.new_items.extend(finding.id for finding in new_findings_in_batch) new_findings_in_batch.clear() batch_findings.clear() # Partition the batch by each finding's own push_to_jira flag so one diff --git a/unittests/test_reimporter_persist_seam.py b/unittests/test_reimporter_persist_seam.py index 7ff98ee4da4..171b40dbb7b 100644 --- a/unittests/test_reimporter_persist_seam.py +++ b/unittests/test_reimporter_persist_seam.py @@ -160,6 +160,26 @@ def cve_set(test): self.assertEqual(cve_set(stock_test), cve_set(buffered_test)) + def test_new_items_holds_real_ids_even_for_a_deferred_write(self): + """ + new_items must report the finding's real id, not a premature read of it. + + process_finding_that_was_not_matched() queues each new finding; persist_new_findings() + writes the queued batch and hands the same objects back, written or not depending on + persist_new_finding(). A caller that reads finding.id right there -- rather than after + the batch's write is guaranteed to have happened -- captures None for a deferred + edition. That silently breaks any sync-wide bookkeeping keyed on new_items (for + example a chunked importer's seen-id set), which then cannot recognize the finding by + id and treats it as never having been reported. + """ + test = self._empty_test("seam-ids-buffered") + + importer = self._reimport(test, BufferingReImporter) + + actual_ids = set(Finding.objects.filter(test=test).values_list("id", flat=True)) + self.assertNotIn(None, importer.new_items, "a deferred write must not leave None in new_items") + self.assertEqual(set(importer.new_items), actual_ids) + class TestReimportMatchCandidateOrdering(DojoTestCase): From 358492a437de57a739c76f16d3d8c000df76d352 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:43:58 -0600 Subject: [PATCH 4/5] fix(importers): resolve reimport group JIRA sync by relation, not a missing id column process_groups_for_all_findings() re-derives which finding groups to push to JIRA from the reactivated/unchanged id buckets with a query. It selected values_list("finding_group_id"), but Finding has no finding_group_id column -- Finding.finding_group is a cached_property over the reverse M2M finding_group_set -- so the query raised FieldError and 500'd the reimport whenever finding groups were enabled and JIRA push/sync was on. Select the finding_group relation (the reverse query name, already used in the filter) instead. Add a regression test covering that groups+JIRA path. Co-Authored-By: Claude Opus 4.8 --- dojo/importers/default_reimporter.py | 5 +- unittests/test_reimport_group_jira_sync.py | 108 +++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 unittests/test_reimport_group_jira_sync.py diff --git a/dojo/importers/default_reimporter.py b/dojo/importers/default_reimporter.py index e5c9da60251..ecdb587eb1d 100644 --- a/dojo/importers/default_reimporter.py +++ b/dojo/importers/default_reimporter.py @@ -1473,13 +1473,16 @@ def process_groups_for_all_findings( if self.findings_groups_enabled and (self.push_to_jira or getattr(self.jira_instance, "finding_jira_sync", False)): # reactivated_items/unchanged_items are ids (M1), so the group membership this # used to read straight off in-memory instances is resolved in one query instead. + # finding_group is the reverse M2M query name (Finding has no finding_group_id + # column -- Finding.finding_group is a cached_property over finding_group_set), so + # select that relation, not a non-existent *_id field, which raises FieldError. finding_group_ids = ( Finding.objects.filter( id__in=self.reactivated_items + self.unchanged_items, is_mitigated=False, finding_group__isnull=False, ) - .values_list("finding_group_id", flat=True) + .values_list("finding_group", flat=True) .distinct() ) for finding_group in Finding_Group.objects.filter(id__in=finding_group_ids): diff --git a/unittests/test_reimport_group_jira_sync.py b/unittests/test_reimport_group_jira_sync.py new file mode 100644 index 00000000000..d006f58b154 --- /dev/null +++ b/unittests/test_reimport_group_jira_sync.py @@ -0,0 +1,108 @@ +from unittest.mock import patch + +from django.utils.timezone import now + +from dojo.importers.default_reimporter import DefaultReImporter +from dojo.models import ( + Development_Environment, + Dojo_User, + Engagement, + Finding, + Finding_Group, + Product, + Product_Type, + Test, + Test_Type, +) + +from .dojo_test_case import DojoTestCase + + +class TestReimportGroupJiraSync(DojoTestCase): + + """ + process_groups_for_all_findings() re-derives which finding groups to push to JIRA from + the reactivated/unchanged buckets. Since those buckets became plain ids, the group + membership is resolved with a query instead of off in-memory instances. + + Finding has no finding_group_id column -- Finding.finding_group is a cached_property over + the reverse M2M finding_group_set -- so selecting "finding_group_id" raises FieldError and + 500s the whole reimport, but only when finding groups are enabled AND JIRA push/sync is on, + which no other test exercises. This pins that the query runs and resolves the right group. + """ + + @classmethod + def setUpTestData(cls): + cls.user = Dojo_User.objects.create(username="reimport-group-jira-user") + cls.prod_type = Product_Type.objects.create(name="Reimport Group JIRA Type") + cls.product = Product.objects.create( + name="Reimport Group JIRA Product", prod_type=cls.prod_type, description="x", + ) + cls.engagement = Engagement.objects.create( + name="Reimport Group JIRA Engagement", + product=cls.product, + target_start=now(), + target_end=now(), + ) + cls.test_type = Test_Type.objects.create(name="Reimport Group JIRA Test Type") + cls.environment = Development_Environment.objects.create(name="Reimport Group JIRA Env") + cls.test = Test.objects.create( + engagement=cls.engagement, + test_type=cls.test_type, + environment=cls.environment, + target_start=now(), + target_end=now(), + ) + cls.finding = Finding.objects.create( + title="grouped active finding", test=cls.test, severity="High", reporter=cls.user, + ) + cls.group = Finding_Group.objects.create( + name="reimport group", test=cls.test, creator=cls.user, + ) + cls.group.findings.set([cls.finding]) + + def _reimporter(self): + importer = DefaultReImporter( + close_old_findings=False, + test=self.test, + user=self.user, + lead=self.user, + scan_date=None, + environment=self.environment, + active=True, + verified=False, + scan_type=self.test_type.name, + ) + # The buckets this method reads are ids after the by-id refactor. An unchanged, still + # active finding that belongs to a group is the shape that hits the group query. + importer.reactivated_items = [] + importer.unchanged_items = [self.finding.id] + importer.group_names_to_findings_dict = {} # skip the per-group loop; isolate the query + importer.findings_groups_enabled = True + importer.push_to_jira = True + importer.jira_instance = None + return importer + + def test_group_jira_sync_query_resolves_the_group_without_a_fielderror(self): + importer = self._reimporter() + + with patch("dojo.importers.default_reimporter.jira_services.push") as mock_push: + # Before the fix this raised FieldError: Cannot resolve keyword 'finding_group_id'. + importer.process_groups_for_all_findings() + + mock_push.assert_called_once() + pushed = mock_push.call_args.args[0] + self.assertEqual(pushed.id, self.group.id) + + def test_no_group_push_when_no_unchanged_or_reactivated_finding_is_grouped(self): + """ + A guard so the assertion above proves the query, not a blanket push: with empty + buckets the group query returns nothing and JIRA is never touched. + """ + importer = self._reimporter() + importer.unchanged_items = [] + + with patch("dojo.importers.default_reimporter.jira_services.push") as mock_push: + importer.process_groups_for_all_findings() + + mock_push.assert_not_called() From 359cdb889bf959d4927ac091b1db29bcab84477b Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:09:38 -0600 Subject: [PATCH 5/5] test(importers): align notify_scan_added callers and perf baselines with the by-id refactor Rebasing onto dev surfaced two follow-ups to the buckets-by-id change: - test_import_execution_mode.NotificationDeduplicationRefreshTest (added by #15007 for the dedup-wait feature) still called notify_scan_added() with Finding instances, but this PR made new_findings/reactivated/untouched take ids (production callers already pass ids). Pass ids so the id__in/_hydrate path resolves instead of raising "int() argument must be ... not 'Finding'". - test_tag_inheritance_perf's ZAP import/reimport baselines drift up by 1-2 queries -- the by-id re-fetch/hydrate this PR adds, the same deltas already reflected in test_importers_performance. Re-pin the six baselines. Co-Authored-By: Claude Opus 4.8 --- unittests/test_import_execution_mode.py | 6 +++--- unittests/test_tag_inheritance_perf.py | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/unittests/test_import_execution_mode.py b/unittests/test_import_execution_mode.py index b240056a2af..65f1b90b4b0 100644 --- a/unittests/test_import_execution_mode.py +++ b/unittests/test_import_execution_mode.py @@ -208,7 +208,7 @@ def test_deduplicated_new_findings_excluded_when_complete(self, mock_notify): # Simulate background deduplication having flagged the second finding. Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=2, new_findings=[real, dupe]) + importer.notify_scan_added(test, updated_count=2, new_findings=[real.id, dupe.id]) kwargs = mock_notify.call_args.kwargs self.assertEqual([f.id for f in kwargs["findings_new"]], [real.id]) @@ -226,7 +226,7 @@ def test_async_mode_does_not_refresh(self, mock_notify): dupe.save() Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=1, new_findings=[dupe]) + importer.notify_scan_added(test, updated_count=1, new_findings=[dupe.id]) kwargs = mock_notify.call_args.kwargs # historical behavior: duplicate still listed/counted as new @@ -243,7 +243,7 @@ def test_all_new_findings_duplicate_yields_empty_event(self, mock_notify): dupe.save() Finding.objects.filter(pk=dupe.pk).update(duplicate=True) - importer.notify_scan_added(test, updated_count=1, new_findings=[dupe]) + importer.notify_scan_added(test, updated_count=1, new_findings=[dupe.id]) kwargs = mock_notify.call_args.kwargs self.assertEqual(kwargs["findings_new"], []) diff --git a/unittests/test_tag_inheritance_perf.py b/unittests/test_tag_inheritance_perf.py index 69076441b62..0f4c4c5be96 100644 --- a/unittests/test_tag_inheritance_perf.py +++ b/unittests/test_tag_inheritance_perf.py @@ -641,9 +641,9 @@ def test_baseline_zap_scan_reimport_with_new_findings_v3(self): # matching loop finishes, instead of saving each one inline as soon as it fails to # match (see process_finding_that_was_not_matched and _drain_pending_new_findings). # Reimport-no-change is unaffected because it creates no new findings to defer. - EXPECTED_ZAP_IMPORT_V2 = 293 - EXPECTED_ZAP_IMPORT_V3 = 318 - EXPECTED_ZAP_REIMPORT_NO_CHANGE_V2 = 74 - EXPECTED_ZAP_REIMPORT_NO_CHANGE_V3 = 85 - EXPECTED_ZAP_REIMPORT_WITH_NEW_V2 = 157 - EXPECTED_ZAP_REIMPORT_WITH_NEW_V3 = 185 + EXPECTED_ZAP_IMPORT_V2 = 294 + EXPECTED_ZAP_IMPORT_V3 = 319 + EXPECTED_ZAP_REIMPORT_NO_CHANGE_V2 = 75 + EXPECTED_ZAP_REIMPORT_NO_CHANGE_V3 = 86 + EXPECTED_ZAP_REIMPORT_WITH_NEW_V2 = 159 + EXPECTED_ZAP_REIMPORT_WITH_NEW_V3 = 187