Browse Source

Fixed #16063 -- Adjusted admin changelist searches spanning multi-valued relationships.

This reduces the likelihood of admin searches issuing queries with
excessive joins.
Jacob Walls 3 years ago
parent
commit
76ccce64cc

+ 3 - 1
django/contrib/admin/options.py

@@ -1031,6 +1031,7 @@ class ModelAdmin(BaseModelAdmin):
         if search_fields and search_term:
         if search_fields and search_term:
             orm_lookups = [construct_search(str(search_field))
             orm_lookups = [construct_search(str(search_field))
                            for search_field in search_fields]
                            for search_field in search_fields]
+            term_queries = []
             for bit in smart_split(search_term):
             for bit in smart_split(search_term):
                 if bit.startswith(('"', "'")) and bit[0] == bit[-1]:
                 if bit.startswith(('"', "'")) and bit[0] == bit[-1]:
                     bit = unescape_string_literal(bit)
                     bit = unescape_string_literal(bit)
@@ -1038,7 +1039,8 @@ class ModelAdmin(BaseModelAdmin):
                     *((orm_lookup, bit) for orm_lookup in orm_lookups),
                     *((orm_lookup, bit) for orm_lookup in orm_lookups),
                     _connector=models.Q.OR,
                     _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(
             may_have_duplicates |= any(
                 lookup_spawns_duplicates(self.opts, search_spec)
                 lookup_spawns_duplicates(self.opts, search_spec)
                 for search_spec in orm_lookups
                 for search_spec in orm_lookups

+ 32 - 0
docs/ref/contrib/admin/index.txt

@@ -1185,6 +1185,22 @@ subclass::
     :meth:`ModelAdmin.get_search_results` to provide additional or alternate
     :meth:`ModelAdmin.get_search_results` to provide additional or alternate
     search behavior.
     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
 .. attribute:: ModelAdmin.search_help_text
 
 
     .. versionadded:: 4.0
     .. 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')``
     field, for example ``... OR UPPER("polls_choice"."votes"::text) = UPPER('4')``
     on PostgreSQL.
     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)
 .. method:: ModelAdmin.save_related(request, form, formsets, change)
 
 
     The ``save_related`` method is given the ``HttpRequest``, the parent
     The ``save_related`` method is given the ``HttpRequest``, the parent

+ 20 - 0
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
 Upstream support for MariaDB 10.2 ends in May 2022. Django 4.1 supports MariaDB
 10.3 and higher.
 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
 Miscellaneous
 -------------
 -------------
 
 

+ 2 - 0
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)
     Blog.objects.filter(entry__authors__isnull=False, entry__authors__name__isnull=True)
 
 
+.. _spanning-multi-valued-relationships:
+
 Spanning multi-valued relationships
 Spanning multi-valued relationships
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 

+ 6 - 0
tests/admin_changelist/admin.py

@@ -36,6 +36,12 @@ class ParentAdmin(admin.ModelAdmin):
     list_select_related = ['child']
     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):
 class ChildAdmin(admin.ModelAdmin):
     list_display = ['name', 'parent']
     list_display = ['name', 'parent']
     list_per_page = 10
     list_per_page = 10

+ 39 - 3
tests/admin_changelist/tests.py

@@ -30,8 +30,8 @@ from .admin import (
     DynamicListDisplayLinksChildAdmin, DynamicListFilterChildAdmin,
     DynamicListDisplayLinksChildAdmin, DynamicListFilterChildAdmin,
     DynamicSearchFieldsChildAdmin, EmptyValueChildAdmin, EventAdmin,
     DynamicSearchFieldsChildAdmin, EmptyValueChildAdmin, EventAdmin,
     FilteredChildAdmin, GroupAdmin, InvitationAdmin,
     FilteredChildAdmin, GroupAdmin, InvitationAdmin,
-    NoListDisplayLinksParentAdmin, ParentAdmin, QuartetAdmin, SwallowAdmin,
-    site as custom_site,
+    NoListDisplayLinksParentAdmin, ParentAdmin, ParentAdminTwoSearchFields,
+    QuartetAdmin, SwallowAdmin, site as custom_site,
 )
 )
 from .models import (
 from .models import (
     Band, CharPK, Child, ChordsBand, ChordsMusician, Concert, CustomIdUser,
     Band, CharPK, Child, ChordsBand, ChordsMusician, Concert, CustomIdUser,
@@ -153,6 +153,42 @@ class ChangeListTests(TestCase):
         cl = ia.get_changelist_instance(request)
         cl = ia.get_changelist_instance(request)
         self.assertEqual(cl.queryset.query.select_related, {'player': {}, 'band': {}})
         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):
     def test_result_list_empty_changelist_value(self):
         """
         """
         Regression test for #14982: EMPTY_CHANGELIST_VALUE should be honored
         Regression test for #14982: EMPTY_CHANGELIST_VALUE should be honored
@@ -555,7 +591,7 @@ class ChangeListTests(TestCase):
             ('Finlayson', 1),
             ('Finlayson', 1),
             ('Finlayson Hype', 0),
             ('Finlayson Hype', 0),
             ('Jonathan Finlayson Duo', 1),
             ('Jonathan Finlayson Duo', 1),
-            ('Mary Jonathan Duo', 1),
+            ('Mary Jonathan Duo', 0),
             ('Oscar Finlayson Duo', 0),
             ('Oscar Finlayson Duo', 0),
         ):
         ):
             with self.subTest(search_string=search_string):
             with self.subTest(search_string=search_string):