From a8a81aae20a81e012fddc24f3ede556501af64a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Sun, 27 May 2012 02:08:44 +0300 Subject: [PATCH] Fixed #18343 -- Cleaned up deferred model implementation Generic cleanup and dead code removal in deferred model field loading and model.__reduce__(). Also fixed an issue where if an inherited model with a parent field chain parent_ptr_id -> id would be deferred loaded, then accessing the id field caused caused a database query, even if the id field's value is already loaded in the parent_ptr_id field. --- django/db/models/base.py | 8 +--- django/db/models/query_utils.py | 49 ++++++++++++++------- tests/modeltests/defer/tests.py | 15 +++++++ tests/modeltests/field_subclassing/tests.py | 4 ++ 4 files changed, 52 insertions(+), 24 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index 08bfc73a879..13238fc9dcf 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -404,7 +404,6 @@ class Model(object): # and as a result, the super call will cause an infinite recursion. # See #10547 and #12121. defers = [] - pk_val = None if self._deferred: from django.db.models.query_utils import deferred_class_factory factory = deferred_class_factory @@ -412,12 +411,7 @@ class Model(object): if isinstance(self.__class__.__dict__.get(field.attname), DeferredAttribute): defers.append(field.attname) - if pk_val is None: - # The pk_val and model values are the same for all - # DeferredAttribute classes, so we only need to do this - # once. - obj = self.__class__.__dict__[field.attname] - model = obj.model_ref() + model = self._meta.proxy_for_model else: factory = simple_class_factory return (model_unpickle, (model, defers, factory), data) diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py index a56ab5ca80d..5676fdce9a7 100644 --- a/django/db/models/query_utils.py +++ b/django/db/models/query_utils.py @@ -6,8 +6,6 @@ large and/or so that they can be used by other modules without getting into circular import difficulties. """ -import weakref - from django.db.backends import util from django.utils import tree @@ -70,8 +68,6 @@ class DeferredAttribute(object): """ def __init__(self, field_name, model): self.field_name = field_name - self.model_ref = weakref.ref(model) - self.loaded = False def __get__(self, instance, owner): """ @@ -79,27 +75,32 @@ class DeferredAttribute(object): Returns the cached value. """ from django.db.models.fields import FieldDoesNotExist + non_deferred_model = instance._meta.proxy_for_model + opts = non_deferred_model._meta assert instance is not None - cls = self.model_ref() data = instance.__dict__ if data.get(self.field_name, self) is self: # self.field_name is the attname of the field, but only() takes the # actual name, so we need to translate it here. try: - cls._meta.get_field_by_name(self.field_name) - name = self.field_name + f = opts.get_field_by_name(self.field_name)[0] except FieldDoesNotExist: - name = [f.name for f in cls._meta.fields - if f.attname == self.field_name][0] - # We use only() instead of values() here because we want the - # various data coersion methods (to_python(), etc.) to be called - # here. - val = getattr( - cls._base_manager.filter(pk=instance.pk).only(name).using( - instance._state.db).get(), - self.field_name - ) + f = [f for f in opts.fields + if f.attname == self.field_name][0] + name = f.name + # Lets see if the field is part of the parent chain. If so we + # might be able to reuse the already loaded value. Refs #18343. + val = self._check_parent_chain(instance, name) + if val is None: + # We use only() instead of values() here because we want the + # various data coersion methods (to_python(), etc.) to be + # called here. + val = getattr( + non_deferred_model._base_manager.only(name).using( + instance._state.db).get(pk=instance.pk), + self.field_name + ) data[self.field_name] = val return data[self.field_name] @@ -110,6 +111,20 @@ class DeferredAttribute(object): """ instance.__dict__[self.field_name] = value + def _check_parent_chain(self, instance, name): + """ + Check if the field value can be fetched from a parent field already + loaded in the instance. This can be done if the to-be fetched + field is a primary key field. + """ + opts = instance._meta + f = opts.get_field_by_name(name)[0] + link_field = opts.get_ancestor_link(f.model) + if f.primary_key and f != link_field: + return getattr(instance, link_field.attname) + return None + + def select_related_descend(field, restricted, requested, reverse=False): """ Returns True if this field should be used to descend deeper for diff --git a/tests/modeltests/defer/tests.py b/tests/modeltests/defer/tests.py index 09138293af6..eb09162b01e 100644 --- a/tests/modeltests/defer/tests.py +++ b/tests/modeltests/defer/tests.py @@ -158,3 +158,18 @@ class DeferTests(TestCase): self.assert_delayed(child, 1) self.assertEqual(child.name, 'p1') self.assertEqual(child.value, 'xx') + + def test_defer_inheritance_pk_chaining(self): + """ + When an inherited model is fetched from the DB, its PK is also fetched. + When getting the PK of the parent model it is useful to use the already + fetched parent model PK if it happens to be available. Tests that this + is done. + """ + s1 = Secondary.objects.create(first="x1", second="y1") + bc = BigChild.objects.create(name="b1", value="foo", related=s1, + other="bar") + bc_deferred = BigChild.objects.only('name').get(pk=bc.pk) + with self.assertNumQueries(0): + bc_deferred.id + self.assertEqual(bc_deferred.pk, bc_deferred.id) diff --git a/tests/modeltests/field_subclassing/tests.py b/tests/modeltests/field_subclassing/tests.py index c6a2335f37c..48755123f2b 100644 --- a/tests/modeltests/field_subclassing/tests.py +++ b/tests/modeltests/field_subclassing/tests.py @@ -17,6 +17,10 @@ class CustomField(TestCase): self.assertTrue(isinstance(d.data, list)) self.assertEqual(d.data, [1, 2, 3]) + d = DataModel.objects.defer("data").get(pk=d.pk) + self.assertTrue(isinstance(d.data, list)) + self.assertEqual(d.data, [1, 2, 3]) + # Refetch for save d = DataModel.objects.defer("data").get(pk=d.pk) d.save()