Opened 9 months ago

Closed 6 months ago

Last modified 6 months ago

#23076 closed Bug (wontfix)

Cascaded deletion of polymorphic models fails

Reported by: jernej@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: mmitar@… 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.

Change History (8)

comment:1 Changed 9 months ago by mitar

  • Cc mmitar@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 9 months ago by slurms

  • Needs tests set

comment:3 Changed 9 months ago 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.

comment:4 Changed 6 months ago by timgraham

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 Changed 6 months ago by carljm

  • Resolution set to wontfix
  • Status changed from new to closed

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 follow-up: Changed 6 months ago 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?

comment:7 Changed 6 months ago 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 containing only one element, so for each loops doing only one iteration.

Last edited 6 months ago by mitar (previous) (diff)

comment:8 in reply to: ↑ 6 Changed 6 months ago by carljm

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.

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