From 32e35b6817c76208b24522801f7a8288add0d4e4 Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 29 Oct 2026 20:06:06 -0600 Subject: [PATCH 1/2] perf(findings): resolve tag filter names to tag ids before matching tag_exists() matched the tag through a join on the tag table's name, so the planner could not tell which tag a query was about and assumed the average number of rows per tag. For a tag carried by most findings that estimate was off by three orders of magnitude (573 expected, 400,001 actual), and the finding list's COUNT and id queries took a nested-loop semi-join that probed every tagged row: 20.6 s for one COUNT over 400k tagged findings, 2.5 s once the subquery names the tag id. The matching tags are now read first (one indexed lookup on the tag table) and the EXISTS subquery tests the through table against those ids. A lookup that matches more than TAG_ID_LITERAL_LIMIT tags (a short icontains value) keeps the join. Results, counts and paging are unchanged: the existing equivalence tests against the old JOIN form pass as they are. Co-Authored-By: Claude Opus 5.5 --- dojo/filters.py | 24 +++++++++++++--- unittests/test_finding_tag_filters.py | 41 +++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/dojo/filters.py b/dojo/filters.py index 13b2b870100..7a68b26392b 100644 --- a/dojo/filters.py +++ b/dojo/filters.py @@ -117,6 +117,14 @@ def tag_exists(model, field_name, lookup, value): For a related model's tags (``test__engagement__tags__name``) the subquery runs on that model's through table and correlates on the relation's id. + + The matching tags are resolved to their ids first (one indexed read of the tag table), + and the subquery tests the through table against those ids. Matching the name through + a join instead hides which tag the query is about: the planner then assumes the + average number of rows per tag, so for a tag carried by most rows it expected hundreds + where there were hundreds of thousands, and chose a plan that probed every one of them + (20 s instead of 2.5 s for one COUNT over 400k tagged findings). A lookup that matches + more than TAG_ID_LITERAL_LIMIT tags (a short ``icontains`` value) keeps the join. """ *path, tags_field_name, name = field_name.split("__") if name != "name": @@ -126,15 +134,23 @@ def tag_exists(model, field_name, lookup, value): for part in path: tagged_model = tagged_model._meta.get_field(part).related_model tags_field = tagged_model._meta.get_field(tags_field_name) - tag_lookup = f"{tags_field.m2m_reverse_field_name()}__name" - if lookup != "exact": - tag_lookup += f"__{lookup}" + name_lookup = "name" if lookup == "exact" else f"name__{lookup}" + tag_ids = list(tags_field.related_model.objects.filter(**{name_lookup: value}).values_list("pk", flat=True)[ + :TAG_ID_LITERAL_LIMIT + 1]) + if len(tag_ids) > TAG_ID_LITERAL_LIMIT: + tag_condition = {f"{tags_field.m2m_reverse_field_name()}__{name_lookup}": value} + else: + tag_condition = {f"{tags_field.m2m_reverse_field_name()}__in": tag_ids} outer = "__".join([*path, "pk"]) if path else "pk" return Exists(tags_field.remote_field.through.objects.filter( - **{tags_field.m2m_field_name(): OuterRef(outer), tag_lookup: value}, + **{tags_field.m2m_field_name(): OuterRef(outer), **tag_condition}, )) +# How many tag ids tag_exists writes into the query as literals before it matches by name instead. +TAG_ID_LITERAL_LIMIT = 1000 + + class TagExistsFilterMixin: """ diff --git a/unittests/test_finding_tag_filters.py b/unittests/test_finding_tag_filters.py index cc4e6cb909c..885dae1a341 100644 --- a/unittests/test_finding_tag_filters.py +++ b/unittests/test_finding_tag_filters.py @@ -7,6 +7,7 @@ of SQL per page. Each filter now tests membership with Exists() on the tags through table. These tests pin the result sets to the old JOIN semantics and pin the SQL shape. """ +from unittest import mock from urllib.parse import parse_qsl from django.contrib.auth.models import User @@ -402,3 +403,43 @@ def test_related_paths_resolve(self): (Test, "engagement__tags__name"), (Engagement, "product__tags__name"), (Product, "tags__name")]: with self.subTest(model=model.__name__, field=field): self.assertIsNotNone(list(model.objects.filter(tag_exists(model, field, "in", ["x"])))) + + def test_matches_on_tag_ids_not_on_names(self): + # The planner can only see how common a tag is when the query names its id. Joined by name, it + # assumed the average tag and mis-planned popular ones (a 20 s COUNT instead of 2.5 s). + finding = Finding.objects.create( + title="Tag id finding", severity="High", reporter=User.objects.create(username="tag-id-reporter"), + test=Test.objects.create( + engagement=Engagement.objects.create( + name="Tag id engagement", target_start=timezone.now(), target_end=timezone.now(), + product=Product.objects.create( + name="Tag id asset", description="x", prod_type=Product_Type.objects.create(name="Tag id org"))), + test_type=Test_Type.objects.get_or_create(name="Tag id scan")[0], + target_start=timezone.now(), target_end=timezone.now())) + finding.tags = ["tag-id-one", "tag-id-two"] + finding.save() + tag_model = Finding._meta.get_field("tags").related_model + wanted = tag_model.objects.get(name="tag-id-one").pk + for lookup, value in [("exact", "tag-id-one"), ("in", ["tag-id-one"]), ("icontains", "ID-ONE")]: + with self.subTest(lookup=lookup): + qs = Finding.objects.filter(tag_exists(Finding, "tags__name", lookup, value)) + sql = str(qs.query) + subquery = sql[sql.index("EXISTS"):] + self.assertNotIn(tag_model._meta.db_table, subquery) + self.assertIn(f"IN ({wanted})", subquery) + self.assertEqual(list(qs), [finding]) + self.assertNotIn(finding, Finding.objects.exclude(tag_exists(Finding, "tags__name", lookup, value))) + + def test_an_unknown_tag_matches_nothing_and_its_negation_everything(self): + everything = Finding.objects.count() + self.assertEqual(Finding.objects.filter(tag_exists(Finding, "tags__name", "in", ["no-such-tag"])).count(), 0) + self.assertEqual( + Finding.objects.filter(~tag_exists(Finding, "tags__name", "in", ["no-such-tag"])).count(), everything) + + def test_a_lookup_matching_many_tags_keeps_the_name_join(self): + tag_model = Finding._meta.get_field("tags").related_model + tag_model.objects.bulk_create( + [tag_model(name=f"many-tags-{n}", slug=f"many-tags-{n}") for n in range(4)]) + with mock.patch("dojo.filters.TAG_ID_LITERAL_LIMIT", 3): + sql = str(Finding.objects.filter(tag_exists(Finding, "tags__name", "icontains", "many-tags")).query) + self.assertIn(tag_model._meta.db_table, sql[sql.index("EXISTS"):]) From 1dee83c785c91d968394ac4d522d6ea94d5cdbac Mon Sep 17 00:00:00 2001 From: Cody Maffucci <46459665+Maffooch@users.noreply.github.com> Date: Thu, 29 Oct 2026 23:57:51 -0600 Subject: [PATCH 2/2] perf(findings): resolve every AND-ed tag in one read, and match nothing for an unknown one tags__and (and its test__, test__engagement__ and test__engagement__product__ forms) looked each listed tag up on its own, one query per tag. The names are now resolved in one name__in read, and one EXISTS per tag id is built from it. A listed name that is no tag cannot be present, so the filter returns an empty queryset without building the subqueries. Tests: the over-limit fallback is checked on the rows it returns, not only its SQL, and an unknown tag is checked through the FilterSet for tags__and, test__engagement__tags__and and not_tags. Co-Authored-By: Claude Opus 5.5 --- dojo/filters.py | 48 +++++++++++++++------- unittests/test_finding_tag_filters.py | 57 +++++++++++++++++++++------ 2 files changed, 79 insertions(+), 26 deletions(-) diff --git a/dojo/filters.py b/dojo/filters.py index 7a68b26392b..79e00084507 100644 --- a/dojo/filters.py +++ b/dojo/filters.py @@ -126,6 +126,21 @@ def tag_exists(model, field_name, lookup, value): (20 s instead of 2.5 s for one COUNT over 400k tagged findings). A lookup that matches more than TAG_ID_LITERAL_LIMIT tags (a short ``icontains`` value) keeps the join. """ + path, tags_field = _tags_field(model, field_name) + name_lookup = "name" if lookup == "exact" else f"name__{lookup}" + tag_ids = list(tags_field.related_model.objects.filter(**{name_lookup: value}).values_list("pk", flat=True)[ + :TAG_ID_LITERAL_LIMIT + 1]) + if len(tag_ids) > TAG_ID_LITERAL_LIMIT: + return _tags_exist(path, tags_field, {f"{tags_field.m2m_reverse_field_name()}__{name_lookup}": value}) + return _tags_exist(path, tags_field, {f"{tags_field.m2m_reverse_field_name()}__in": tag_ids}) + + +# How many tag ids tag_exists writes into the query as literals before it matches by name instead. +TAG_ID_LITERAL_LIMIT = 1000 + + +def _tags_field(model, field_name): + """The relation path to the tagged model and its tags field, for a ``[__]tags__name`` field.""" *path, tags_field_name, name = field_name.split("__") if name != "name": msg = f"tag_exists expects a __tags__name field, got {field_name}" @@ -133,24 +148,16 @@ def tag_exists(model, field_name, lookup, value): tagged_model = model for part in path: tagged_model = tagged_model._meta.get_field(part).related_model - tags_field = tagged_model._meta.get_field(tags_field_name) - name_lookup = "name" if lookup == "exact" else f"name__{lookup}" - tag_ids = list(tags_field.related_model.objects.filter(**{name_lookup: value}).values_list("pk", flat=True)[ - :TAG_ID_LITERAL_LIMIT + 1]) - if len(tag_ids) > TAG_ID_LITERAL_LIMIT: - tag_condition = {f"{tags_field.m2m_reverse_field_name()}__{name_lookup}": value} - else: - tag_condition = {f"{tags_field.m2m_reverse_field_name()}__in": tag_ids} + return path, tagged_model._meta.get_field(tags_field_name) + + +def _tags_exist(path, tags_field, tag_condition): outer = "__".join([*path, "pk"]) if path else "pk" return Exists(tags_field.remote_field.through.objects.filter( **{tags_field.m2m_field_name(): OuterRef(outer), **tag_condition}, )) -# How many tag ids tag_exists writes into the query as literals before it matches by name instead. -TAG_ID_LITERAL_LIMIT = 1000 - - class TagExistsFilterMixin: """ @@ -180,15 +187,26 @@ class TagNameInFilter(TagExistsFilterMixin, CharFieldInFilter): class TagNameANDFilter(CharFieldFilterANDExpression): - """Every listed tag must be present: one Exists() per tag instead of GROUP BY ... HAVING COUNT.""" + """ + Every listed tag must be present: one Exists() per tag instead of GROUP BY ... HAVING COUNT. + + The listed names are resolved to tag ids in one read (see tag_exists for why the subqueries name + ids). A name that is no tag cannot be present, so the whole filter matches nothing. + """ multiplies_rows = False def filter(self, queryset, value): if not value: return queryset - for tag in set(value.split(",")): - queryset = queryset.filter(tag_exists(queryset.model, self.field_name, "exact", tag)) + names = set(value.split(",")) + path, tags_field = _tags_field(queryset.model, self.field_name) + tag_ids = dict(tags_field.related_model.objects.filter(name__in=names).values_list("name", "pk")) + if len(tag_ids) < len(names): + return queryset.none() + for tag_id in tag_ids.values(): + queryset = queryset.filter( + _tags_exist(path, tags_field, {f"{tags_field.m2m_reverse_field_name()}__in": [tag_id]})) return queryset diff --git a/unittests/test_finding_tag_filters.py b/unittests/test_finding_tag_filters.py index 885dae1a341..caa12799d92 100644 --- a/unittests/test_finding_tag_filters.py +++ b/unittests/test_finding_tag_filters.py @@ -11,8 +11,10 @@ from urllib.parse import parse_qsl from django.contrib.auth.models import User +from django.db import connection from django.db.models import Count from django.test import override_settings +from django.test.utils import CaptureQueriesContext from django.urls import reverse from django.utils import timezone from django_filters import BooleanFilter, CharFilter @@ -145,6 +147,27 @@ def test_tags_and(self): for value in ["alpha", "alpha,beta", "alpha,beta,gamma", "beta,gamma-ray", "alpha,alpha", "ALPHA,beta", "alpha,"]: self.assert_same_as_legacy({"tags__and": value}) + def test_unknown_tags_through_the_filterset(self): + # A name that is no tag: tags__and cannot be satisfied, not_tags excludes nothing. + for params, expected in [ + ({"tags__and": "no-such-tag"}, []), + ({"tags__and": "alpha,no-such-tag"}, []), + ({"test__engagement__tags__and": "shared,no-such-tag"}, []), + ({"not_tags": "no-such-tag"}, _ids(self.base)), + ({"not_tags": "no-such-tag,beta"}, _ids(self.base.exclude(tags__name="beta"))), + ]: + with self.subTest(params=params): + self.assertEqual(_ids(self.filtered(**params)), expected) + self.assert_same_as_legacy(params) + + def test_tags_and_reads_the_tag_table_once(self): + # Every AND-ed name is resolved in one read, however many are listed; the page query then names ids. + tag_table = Finding._meta.get_field("tags").related_model._meta.db_table + with CaptureQueriesContext(connection) as queries: + ids = _ids(self.filtered(tags__and="alpha,beta,gamma")) + self.assertEqual(sum(f'FROM "{tag_table}"' in query["sql"] for query in queries.captured_queries), 1) + self.assertEqual(ids, _ids(self.filtered(LegacyJoinTagFilter, tags__and="alpha,beta,gamma"))) + def test_has_tags(self): for value in ["true", "false"]: self.assert_same_as_legacy({"has_tags": value}) @@ -404,20 +427,24 @@ def test_related_paths_resolve(self): with self.subTest(model=model.__name__, field=field): self.assertIsNotNone(list(model.objects.filter(tag_exists(model, field, "in", ["x"])))) - def test_matches_on_tag_ids_not_on_names(self): - # The planner can only see how common a tag is when the query names its id. Joined by name, it - # assumed the average tag and mis-planned popular ones (a 20 s COUNT instead of 2.5 s). + def _finding_with_tags(self, name, tags): finding = Finding.objects.create( - title="Tag id finding", severity="High", reporter=User.objects.create(username="tag-id-reporter"), + title=f"{name} finding", severity="High", reporter=User.objects.create(username=f"{name}-reporter"), test=Test.objects.create( engagement=Engagement.objects.create( - name="Tag id engagement", target_start=timezone.now(), target_end=timezone.now(), + name=f"{name} engagement", target_start=timezone.now(), target_end=timezone.now(), product=Product.objects.create( - name="Tag id asset", description="x", prod_type=Product_Type.objects.create(name="Tag id org"))), - test_type=Test_Type.objects.get_or_create(name="Tag id scan")[0], + name=f"{name} asset", description="x", prod_type=Product_Type.objects.create(name=f"{name} org"))), + test_type=Test_Type.objects.get_or_create(name=f"{name} scan")[0], target_start=timezone.now(), target_end=timezone.now())) - finding.tags = ["tag-id-one", "tag-id-two"] + finding.tags = tags finding.save() + return finding + + def test_matches_on_tag_ids_not_on_names(self): + # The planner can only see how common a tag is when the query names its id. Joined by name, it + # assumed the average tag and mis-planned popular ones (a 20 s COUNT instead of 2.5 s). + finding = self._finding_with_tags("Tag id", ["tag-id-one", "tag-id-two"]) tag_model = Finding._meta.get_field("tags").related_model wanted = tag_model.objects.get(name="tag-id-one").pk for lookup, value in [("exact", "tag-id-one"), ("in", ["tag-id-one"]), ("icontains", "ID-ONE")]: @@ -437,9 +464,17 @@ def test_an_unknown_tag_matches_nothing_and_its_negation_everything(self): Finding.objects.filter(~tag_exists(Finding, "tags__name", "in", ["no-such-tag"])).count(), everything) def test_a_lookup_matching_many_tags_keeps_the_name_join(self): + tagged = self._finding_with_tags("Many tags", ["many-tags-0"]) + untagged = self._finding_with_tags("Few tags", ["other-tag"]) tag_model = Finding._meta.get_field("tags").related_model tag_model.objects.bulk_create( - [tag_model(name=f"many-tags-{n}", slug=f"many-tags-{n}") for n in range(4)]) + [tag_model(name=f"many-tags-{n}", slug=f"many-tags-{n}") for n in range(1, 4)]) with mock.patch("dojo.filters.TAG_ID_LITERAL_LIMIT", 3): - sql = str(Finding.objects.filter(tag_exists(Finding, "tags__name", "icontains", "many-tags")).query) - self.assertIn(tag_model._meta.db_table, sql[sql.index("EXISTS"):]) + condition = tag_exists(Finding, "tags__name", "icontains", "many-tags") + matched = Finding.objects.filter(condition) + self.assertIn(tag_model._meta.db_table, str(matched.query)[str(matched.query).index("EXISTS"):]) + # The join form matches the same rows the id form would. + self.assertEqual(list(matched), [tagged]) + excluded = set(Finding.objects.exclude(condition)) + self.assertNotIn(tagged, excluded) + self.assertIn(untagged, excluded)