p15693087
/
django
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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 6307c3f1a1
revealed a possible data loss issue in the admin.
This commit is contained in:
Mariusz Felisiak 2021-04-26 09:22:46 +02:00
parent cd74aad90e
commit 1871182031
3 changed files with 70 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
django/contrib/admin/views/main.py
View File

@ -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,8 +484,16 @@ class ChangeList:
)
# Remove duplicates from results, if necessary
if filters_may_have_duplicates | search_may_have_duplicates:
return qs.distinct()
else:
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):

6
docs/releases/3.2.1.txt
View File

@ -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.

73
tests/admin_changelist/tests.py
View File

@ -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):
"""