Opened 10 years ago

Last modified 3 years ago

#23076 new Bug

Cascaded deletion of polymorphic models fails

Reported by: jernej@… Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: mmitar@…, james@…, tzanke@…, Rich Rauenzahn, wkoot, Simon Brulhart Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 2940) that fixes this issue and it is directly based on a patch submitted by Kronuz at the end of ticket #16128.

Attachments (1)

delete_fails_with_m2m_to_proxy_model.patch (3.4 KB ) - added by James Murty 8 years ago.
Patch file for Django PR #5399

Download all attachments as: .zip

Change History (23)

comment:1 by Mitar, 10 years ago

Cc: mmitar@… added

comment:2 by Nick Sandford, 10 years ago

Needs tests: set

comment:3 by Tim Graham, 10 years ago

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.

comment:4 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

Accepting on the basis of the problem. Not sure about the solution and the patch still needs tests.

comment:5 by Carl Meyer, 9 years ago

Resolution: wontfix
Status: newclosed

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.

comment:6 by Jernej Kos, 9 years ago

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?

comment:7 by Mitar, 9 years ago

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.

Version 0, edited 9 years ago by Mitar (next)

in reply to:  6 comment:8 by Carl Meyer, 9 years ago

Replying to 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.

comment:9 by James Murty, 8 years ago

Resolution: wontfix
Status: closednew

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 master branch that adds unit tests demonstrating the issue when run against PostgreSQL: https://github.com/django/django/pull/5404

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.

[Edit: Updated PR link to latest version based on master branch]

Last edited 8 years ago by James Murty (previous) (diff)

by James Murty, 8 years ago

Patch file for Django PR #5399

comment:10 by James Murty, 8 years ago

Cc: james@… added

comment:11 by Anssi Kääriäinen, 8 years ago

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.

comment:12 by TZanke, 8 years ago

Cc: tzanke@… added

comment:13 by James Murty, 8 years ago

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.

Last edited 8 years ago by James Murty (previous) (diff)

comment:14 by James Murty, 8 years ago

Please see the new ticket #25520 which is based on this issue but is more specific to the new proposed tests (and potential patch)

comment:15 by Simon Charette <charette.s@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 211486f3:

Fixed #23076, #25505 -- Fixed deletion of intermediate proxy models.

Thanks to James Murty for his work on an alternate patch.

comment:16 by Jonathan Stray, 7 years ago

Resolution: fixed
Status: closednew

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 bug, there does not appear to be any solution that could be implemented without changing Django core.

Perhaps something much more like this old pull request will be required. Or perhaps something like this monkeypatch for 1.8 linked from the django-polymorhpic bug. The core part of that patch is closely related to the proxy model deletion fix which closed this issue previously.

Update: the change that closed this bug was effectively reverted as part of larger changes to fix the related #18012

Last edited 7 years ago by Jonathan Stray (previous) (diff)

comment:17 by Jonathan Stray, 7 years ago

I have confirmed, by testing with my app where the IntegrityError appears, that the originally committed fix for this bug (which was reverted a few days later) doesn't actually fix it.

comment:18 by Jonathan Stray, 7 years ago

Aha! There is a simple workaround for 1.11, from this 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

Last edited 7 years ago by Jonathan Stray (previous) (diff)

comment:19 by Rich Rauenzahn, 5 years ago

Cc: Rich Rauenzahn added

comment:20 by Rich Rauenzahn, 5 years ago

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) 


comment:21 by wkoot, 5 years ago

Cc: wkoot added

comment:22 by Simon Brulhart, 3 years ago

Cc: Simon Brulhart added
Note: See TracTickets for help on using tickets.
Back to Top