Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#32682 closed Bug (fixed)

Deleting objects after searching related many to many field crashes the admin page

Reported by: Zain Patel Owned by: Mariusz Felisiak
Component: contrib.admin Version: 3.2
Severity: Release blocker Keywords: regression, admin, delete
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 Changed 7 months ago by Mariusz Felisiak

Owner: changed from nobody to Mariusz Felisiak
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

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:

comment:2 Changed 7 months ago by Mariusz Felisiak

Has patch: set

comment:3 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 4074f38e:

Refs #32682 -- Fixed QuerySet.delete() crash on querysets with self-referential subqueries on MySQL.

comment:4 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In cd74aad:

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.

comment:5 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 18711820:

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.

comment:6 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7ad70340:

[3.2.x] Refs #32682 -- Fixed QuerySet.delete() crash on querysets with self-referential subqueries on MySQL.

Backport of 4074f38e1dcc93b859bbbfd6abd8441c3bca36b3 from main

comment:7 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In fbea64b:

[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

comment:8 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 34981f39:

[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

comment:9 Changed 7 months ago by GitHub <noreply@…>

In baba733d:

Refs #32682 -- Renamed lookup_needs_distinct() to lookup_spawns_duplicates().

Follow up to 187118203197801c6cb72dc8b06b714b23b6dd3d.

Note: See TracTickets for help on using tickets.
Back to Top