From 065e6b6153a2656a9d60ca7da72236de0843e6a5 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Sat, 23 Apr 2011 03:55:21 +0000 Subject: [PATCH] Fixed #15819 - Fixed 1.3 regression from r15526 causing duplicate search results in admin with search_fields traversing to non-M2M related models. Thanks to Adam Kochanowski for the report and Ryan Kaskel for the patch. git-svn-id: http://code.djangoproject.com/svn/django/trunk@16093 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/views/main.py | 14 +++- .../regressiontests/admin_changelist/tests.py | 84 +++++++++++++++---- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 170d1687871..0cfc43dd03b 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -26,6 +26,15 @@ ERROR_FLAG = 'e' # Text to display within change-list table cells if the value is blank. EMPTY_CHANGELIST_VALUE = ugettext_lazy('(None)') +def field_needs_distinct(field): + if ((hasattr(field, 'rel') and + isinstance(field.rel, models.ManyToManyRel)) or + (isinstance(field, models.related.RelatedObject) and + not field.field.unique)): + return True + return False + + class ChangeList(object): def __init__(self, request, model, list_display, list_display_links, list_filter, date_hierarchy, search_fields, list_select_related, list_per_page, list_editable, model_admin): self.model = model @@ -189,8 +198,7 @@ class ChangeList(object): f = self.lookup_opts.get_field_by_name(field_name)[0] except models.FieldDoesNotExist: raise IncorrectLookupParameters - if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel): - use_distinct = True + use_distinct = field_needs_distinct(f) # if key ends with __in, split parameter into separate values if key.endswith('__in'): @@ -264,7 +272,7 @@ class ChangeList(object): for search_spec in orm_lookups: field_name = search_spec.split('__', 1)[0] f = self.lookup_opts.get_field_by_name(field_name)[0] - if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel): + if field_needs_distinct(f): use_distinct = True break diff --git a/tests/regressiontests/admin_changelist/tests.py b/tests/regressiontests/admin_changelist/tests.py index 0e61addc118..5186508dd9c 100644 --- a/tests/regressiontests/admin_changelist/tests.py +++ b/tests/regressiontests/admin_changelist/tests.py @@ -1,6 +1,6 @@ from django.contrib import admin from django.contrib.admin.options import IncorrectLookupParameters -from django.contrib.admin.views.main import ChangeList +from django.contrib.admin.views.main import ChangeList, SEARCH_VAR from django.core.paginator import Paginator from django.template import Context, Template from django.test import TransactionTestCase @@ -148,12 +148,14 @@ class ChangeListTests(TransactionTestCase): band.genres.add(blues) m = BandAdmin(Band, admin.site) - cl = ChangeList(MockFilteredRequestA(blues.pk), Band, m.list_display, + request = MockFilterRequest('genres', blues.pk) + + cl = ChangeList(request, Band, 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_editable, m) - cl.get_results(MockFilteredRequestA(blues.pk)) + cl.get_results(request) # There's only one Group instance self.assertEqual(cl.result_count, 1) @@ -169,12 +171,14 @@ class ChangeListTests(TransactionTestCase): Membership.objects.create(group=band, music=lead, role='bass player') m = GroupAdmin(Group, admin.site) - cl = ChangeList(MockFilteredRequestB(lead.pk), Group, m.list_display, + request = MockFilterRequest('members', lead.pk) + + cl = ChangeList(request, Group, 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_editable, m) - cl.get_results(MockFilteredRequestB(lead.pk)) + cl.get_results(request) # There's only one Group instance self.assertEqual(cl.result_count, 1) @@ -191,12 +195,14 @@ class ChangeListTests(TransactionTestCase): Membership.objects.create(group=four, music=lead, role='guitar player') m = QuartetAdmin(Quartet, admin.site) - cl = ChangeList(MockFilteredRequestB(lead.pk), Quartet, m.list_display, + request = MockFilterRequest('members', lead.pk) + + cl = ChangeList(request, Quartet, 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_editable, m) - cl.get_results(MockFilteredRequestB(lead.pk)) + cl.get_results(request) # There's only one Quartet instance self.assertEqual(cl.result_count, 1) @@ -213,16 +219,59 @@ class ChangeListTests(TransactionTestCase): Invitation.objects.create(band=three, player=lead, instrument='bass') m = ChordsBandAdmin(ChordsBand, admin.site) - cl = ChangeList(MockFilteredRequestB(lead.pk), ChordsBand, m.list_display, + request = MockFilterRequest('members', lead.pk) + + cl = ChangeList(request, ChordsBand, 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_editable, m) - cl.get_results(MockFilteredRequestB(lead.pk)) + cl.get_results(request) # There's only one ChordsBand instance self.assertEqual(cl.result_count, 1) + def test_distinct_for_non_unique_related_object_in_list_filter(self): + """ + Regressions tests for #15819: If a field listed in list_filters + is a non-unique related object, distinct() must be called. + """ + parent = Parent.objects.create(name='Mary') + # Two children with the same name + Child.objects.create(parent=parent, name='Daniel') + Child.objects.create(parent=parent, name='Daniel') + + m = ParentAdmin(Parent, admin.site) + request = MockFilterRequest('child__name', 'Daniel') + + cl = ChangeList(request, Parent, 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_editable, m) + + # Make sure distinct() was called + self.assertEqual(cl.query_set.count(), 1) + + def test_distinct_for_non_unique_related_object_in_search_fields(self): + """ + Regressions tests for #15819: If a field listed in search_fields + is a non-unique related object, distinct() must be called. + """ + parent = Parent.objects.create(name='Mary') + Child.objects.create(parent=parent, name='Danielle') + Child.objects.create(parent=parent, name='Daniel') + + m = ParentAdmin(Parent, admin.site) + request = MockSearchRequest('daniel') + + cl = ChangeList(request, Parent, 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_editable, m) + + # Make sure distinct() was called + self.assertEqual(cl.query_set.count(), 1) + def test_pagination(self): """ Regression tests for #12893: Pagination in admins changelist doesn't @@ -254,6 +303,11 @@ class ChangeListTests(TransactionTestCase): self.assertEqual(cl.paginator.page_range, [1, 2, 3]) +class ParentAdmin(admin.ModelAdmin): + list_filter = ['child__name'] + search_fields = ['child__name'] + + class ChildAdmin(admin.ModelAdmin): list_display = ['name', 'parent'] list_per_page = 10 @@ -288,10 +342,10 @@ class QuartetAdmin(admin.ModelAdmin): class ChordsBandAdmin(admin.ModelAdmin): list_filter = ['members'] -class MockFilteredRequestA(object): - def __init__(self, pk): - self.GET = { 'genres' : pk } +class MockFilterRequest(object): + def __init__(self, filter, q): + self.GET = {filter: q} -class MockFilteredRequestB(object): - def __init__(self, pk): - self.GET = { 'members': pk } +class MockSearchRequest(object): + def __init__(self, q): + self.GET = {SEARCH_VAR: q}