From d83167874639b774518c8207589f4b43149e9b51 Mon Sep 17 00:00:00 2001 From: Jens Holdgaard Pedersen Date: Mon, 2 Nov 2026 09:40:54 +0100 Subject: [PATCH] perf(findings): load vulnerability_ids for the whole findings list page GET /api/v2/findings/ ran one dojo_findingvulnerabilityreference query per finding on the page, because vulnerability_ids is serialized on every row and FindingViewSet's queryset doesn't prefetch it. Adding vulnerability_id_prefetch() to get_queryset() would break a subclass or plugin that adds its own (Django raises "lookup was already seen with a different queryset"), which is why #16512 left it out. Instead, FindingViewSet.paginate_queryset() loads the references for the page with prefetch_related_objects(), skipping findings whose references are already prefetched, so such a queryset keeps working and nothing is loaded twice. The list scaling tests no longer leave dojo_findingvulnerabilityreference out of the count, give each finding two vulnerability ids, check their order in the response, and cover a queryset that already prefetches them. Fixes #16652 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0125HXBmURB5ZfBkH3T4LtME --- dojo/finding/api/views.py | 10 ++++++ dojo/vulnerability/queries.py | 21 +++++++++++- unittests/test_apiv2_list_query_scaling.py | 37 +++++++++++++++++++--- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/dojo/finding/api/views.py b/dojo/finding/api/views.py index d689d195f54..4d3d10c357a 100644 --- a/dojo/finding/api/views.py +++ b/dojo/finding/api/views.py @@ -80,6 +80,7 @@ get_system_setting, process_tag_notifications, ) +from dojo.vulnerability.queries import prefetch_vulnerability_ids logger = logging.getLogger(__name__) @@ -206,6 +207,15 @@ def destroy(self, request, *args, **kwargs): finding_helper.delete_finding_with_conflict_retry(instance, push_to_jira=push_to_jira) return Response(status=status.HTTP_204_NO_CONTENT) + def paginate_queryset(self, queryset): + page = super().paginate_queryset(queryset) + # vulnerability_ids is serialized on every row; unprefetched it costs a query per finding. + # Loaded for the page instead of in get_queryset(), so a subclass that adds its own + # vulnerability_id_prefetch() to the queryset doesn't get a duplicate-lookup error. + if page is not None: + prefetch_vulnerability_ids(page) + return page + def get_queryset(self): if locations_enabled(): findings = get_authorized_findings( diff --git a/dojo/vulnerability/queries.py b/dojo/vulnerability/queries.py index 3d1ccd310c2..09ef6cbffec 100644 --- a/dojo/vulnerability/queries.py +++ b/dojo/vulnerability/queries.py @@ -1,6 +1,6 @@ import logging -from django.db.models import OuterRef, Prefetch +from django.db.models import OuterRef, Prefetch, prefetch_related_objects try: from dojo.authorization.query_filters import get_auth_filter @@ -47,6 +47,25 @@ def vulnerability_id_prefetch(prefix=""): ) +def prefetch_vulnerability_ids(findings): + """ + Load the vulnerability id references of ``findings`` (saved Finding instances, e.g. a list page) + in one query, so ``finding_vulnerability_id_strings()`` reads them without a query per finding. + + Findings whose ``vulnerability_references`` are already prefetched are left as they are. This is + applied to a page of instances rather than to a queryset because a queryset that already carries + a ``vulnerability_id_prefetch()`` (a subclass or plugin may add one) raises Django's "lookup was + already seen with a different queryset" error when a second one is added. + """ + missing = [ + finding for finding in findings + if "vulnerability_references" not in getattr(finding, "_prefetched_objects_cache", {}) + ] + if missing: + prefetch_related_objects(missing, vulnerability_id_prefetch()) + return findings + + def finding_vulnerability_id_strings(finding): """ Ordered vulnerability id strings for a saved finding (raw reference relation — no cve merge, diff --git a/unittests/test_apiv2_list_query_scaling.py b/unittests/test_apiv2_list_query_scaling.py index 0507e5c5fd4..55b886e8956 100644 --- a/unittests/test_apiv2_list_query_scaling.py +++ b/unittests/test_apiv2_list_query_scaling.py @@ -24,6 +24,8 @@ from rest_framework.test import APIClient from dojo.api_v2.prefetch.prefetcher import _Prefetcher # noqa: PLC2701 -- the class under test +from dojo.finding.api.views import FindingViewSet +from dojo.finding.helper import save_vulnerability_ids from dojo.models import ( Dojo_User, Engagement, @@ -36,6 +38,7 @@ Test_Type, ) from dojo.notes.api.serializer import NoteSerializer +from dojo.vulnerability.queries import vulnerability_id_prefetch from .dojo_test_case import DojoAPITestCase @@ -95,11 +98,6 @@ def _assert_flat(self, user, url, params, add_row, small=2, large=6): for idx in range(small, large): add_row(idx) large_sqls, payload = self._count(user, url, params) - # vulnerability_ids still reads per row here. A plugin that extends this queryset (Pro) adds its own - # vulnerability_id_prefetch(), and a second Prefetch on the same lookup raises, so it is left out of the - # count rather than prefetched in this queryset. - small_sqls = [sql for sql in small_sqls if "dojo_findingvulnerabilityreference" not in sql] - large_sqls = [sql for sql in large_sqls if "dojo_findingvulnerabilityreference" not in sql] grown = _shapes(large_sqls) - _shapes(small_sqls) self.assertEqual( len(small_sqls), len(large_sqls), @@ -116,6 +114,8 @@ def _add_finding(self, idx): public = Notes.objects.create(entry=f"public {idx}", author=self.admin) private = Notes.objects.create(entry=f"private {idx}", author=self.admin, private=True) finding.notes.add(public, private) + # Two ids, so the page has vulnerability ids to load and an order to keep + save_vulnerability_ids(finding, [f"CVE-2026-{idx:04d}", f"GHSA-list-scaling-{idx}"]) return finding def _add_engagement(self, idx): @@ -130,6 +130,33 @@ def test_findings_list_is_flat_for_a_member_with_notes(self): for row in payload["results"]: self.assertEqual(["public"], [note["entry"].split()[0] for note in row["notes"]]) + def test_findings_list_vulnerability_ids_are_flat(self): + payload = self._assert_flat( + self.admin, "/api/v2/findings/", {"limit": 100, "test": self.test.id}, self._add_finding, + ) + by_id = {row["id"]: row["vulnerability_ids"] for row in payload["results"]} + # _add_finding creates them in idx order + ids = list(Finding.objects.filter(test=self.test).order_by("id").values_list("id", flat=True)) + self.assertEqual(6, len(ids)) + for idx, finding_id in enumerate(ids): + self.assertEqual( + [{"vulnerability_id": f"CVE-2026-{idx:04d}"}, {"vulnerability_id": f"GHSA-list-scaling-{idx}"}], + by_id[finding_id], + ) + + def test_findings_list_with_a_queryset_that_already_prefetches_vulnerability_ids(self): + """A subclass (a plugin) that adds its own vulnerability_id_prefetch() keeps working and stays flat.""" + original = FindingViewSet.get_queryset + + def get_queryset(viewset): + return original(viewset).prefetch_related(vulnerability_id_prefetch()) + + with patch.object(FindingViewSet, "get_queryset", get_queryset): + payload = self._assert_flat( + self.admin, "/api/v2/findings/", {"limit": 100, "test": self.test.id}, self._add_finding, + ) + self.assertEqual(6, sum(1 for row in payload["results"] if len(row["vulnerability_ids"]) == 2)) + def test_findings_list_prefetch_test_is_flat_for_a_member(self): payload = self._assert_flat( self.member, "/api/v2/findings/", {"limit": 100, "prefetch": "test"}, self._add_finding,