Fixed #13902 -- Fixed admin list_filter so it doesn't show duplicate results…

Fixed #13902 -- Fixed admin list_filter so it doesn't show duplicate results when it includes a field spec that involves m2m relationships with an intermediate model. Thanks Iván Raskovsky for the report and patch.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@15526 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/views/main.py

@@ -169,6 +169,8 @@ class ChangeList(object):
         return order_field, order_type
 
     def get_query_set(self):
+        use_distinct = False
+
         qs = self.root_query_set
         lookup_params = self.params.copy() # a dictionary of the query string
         for i in (ALL_VAR, ORDER_VAR, ORDER_TYPE_VAR, SEARCH_VAR, IS_POPUP_VAR):
@@ -181,6 +183,17 @@ class ChangeList(object):
                 del lookup_params[key]
                 lookup_params[smart_str(key)] = value
 
+            if not use_distinct:
+                # Check if it's a relationship that might return more than one
+                # instance
+                field_name = key.split('__', 1)[0]
+                try:
+                    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
+
             # if key ends with __in, split parameter into separate values
             if key.endswith('__in'):
                 value = value.split(',')
@@ -246,12 +259,18 @@ class ChangeList(object):
             for bit in self.query.split():
                 or_queries = [models.Q(**{construct_search(str(field_name)): bit}) for field_name in self.search_fields]
                 qs = qs.filter(reduce(operator.or_, or_queries))
-            for field_name in self.search_fields:
-                if '__' in field_name:
-                    qs = qs.distinct()
-                    break
+            if not use_distinct:
+                for search_field in self.search_fields:
+                    field_name = search_field.split('__', 1)[0]
+                    f = self.lookup_opts.get_field_by_name(field_name)[0]
+                    if hasattr(f, 'rel') and isinstance(f.rel, models.ManyToManyRel):
+                        use_distinct = True
+                        break
 
-        return qs
+        if use_distinct:
+            return qs.distinct()
+        else:
+            return qs
 
     def url_for_result(self, result):
         return "%s/" % quote(getattr(result, self.pk_attname))

tests/regressiontests/admin_changelist/models.py

 from django.db import models
-from django.contrib import admin
 
 class Parent(models.Model):
     name = models.CharField(max_length=128)
@@ -7,3 +6,44 @@ class Parent(models.Model):
 class Child(models.Model):
     parent = models.ForeignKey(Parent, editable=False, null=True)
     name = models.CharField(max_length=30, blank=True)
+
+class Genre(models.Model):
+    name = models.CharField(max_length=20)
+
+class Band(models.Model):
+    name = models.CharField(max_length=20)
+    nr_of_members = models.PositiveIntegerField()
+    genres = models.ManyToManyField(Genre)
+
+class Musician(models.Model):
+    name = models.CharField(max_length=30)
+
+    def __unicode__(self):
+        return self.name
+
+class Group(models.Model):
+    name = models.CharField(max_length=30)
+    members = models.ManyToManyField(Musician, through='Membership')
+
+    def __unicode__(self):
+        return self.name
+
+class Membership(models.Model):
+    music = models.ForeignKey(Musician)
+    group = models.ForeignKey(Group)
+    role = models.CharField(max_length=15)
+
+class Quartet(Group):
+    pass
+
+class ChordsMusician(Musician):
+    pass
+
+class ChordsBand(models.Model):
+    name = models.CharField(max_length=30)
+    members = models.ManyToManyField(ChordsMusician, through='Invitation')
+
+class Invitation(models.Model):
+    player = models.ForeignKey(ChordsMusician)
+    band = models.ForeignKey(ChordsBand)
+    instrument = models.CharField(max_length=15)

tests/regressiontests/admin_changelist/tests.py

@@ -5,7 +5,8 @@ from django.core.paginator import Paginator
 from django.template import Context, Template
 from django.test import TransactionTestCase
 
-from regressiontests.admin_changelist.models import Child, Parent
+from models import (Child, Parent, Genre, Band, Musician, Group, Quartet,
+    Membership, ChordsMusician, ChordsBand, Invitation)
 
 
 class ChangeListTests(TransactionTestCase):
@@ -135,18 +136,122 @@ class ChangeListTests(TransactionTestCase):
         cl.get_results(request)
         self.assertIsInstance(cl.paginator, CustomPaginator)
 
+    def test_distinct_for_m2m_in_list_filter(self):
+        """
+        Regression test for #13902: When using a ManyToMany in list_filter,
+        results shouldn't apper more than once. Basic ManyToMany.
+        """
+        blues = Genre.objects.create(name='Blues')
+        band = Band.objects.create(name='B.B. King Review', nr_of_members=11)
+
+        band.genres.add(blues)
+        band.genres.add(blues)
+
+        m = BandAdmin(Band, admin.site)
+        cl = ChangeList(MockFilteredRequestA(), 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())
+
+        # There's only one Group instance
+        self.assertEqual(cl.result_count, 1)
+
+    def test_distinct_for_through_m2m_in_list_filter(self):
+        """
+        Regression test for #13902: When using a ManyToMany in list_filter,
+        results shouldn't apper more than once. With an intermediate model.
+        """
+        lead = Musician.objects.create(name='Vox')
+        band = Group.objects.create(name='The Hype')
+        Membership.objects.create(group=band, music=lead, role='lead voice')
+        Membership.objects.create(group=band, music=lead, role='bass player')
+
+        m = GroupAdmin(Group, admin.site)
+        cl = ChangeList(MockFilteredRequestB(), 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())
+
+        # There's only one Group 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,
+        results shouldn't apper more than once. Model managed in the
+        admin inherits from the one that defins the relationship.
+        """
+        lead = Musician.objects.create(name='John')
+        four = Quartet.objects.create(name='The Beatles')
+        Membership.objects.create(group=four, music=lead, role='lead voice')
+        Membership.objects.create(group=four, music=lead, role='guitar player')
+
+        m = QuartetAdmin(Quartet, admin.site)
+        cl = ChangeList(MockFilteredRequestB(), 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())
+
+        # There's only one Quartet instance
+        self.assertEqual(cl.result_count, 1)
+
+    def test_distinct_for_m2m_to_inherited_in_list_filter(self):
+        """
+        Regression test for #13902: When using a ManyToMany in list_filter,
+        results shouldn't apper more than once. Target of the relationship
+        inherits from another.
+        """
+        lead = ChordsMusician.objects.create(name='Player A')
+        three = ChordsBand.objects.create(name='The Chords Trio')
+        Invitation.objects.create(band=three, player=lead, instrument='guitar')
+        Invitation.objects.create(band=three, player=lead, instrument='bass')
+
+        m = ChordsBandAdmin(ChordsBand, admin.site)
+        cl = ChangeList(MockFilteredRequestB(), 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())
+
+        # There's only one ChordsBand instance
+        self.assertEqual(cl.result_count, 1)
+
 
 class ChildAdmin(admin.ModelAdmin):
     list_display = ['name', 'parent']
     def queryset(self, request):
         return super(ChildAdmin, self).queryset(request).select_related("parent__name")
 
-
 class MockRequest(object):
     GET = {}
 
-
 class CustomPaginator(Paginator):
     def __init__(self, queryset, page_size, orphans=0, allow_empty_first_page=True):
         super(CustomPaginator, self).__init__(queryset, 5, orphans=2,
             allow_empty_first_page=allow_empty_first_page)
+
+
+class BandAdmin(admin.ModelAdmin):
+    list_filter = ['genres']
+
+class GroupAdmin(admin.ModelAdmin):
+    list_filter = ['members']
+
+class QuartetAdmin(admin.ModelAdmin):
+    list_filter = ['members']
+
+class ChordsBandAdmin(admin.ModelAdmin):
+    list_filter = ['members']
+
+class MockFilteredRequestA(object):
+    GET = { 'genres': 1 }
+
+class MockFilteredRequestB(object):
+    GET = { 'members': 1 }