[Django] #23076: Cascaded deletion of polymorphic models fails
Django
----------------------------------------------+--------------------
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Currently, when deleting polymorphic models (for example by using `django-
polymorphic`), the `django.db.models.deletion.Collector` incorrectly
assumes that all models are of the same type as the first model in the
list. Then, it generates incorrect delete statements when polymorphic
models are used which then result in an `IntegrityError` when the
transaction is committed.
This is a patch that applies to 1.6.x and master (see pull request
[https://github.com/django/django/pull/2940 2940]) that fixes this issue
and it is directly based on a patch submitted by Kronuz at the end of
ticket #16128.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mitar):
* cc: mmitar@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:1>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by slurms):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:2>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by timo):
The related issue in django-polymorphic seems to be
https://github.com/chrisglass/django_polymorphic/issues/34.
I'd like someone with more knowledge of the ORM to review this and comment
on the solution. It's difficult for me to tell from the patch, but I guess
there may be a performance penalty if we don't assume that all objects are
of the same type.
The patch obviously requires tests in order to be merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:3>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Accepting on the basis of the problem. Not sure about the solution and the
patch still needs tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:4>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
I don't think this is a bug in Django, or Django's problem to fix. The
patch adds ~40 new lines of code to the deletion collector and makes it
significantly more complicated (and almost certainly introduces
performance regressions; one of them is even called out with a `FIXME` in
the patch), all to support something which violates one of the basic
assumptions of the Django ORM (that querysets are homogenous).
Closing as wontfix.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:5>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: wontfix
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by kostko):
Sorry, but the `FIXME` you mention is also present in current Django
master and is not introduced by this patch. The patch simply augments
current code and leaves existing stuff as it is (including the comment).
The only part that it adds is the one that iterates over actual models.
Please see:
https://github.com/django/django/blob/master/django/db/models/deletion.py#L191
As for the fix, please advise how would one then properly handle cascaded
deletion of polymorphic models? Or are you saying that polymorphic models
should not be supported and/or used with the Django's ORM?
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:6>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: wontfix
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mitar):
I don't think it introduces performance regressions, for non-polymorphic
models the code behaves exactly the same as it is currently,
`concrete_model_objs` being containing only one element, so for each loops
doing only one iteration.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:7>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: wontfix
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
Replying to [comment:6 kostko]:
> Sorry, but the `FIXME` you mention is also present in current Django
master and is not introduced by this patch. The patch simply augments
current code and leaves existing stuff as it is (including the comment).
The only part that it adds is the one that iterates over actual models.
Please see:
>
>
https://github.com/django/django/blob/master/django/db/models/deletion.py#L191
Yes, you're right, sorry about that. The diff showed it in green (probably
because of indentation changes) and I didn't notice that it had already
been there previously. My review of the patch was cursory, since I don't
think this is a bug in Django regardless.
> As for the fix, please advise how would one then properly handle
cascaded deletion of polymorphic models? Or are you saying that
polymorphic models should not be supported and/or used with the Django's
ORM?
I'm saying that until/unless polymorphic models are part of Django and
fully supported by the ORM, Django's code should not be made more complex
in order to accommodate them (unless the changes are otherwise generally
beneficial, for instance providing additional customization points), and
it is the responsibility of a third-party add-on that implements them to
make it work.
For an illustration of the problem, see the fact that the pull request had
no tests, and it would be impossible to implement tests for it without
deep poking into ORM internals. If you can't reproduce the problem using
supported APIs, it's a good sign that the fix for the problem may not
belong in Django.
I'm not familiar enough with django-polymorphic to say how it should fix
the problem. It seems to me that an overridden `queryset.delete()` might
be able to adjust things such that the existing collector could handle it.
If an additional hook or customization point in Django would allow django-
polymorphic to insert the delete-cascade behavior it needs, I'd be more
inclined to support a patch like that, rather than a patch that adds
support to Django's delete-collector for a scenario that Django doesn't
support.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:8>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jmurty):
* status: closed => new
* resolution: wontfix =>
Comment:
Please consider re-opening this issue as it is related to Django core's
handling of proxy models in general, not just django-polymorphic.
The root cause of this issue is that Django's deletion collector does not
include M2M reverse references to '''proxy''' parents/ancestors of a model
when deleting it; it only finds these relationships for '''concrete'''
parent models. This means that deleting a proxy-based model object with
extant M2M relationships targeting a proxy parent will fail with integrity
errors, if you are using a database like PostgreSQL that enforces foreign
key constraints.
I have created a Pull Requests based on the current 1.7.x branch that adds
unit tests demonstrating the issue when run against PostgreSQL:
https://github.com/django/django/pull/5399
The same issue affects 1.8, and I can follow up with a PR for that branch
if it would be useful?
We have done some work investigating this issue and have the rough
beginnings of patches for the 1.7 and 1.8 branches, using a different
approach to the patches earlier in this ticket – namely, adjusting the
`get_deleted_objects` and related code path to find reverse M2M
relationships on proxy models.
We could proceed with these patches, but only if the issue is considered
relevant and serious enough to fix in Django core.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:9>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by jmurty):
* Attachment "delete_fails_with_m2m_to_proxy_model.patch" added.
Patch file for Django PR #5399
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by jmurty):
* cc: james@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:10>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by akaariai):
I suggest you do the work against master. It is unlikely we will fix this
in 1.8 or earlier, as this isn't a regression or severe enough failure.
This might be related to #25505 and #18012.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:11>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by TZanke):
* cc: tzanke@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:12>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by jmurty):
Thanks for the feedback. I have submitted PR #5404 with the tests rebased
onto master instead of the 1.7.x branch, and will do any related work
against master.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:13>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by jmurty):
Please see the new ticket #25520 which is based on this issue but is more
specific to the new proposed tests (and potential patch)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:14>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Simon Charette <charette.s@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"211486f3ab5602cdc332f03cd03fad3cb929d154" 211486f3]:
{{{
#!CommitTicketReference repository=""
revision="211486f3ab5602cdc332f03cd03fad3cb929d154"
Fixed #23076, #25505 -- Fixed deletion of intermediate proxy models.
Thanks to James Murty for his work on an alternate patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:15>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Jonathan Stray):
* status: closed => new
* resolution: fixed =>
Comment:
As of 1.11 there is still an `IntegrityError` when deleting `django-
polymorphic` objects with Postgres. As before this occurs when deleting a
set of polymorphic base objects, not all of which has the same polymorphic
subtype.
Looking over the related `django-polymorphic` [https://github.com/django-
polymorphic/django-polymorphic/issues/34 bug], there does not appear to be
any solution that could be implemented without changing Django core.
Perhaps something much more like this
[https://github.com/django/django/pull/2940 old pull request] will be
required. Or perhaps something like this
[https://gist.github.com/jmurty/2034c24b6f91a3eaf51a monkeypatch] for 1.8
linked from the `django-polymorhpic` bug. The
[https://gist.github.com/jmurty/2034c24b6f91a3eaf51a#file-django-proxy-
delete-hack-py-L36 core part] of that patch is closely related to the
proxy model deletion
[https://github.com/django/django/commit/211486f3ab5602cdc332f03cd03fad3cb929d154
fix] which closed this issue previously.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:16>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by Jonathan Stray):
I have confirmed, by testing with my app where the `IntegrityError`
appears, that the originally committed
[https://github.com/django/django/commit/211486f3ab5602cdc332f03cd03fad3cb929d154
fix] for this bug (which was reverted a few days later) doesn't actually
fix it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:17>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by Jonathan Stray):
Aha! There is a simple workaround for 1.11, from this [https://github.com
/django-polymorphic/django-polymorphic/issues/229#issuecomment-246613138
django-polymorphic bug].
On the base class, set
{{{
class Animal(PolymorphicModel):
...
class Meta:
base_manager_name = 'base_objects
}}}
I have no idea why this works, or if this should ultimately be fixed in
`django` or `django-polymorphic`
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:18>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Rich Rauenzahn):
* cc: Rich Rauenzahn (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:19>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Comment (by Rich Rauenzahn):
Another workaround is overriding and customizing the `on_delete`:
{{{
# https://github.com/django-polymorphic/django-
polymorphic/issues/229#issuecomment-398434412
def NON_POLYMORPHIC_CASCADE(collector, field, sub_objs, using):
return models.CASCADE(collector, field, sub_objs.non_polymorphic(),
using)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:20>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by wkoot):
* cc: wkoot (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:21>
Django
Reporter: jernej@… | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Changes (by Simon Brulhart):
* cc: Simon Brulhart (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23076#comment:22>