[Django] #33263: DeleteView in Django4.0 does not call .delete() method
Django
-----------------------------------------+------------------------
Reporter: eprikazc | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
I am giving a try to Django 4.0 on one of my projects and I see that
DeleteView.post does not call .delete() method anymore.
I understand that it was introduced when implementing the ticket
https://code.djangoproject.com/ticket/21936
particularly this line
https://github.com/django/django/commit/3a45fea0832c5910acee6e0d29f230f347a50462
#diff-bf5815bb9e60d6b9f1a261957863a70cc9ad03efdbd7941c0e1659b7ceb2895fR250
Some codebases in the wild might assume that .delete() method get always
called when DeleteView performs a deletion. I.e. .delete() method can be
seen not just as a handler for DELETE http method, but as part of public
interface of DeleteView - a place to specify what happens on object
deletion. For example, in my project I often override .delete() method to
do extra work - to make external API call or add a flash message, etc.
With Django4.0 it isn't working anymore.
If it is a bug - here is my attempt with test and fix for it -
https://github.com/django/django/pull/15055
If it is not a bug but intended behavior - perhaps it should be more
clearly documented, since it might break backwards compatibility for some
codebases.
--
Ticket URL: <https://code.djangoproject.com/ticket/33263>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:1>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
OK, so the change here was intended, and doc'd but likely needs calling
out better in the release notes.
This was a deliberate change in #21936, finally resolved in
[https://github.com/django/django/pull/14634 PR 14634] after 8 years and
(I think) 4 previous PRs.
The mainline there was:
[https://github.com/django/django/pull/2585 PR 2585] to
[https://github.com/django/django/pull/13362 PR 13362] to
[https://github.com/django/django/pull/14634 PR 14634].
The PR adjusted the inheritance structure to use `FormMixin` for `post`,
allowing the confirmation step and success message. This is mentioned in
the generic view docs and the
[https://docs.djangoproject.com/en/4.0/releases/4.0/#generic-views Django
4.0 release notes].
The interplay with `DeletionMixin` complicated the issue significantly.
This was first picked up by
[https://code.djangoproject.com/ticket/21936#comment:9 Caroline Simpson
working on the original PR].
Happy to take input but after staring at it multiple times, for a long
time, the only stable conclusion was to separate the `post()` HTTP handler
from the `delete()` HTTP handler — proxying one to the other doesn't leave
space for the form and messaging behaviour. (See the historical PRs for
attempts in this direction.) Someone sending an API-like HTTP DELETE
method request gets the old behaviour here. It's only browser based POST
requests that are affected.
Continuing to proxy `post()` to `delete()`, as the PR here suggests, ends
up duplicating the `get_object()` and `get_success_url()` calls
(essentially from the two mixing, but inlined in this case because of the
tangled inheritance structure,
[https://github.com/django/django/pull/14634/files#diff-
bf5815bb9e60d6b9f1a261957863a70cc9ad03efdbd7941c0e1659b7ceb2895fR237-R240
see comment].) Doing that is less than ideal: with the added `FormMixin`
behaviour, `post()` is a much more complex handler than `delete()` —
they're no longer equivalent.
On the final PR
[https://github.com/django/django/pull/14634#discussion_r669325533 Mariusz
and I discussed adding an extra `_delete` hook, to be called by both
`delete()` and `form_valid()` but agreed is was too much API].
The correct approach is to implement your custom logic in `form_valid`,
and per any `FormMixin` subclass. Or **if** the view is genuinely handling
DELETE HTTP requests as well as POSTs, move the shared logic into a method
called by **both** `form_valid` and `delete` (which would be the hook we
decided not to add to the built-in CBV API).
I'd suggest a small addition to the release notes here, along the lines of
`"To accommodate this change custom logic in `delete()` methods should be
moved to `form_valid()` or a shared helper method as needed."`
I hope that makes sense. As I say, open to further input, but this seemed
the minimum change that satisfied the desiderata.
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:2>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> I'd suggest a small addition to the release notes here...
Pull request to that effect in https://github.com/django/django/pull/15059
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:3>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Eugene Prikazchikov):
I understand that with new approach you need to move that kind of logic to
`form_valid()`, not to 'delete()` and I like that idea.
I was lucky enough to detect the broken behavior early on - the project is
covered by unit tests quite well and tests started to fail.
When migrating project without much tests, you can easily miss it, your
delete method will just be silently ignored. In the worst case you will
know about the problem only when your users start to complain.
I am wondering if we could make it more obvious to developers that
behavior has changed and their existing code **must** be updated.
Normally such breaking changes should be taken through deprecation
process, no?
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:4>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> Normally such breaking changes should be taken through deprecation
process, no?
Yes. I'm wondering what one would look like? Hmmm 🤔
We could add an **additional** note to breaking changes.
(I think the new features justify the breakage, so I'd not want to revert
to maintain BC here.)
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:5>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Eugene Prikazchikov):
By deprecation process I meant that initially behavior is kept the same,
but the warning is emitted in runtime. In our case, when `.delete` method
is reached during `POST` request, we could emit a warning like "the logic
from .delete() method must be moved to .form_delete()". Thus, people
should have enough time to update their code.
And then in one of following releases we change the behavior. Like
described here - https://docs.djangoproject.com/en/3.2/howto/upgrade-
version/#resolving-deprecation-warnings
Though perhaps a note in **backwards incompatible changes** would be
enough (like https://docs.djangoproject.com/en/3.2/releases/1.11
/#backwards-incompatible-1-11)
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:6>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Yes, thanks — what I meant was that having a deprecation here stops us
adopting the new behaviour. (What I can't see if a way of having it both
ways, defaulting to the new behaviour but falling back to the old... — I
guess it'd be possible.)
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:7>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Eugene — I've added [https://github.com/django/django/pull/15059/files
an additional backwards incompatible change note on the PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:8>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Eugene Prikazchikov):
OK, that sounds good.
As for:
> a deprecation here stops us adopting the new behaviour.
It is not quite true. In the patch that I linked in the ticket new
behavior is preserved - DeleteView still inherits from `FormMixin` and
`form_valid` is still executed. All checks passed - so behavior has not
changed. To prevent existing Delete views overriding `delete` method from
breaking, in this patch `form_valid()` calls `delete()`. If we want to add
deprecation warning we could extend the patch. In `DeletionMixin.delete`
we could emit a warning if HTTP method is "POST". Here:
https://github.com/django/django/pull/15055/files#diff-
bf5815bb9e60d6b9f1a261957863a70cc9ad03efdbd7941c0e1659b7ceb2895fR211
{{{
if request.method == "POST":
# we arrived here from .form_valid(), let's emit warning
}}}
Anyway, if you believe that updating release notes is enough - I am fine
with that.
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:9>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Let's see what others say.
I think calling `delete()` isn't error free, since it re-calls the view-
dispatch methods, `get_object()` and `get_success_url()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:10>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Eugene Prikazchikov):
Yes, I see that `get_object()` and `get_success_url()` get called twice.
Not ideal, yet that should be harmless - "get_" methods usually do not
have side effects,
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:11>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Fetching from the database is a bit of a side-effect I’d argue. 😃
But **also** as proposed the default implementation would raise a warning,
so I’d have to override `form_valid` to silence that, only to delete that
at the end of the deprecation period.
We can back and forth all day, let’s see what others say.
In any case, thanks for the report!
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:12>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
I agree with Carlton, if we want to move forward with this feature we need
to make a breaking change, it's justified. We have many previous attempts
and 8 years of discussions, and it seems to me that the chosen path is the
best one. Extra clarifications in release notes should suffice.
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:13>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I've audited my different projects and saw that almost all of them are
affected. I'm afraid this might break badly some apps.
Isn't it possible to detect if a subclass is overriding `delete` and warn
in that case?
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:14>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:14 Claude Paroz]:
> Isn't it possible to detect if a subclass is overriding `delete` and
warn in that case?
Yes it should be possible, Can you take a look at
[https://github.com/django/django/pull/15063 PR]?
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:15>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2c01ebb4be5d53cbf6450f356c10e436025d6d07" 2c01ebb]:
{{{
#!CommitTicketReference repository=""
revision="2c01ebb4be5d53cbf6450f356c10e436025d6d07"
Refs #33263 -- Expanded release notes for DeleteView adopting FormMixin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:16>
Django
Reporter: Eugene Prikazchikov | Owner: nobody
Type: Bug | Status: new
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3151daaa6ca31ebbb78cf178a8834a5fdf495d65" 3151daaa]:
{{{
#!CommitTicketReference repository=""
revision="3151daaa6ca31ebbb78cf178a8834a5fdf495d65"
[4.0.x] Refs #33263 -- Expanded release notes for DeleteView adopting
FormMixin.
Backport of 2c01ebb4be5d53cbf6450f356c10e436025d6d07 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:17>
Django
Reporter: Eugene Prikazchikov | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: nobody => Mariusz Felisiak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:18>
Django
Reporter: Eugene Prikazchikov | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"6bc437c0d82675ebe6aa92c8e249892205c316ef" 6bc437c0]:
{{{
#!CommitTicketReference repository=""
revision="6bc437c0d82675ebe6aa92c8e249892205c316ef"
Refs #33263 -- Added warning to BaseDeleteView when delete() method is
overridden.
Follow up to 3a45fea0832c5910acee6e0d29f230f347a50462.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:19>
Django
Reporter: Eugene Prikazchikov | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"45de30dc693fcd63d33816a85fb02abd5f7a10b4" 45de30dc]:
{{{
#!CommitTicketReference repository=""
revision="45de30dc693fcd63d33816a85fb02abd5f7a10b4"
[4.0.x] Refs #33263 -- Added warning to BaseDeleteView when delete()
method is overridden.
Follow up to 3a45fea0832c5910acee6e0d29f230f347a50462.
Backport of 6bc437c0d82675ebe6aa92c8e249892205c316ef from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:20>
Django
Reporter: Eugene Prikazchikov | Owner: Mariusz
| Felisiak
Component: Generic views | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:21>
Django
Reporter: Eugene Prikazchikov | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"003081468e28db5af4d7c983f4d8df4c74c20607" 0030814]:
{{{
#!CommitTicketReference repository=""
revision="003081468e28db5af4d7c983f4d8df4c74c20607"
Refs #33263 -- Removed warning in BaseDeleteView when delete() method is
overridden.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33263#comment:22>