From d084176cc1273d5faf6f88eedb4c490e961f3a68 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud Date: Tue, 14 Apr 2015 11:09:58 +0200 Subject: [PATCH] Fixed #16609 -- Fixed duplicate admin results when searching nested M2M relations. This was fixed earlier but only when the M2M relation was at the first level on the object. This commit fixes the issue even when the M2M is at deeper levels, such as behind a foreign key. --- django/contrib/admin/utils.py | 19 ++++++++--- tests/admin_changelist/admin.py | 5 +++ tests/admin_changelist/models.py | 5 +++ tests/admin_changelist/tests.py | 58 +++++++++++++++++++++++++++++--- 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py index f77d848f64..1a4f76edfb 100644 --- a/django/contrib/admin/utils.py +++ b/django/contrib/admin/utils.py @@ -10,6 +10,7 @@ from django.core.urlresolvers import NoReverseMatch, reverse from django.db import models from django.db.models.constants import LOOKUP_SEP from django.db.models.deletion import Collector +from django.db.models.sql.constants import QUERY_TERMS from django.forms.forms import pretty_name from django.utils import formats, six, timezone from django.utils.encoding import force_str, force_text, smart_text @@ -22,10 +23,20 @@ def lookup_needs_distinct(opts, lookup_path): """ Returns True if 'distinct()' should be used to query the given lookup path. """ - field_name = lookup_path.split('__', 1)[0] - field = opts.get_field(field_name) - if hasattr(field, 'get_path_info') and any(path.m2m for path in field.get_path_info()): - return True + lookup_fields = lookup_path.split('__') + # Remove the last item of the lookup path if it is a query term + if lookup_fields[-1] in QUERY_TERMS: + lookup_fields = lookup_fields[:-1] + # Now go through the fields (following all relations) and look for an m2m + for field_name in lookup_fields: + field = opts.get_field(field_name) + if hasattr(field, 'get_path_info'): + # This field is a relation, update opts to follow the relation + path_info = field.get_path_info() + opts = path_info[-1].to_opts + if any(path.m2m for path in path_info): + # This field is a m2m relation so we know we need to call distinct + return True return False diff --git a/tests/admin_changelist/admin.py b/tests/admin_changelist/admin.py index 926a45d518..bae857f30c 100644 --- a/tests/admin_changelist/admin.py +++ b/tests/admin_changelist/admin.py @@ -60,6 +60,11 @@ class GroupAdmin(admin.ModelAdmin): list_filter = ['members'] +class ConcertAdmin(admin.ModelAdmin): + list_filter = ['group__members'] + search_fields = ['group__members__name'] + + class QuartetAdmin(admin.ModelAdmin): list_filter = ['members'] diff --git a/tests/admin_changelist/models.py b/tests/admin_changelist/models.py index 76249b2cd3..2b8d2bc7f7 100644 --- a/tests/admin_changelist/models.py +++ b/tests/admin_changelist/models.py @@ -44,6 +44,11 @@ class Group(models.Model): return self.name +class Concert(models.Model): + name = models.CharField(max_length=30) + group = models.ForeignKey(Group) + + class Membership(models.Model): music = models.ForeignKey(Musician) group = models.ForeignKey(Group) diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 7342e45830..ec8018e8af 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -17,17 +17,17 @@ from django.test.client import RequestFactory from django.utils import formats, six from .admin import ( - BandAdmin, ChildAdmin, ChordsBandAdmin, CustomPaginationAdmin, - CustomPaginator, DynamicListDisplayChildAdmin, + BandAdmin, ChildAdmin, ChordsBandAdmin, ConcertAdmin, + CustomPaginationAdmin, CustomPaginator, DynamicListDisplayChildAdmin, DynamicListDisplayLinksChildAdmin, DynamicListFilterChildAdmin, DynamicSearchFieldsChildAdmin, FilteredChildAdmin, GroupAdmin, InvitationAdmin, NoListDisplayLinksParentAdmin, ParentAdmin, QuartetAdmin, SwallowAdmin, site as custom_site, ) from .models import ( - Band, Child, ChordsBand, ChordsMusician, CustomIdUser, Event, Genre, Group, - Invitation, Membership, Musician, OrderedObject, Parent, Quartet, Swallow, - UnorderedObject, + Band, Child, ChordsBand, ChordsMusician, Concert, CustomIdUser, Event, + Genre, Group, Invitation, Membership, Musician, OrderedObject, Parent, + Quartet, Swallow, UnorderedObject, ) @@ -259,6 +259,31 @@ class ChangeListTests(TestCase): # There's only one Group instance self.assertEqual(cl.result_count, 1) + def test_distinct_for_through_m2m_at_second_level_in_list_filter(self): + """ + When using a ManyToMany in list_filter at the second level behind a + ForeignKey, distinct() must be called and results shouldn't appear more + than once. + """ + lead = Musician.objects.create(name='Vox') + band = Group.objects.create(name='The Hype') + Concert.objects.create(name='Woodstock', group=band) + Membership.objects.create(group=band, music=lead, role='lead voice') + Membership.objects.create(group=band, music=lead, role='bass player') + + m = ConcertAdmin(Concert, admin.site) + request = self.factory.get('/concert/', data={'group__members': lead.pk}) + + cl = ChangeList(request, Concert, 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) + + cl.get_results(request) + + # There's only one Concert instance + self.assertEqual(cl.result_count, 1) + def test_distinct_for_inherited_m2m_in_list_filter(self): """ Regression test for #13902: When using a ManyToMany in list_filter, @@ -348,6 +373,29 @@ class ChangeListTests(TestCase): # Make sure distinct() was called self.assertEqual(cl.queryset.count(), 1) + def test_distinct_for_many_to_many_at_second_level_in_search_fields(self): + """ + When using a ManyToMany in search_fields at the second level behind a + ForeignKey, distinct() must be called and results shouldn't appear more + than once. + """ + lead = Musician.objects.create(name='Vox') + band = Group.objects.create(name='The Hype') + Concert.objects.create(name='Woodstock', group=band) + Membership.objects.create(group=band, music=lead, role='lead voice') + Membership.objects.create(group=band, music=lead, role='bass player') + + m = ConcertAdmin(Concert, admin.site) + request = self.factory.get('/concert/', data={SEARCH_VAR: 'vox'}) + + cl = ChangeList(request, Concert, 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) + + # There's only one Concert instance + self.assertEqual(cl.queryset.count(), 1) + def test_pagination(self): """ Regression tests for #12893: Pagination in admins changelist doesn't