Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#31475 closed Bug (invalid)

Recursion issue when deleting related objects

Reported by: Matt Drew Owned by: nobody
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I ran into this situation yesterday... Basically, if you have a cascaded action such as a SET_NULL on a foreign key, you could end up with a RecursionError.

In English: ClassA is a thing. ClassB has a foreign key to ClassA with on_delete=SET_NULL. ClassC has a foreign key to ClassB with on_delete=CASCADE. When a ClassA is deleted, a query to get all the ClassB objects that link to ClassA happens in order to update the foreign key to NULL. This query is optimized to only fetch the id field.

That works fine by itself, but when ClassB has an init() that references at least two non-id fields, you get into a loop. When the objects are constructed further queries will be required to fetch the fields referenced in init(), but each of those only tries to fetch a single field and instantiate a ClassB with that field, whereupon it finds that it also needs a second ClassB field, so it queries for that field (and ONLY that field), which tries to instantiate a ClassB object with just the second field, which in init() requires the first field so it starts another query for the first field, ...

class ClassA(models.Model):
    some_value = models.CharField(max_length=30)


class ClassB(models.Model):
    foo = models.CharField(max_length=30)
    bar = models.CharField(max_length=30)
    my_a = models.ForeignKey(ClassA, null=True, on_delete=SET_NULL)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self._initial_foo = self.foo # Reason for needing this is not shown
        self._initial_bar = self.bar


class ClassC(models.Model):
    baz = models.CharField(max_length=30)
    my_b = models.ForeignKey(ClassB, null=True, on_delete=CASCADE)

a = ClassA(some_value='hi bob')
a.save()
b = ClassB(my_a=a, foo='foo', bar='bar')
b.save()
c = ClassC(my_b=b, baz='baz')
c.save()
ClassA.objects.all().delete()

Result: RecursionError

If I add a bogus post-delete to ClassB, then django will fetch all the ClassB fields in the query to update the ClassB.my_a references to NULL when a ClassA is deleted, and the recursion doesn't happen.

@receiver(models.signals.post_delete, sender=ClassB)
def do_nothing(sender, *args, **kwargs):
    pass

I'm not sure how best to address this. Maybe some Meta field that lets you specify the minimum fields needed to instantiate an object? Maybe just don't try to only fetch the id column? I'll leave it to you.

Change History (3)

comment:1 by Simon Charette, 4 years ago

Resolution: invalid
Status: newclosed

Duplicate of #31435.

In short your code never supported field deferring and was causing silent queries on model initialization.

e.g.

list(ClassB.objects.only('pk'))

Was causing 2N+1 database queries where N = ClassB.objects.count().

There's more details in the linked issue but you should be overriding from_db to achieve this purpose or use self.__dict__.get to account for the fact the field might be deferred.

comment:2 by Matt Drew, 4 years ago

Thanks, Simon!

comment:3 by Simon Charette, 4 years ago

Given we've had two reports due to __init__ overrides so far we might want to consider disabling the optimization introduced in f110de5c04818b8f915dcf65da37a50c1424c6e6 when model_cls.__init__ is not Model.__init__ or post_init receivers are attached.

I'm kind of torn here though because accessing field unconditionally during model initialization will break down the road somewhere else but learning about this misuse through a recursion error traceback is definitely confusing. Thoughts?

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