Opened 4 years ago

Closed 4 years ago

Last modified 7 months ago

#31435 closed Bug (invalid)

RecursionError when deleting a model with one to many relationship.

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

Description

This issue appeared with upgrade from Django 2 to Django 3.0. I double checked and it definitely works with version <3.0.

There is a model with a duration field and the "init" method is overridden to save "old" values of this duration field.

class TimeSeries(models.Model):
  frequency = models.DurationField()
  def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self._old_frequency = self.frequency

Another model extends this "TimeSeries" model and has a foreign key reference to the model which I tried to delete.

class RefModel(TimeSeries):
  main_model = models.ForeignKey(
    MainModel, on_delete=models.CASCADE, related_name="refs"
  )

class MainModel(models.Model):
  pass

So I created an instance of "MainModel" "main_a" and added some "RefModel" instances with relationhip to "main_a".
When I call "main_a.delete()" it ends up in a stack overflow with the following stacktrace:

Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/lib/python3.8/site-packages/django/db/models/base.py", line 937, in delete
    collector.collect([self], keep_parents=keep_parents)
  File "/lib/python3.8/site-packages/django/db/models/deletion.py", line 244, in collect
    if sub_objs:
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 280, in __bool__
    self._fetch_all()
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 1261, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 75, in __iter__
    obj = model_cls.from_db(db, init_list, row[model_fields_start:model_fields_end])
  File "/lib/python3.8/site-packages/django/db/models/base.py", line 512, in from_db
    new = cls(*values)
  File "/lib/python3.8/site-packages/django_timeseries/models.py", line 27, in __init__
    self._old_frequency = self.frequency
  File "/webserver/business_rules/thread_safe_model_patcher.py", line 51, in _patched_get_attribute
    return Model.get(model_self, item)
  File "/lib/python3.8/site-packages/django/db/models/query_utils.py", line 139, in __get__
    instance.refresh_from_db(fields=[field_name])
  File "/lib/python3.8/site-packages/django/db/models/base.py", line 627, in refresh_from_db
    db_instance = db_instance_qs.get()
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 411, in get
    num = len(clone)
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 258, in __len__
    self._fetch_all()
  File "/lib/python3.8/site-packages/django/db/models/query.py", line 1261, in _fetch_all
<--- Stacktrace repeats itself from here on --->

"django_timeseries" is a custom package which defines the "TimeSeries" class.
"thread_safe_model_patcher.py" monkeypatches the "Model" class. It has no effect on whether the error happens. I removed the this patch and it still errors.

It seems to me that deleting "main_a" deletes all "RefModel" instances related to it (like it should) but while doing this, it gets stuck initializing the super class "TimeSeries" because the "init" accesses the "frequency" field.

Change History (8)

comment:1 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed
Summary: RecursionError when deleting a model with one to many relationshipRecursionError when deleting a model with one to many relationship.

django-timeseries looks abandoned and it doesn't support Django 2.0+. Moreover I was not able to reproduce this issue with a sample project

    def test_regression(self):
        obj = MainModel.objects.create()
        r1 = RefModel.objects.create(frequency=timedelta(1), main_model=obj)
        r2 = RefModel.objects.create(frequency=timedelta(2), main_model=obj)
        obj.delete()

Can you provide a sample project (without django-timeseries) that reproduces this issue?

comment:2 by Simon Charette, 4 years ago

Judging by the trace of the exception it seems like you already forked django-timeseries or are using a different package with the same name since the Pypi one doesn't have a models.py file.

In all cases your code was broken because you are accessing a deferred field in your TimeSeries.__init__ model which is not allowed.

You can test it out but on any version of Django doing TimeSeries.objects.defer('frequency') would cause N + 1 queries to be performed where N = TimeSeries.objects.count().

You want to do self._old_frequency = self.__dict__.get('frequency') in there.

For the record the commit that introduced the recursion error for your bogus Model.__init__ is f110de5c04818b8f915dcf65da37a50c1424c6e6 which started deferring fields of model when possible to avoid fetching large amount of data when unnecessary on deletion (#30191).

It is also documented that the right way to implement custom model loading should be done using Model.from_db(). I guess you could assign django.db.models.DEFERRED to _old_frequency in there and special case it's access when necessary instead of assigning None to it.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:3 by Simon Charette, 4 years ago

Keywords: defer recursionerror added; recursion stack overflow reference foreign key one to many removed

in reply to:  2 comment:4 by Fabian Allendorf, 4 years ago

Replying to Simon Charette:

Judging by the trace of the exception it seems like you already forked django-timeseries or are using a different package with the same name since the Pypi one doesn't have a models.py file.

In all cases your code was broken because you are accessing a deferred field in your TimeSeries.__init__ model which is not allowed.

You can test it out but on any version of Django doing TimeSeries.objects.defer('frequency') would cause N + 1 queries to be performed where N = TimeSeries.objects.count().

You want to do self._old_frequency = self.__dict__.get('frequency') in there.

The package django-timseries caused a lot of confusion it seems. I should have added that its a private package we have developed at our company and not this one https://pypi.org/project/django-timeseries/.

Thank you, I think that is exactly the point I was missing.

comment:5 by Mariusz Felisiak, 4 years ago

Resolution: needsinfoinvalid

comment:6 by Natalia <124304+nessita@…>, 7 months ago

In e47298a:

Refs #31435 -- Doc'd potential infinite recursion when accessing model fields in init.

comment:7 by Natalia <124304+nessita@…>, 7 months ago

In 0e34ac8:

[5.0.x] Refs #31435 -- Doc'd potential infinite recursion when accessing model fields in init.

Backport of e47298aec4fa04416e7082331fbd44bd9f2662aa from main

comment:8 by Natalia <124304+nessita@…>, 7 months ago

In 6697880:

[4.2.x] Refs #31435 -- Doc'd potential infinite recursion when accessing model fields in init.

Backport of e47298aec4fa04416e7082331fbd44bd9f2662aa from main

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