Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Ticket #18343 -- Cleaned up deferred model implementation #80

Closed
wants to merge 2 commits into from

1 participant

Anssi Kääriäinen
Anssi Kääriäinen
Owner

Removed some dead-code, and some dump logic in deferred field loading
and deferred model reduce(). The biggest user visible change is
that primary keys do not need fetching from the DB in some inheritance
cases.

All tests pass on SQLite.

Anssi Kääriäinen [1/2] #18343 -- Cleaned up deferred model implementation
Generic cleanup and dead code removal in deferred model field loading
and model.__reduce__().
831c07b
Anssi Kääriäinen
Owner

I splitted the patches into two for easier review.

Anssi Kääriäinen [2/2] #18343 -- deferred model pk handling in inheritance case
Assuming an inherited model with parent_ptr_id -> id, doing a deferred
load and then fetching the id field would cause a database query, even
if the id field's value is already loaded in the parent_ptr_id field.
6474daf
Anssi Kääriäinen
Owner

Pushed in manually

Anssi Kääriäinen akaariai closed this May 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

May 22, 2012
Anssi Kääriäinen [1/2] #18343 -- Cleaned up deferred model implementation
Generic cleanup and dead code removal in deferred model field loading
and model.__reduce__().
831c07b
Anssi Kääriäinen [2/2] #18343 -- deferred model pk handling in inheritance case
Assuming an inherited model with parent_ptr_id -> id, doing a deferred
load and then fetching the id field would cause a database query, even
if the id field's value is already loaded in the parent_ptr_id field.
6474daf
This page is out of date. Refresh to see the latest.
8  django/db/models/base.py
@@ -404,7 +404,6 @@ def __reduce__(self):
404 404
         # and as a result, the super call will cause an infinite recursion.
405 405
         # See #10547 and #12121.
406 406
         defers = []
407  
-        pk_val = None
408 407
         if self._deferred:
409 408
             from django.db.models.query_utils import deferred_class_factory
410 409
             factory = deferred_class_factory
@@ -412,12 +411,7 @@ def __reduce__(self):
412 411
                 if isinstance(self.__class__.__dict__.get(field.attname),
413 412
                         DeferredAttribute):
414 413
                     defers.append(field.attname)
415  
-                    if pk_val is None:
416  
-                        # The pk_val and model values are the same for all
417  
-                        # DeferredAttribute classes, so we only need to do this
418  
-                        # once.
419  
-                        obj = self.__class__.__dict__[field.attname]
420  
-                        model = obj.model_ref()
  414
+            model = self._meta.proxy_for_model
421 415
         else:
422 416
             factory = simple_class_factory
423 417
         return (model_unpickle, (model, defers, factory), data)
58  django/db/models/query_utils.py
@@ -6,8 +6,6 @@
6 6
 circular import difficulties.
7 7
 """
8 8
 
9  
-import weakref
10  
-
11 9
 from django.db.backends import util
12 10
 from django.utils import tree
13 11
 
@@ -70,8 +68,6 @@ class DeferredAttribute(object):
70 68
     """
71 69
     def __init__(self, field_name, model):
72 70
         self.field_name = field_name
73  
-        self.model_ref = weakref.ref(model)
74  
-        self.loaded = False
75 71
 
76 72
     def __get__(self, instance, owner):
77 73
         """
@@ -79,27 +75,33 @@ def __get__(self, instance, owner):
79 75
         Returns the cached value.
80 76
         """
81 77
         from django.db.models.fields import FieldDoesNotExist
  78
+        # Fetch the non-deferred model
  79
+        non_deferred_model = instance._meta.proxy_for_model
  80
+        opts = non_deferred_model._meta
82 81
 
83 82
         assert instance is not None
84  
-        cls = self.model_ref()
85 83
         data = instance.__dict__
86 84
         if data.get(self.field_name, self) is self:
87 85
             # self.field_name is the attname of the field, but only() takes the
88 86
             # actual name, so we need to translate it here.
89 87
             try:
90  
-                cls._meta.get_field_by_name(self.field_name)
91  
-                name = self.field_name
  88
+                f = opts.get_field_by_name(self.field_name)[0]
92 89
             except FieldDoesNotExist:
93  
-                name = [f.name for f in cls._meta.fields
94  
-                    if f.attname == self.field_name][0]
95  
-            # We use only() instead of values() here because we want the
96  
-            # various data coersion methods (to_python(), etc.) to be called
97  
-            # here.
98  
-            val = getattr(
99  
-                cls._base_manager.filter(pk=instance.pk).only(name).using(
100  
-                    instance._state.db).get(),
101  
-                self.field_name
102  
-            )
  90
+                f = [f for f in opts.fields
  91
+                     if f.attname == self.field_name][0]
  92
+            name = f.name
  93
+            # Lets see if the field is part of the parent chain. If so we
  94
+            # might be able to reuse the already loaded value. Refs #18343.
  95
+            val = self._check_parent_chain(instance, opts, name)
  96
+            if val is None:
  97
+                # We use only() instead of values() here because we want the
  98
+                # various data coersion methods (to_python(), etc.) to be
  99
+                # called here.
  100
+                val = getattr(
  101
+                    non_deferred_model._base_manager.only(name).using(
  102
+                        instance._state.db).get(pk=instance.pk),
  103
+                    self.field_name
  104
+                )
103 105
             data[self.field_name] = val
104 106
         return data[self.field_name]
105 107
 
@@ -110,6 +112,28 @@ def __set__(self, instance, value):
110 112
         """
111 113
         instance.__dict__[self.field_name] = value
112 114
 
  115
+    def _check_parent_chain(self, instance, opts, name):
  116
+        """
  117
+        Check if the field is part of any parent chain in the model.
  118
+        If so, return the child field's value. This is done so that
  119
+        if we have parent_ptr_id already in the model, and we are
  120
+        fetching the id field, we will not need to refetch the id
  121
+        value which is guaranteed to be the same as the parent_ptr_id
  122
+        value.
  123
+        """
  124
+        for parent, field in opts.parents.items():
  125
+            # If fetching field 'id', and the 'id' value is found here, we
  126
+            # will continue to fetch the instance's parent_ptr_id. It might
  127
+            # be also a deferred field, and we end up here again, now fetching
  128
+            # parent_ptr_id value instead.
  129
+            if parent._meta.pk.attname == name:
  130
+                return getattr(instance, field.attname)
  131
+            ret = self._check_parent_chain(instance, parent._meta, name)
  132
+            if ret:
  133
+                return ret
  134
+        return None
  135
+
  136
+
113 137
 def select_related_descend(field, restricted, requested, reverse=False):
114 138
     """
115 139
     Returns True if this field should be used to descend deeper for
15  tests/modeltests/defer/tests.py
@@ -158,3 +158,18 @@ def test_defer_proxy(self):
158 158
         self.assert_delayed(child, 1)
159 159
         self.assertEqual(child.name, 'p1')
160 160
         self.assertEqual(child.value, 'xx')
  161
+
  162
+    def test_defer_inheritance_pk_chaining(self):
  163
+        """
  164
+        When an inherited model is fetched from the DB, its PK is also fetched.
  165
+        When getting the PK of the parent model it is useful to use the already
  166
+        fetched parent model PK if it happens to be available. Tests that this
  167
+        is done.
  168
+        """
  169
+        s1 = Secondary.objects.create(first="x1", second="y1")
  170
+        bc = BigChild.objects.create(name="b1", value="foo", related=s1,
  171
+                                     other="bar")
  172
+        bc_deferred = BigChild.objects.only('name').get(pk=bc.pk)
  173
+        with self.assertNumQueries(0):
  174
+            bc_deferred.id
  175
+        self.assertEqual(bc_deferred.pk, bc_deferred.id)
4  tests/modeltests/field_subclassing/tests.py
@@ -18,6 +18,10 @@ def test_defer(self):
18 18
         self.assertEqual(d.data, [1, 2, 3])
19 19
 
20 20
         d = DataModel.objects.defer("data").get(pk=d.pk)
  21
+        self.assertTrue(isinstance(d.data, list))
  22
+        self.assertEqual(d.data, [1, 2, 3])
  23
+        # Refetch for save
  24
+        d = DataModel.objects.defer("data").get(pk=d.pk)
21 25
         d.save()
22 26
 
23 27
         d = DataModel.objects.get(pk=d.pk)
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.