From 76ccce64cc3d66cfec075651c3d2239fda747dc2 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Tue, 5 Oct 2021 21:38:15 -0400 Subject: [PATCH] Fixed #16063 -- Adjusted admin changelist searches spanning multi-valued relationships. This reduces the likelihood of admin searches issuing queries with excessive joins. --- django/contrib/admin/options.py | 4 ++- docs/ref/contrib/admin/index.txt | 32 ++++++++++++++++++++++++ docs/releases/4.1.txt | 20 +++++++++++++++ docs/topics/db/queries.txt | 2 ++ tests/admin_changelist/admin.py | 6 +++++ tests/admin_changelist/tests.py | 42 +++++++++++++++++++++++++++++--- 6 files changed, 102 insertions(+), 4 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 66f3396a6b..c5968a79ed 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1031,6 +1031,7 @@ class ModelAdmin(BaseModelAdmin): if search_fields and search_term: orm_lookups = [construct_search(str(search_field)) for search_field in search_fields] + term_queries = [] for bit in smart_split(search_term): if bit.startswith(('"', "'")) and bit[0] == bit[-1]: bit = unescape_string_literal(bit) @@ -1038,7 +1039,8 @@ class ModelAdmin(BaseModelAdmin): *((orm_lookup, bit) for orm_lookup in orm_lookups), _connector=models.Q.OR, ) - queryset = queryset.filter(or_queries) + term_queries.append(or_queries) + queryset = queryset.filter(models.Q(*term_queries)) may_have_duplicates |= any( lookup_spawns_duplicates(self.opts, search_spec) for search_spec in orm_lookups diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index 5caea603c1..ac8f44b765 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -1185,6 +1185,22 @@ subclass:: :meth:`ModelAdmin.get_search_results` to provide additional or alternate search behavior. + .. versionchanged:: 4.1 + + Searches using multiple search terms are now applied in a single call + to ``filter()``, rather than in sequential ``filter()`` calls. + + For multi-valued relationships, this means that rows from the related + model must match all terms rather than any term. For example, if + ``search_fields`` is set to ``['child__name', 'child__age']``, and a + user searches for ``'Jamal 17'``, parent rows will be returned only if + there is a relationship to some 17-year-old child named Jamal, rather + than also returning parents who merely have a younger or older child + named Jamal in addition to some other 17-year-old. + + See the :ref:`spanning-multi-valued-relationships` topic for more + discussion of this difference. + .. attribute:: ModelAdmin.search_help_text .. versionadded:: 4.0 @@ -1403,6 +1419,22 @@ templates used by the :class:`ModelAdmin` views: field, for example ``... OR UPPER("polls_choice"."votes"::text) = UPPER('4')`` on PostgreSQL. + .. versionchanged:: 4.1 + + Searches using multiple search terms are now applied in a single call + to ``filter()``, rather than in sequential ``filter()`` calls. + + For multi-valued relationships, this means that rows from the related + model must match all terms rather than any term. For example, if + ``search_fields`` is set to ``['child__name', 'child__age']``, and a + user searches for ``'Jamal 17'``, parent rows will be returned only if + there is a relationship to some 17-year-old child named Jamal, rather + than also returning parents who merely have a younger or older child + named Jamal in addition to some other 17-year-old. + + See the :ref:`spanning-multi-valued-relationships` topic for more + discussion of this difference. + .. method:: ModelAdmin.save_related(request, form, formsets, change) The ``save_related`` method is given the ``HttpRequest``, the parent diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt index 3fdd7f53c6..5f184d2349 100644 --- a/docs/releases/4.1.txt +++ b/docs/releases/4.1.txt @@ -290,6 +290,26 @@ Dropped support for MariaDB 10.2 Upstream support for MariaDB 10.2 ends in May 2022. Django 4.1 supports MariaDB 10.3 and higher. +Admin changelist searches spanning multi-valued relationships changes +--------------------------------------------------------------------- + +Admin changelist searches using multiple search terms are now applied in a +single call to ``filter()``, rather than in sequential ``filter()`` calls. + +For multi-valued relationships, this means that rows from the related model +must match all terms rather than any term. For example, if ``search_fields`` +is set to ``['child__name', 'child__age']``, and a user searches for +``'Jamal 17'``, parent rows will be returned only if there is a relationship to +some 17-year-old child named Jamal, rather than also returning parents who +merely have a younger or older child named Jamal in addition to some other +17-year-old. + +See the :ref:`spanning-multi-valued-relationships` topic for more discussion of +this difference. In Django 4.0 and earlier, +:meth:`~django.contrib.admin.ModelAdmin.get_search_results` followed the +second example query, but this undocumented behavior led to queries with +excessive joins. + Miscellaneous ------------- diff --git a/docs/topics/db/queries.txt b/docs/topics/db/queries.txt index c24c06da68..eb68c7b4da 100644 --- a/docs/topics/db/queries.txt +++ b/docs/topics/db/queries.txt @@ -525,6 +525,8 @@ those latter objects, you could write:: Blog.objects.filter(entry__authors__isnull=False, entry__authors__name__isnull=True) +.. _spanning-multi-valued-relationships: + Spanning multi-valued relationships ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/admin_changelist/admin.py b/tests/admin_changelist/admin.py index f8051f64fa..929539ea88 100644 --- a/tests/admin_changelist/admin.py +++ b/tests/admin_changelist/admin.py @@ -36,6 +36,12 @@ class ParentAdmin(admin.ModelAdmin): list_select_related = ['child'] +class ParentAdminTwoSearchFields(admin.ModelAdmin): + list_filter = ['child__name'] + search_fields = ['child__name', 'child__age'] + list_select_related = ['child'] + + class ChildAdmin(admin.ModelAdmin): list_display = ['name', 'parent'] list_per_page = 10 diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 92ea0dd718..86fcab531a 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -30,8 +30,8 @@ from .admin import ( DynamicListDisplayLinksChildAdmin, DynamicListFilterChildAdmin, DynamicSearchFieldsChildAdmin, EmptyValueChildAdmin, EventAdmin, FilteredChildAdmin, GroupAdmin, InvitationAdmin, - NoListDisplayLinksParentAdmin, ParentAdmin, QuartetAdmin, SwallowAdmin, - site as custom_site, + NoListDisplayLinksParentAdmin, ParentAdmin, ParentAdminTwoSearchFields, + QuartetAdmin, SwallowAdmin, site as custom_site, ) from .models import ( Band, CharPK, Child, ChordsBand, ChordsMusician, Concert, CustomIdUser, @@ -153,6 +153,42 @@ class ChangeListTests(TestCase): cl = ia.get_changelist_instance(request) self.assertEqual(cl.queryset.query.select_related, {'player': {}, 'band': {}}) + def test_many_search_terms(self): + parent = Parent.objects.create(name='Mary') + Child.objects.create(parent=parent, name='Danielle') + Child.objects.create(parent=parent, name='Daniel') + + m = ParentAdmin(Parent, custom_site) + request = self.factory.get('/parent/', data={SEARCH_VAR: 'daniel ' * 80}) + request.user = self.superuser + + cl = m.get_changelist_instance(request) + with CaptureQueriesContext(connection) as context: + object_count = cl.queryset.count() + self.assertEqual(object_count, 1) + self.assertEqual(context.captured_queries[0]['sql'].count('JOIN'), 1) + + def test_related_field_multiple_search_terms(self): + """ + Searches over multi-valued relationships return rows from related + models only when all searched fields match that row. + """ + parent = Parent.objects.create(name='Mary') + Child.objects.create(parent=parent, name='Danielle', age=18) + Child.objects.create(parent=parent, name='Daniel', age=19) + + m = ParentAdminTwoSearchFields(Parent, custom_site) + + request = self.factory.get('/parent/', data={SEARCH_VAR: 'danielle 19'}) + request.user = self.superuser + cl = m.get_changelist_instance(request) + self.assertEqual(cl.queryset.count(), 0) + + request = self.factory.get('/parent/', data={SEARCH_VAR: 'daniel 19'}) + request.user = self.superuser + cl = m.get_changelist_instance(request) + self.assertEqual(cl.queryset.count(), 1) + def test_result_list_empty_changelist_value(self): """ Regression test for #14982: EMPTY_CHANGELIST_VALUE should be honored @@ -555,7 +591,7 @@ class ChangeListTests(TestCase): ('Finlayson', 1), ('Finlayson Hype', 0), ('Jonathan Finlayson Duo', 1), - ('Mary Jonathan Duo', 1), + ('Mary Jonathan Duo', 0), ('Oscar Finlayson Duo', 0), ): with self.subTest(search_string=search_string):