diff --git a/dojo/filters.py b/dojo/filters.py index 13b2b870100..79e00084507 100644 --- a/dojo/filters.py +++ b/dojo/filters.py @@ -117,7 +117,30 @@ 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 = _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}" @@ -125,13 +148,13 @@ 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) - tag_lookup = f"{tags_field.m2m_reverse_field_name()}__name" - if lookup != "exact": - tag_lookup += f"__{lookup}" + 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_lookup: value}, + **{tags_field.m2m_field_name(): OuterRef(outer), **tag_condition}, )) @@ -164,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 cc4e6cb909c..caa12799d92 100644 --- a/unittests/test_finding_tag_filters.py +++ b/unittests/test_finding_tag_filters.py @@ -7,11 +7,14 @@ 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 +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 @@ -144,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}) @@ -402,3 +426,55 @@ 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 _finding_with_tags(self, name, tags): + finding = Finding.objects.create( + title=f"{name} finding", severity="High", reporter=User.objects.create(username=f"{name}-reporter"), + test=Test.objects.create( + engagement=Engagement.objects.create( + name=f"{name} engagement", target_start=timezone.now(), target_end=timezone.now(), + product=Product.objects.create( + 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 = 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")]: + 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): + 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(1, 4)]) + with mock.patch("dojo.filters.TAG_ID_LITERAL_LIMIT", 3): + 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)