summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMariusz Felisiak <felisiak.mariusz@gmail.com>2021-04-26 09:22:46 +0200
committerMariusz Felisiak <felisiak.mariusz@gmail.com>2021-04-27 10:39:55 +0200
commit34981f399a68b633682e0f6a3ded2e31ffd6d243 (patch)
tree697d788e86de40d9bd623e8ccc09c4cfcfc28557
parentfbea64b8ce6a82dd34b1f78cb884306455106185 (diff)
[3.2.x] Fixed #32682 -- Made admin changelist use Exists() instead of distinct() for preventing duplicates.
Thanks Zain Patel for the report and Simon Charette for reviews. The exception introduced in 6307c3f1a123f5975c73b231e8ac4f115fd72c0d revealed a possible data loss issue in the admin. Backport of 187118203197801c6cb72dc8b06b714b23b6dd3d from main
-rw-r--r--django/contrib/admin/views/main.py23
-rw-r--r--docs/releases/3.2.1.txt6
-rw-r--r--tests/admin_changelist/tests.py73
3 files changed, 70 insertions, 32 deletions
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 8da1c2f799..c53e401577 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -285,7 +285,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.
@@ -305,8 +305,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.
@@ -325,12 +329,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')
@@ -347,8 +354,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
@@ -368,8 +379,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
@@ -389,11 +404,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
@@ -405,8 +424,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)
@@ -424,10 +447,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')
@@ -438,13 +461,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')
@@ -460,6 +487,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')
@@ -552,23 +583,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):
"""