[Django] #32682: Deleting objects after searching related many to many field crashes the admin page
Django
admin page
-------------------------------------+-------------------------------------
Reporter: Zain | Owner: nobody
Patel |
Type: Bug | Status: new
Component: | Version: 3.2
contrib.admin | Keywords: regression, admin,
Severity: Normal | delete
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Minimal reproduction:
{{{
# models.py
class Post(models.Model):
title = models.String(...)
authors = models.ManyToMany("User", ...)
class User(models.Model):
email = models.String(...)
# admin.py
class PostAdmin(admin.ModelAdmin):
search_fields = ("title", "authors__email")
}}}
then opening the admin site, opening the post page that contains only one
post (any title and author assigned) and entering a search term (e.g the
first 2 characters of the title), selecting the post and then using the
delete action results in an Internal Sever Error 500 with an error/stack-
trace:
{{{
Internal Server Error: /admin/post/post/
Traceback (most recent call last):
File "...lib/python3.7/site-packages/django/core/handlers/exception.py",
line 47, in inner
response = get_response(request)
File "...lib/python3.7/site-packages/django/core/handlers/base.py", line
181, in _get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "...lib/python3.7/site-packages/django/contrib/admin/options.py",
line 616, in wrapper
return self.admin_site.admin_view(view)(*args, **kwargs)
File "...lib/python3.7/site-packages/django/utils/decorators.py", line
130, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "...lib/python3.7/site-packages/django/views/decorators/cache.py",
line 44, in _wrapped_view_func
response = view_func(request, *args, **kwargs)
File "...lib/python3.7/site-packages/django/contrib/admin/sites.py",
line 241, in inner
return view(request, *args, **kwargs)
File "...lib/python3.7/site-packages/django/utils/decorators.py", line
43, in _wrapper
return bound_method(*args, **kwargs)
File "...lib/python3.7/site-packages/django/utils/decorators.py", line
130, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "...lib/python3.7/site-packages/django/contrib/admin/options.py",
line 1737, in changelist_view
response = self.response_action(request,
queryset=cl.get_queryset(request))
File "...lib/python3.7/site-packages/django/contrib/admin/options.py",
line 1406, in response_action
response = func(self, request, queryset)
File "...lib/python3.7/site-packages/django/contrib/admin/actions.py",
line 45, in delete_selected
modeladmin.delete_queryset(request, queryset)
File "...lib/python3.7/site-packages/django/contrib/admin/options.py",
line 1107, in delete_queryset
queryset.delete()
File "...lib/python3.7/site-packages/django/db/models/query.py", line
728, in delete
raise TypeError('Cannot call delete() after .distinct().')
TypeError: Cannot call delete() after .distinct().
"POST /admin/post/post/?q=my HTTP/1.1" 500 137654
}}}
I can confirm that `pip install django==3.1.8` fixes the error, and after
having a look at the diff between stable/3.2.x and 3.1.8, I suspect the
"regression" comes about from the work done on preserving the filters on
delete or something along those lines - I haven't done a thorough
investigation yet. Presumably `.distinct()` is being called because of the
search involving the many to many field.
I am using a Postgres database.
--
Ticket URL: <https://code.djangoproject.com/ticket/32682>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution:
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mariusz Felisiak
* status: new => assigned
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
This exception was introduce in 6307c3f1a123f5975c73b231e8ac4f115fd72c0d
and revealed a possible data loss issue in the admin. IMO we should use
`Exists()` instead of `distinct()`, e.g.
{{{
diff --git a/django/contrib/admin/views/main.py
b/django/contrib/admin/views/main.py
index fefed29933..e9816ddd15 100644
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -475,9 +475,8 @@ class ChangeList:
if not qs.query.select_related:
qs = self.apply_select_related(qs)
- # Set ordering.
+ # Get ordering.
ordering = self.get_ordering(request, qs)
- qs = qs.order_by(*ordering)
# Apply search results
qs, search_use_distinct =
self.model_admin.get_search_results(request, qs, self.query)
@@ -487,11 +486,14 @@ class ChangeList:
new_params=remaining_lookup_params,
remove=self.get_filters_params(),
)
+
# Remove duplicates from results, if necessary
if filters_use_distinct | search_use_distinct:
- return qs.distinct()
+ from django.db.models import Exists, OuterRef
+ qs = qs.filter(pk=OuterRef('pk'))
+ return
self.root_queryset.filter(Exists(qs)).order_by(*ordering)
else:
- return qs
+ return qs.order_by(*ordering)
def apply_select_related(self, qs):
if self.list_select_related is True:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:1>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution:
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14313 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:2>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"187118203197801c6cb72dc8b06b714b23b6dd3d" 18711820]:
{{{
#!CommitTicketReference repository=""
revision="187118203197801c6cb72dc8b06b714b23b6dd3d"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:5>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution:
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"cd74aad90e09865ae6cd8ca0377ef0a5008d14e9" cd74aad]:
{{{
#!CommitTicketReference repository=""
revision="cd74aad90e09865ae6cd8ca0377ef0a5008d14e9"
Refs #32682 -- Renamed use_distinct variable to may_have_duplicates.
QuerySet.distinct() is not the only way to avoid duplicate, it's also
not preferred.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:4>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution:
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4074f38e1dcc93b859bbbfd6abd8441c3bca36b3" 4074f38e]:
{{{
#!CommitTicketReference repository=""
revision="4074f38e1dcc93b859bbbfd6abd8441c3bca36b3"
Refs #32682 -- Fixed QuerySet.delete() crash on querysets with self-
referential subqueries on MySQL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:3>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"7ad7034054c10bfa919ceec4623fa7f30a68bba2" 7ad70340]:
{{{
#!CommitTicketReference repository=""
revision="7ad7034054c10bfa919ceec4623fa7f30a68bba2"
[3.2.x] Refs #32682 -- Fixed QuerySet.delete() crash on querysets with
self-referential subqueries on MySQL.
Backport of 4074f38e1dcc93b859bbbfd6abd8441c3bca36b3 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:6>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"34981f399a68b633682e0f6a3ded2e31ffd6d243" 34981f39]:
{{{
#!CommitTicketReference repository=""
revision="34981f399a68b633682e0f6a3ded2e31ffd6d243"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:8>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"fbea64b8ce6a82dd34b1f78cb884306455106185" fbea64b]:
{{{
#!CommitTicketReference repository=""
revision="fbea64b8ce6a82dd34b1f78cb884306455106185"
[3.2.x] Refs #32682 -- Renamed use_distinct variable to
may_have_duplicates.
QuerySet.distinct() is not the only way to avoid duplicate, it's also
not preferred.
Backport of cd74aad90e09865ae6cd8ca0377ef0a5008d14e9 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:7>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"baba733dccadf1bdd2a53e5bc9ec7a3ac7ade589" baba733d]:
{{{
#!CommitTicketReference repository=""
revision="baba733dccadf1bdd2a53e5bc9ec7a3ac7ade589"
Refs #32682 -- Renamed lookup_needs_distinct() to
lookup_spawns_duplicates().
Follow up to 187118203197801c6cb72dc8b06b714b23b6dd3d.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:9>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d569c1dcfeb26ca9ee391e5dfeadedf2b5ed4253" d569c1dc]:
{{{
#!CommitTicketReference repository=""
revision="d569c1dcfeb26ca9ee391e5dfeadedf2b5ed4253"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:11>
Django
admin page
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Release blocker | Resolution: fixed
Keywords: regression, admin, | Triage Stage: Accepted
delete |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"28e2077148f7602d29165e90965974698819cbba" 28e20771]:
{{{
#!CommitTicketReference repository=""
revision="28e2077148f7602d29165e90965974698819cbba"
Refs #32433 -- Reallowed calling QuerySet.delete() after distinct().
While values(*field_excluding_pk).distinct() and
distinct(*field_excluding_pk) can reduce the number of resulting rows
in a way that makes subsequent delete() calls ambiguous standalone
.distinct() calls cannot.
Since delete() already disallows chain usages with values() the only
case that needs to be handled, as originally reported, is when
DISTINCT ON is used via distinct(*fields).
Refs #32682 which had to resort to subqueries to prevent duplicates in
the admin and caused significant performance regressions on MySQL
(refs #34639).
This partly reverts 6307c3f1a123f5975c73b231e8ac4f115fd72c0d.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32682#comment:10>