Skip to content

refactor(dedupe): one definition of the location prefetch - #15520

Merged
Maffooch merged 2 commits into
DefectDojo:devfrom
valentijnscholten:refactor/dedupe-location-prefetch-helper
Aug 14, 2026
Merged

refactor(dedupe): one definition of the location prefetch#15520
Maffooch merged 2 commits into
DefectDojo:devfrom
valentijnscholten:refactor/dedupe-location-prefetch-helper

Conversation

@valentijnscholten

@valentijnscholten valentijnscholten commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Which location relation to prefetch is a property of what Finding.get_locations() reads, not of any individual caller — but it was spelled out separately at every call site, and that drifts in both directions. The hash-recompute paths disagreed about it for months, and the batch dedupe loader prefetched the deprecated endpoints relation, which raises under V3_FEATURE_LOCATIONS on an instance migrated from endpoints.
  • Adds location_prefetch_lookups(prefix="") to dojo/location/queries.py, next to vulnerability_id_prefetch, and routes build_candidate_scope_queryset and manage.py dedupe through it.
  • The dedupe command's V3 branch prefetched locations — one hop — where get_locations() reads location.url, leaving a query per location reference. It now prefetches the full path like the other call sites, and its two near-identical select_related/prefetch_related blocks collapse into one.
  • get_finding_models_for_deduplication is deliberately left alone: fix(dedupe): prefetch locations, not endpoints, in the batch dedupe loader #15508 fixes the crash there and re-baselines the perf query counts that change shifts. I measured that change independently while scoping this PR — +2 queries on each V3 step, and −104 on the second import — which matches fix(dedupe): prefetch locations, not endpoints, in the batch dedupe loader #15508's numbers, so re-deriving them here would only create a conflict. A TODO marks the one-line swap to the helper once it lands.
  • unittests/test_dedupe_location_prefetch.py pins the helper (both flag states, the full location.url path, and the relation-prefix form) and asserts both build_candidate_scope_queryset modes use it. Queryset shape only — no fixtures, no timing. 6 tests pass, and test_deduplication_logic still passes at 85.
  • The prefix argument exists for callers that page a model reaching the finding through a relation rather than paging Finding itself.

Extension points instead of a fork (second commit)

  • Distributions that store extra hash fields or need extra scoping have maintained near-verbatim copies of this command, and a copy is what lets them drift — the vulnerability-id prefetch was fixed in a copy first and had to be fixed again elsewhere months later, and the location prefetch was fixed elsewhere and never reached a copy, leaving it prefetching the deprecated endpoint relation.
  • Such a distribution can now subclass this command and override a hook: extra arguments, extra scope and its description, the hash generator, an extra recompute pass, the two batch-dedupe entry points, and product grading. Narrow on purpose — a stale override is a signature mismatch rather than silent drift.
  • Behaviour is unchanged; every default does what the code did before. The scope build is unified into one queryset that filters down instead of a parser/no-parser if/else, which is what makes an extra scope filter composable.
  • unittests/test_dedupe_command_hooks.py covers both halves: every hook has a working default, and a subclass's hooks are actually reached — including that --dedupe_only skips the extra hash pass, and that both batch paths and grading go through their hooks. This command had no tests at all before. 10 tests, verified locally.

@valentijnscholten
valentijnscholten force-pushed the refactor/dedupe-location-prefetch-helper branch from 69c0bf5 to a3b76c1 Compare August 5, 2026 07:51
@valentijnscholten valentijnscholten added the affects_pro PRs that affect Pro and need a coordinated release/merge moment. label Aug 5, 2026
@valentijnscholten
valentijnscholten marked this pull request as ready for review August 5, 2026 17:41
@valentijnscholten valentijnscholten added this to the 3.3.0 milestone Aug 5, 2026
@devGregA
devGregA self-requested a review August 8, 2026 15:40
@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 refactor/dedupe-location-prefetch-helper branch 2 times, most recently from 261e420 to d334b6e Compare August 10, 2026 20:45
@github-actions

Copy link
Copy Markdown
Contributor

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

@valentijnscholten
valentijnscholten force-pushed the refactor/dedupe-location-prefetch-helper branch from d334b6e to b4c01bc Compare August 11, 2026 19:17
@valentijnscholten

Copy link
Copy Markdown
Member Author

CI note: the remaining red is a dev-level failure, not this PR

After rebasing onto current dev, the only failing check is:

  • test-user-interface / User Interface Tests (tests/notification_webhook_test.py …)test_list_webhooks_page_loads fails in tearDown's assertNoConsoleErrors() with a SEVERE browser-console entry on the webhooks page ("Unit Tests Complete" is just the gate that rolls this up).

This isn't caused by this PR:

  • This PR only changes dedupe/prefetch Python (dojo/finding/deduplication.py, dojo/location/queries.py, dojo/management/commands/dedupe.py) — it has no webhooks-page, template, or JS surface, so it can't produce a console error on /notifications/webhook.
  • The same webhook UI batch is currently failing on unrelated PR docs(connectors): document the data-visibility warnings #15634, so it's not specific to this branch.
  • The webhooks-page console error most likely comes from the recent dev refactor refactor(ui): remove the classic Bootstrap UI #15565 ("remove the classic Bootstrap UI"), which touched notification_webhook_test.py; the test also has prior flakiness history (751f757 "Stabilize flaky notification-webhook integration test").

Everything else is green (44 checks). This PR should go green once the dev-level webhooks console error is resolved. Happy to chase that root cause separately if useful, but it's out of scope for this dedupe refactor.

@valentijnscholten
valentijnscholten force-pushed the refactor/dedupe-location-prefetch-helper branch from b4c01bc to 096e512 Compare August 13, 2026 15:46
Valentijn Scholten added 2 commits August 14, 2026 12:08
Which location relation to prefetch is a property of what Finding.get_locations()
reads, not of any individual caller, but it was spelled out separately at every call
site. That drifts, in both directions: the hash-recompute paths disagreed about it for
months, and the batch dedupe loader prefetched the deprecated endpoints relation, which
raises under V3_FEATURE_LOCATIONS on an instance migrated from endpoints.

Adds location_prefetch_lookups() to dojo/location/queries.py, alongside
vulnerability_id_prefetch, and routes build_candidate_scope_queryset and the dedupe
command through it. The command's V3 branch prefetched "locations", one hop, where
get_locations() reads location.url; it now prefetches the whole path like the other
call sites, and its two near-identical select_related/prefetch_related blocks collapse
into one.

get_finding_models_for_deduplication is left alone deliberately: DefectDojo#15508 fixes the
crash there and re-baselines the perf query counts it shifts. Measured that change
independently here -- +2 queries on each V3 step, and -104 on the second import --
which matches the numbers in that PR. A TODO marks the one-line swap to the helper
once it lands.

The prefix argument exists for callers that page a model reaching the finding through
a relation rather than paging Finding itself.
… a fork

Distributions that store extra hash fields or need extra scoping have maintained
near-verbatim copies of this command, and a copy is what lets the two drift. The
vulnerability-id prefetch was fixed in a copy first and had to be fixed again elsewhere
months later; the location prefetch was fixed elsewhere and never reached a copy,
leaving it prefetching the deprecated endpoint relation that raises under
V3_FEATURE_LOCATIONS.

Such a distribution can now subclass this command and override a hook instead. The
hooks are deliberately narrow -- extra arguments, extra scope and its description, the
hash generator, an extra recompute pass, the two batch-dedupe entry points, and product
grading -- so what an edition adds stays visible and a stale override is a signature
mismatch rather than silent drift.

Behaviour is unchanged: every default does what the code did before. The scope build is
also unified, replacing the parser/no-parser if/else with one queryset that filters
down, which is what makes an extra scope filter composable.

unittests/test_dedupe_command_hooks.py covers both halves of the contract: every hook
has a working default, and a subclass's hooks are actually reached by the run --
including that --dedupe_only skips the extra hash pass and that both batch paths and
grading go through their hooks. This command had no tests at all before.
@Maffooch
Maffooch force-pushed the refactor/dedupe-location-prefetch-helper branch from 096e512 to e57abcf Compare August 14, 2026 18:08
@Maffooch
Maffooch merged commit 40a1129 into DefectDojo:dev Aug 14, 2026
70 of 72 checks passed
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.

4 participants