From 0fd9f7c95f748764867dc148a2bacef807466d85 Mon Sep 17 00:00:00 2001 From: Tomek Paczkowski Date: Tue, 4 Jun 2013 23:35:11 +0200 Subject: [PATCH] Fixed #19080 -- Fine-grained control over select_related in admin --- django/contrib/admin/validation.py | 10 +++++-- django/contrib/admin/views/main.py | 44 ++++++++++++++++++------------ docs/ref/contrib/admin/index.txt | 22 +++++++++++---- tests/admin_changelist/admin.py | 5 ++++ tests/admin_changelist/tests.py | 31 ++++++++++++++++++--- tests/modeladmin/tests.py | 6 ++-- 6 files changed, 88 insertions(+), 30 deletions(-) diff --git a/django/contrib/admin/validation.py b/django/contrib/admin/validation.py index 59c5ad35ef4..222d433e53d 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 dbed21265cf..8ea7e10fc01 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 8f457e77ac3..1a5e9f20731 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 8387ba77a1e..175b1972c93 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 38b78faaf4a..7f3f0d162ea 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 73e88024cd9..805b57c0707 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, )