Cascading bug on deletion (Django 1.8)

[martinmaillard]

I have something similar to the following situation:

    A ------- B                 
            /   \
          B1     B2 ------- C

In code:

class A(models.Model):
    pass

class B(polymorphic.PolymorphicModel):
    a = models.ForeignKey(A)

class B1(B):
    pass

class B2(B):
    pass

class C(models.Model):
    b = models.ForeignKey(B1)

If I try to delete an instance of A, the following error occurs:

ValueError: Cannot query "B2 object": Must be "B1" instance.

The full example and traceback:

a = A.objects.create()
b1 = B1.objects.create(a=a)
b2 = B2.objects.create(a=a)
c = C.objects.create(b=b1)

a.delete()

File "[...]/text.py", line 137, in test_cascading_bug
  a.delete()
File "[...]/django/db/models/base.py", line 895, in delete
  collector.collect([self])
File "[...]/django/db/models/deletion.py", line 229, in collect
  field.rel.on_delete(self, field, sub_objs, self.using)
File "[...]/django/db/models/deletion.py", line 18, in CASCADE
  source_attr=field.name, nullable=field.null)
File "[...]/django/db/models/deletion.py", line 225, in collect
  sub_objs = self.related_objects(related, batch)
File "[...]/django/db/models/deletion.py", line 247, in related_objects
  **{"%s__in" % related.field.name: objs}
File "[...]/django/db/models/query.py", line 679, in filter
  return self._filter_or_exclude(False, *args, **kwargs)
File "[...]/django/db/models/query.py", line 697, in _filter_or_exclude
  clone.query.add_q(Q(*args, **kwargs))
File "[...]/django/db/models/sql/query.py", line 1309, in add_q
  clause, require_inner = self._add_q(where_part, self.used_aliases)
File "[...]/django/db/models/sql/query.py", line 1337, in _add_q
  allow_joins=allow_joins, split_subq=split_subq,
File "[...]/django/db/models/sql/query.py", line 1178, in build_filter
  self.check_related_objects(field, value, opts)
File "[...]/django/db/models/sql/query.py", line 1076, in check_related_objects
  self.check_query_object_type(v, opts)
File "[...]/django/db/models/sql/query.py", line 1057, in check_query_object_type
  (value, opts.object_name))
ValueError: Cannot query "B2 object": Must be "B1" instance.

I'm not 100% sure it is caused by django-polymorphic, but I could not reproduce the error without it...

[dsilvers]

Seeing this same bug on a very similar model setup (multiple levels of inheritance with django-polymorphic models, various ForeignKey fields).

Django==1.8.7
django-polymorphic==0.8.1

Our workaround is to handle cascading deletes by overriding delete(), but it's a less than ideal solution.

Let me know if there's any information I can provide, but Martin's bug report above is basically the same as what we've got on our end of things.

[jonashaag]

Affects me too

[jonathanballs]

I'm getting this as well. Very frustrating. @dsilvers Overriding delete may well be the only solution, I'll look into it. Has it affected performance at all? @chrisglass Would you accept a pull request for delete being overridden?

[dsilvers]

@bonniejools No idea on performance. We have a total of zero users currently.

[vdboor]

@bonniejools I'm curious what you've overwritten. Does the issue also occur on standard Django models, or only when the polymorphic models/managers are used?

[dsilvers]

Workaround looks something like this, based on @martinmaillard's example at top:

class A(models.Model):
    def delete(self):
        B.objects.non_polymorphic().filter(a=self).all().delete()
        super(A, self).delete()

[EllenLippe]

I had the same problem, using the polymorphic model with Django 1.9.2 and django-polymorphic 0.8.1 and 0.9.1.

What worked for me, was calling delete on each object instead of on the queryset.

I.e, the following approach caused integrity error:
A.objects.all().delete()

whereas the following worked just fine:
for x in A.objects.all():
x.delete()

where class A is a PolymorphicModel

No need for overriding the delete method

[MattFisher]

Also seeing this bug, Django 1.8.9, django-polymorphic 0.8.1
Additionally, when the delete is triggered by a cascade, overriding A's delete() method won't help.

 Z <--- A <-- B                 
            /   \
          B1     B2

Z.objects.all().delete() results in ValueError: Cannot query "B1 object": Must be "B2" instance.
Potentially I could override Z's delete() as well, but it starts to seem messy.

[hyusetiawan]

any update on this?
1.10.1 has landed and this issue is still happening.
https://code.djangoproject.com/ticket/23076 is closed (see bottom)

[expresspotato]

Really? This is still an issue, basic django functionality. Why are the dev's working on exotic problem xyz when this doesn't even work...

[j2kun]

Can confirm this is still happening to me

[tidenhub]

We just run into the same issue with Django 1.10.6 and django-polymorphic 1.1

[TZanke]

At the moment i fixed it with:

from polymorphic.models import PolymorphicModel
from polymorphic.managers import PolymorphicManager
from polymorphic.query import PolymorphicQuerySet

class DeletablePolymorphicQuerySet(PolymorphicQuerySet):
    def delete(self):
        for instance in self:
            instance.delete()

class DeletablePolymorphicManager(PolymorphicManager):
    queryset_class = DeletablePolymorphicQuerySet

class ABC(PolymorphicModel):
    objects = DeletablePolymorphicManager()

[TZanke]

My last fix only solved the problem if the deletion starts within a Polymorphic Model.
After some search my current solutions is based on #229

class ABC(PolymorphicModel):

    if django.VERSION[:2] < (1, 10):
        # Fix delete. Workaround for https://github.com/chrisglass/django_polymorphic/issues/34
        _base_manager = models.Manager()

    class Meta:
        if django.VERSION[:2] >= (1, 10):
            # Fix delete. Workaround for https://github.com/django-polymorphic/django-polymorphic/issues/229#issuecomment-246613138
            base_manager_name = 'base_objects'

[jonashaag]

Why waste your time on workarounds instead of simply fixing it properly in the django-polymorphic codebase? PRs welcome.

[jstray]

So what would a "proper" fix be? I'll submit the PR if someone can explain what needs to happen.

[iamkingalvarado]

@TZanke Thanks, It worked for me on Django 1.11 and django-polymorphic 1.13

[antwan]

Still affected.

[expresspotato]

Still doesn't work for me either...

[alex2304]

But if we change the manager, polymorphic model looses the ability to cast children to appropriate type!

How to deal with it? Could someone explain?

[toniengelhardt]

Any updates on this?

[benkonrath]

I'm using this custom function for on_delete and it works perfectly now (Django 2.2).
#229 (comment)

def NON_POLYMORPHIC_CASCADE(collector, field, sub_objs, using):
    return models.CASCADE(collector, field, sub_objs.non_polymorphic(), using)

I think this should be documented as the solution so this issue can be closed.

[jaradc]

Just want to say:

1. @martinmaillard (the OP) really excellent reproducible example.
2. totally agree with @expresspotato - whatever the django-polymorphic team is working on, this bug should take priority ... #1. It's been years, right? I see people saying "PRs are welcome" but this is a leadership question - what IS the "right" solution? Someone deeply involved with this project needs to step up and say "Overriding the delete method is the solution" or "Creating a NON_POLYMORPHIC_XYZ on_delete= function is the right way." Then close these multi-parallel issue threads going about this same problem. I get it, people are busy. But right now, this project - django-polymorphic - is broken if you literally can't delete related objects after install without a "work-around".
3. @benkonrath Thanks for point me in the right direction. That solution is clean, and doesn't require overriding the delete method as is suggested.

For now, I'll go with this. For anyone wanting a more understandable solution, here it is.

models.py

from django.db import models
from polymorphic.models import PolymorphicModel

from django.contrib.auth.models import AbstractUser


class User(AbstractUser):
    pass


class Content(PolymorphicModel):
    user = models.ForeignKey(User, on_delete=models.CASCADE)


class Post(Content):
    title = models.CharField(max_length=100)


class Note(Content):
    title = models.CharField(max_length=100)


class Bookmark(models.Model):
    post = models.ForeignKey(Post, on_delete=models.CASCADE)

Create a user and some posts and notes and a bookmark.

from myapp.models import User, Post, Note, Bookmark

u = User.objects.create(username='abc')
p1 = Post.objects.create(user=u, title="Some title")
p2 = Post.objects.create(user=u, title="Another title")
n1 = Note.objects.create(user=u, title="Django Polymorphic")
n2 = Note.objects.create(user=u, title="Is Broken")
b = Bookmark.objects.create(post=p1)

See the user's content:

>>> u.content_set.all()
<PolymorphicQuerySet [<Post: Post object (1)>, <Post: Post object (2)>, <Note: Note object (3)>, <Note: Note object (4)>]>

Now try to delete user instance:

u.delete()
ValueError: Cannot query "Note object (3)": Must be "Post" instance.

Solution Is To...

from django.db import models
from polymorphic.models import PolymorphicModel

from django.contrib.auth.models import AbstractUser


class User(AbstractUser):
    pass


def NON_POLYMORPHIC_CASCADE(collector, field, sub_objs, using):
    return models.CASCADE(collector, field, sub_objs.non_polymorphic(), using)

class Content(PolymorphicModel):
    user = models.ForeignKey(User, on_delete=NON_POLYMORPHIC_CASCADE)


class Post(Content):
    title = models.CharField(max_length=100)


class Note(Content):
    title = models.CharField(max_length=100)


class Bookmark(models.Model):
    post = models.ForeignKey(Post, on_delete=models.CASCADE)

Make migrations and migrate:

python manage.py makemigrations myapp
python manage.py migrate

Get the user again

>>> u = User.objects.get(username='abc')
>>> u.delete()
(10, {'myapp.Bookmark': 1, 'myapp.Note': 2, 'myapp.Post': 2, 'myapp.Content': 4, 'myapp.User': 1})
>>> User.objects.all()
<QuerySet []>