From d569c1dcfeb26ca9ee391e5dfeadedf2b5ed4253 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Tue, 4 Jul 2023 17:45:54 -0400 Subject: [PATCH] Fixed #34639 -- Reverted "Fixed #32682 -- Made admin changelist use Exists() instead of distinct() for preventing duplicates." This reverts commit 187118203197801c6cb72dc8b06b714b23b6dd3d which moved to using Exists() instead due to an overly strict distinct().delete() check added in #32433. --- django/contrib/admin/views/main.py | 23 +++++++------ tests/admin_changelist/tests.py | 52 ++++++++++++------------------ 2 files changed, 32 insertions(+), 43 deletions(-) diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index c659474e7d..44001f00f9 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -29,7 +29,7 @@ from django.core.exceptions import ( SuspiciousOperation, ) from django.core.paginator import InvalidPage -from django.db.models import Exists, F, Field, ManyToOneRel, OrderBy, OuterRef +from django.db.models import F, Field, ManyToOneRel, OrderBy from django.db.models.expressions import Combinable from django.urls import reverse from django.utils.deprecation import RemovedInDjango60Warning @@ -566,6 +566,13 @@ class ChangeList: # ValueError, ValidationError, or ?. raise IncorrectLookupParameters(e) + if not qs.query.select_related: + qs = self.apply_select_related(qs) + + # Set ordering. + ordering = self.get_ordering(request, qs) + qs = qs.order_by(*ordering) + # Apply search results qs, search_may_have_duplicates = self.model_admin.get_search_results( request, @@ -580,17 +587,9 @@ class ChangeList: ) # Remove duplicates from results, if necessary if filters_may_have_duplicates | search_may_have_duplicates: - qs = qs.filter(pk=OuterRef("pk")) - qs = self.root_queryset.filter(Exists(qs)) - - # Set ordering. - ordering = self.get_ordering(request, qs) - qs = qs.order_by(*ordering) - - if not qs.query.select_related: - qs = self.apply_select_related(qs) - - return qs + return qs.distinct() + else: + return qs def apply_select_related(self, qs): if self.list_select_related is True: diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index aabe8bedc8..176b49c66c 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -467,7 +467,7 @@ class ChangeListTests(TestCase): cl.get_results(request) self.assertIsInstance(cl.paginator, CustomPaginator) - def test_no_duplicates_for_m2m_in_list_filter(self): + def test_distinct_for_m2m_in_list_filter(self): """ Regression test for #13902: When using a ManyToMany in list_filter, results shouldn't appear more than once. Basic ManyToMany. @@ -488,11 +488,10 @@ class ChangeListTests(TestCase): # There's only one Group instance self.assertEqual(cl.result_count, 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_through_m2m_in_list_filter(self): + def test_distinct_for_through_m2m_in_list_filter(self): """ Regression test for #13902: When using a ManyToMany in list_filter, results shouldn't appear more than once. With an intermediate model. @@ -512,14 +511,14 @@ class ChangeListTests(TestCase): # There's only one Group instance self.assertEqual(cl.result_count, 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_through_m2m_at_second_level_in_list_filter(self): + 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, results shouldn't appear more than once. + 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") @@ -537,11 +536,10 @@ class ChangeListTests(TestCase): # There's only one Concert instance self.assertEqual(cl.result_count, 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_inherited_m2m_in_list_filter(self): + def test_distinct_for_inherited_m2m_in_list_filter(self): """ Regression test for #13902: When using a ManyToMany in list_filter, results shouldn't appear more than once. Model managed in the @@ -562,11 +560,10 @@ class ChangeListTests(TestCase): # There's only one Quartet instance self.assertEqual(cl.result_count, 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_m2m_to_inherited_in_list_filter(self): + 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 appear more than once. Target of the relationship @@ -586,15 +583,11 @@ class ChangeListTests(TestCase): # There's only one ChordsBand instance self.assertEqual(cl.result_count, 1) - # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) - cl.queryset.delete() - self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_non_unique_related_object_in_list_filter(self): + 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, results shouldn't appear more than once. + 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 @@ -606,10 +599,9 @@ class ChangeListTests(TestCase): request.user = self.superuser cl = m.get_changelist_instance(request) - # Exists() is applied. + # Make sure distinct() was called self.assertEqual(cl.queryset.count(), 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) @@ -629,10 +621,10 @@ class ChangeListTests(TestCase): self.assertEqual(1, len(messages)) self.assertEqual(error, messages[0]) - def test_no_duplicates_for_non_unique_related_object_in_search_fields(self): + 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, Exists() must be applied. + is a non-unique related object, distinct() must be called. """ parent = Parent.objects.create(name="Mary") Child.objects.create(parent=parent, name="Danielle") @@ -643,17 +635,16 @@ class ChangeListTests(TestCase): request.user = self.superuser cl = m.get_changelist_instance(request) - # Exists() is applied. + # Make sure distinct() was called self.assertEqual(cl.queryset.count(), 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) - def test_no_duplicates_for_many_to_many_at_second_level_in_search_fields(self): + 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, Exists() must be applied and results shouldn't appear more + ForeignKey, distinct() must be called and results shouldn't appear more than once. """ lead = Musician.objects.create(name="Vox") @@ -670,7 +661,6 @@ class ChangeListTests(TestCase): # There's only one Concert instance self.assertEqual(cl.queryset.count(), 1) # Queryset must be deletable. - self.assertIs(cl.queryset.query.distinct, False) cl.queryset.delete() self.assertEqual(cl.queryset.count(), 0) @@ -820,23 +810,23 @@ class ChangeListTests(TestCase): cl = m.get_changelist_instance(request) self.assertCountEqual(cl.queryset, [abcd]) - def test_no_exists_for_m2m_in_list_filter_without_params(self): + def test_no_distinct_for_m2m_in_list_filter_without_params(self): """ If a ManyToManyField is in list_filter but isn't in any lookup params, - the changelist's query shouldn't have Exists(). + the changelist's query shouldn't have distinct. """ m = BandAdmin(Band, custom_site) for lookup_params in ({}, {"name": "test"}): request = self.factory.get("/band/", lookup_params) request.user = self.superuser cl = m.get_changelist_instance(request) - self.assertNotIn(" EXISTS", str(cl.queryset.query)) + self.assertIs(cl.queryset.query.distinct, False) - # A ManyToManyField in params does have Exists() applied. + # A ManyToManyField in params does have distinct applied. request = self.factory.get("/band/", {"genres": "0"}) request.user = self.superuser cl = m.get_changelist_instance(request) - self.assertIn(" EXISTS", str(cl.queryset.query)) + self.assertIs(cl.queryset.query.distinct, True) def test_pagination(self): """