Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#12876 closed (fixed)

maximum recursion error when caching querysets on models

Reported by: zbyte64 Owned by: elachuni
Component: Database layer (models, ORM) Version: 1.2-beta
Severity: Keywords: pycamp2010
Cc: zbyte64@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The following test case causes a runtime exception against django 1.2 beta 1:

from django.test import TestCase
from django.db import models

class TestModel(models.Model):
    def cachedmethod(self):
        if not hasattr(self, '_cachedmethod'):
            self._cachedmethod = RelatedModel.objects.all().filter(test_model=self)
        return self._cachedmethod

class RelatedModel(models.Model):
    test_model = models.ForeignKey(TestModel)
    category = models.CharField(max_length=50)

class dummy(object):
    def __init__(self, parent):
        self.parent = parent
    
    def cachedmethod(self):    
        if not hasattr(self, '_cachedmethod'):
            self._cachedmethod = RelatedModel.objects.all().filter(test_model=self.parent)
        return self._cachedmethod

class BreakMeTest(TestCase):
    def test_caching(self):
        parent = TestModel()
        parent.save()
        related = RelatedModel(category='foo', test_model=parent)
        related.save()
        
        adummy = dummy(parent)
        adummy.cachedmethod()
        adummy.cachedmethod().filter(category='foo') #this doesn't break it
        parent.cachedmethod()
        parent.cachedmethod().filter(category='foo') #this breaks it

Attachments (2)

results.txt (99.5 KB) - added by zbyte64 5 years ago.
Stack trace of exception
bug12876.patch (2.9 KB) - added by elachuni 5 years ago.
Patch with a brief doctest

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by zbyte64

Stack trace of exception

comment:1 Changed 5 years ago by russellm

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by russellm

  • Component changed from Uncategorized to Database layer (models, ORM)

comment:3 follow-up: Changed 5 years ago by elachuni

  • Owner changed from nobody to elachuni

django.db.models.sql.Query defines a custom __deepcopy__ that calls self.clone(), disregarding the provided memo dict.

So, if the query contains a reference to a model instance that in turn contains a reference to a reference (as in this case), deepcopy will end up in an endless loop.

The model itself could work around this problem by providing its own __deepcopy__ implementation, that just didn't include the query. For the example in the bugreport:

class TestModel(models.Model):
    def __deepcopy__(self, memo):
        return TestModel(id=self.id)
    ....

comment:4 Changed 5 years ago by ramiro

  • Has patch set
  • Keywords pycamp2010 added

comment:5 in reply to: ↑ 3 Changed 5 years ago by elachuni

The attached patch adds an optional argument 'memo' to clone, to be able to correctly implement deepcopy.

Replying to elachuni:

So, if the query contains a reference to a model instance that in turn contains a reference to a reference (...)

Should have been a reference to a query here.

Changed 5 years ago by elachuni

Patch with a brief doctest

comment:6 Changed 5 years ago by Alex

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 5 years ago by russellm

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

(In [12700]) Fixed #12876 -- Corrected a problem with recursive relations under deepcopy. Thanks to elachuni for the patch.

comment:8 Changed 5 years ago by russellm

(In [12702]) [1.1.X] Fixed #12876 -- Corrected a problem with recursive relations under deepcopy. Thanks to elachuni for the patch.

Backport of r12700 from trunk.

comment:9 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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