diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index dc43e7849d..a54ef25f23 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -17,7 +17,7 @@ from django.core.exceptions import ( FieldDoesNotExist, ImproperlyConfigured, SuspiciousOperation, ) from django.core.paginator import InvalidPage -from django.db.models import F, Field, ManyToOneRel, OrderBy +from django.db.models import Exists, F, Field, ManyToOneRel, OrderBy, OuterRef from django.db.models.expressions import Combinable from django.urls import reverse from django.utils.http import urlencode @@ -472,13 +472,6 @@ 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, qs, self.query, @@ -491,9 +484,17 @@ class ChangeList: ) # Remove duplicates from results, if necessary if filters_may_have_duplicates | search_may_have_duplicates: - return qs.distinct() - else: - return qs + 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 def apply_select_related(self, qs): if self.list_select_related is True: diff --git a/docs/releases/3.2.1.txt b/docs/releases/3.2.1.txt index f99dacac0c..83d317bc7e 100644 --- a/docs/releases/3.2.1.txt +++ b/docs/releases/3.2.1.txt @@ -59,3 +59,9 @@ Bugfixes * Fixed a bug in Django 3.2 where variable lookup errors were logged when rendering some admin templates (:ticket:`32681`). + +* Fixed a bug in Django 3.2 where an admin changelist would crash when deleting + objects filtered against multi-valued relationships (:ticket:`32682`). The + admin changelist now uses ``Exists()`` instead ``QuerySet.distinct()`` + because calling ``delete()`` after ``distinct()`` is not allowed in Django + 3.2 to address a data loss possibility. diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index dc1b987a1e..c5910d6bdb 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -299,7 +299,7 @@ class ChangeListTests(TestCase): cl.get_results(request) self.assertIsInstance(cl.paginator, CustomPaginator) - def test_distinct_for_m2m_in_list_filter(self): + def test_no_duplicates_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. @@ -319,8 +319,12 @@ 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_distinct_for_through_m2m_in_list_filter(self): + def test_no_duplicates_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. @@ -339,12 +343,15 @@ 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_distinct_for_through_m2m_at_second_level_in_list_filter(self): + def test_no_duplicates_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. + ForeignKey, results shouldn't appear more than once. """ lead = Musician.objects.create(name='Vox') band = Group.objects.create(name='The Hype') @@ -361,8 +368,12 @@ 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_distinct_for_inherited_m2m_in_list_filter(self): + def test_no_duplicates_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 @@ -382,8 +393,12 @@ 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_distinct_for_m2m_to_inherited_in_list_filter(self): + def test_no_duplicates_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 @@ -403,11 +418,15 @@ 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_distinct_for_non_unique_related_object_in_list_filter(self): + def test_no_duplicates_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. + Regressions tests for #15819: If a field listed in list_filters is a + non-unique related object, results shouldn't appear more than once. """ parent = Parent.objects.create(name='Mary') # Two children with the same name @@ -419,8 +438,12 @@ class ChangeListTests(TestCase): request.user = self.superuser cl = m.get_changelist_instance(request) - # Make sure distinct() was called + # Exists() is applied. 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_changelist_search_form_validation(self): m = ConcertAdmin(Concert, custom_site) @@ -438,10 +461,10 @@ class ChangeListTests(TestCase): self.assertEqual(1, len(messages)) self.assertEqual(error, messages[0]) - def test_distinct_for_non_unique_related_object_in_search_fields(self): + def test_no_duplicates_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. + is a non-unique related object, Exists() must be applied. """ parent = Parent.objects.create(name='Mary') Child.objects.create(parent=parent, name='Danielle') @@ -452,13 +475,17 @@ class ChangeListTests(TestCase): request.user = self.superuser cl = m.get_changelist_instance(request) - # Make sure distinct() was called + # Exists() is applied. 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_distinct_for_many_to_many_at_second_level_in_search_fields(self): + def test_no_duplicates_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 + ForeignKey, Exists() must be applied and results shouldn't appear more than once. """ lead = Musician.objects.create(name='Vox') @@ -474,6 +501,10 @@ class ChangeListTests(TestCase): cl = m.get_changelist_instance(request) # 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) def test_pk_in_search_fields(self): band = Group.objects.create(name='The Hype') @@ -566,23 +597,23 @@ class ChangeListTests(TestCase): cl = m.get_changelist_instance(request) self.assertCountEqual(cl.queryset, [abcd]) - def test_no_distinct_for_m2m_in_list_filter_without_params(self): + def test_no_exists_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 distinct. + the changelist's query shouldn't have Exists(). """ 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.assertFalse(cl.queryset.query.distinct) + self.assertNotIn(' EXISTS', str(cl.queryset.query)) - # A ManyToManyField in params does have distinct applied. + # A ManyToManyField in params does have Exists() applied. request = self.factory.get('/band/', {'genres': '0'}) request.user = self.superuser cl = m.get_changelist_instance(request) - self.assertTrue(cl.queryset.query.distinct) + self.assertIn(' EXISTS', str(cl.queryset.query)) def test_pagination(self): """