refactor(importers): extract persist_new_findings as a bulk-write seam - #15599
Merged
Conversation
7 tasks
Contributor
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
valentijnscholten
force-pushed
the
feat/persist-new-findings-seam
branch
from
August 10, 2026 18:57
65d33c4 to
1c5a996
Compare
Contributor
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Contributor
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
valentijnscholten
force-pushed
the
feat/persist-new-findings-seam
branch
from
August 12, 2026 06:57
1c5a996 to
5565aa2
Compare
Contributor
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Both DefaultImporter and DefaultReImporter created new findings one at a time via save_no_options(), interleaved with the per-finding post-processing that needs a real primary key (locations, vulnerability ids, tags, request/response pairs). That leaves no seam for a downstream edition to swap in a bulk write. Extract BaseImporter.persist_new_findings() as an overridable batch write step, called once per batch with the OSS default being the same per-instance save loop as before -- zero behavior change, same SQL, same order, same batch boundaries (proven via byte-for-byte identical failures and query-count assertions against unmodified dev on test_importers_importer.py, test_importers_performance.py, test_import_reimport.py, test_reimport_batch_flush.py, and test_importers_deduplication.py). Reimport also needed match_finding_to_candidate_reimport to stop re-sorting candidates by `.id`: once a batch's new findings only get their primary key at the batch boundary rather than immediately, a same-report duplicate queued by add_new_finding_to_candidates earlier in the same matching batch has no id yet to sort by. The candidate lists are already in priority order by construction (existing candidates fetched pre-sorted, same-report ones appended afterward in processing order), so returning them as-is is both correct and avoids sorting a list that can contain a None id. The unique_id_from_tool_or_hash_code merge now de-duplicates by object identity instead of `.id` for the same reason.
… it is used as a reimport match target A same-report duplicate queued by add_new_finding_to_candidates during a matching batch has no primary key until _drain_pending_new_findings runs at the end of that batch. If a later finding in the same batch matches against it first, process_matched_finding/finding_post_processing/location_handler all require a real pk -- crashing with "Finding instance needs to have a primary key value" (reconcile_cwes), "cannot use Finding as a dict key" (location recording), or surfacing as a 500 from the reimport API. _finalize_specific_pending_new_finding() now persists and post-processes that one pending finding on demand, out of its normal per-batch drain order, the moment it is picked as a match target. Also updates the two reimport-with-new-findings query-count baselines: the _drain_pending_new_findings batching (persist once per matching batch instead of inline per finding) nets one fewer query on both the V2 and V3 paths.
valentijnscholten
force-pushed
the
feat/persist-new-findings-seam
branch
from
August 12, 2026 20:41
5565aa2 to
2b81778
Compare
match_finding_to_candidate_reimport() merged the hash-match and uid-match candidate lists into a dict and returned them in insertion order (all hash matches, then all uid matches), which is not globally id-sorted. The caller reconciles matched_findings[0] and can close the rest as stale, so under the unique_id_from_tool_or_hash_code algorithm an incoming finding that matched a lower-id existing finding by unique_id but a higher-id one by hash would silently reconcile the wrong finding and mitigate the other. Restore a deterministic order with a pk-tolerant stable sort (persisted candidates first by ascending id, then any not-yet-saved same-report duplicate last) instead of dropping ordering entirely -- the plain .id sort was removed because same-report candidates can still have pk=None. Add a behavioral test class pinning the order for every deduplication algorithm so it cannot drift again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…still without hash fields #15580 gave "Checkmarx Scan detailed" a hash_code field list, but test_apiv3_test_dedupe_policy still used it as its example of a unique_id_from_tool scanner with no hash_code_fields, so test_unique_id_scan_type_has_a_different_policy started failing on dev (hash_code_fields is now ['vuln_id_from_tool', 'file_path', 'line'], not None). Point the example at "SonarQube Scan detailed", which is still pure unique_id_from_tool with no hash fields, preserving what the test documents. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
blakeaowens
approved these changes
Aug 14, 2026
Maffooch
approved these changes
Aug 14, 2026
devGregA
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a hook in OS to allow Pro to perform extra processing on newly created / new to be created findings during import/reimport.
Description
Extracts
BaseImporter.persist_new_findings()as an overridable seam for how newfindings get written during import/reimport, without changing any observable behavior
in this repo.
Today,
DefaultImporter/DefaultReImporterwrite each new finding one at a time viasave_no_options(), interleaved with per-finding post-processing that needs a realprimary key (locations, vulnerability IDs, tags, request/response pairs). That leaves
no seam for a downstream edition to swap in a batched write strategy without copying
the whole loop — this PR adds that seam, without adding a batched writer itself.
Changes:
persist_new_findings(prepared_findings) -> list[Finding]toBaseImporter.Default implementation is the existing per-instance
save_no_options()loop, justcalled once per batch instead of inline per finding.
DefaultImporter._process_findings_internalto prepare a batch, callpersist_new_findings, then run the existing per-finding post-processing — same SQL,same order, same batch boundaries as before.
DefaultReImporter._process_findings_internalsimilarly. Reimport ismore involved because
finding_post_processing(vulnerability-ID/CWE reconciliation,file attachment) queries relations that require a primary key, so it's deferred as a
unit for new findings (matched findings are unaffected, unchanged). New findings are
persisted and post-processed once per matching batch (not the larger dedupe-batch
boundary), so a same-report duplicate is a real, queryable row before the next
matching batch's candidate query runs.
match_finding_to_candidate_reimportno longer re-sorts candidates by.id. Once anew finding only gets its primary key at the batch boundary, a same-report duplicate
queued earlier in the same matching batch has no id yet to sort by. The candidate
lists are already in priority order by construction (existing candidates fetched
pre-sorted, same-report ones appended afterward in processing order), so returning
them as-is is correct and avoids sorting a list that can contain a
Noneid. Theunique_id_from_tool_or_hash_codemerge de-duplicates by object identity instead of.idfor the same reason.as a reimport match target (
_finalize_specific_pending_new_finding()persists it ondemand instead), and updates the two reimport-with-new-findings query-count baselines
for the one-fewer-query effect of batching the drain.
Test results
No behavior change intended. Verified against unmodified
devby diffing test outputbefore/after this change on:
unittests/test_importers_importer.pyunittests/test_importers_performance.pyunittests/test_import_reimport.pyunittests/test_reimport_batch_flush.pyunittests/test_importers_deduplication.pyThe diff is byte-for-byte identical in both directions: same set of failing tests, and
where a test asserts an exact query count, the same exact number on both sides. No new
tests were added for the seam extraction itself, since it intentionally makes no
behavior change for this repo to cover — the seam is unused here. The follow-up fix
commit does add/update tests (see its own message).
Documentation
Not applicable — no user-facing behavior changes.
Checklist
dev.dev(refactor, not a bug fix).