Merge pull request #1245 from oinopion/list_select_related

Fixed #19080 -- Fine-grained control over select_related in admin

6 files changed, 88 insertions, 30 deletions

diff --git a/django/contrib/admin/validation.py b/django/contrib/admin/validation.py
index 59c5ad35ef..222d433e53 100644
--- a/django/contrib/admin/validation.py
+++ b/django/contrib/admin/validation.py
@@ -310,8 +310,14 @@ class ModelAdminValidator(BaseValidator):
                                 % (cls.__name__, idx, field))
 
     def validate_list_select_related(self, cls, model):
-        " Validate that list_select_related is a boolean. "
-        check_type(cls, 'list_select_related', bool)
+        " Validate that list_select_related is a boolean, a list or a tuple. "
+        list_select_related = getattr(cls, 'list_select_related', None)
+        if list_select_related:
+            types = (bool, tuple, list)
+            if not isinstance(list_select_related, types):
+                raise ImproperlyConfigured("'%s.list_select_related' should be "
+                                           "either a bool, a tuple or a list" %
+                                           cls.__name__)
 
     def validate_list_per_page(self, cls, model):
         " Validate that list_per_page is an integer. "
diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
index dbed21265c..8ea7e10fc0 100644
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -356,36 +356,46 @@ class ChangeList(six.with_metaclass(RenameChangeListMethods)):
             # ValueError, ValidationError, or ?.
             raise IncorrectLookupParameters(e)
 
-        # Use select_related() if one of the list_display options is a field
-        # with a relationship and the provided queryset doesn't already have
-        # select_related defined.
         if not qs.query.select_related:
-            if self.list_select_related:
-                qs = qs.select_related()
-            else:
-                for field_name in self.list_display:
-                    try:
-                        field = self.lookup_opts.get_field(field_name)
-                    except models.FieldDoesNotExist:
-                        pass
-                    else:
-                        if isinstance(field.rel, models.ManyToOneRel):
-                            qs = qs.select_related()
-                            break
+            qs = self.apply_select_related(qs)
 
         # Set ordering.
         ordering = self.get_ordering(request, qs)
         qs = qs.order_by(*ordering)
 
         # Apply search results
-        qs, search_use_distinct = self.model_admin.get_search_results(request, qs, self.query)
+        qs, search_use_distinct = self.model_admin.get_search_results(
+            request, qs, self.query)
 
-        # Remove duplicates from results, if neccesary
+        # Remove duplicates from results, if necessary
         if filters_use_distinct | search_use_distinct:
             return qs.distinct()
         else:
             return qs
 
+    def apply_select_related(self, qs):
+        if self.list_select_related is True:
+            return qs.select_related()
+
+        if self.list_select_related is False:
+            if self.has_related_field_in_list_display():
+                return qs.select_related()
+
+        if self.list_select_related:
+            return qs.select_related(*self.list_select_related)
+        return qs
+
+    def has_related_field_in_list_display(self):
+        for field_name in self.list_display:
+            try:
+                field = self.lookup_opts.get_field(field_name)
+            except models.FieldDoesNotExist:
+                pass
+            else:
+                if isinstance(field.rel, models.ManyToOneRel):
+                    return True
+        return False
+
     def url_for_result(self, result):
         pk = getattr(result, self.pk_attname)
         return reverse('admin:%s_%s_change' % (self.opts.app_label,
diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt
index 11bc2c7268..7377f11a63 100644
--- a/docs/ref/contrib/admin/index.txt
+++ b/docs/ref/contrib/admin/index.txt
@@ -812,12 +812,24 @@ subclass::
     the list of objects on the admin change list page. This can save you a
     bunch of database queries.
 
-    The value should be either ``True`` or ``False``. Default is ``False``.
+    .. versionchanged:: dev
 
-    Note that Django will use
-    :meth:`~django.db.models.query.QuerySet.select_related`,
-    regardless of this setting if one of the ``list_display`` fields is a
-    ``ForeignKey``.
+    The value should be either a boolean, a list or a tuple. Default is
+    ``False``.
+
+    When value is ``True``, ``select_related()`` will always be called. When
+    value is set to ``False``, Django will look at ``list_display`` and call
+    ``select_related()`` if any ``ForeignKey`` is present.
+
+    If you need more fine-grained control, use a tuple (or list) as value for
+    ``list_select_related``. Empty tuple will prevent Django from calling
+    ``select_related`` at all. Any other tuple will be passed directly to
+    ``select_related`` as parameters. For example::
+
+        class ArticleAdmin(admin.ModelAdmin):
+            list_select_related = ('author', 'category')
+
+    will call ``select_related('author', 'category')``.
 
 .. attribute:: ModelAdmin.ordering
 
diff --git a/tests/admin_changelist/admin.py b/tests/admin_changelist/admin.py
index 8387ba77a1..175b1972c9 100644
--- a/tests/admin_changelist/admin.py
+++ b/tests/admin_changelist/admin.py
@@ -67,6 +67,11 @@ class ChordsBandAdmin(admin.ModelAdmin):
     list_filter = ['members']
 
 
+class InvitationAdmin(admin.ModelAdmin):
+    list_display = ('band', 'player')
+    list_select_related = ('player',)
+
+
 class DynamicListDisplayChildAdmin(admin.ModelAdmin):
     list_display = ('parent', 'name', 'age')
 
diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py
index 38b78faaf4..7f3f0d162e 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -18,7 +18,7 @@ from .admin import (ChildAdmin, QuartetAdmin, BandAdmin, ChordsBandAdmin,
     GroupAdmin, ParentAdmin, DynamicListDisplayChildAdmin,
     DynamicListDisplayLinksChildAdmin, CustomPaginationAdmin,
     FilteredChildAdmin, CustomPaginator, site as custom_site,
-    SwallowAdmin, DynamicListFilterChildAdmin)
+    SwallowAdmin, DynamicListFilterChildAdmin, InvitationAdmin)
 from .models import (Event, Child, Parent, Genre, Band, Musician, Group,
     Quartet, Membership, ChordsMusician, ChordsBand, Invitation, Swallow,
     UnorderedObject, OrderedObject, CustomIdUser)
@@ -46,9 +46,32 @@ class ChangeListTests(TestCase):
         m = ChildAdmin(Child, admin.site)
         request = self.factory.get('/child/')
         cl = ChangeList(request, Child, m.list_display, m.list_display_links,
-                m.list_filter, m.date_hierarchy, m.search_fields,
-                m.list_select_related, m.list_per_page, m.list_max_show_all, m.list_editable, m)
-        self.assertEqual(cl.queryset.query.select_related, {'parent': {'name': {}}})
+                        m.list_filter, m.date_hierarchy, m.search_fields,
+                        m.list_select_related, m.list_per_page,
+                        m.list_max_show_all, m.list_editable, m)
+        self.assertEqual(cl.queryset.query.select_related, {
+            'parent': {'name': {}}
+        })
+
+    def test_select_related_as_tuple(self):
+        ia = InvitationAdmin(Invitation, admin.site)
+        request = self.factory.get('/invitation/')
+        cl = ChangeList(request, Child, ia.list_display, ia.list_display_links,
+                        ia.list_filter, ia.date_hierarchy, ia.search_fields,
+                        ia.list_select_related, ia.list_per_page,
+                        ia.list_max_show_all, ia.list_editable, ia)
+        self.assertEqual(cl.queryset.query.select_related, {'player': {}})
+
+    def test_select_related_as_empty_tuple(self):
+        ia = InvitationAdmin(Invitation, admin.site)
+        ia.list_select_related = ()
+        request = self.factory.get('/invitation/')
+        cl = ChangeList(request, Child, ia.list_display, ia.list_display_links,
+                        ia.list_filter, ia.date_hierarchy, ia.search_fields,
+                        ia.list_select_related, ia.list_per_page,
+                        ia.list_max_show_all, ia.list_editable, ia)
+        self.assertEqual(cl.queryset.query.select_related, False)
+
 
     def test_result_list_empty_changelist_value(self):
         """
diff --git a/tests/modeladmin/tests.py b/tests/modeladmin/tests.py
index 73e88024cd..805b57c070 100644
--- a/tests/modeladmin/tests.py
+++ b/tests/modeladmin/tests.py
@@ -1161,9 +1161,11 @@ class ValidationTests(unittest.TestCase):
         class ValidationTestModelAdmin(ModelAdmin):
             list_select_related = 1
 
-        six.assertRaisesRegex(self,
+        six.assertRaisesRegex(
+            self,
             ImproperlyConfigured,
-            "'ValidationTestModelAdmin.list_select_related' should be a bool.",
+            "'ValidationTestModelAdmin.list_select_related' should be either a "
+            "bool, a tuple or a list",
             ValidationTestModelAdmin.validate,
             ValidationTestModel,
         )