Skip to content

refactor(importers): track reimport finding buckets by id, not instance - #15600

Draft
valentijnscholten wants to merge 3 commits into
feat/persist-new-findings-seamfrom
feat/track-finding-ids-not-instances
Draft

refactor(importers): track reimport finding buckets by id, not instance#15600
valentijnscholten wants to merge 3 commits into
feat/persist-new-findings-seamfrom
feat/track-finding-ids-not-instances

Conversation

@valentijnscholten

@valentijnscholten valentijnscholten commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

Stacked on #15599. DefaultReImporter._process_findings_internal kept every
original/new/reactivated/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. On a large reimport that
pins the full result set in memory for no reason.

  • original_items, new_items, reactivated_items, unchanged_items now
    hold ids, not instances.
  • close_old_findings() and the JIRA finding-group push are the only
    consumers that need real rows; both now requery deliberately, in bounded
    chunks, at the point they need them -- and share self.test rather than
    each row carrying its own copy of the test -> engagement -> product
    chain, the same pattern reimport matching already uses.
  • notify_scan_added() is rewritten to requery a capped, ordered slice
    (NOTIFICATION_SCAN_ADDED_MAX_FINDINGS, default 100) instead of templating
    every touched finding into a notification body.
  • dojo.finding.helper.filter_findings_by_existence (instance-based, one
    caller) is replaced by filter_finding_ids_by_existence (id-based).

A pre-existing bug this surfaced (not introduced by this PR)

close_old_findings() used to receive self.to_mitigate as the original,
never-refreshed Finding instances fetched at the very start of the run.
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 removed that finding from
to_mitigate's bucket arithmetic, so close_old_findings() picked it up
again and redundantly re-mitigated it using the stale, pre-reimport
instance -- silently overwriting fields like verified back to their
pre-reimport value, and only that redundant pass is what produced the
"closed" accounting entry and the "Mitigated by ... re-upload." note; the
inline branches themselves never wrote either.

This PR's honest re-hydrate (a fresh DB read instead of reusing the stale
instance) makes close_old_findings() correctly recognize the finding as
already mitigated and skip it -- which surfaced that skipping it also lost
the accounting entry, the note, and let the stale re-save's field-stomping
stop (a real, if obscure, improvement: a reimport's explicit verified=
override now actually sticks instead of being silently reverted).
Added self.actively_closed_matches tracking and an inline audit note so
both branches now do directly what the accidental double-processing used to
paper over. See the added commit for the full explanation, and
test_import_veracode_reimport_veracode_active_verified_mitigated's updated
assertions for what changed observably.

Test plan

  • unittests.test_reimport_batch_flush -- updated assertions for the new
    id-based buckets, passes.
  • unittests.test_importers_importer -- no new failures vs baseline
    (pre-existing 301-redirect failures in FlexibleImportTestAPI /
    FlexibleReimportTestAPI reproduce identically with this branch
    stashed out, confirmed unrelated).
  • unittests.test_import_reimport -- full file green (256 assertions
    across test_import_reimport.py + neighboring importer test files
    combined), including the veracode double-processing fix above, found
    by running this file for the first time against the complete stack
    rather than assuming "zero behavior change" held everywhere.
  • Full Pro importers/connectors suite (which exercises this OSS code via
    ProReImporter) -- 2391 passed, 119 skipped, 0 failed after fixing a
    local dev-stack env misconfiguration (DD_V3_FEATURE_LOCATIONS) that
    had produced 46 false failures unrelated to this change.
  • Pro's test_importers_performance.py assertNumQueries baselines
    updated; the refactor is a net reduction in query count on every step
    except one that initially regressed by 21 queries (an N+1 from
    freshly-hydrated close_old_findings candidates not sharing
    self.test) -- fixed at the root rather than absorbed into the pinned
    count, which now shows a net decrease vs the pre-refactor baseline.
  • OSS's own unittests.test_importers_performance re-pinned for the
    query-count shift from the veracode double-processing fix above (net
    -3 on the empty-report reimport step, from no longer redundantly
    re-processing an already-closed finding; +1-3 elsewhere from the new
    inline audit note).
  • unittests/test_tag_inheritance_perf.py's reimport-with-new-findings
    baselines were hand-recomputed while rebasing onto a dev commit that
    independently changed the same constants (two additive deltas from a
    common ancestor, combined arithmetically: EXPECTED_ZAP_REIMPORT_WITH_NEW_V3
    193 -> 194). Could not run locally -- this dev stack's Pro integration
    permanently disables watson, which this test class's fixture needs.
    Flagging for CI to confirm.

@valentijnscholten valentijnscholten added this to the 3.3.0 milestone Aug 10, 2026
@valentijnscholten
valentijnscholten force-pushed the feat/persist-new-findings-seam branch from 65d33c4 to 1c5a996 Compare August 10, 2026 18:57
@valentijnscholten
valentijnscholten force-pushed the feat/track-finding-ids-not-instances branch from 180aac2 to e5e0e94 Compare August 10, 2026 18:58
@valentijnscholten
valentijnscholten force-pushed the feat/persist-new-findings-seam branch from 1c5a996 to 5565aa2 Compare August 12, 2026 06:57
@valentijnscholten
valentijnscholten force-pushed the feat/track-finding-ids-not-instances branch from e5e0e94 to 97fcd4d Compare August 12, 2026 06:57
@valentijnscholten valentijnscholten added the affects_pro PRs that affect Pro and need a coordinated release/merge moment. label Aug 12, 2026
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.
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).
_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.
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@valentijnscholten
valentijnscholten force-pushed the feat/track-finding-ids-not-instances branch from b885147 to 148a0ca Compare August 12, 2026 20:41
@github-actions

Copy link
Copy Markdown
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects_pro PRs that affect Pro and need a coordinated release/merge moment. unittests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant