Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fixed #17485 -- Made defer work with select_related. #105

Closed
wants to merge 1 commit into from

2 participants

Michal Petrucha Anssi Kääriäinen
Michal Petrucha

Calling select_related on a deferred field now results in an exception.
Thanks to Andrei Antoukh and Anssi Kääriäinen for review.

See https://code.djangoproject.com/ticket/17485 for discussion.

Michal Petrucha Fixed #17485 -- Made defer work with select_related.
Calling select_related on a deferred field now results in an exception.
Thanks to Andrei Antoukh and Anssi Kääriäinen for review.
2d20771
Anssi Kääriäinen
Owner

Committed manually in b6c356b with minor merge-conflicts resolved.

Anssi Kääriäinen
Owner

Committed manually in b6c356b with minor merge-conflicts resolved.

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

Showing 1 unique commit by 1 author.

Jun 04, 2012
Michal Petrucha Fixed #17485 -- Made defer work with select_related.
Calling select_related on a deferred field now results in an exception.
Thanks to Andrei Antoukh and Anssi Kääriäinen for review.
2d20771
This page is out of date. Refresh to see the latest.
7  django/db/models/query.py
@@ -1296,7 +1296,7 @@ def get_klass_info(klass, max_depth=0, cur_depth=0, requested=None,
1296 1296
         # Build the list of fields that *haven't* been requested
1297 1297
         for field, model in klass._meta.get_fields_with_model():
1298 1298
             if field.name not in load_fields:
1299  
-                skip.add(field.name)
  1299
+                skip.add(field.attname)
1300 1300
             elif local_only and model is not None:
1301 1301
                 continue
1302 1302
             else:
@@ -1327,7 +1327,7 @@ def get_klass_info(klass, max_depth=0, cur_depth=0, requested=None,
1327 1327
 
1328 1328
     related_fields = []
1329 1329
     for f in klass._meta.fields:
1330  
-        if select_related_descend(f, restricted, requested):
  1330
+        if select_related_descend(f, restricted, requested, load_fields):
1331 1331
             if restricted:
1332 1332
                 next = requested[f.name]
1333 1333
             else:
@@ -1339,7 +1339,8 @@ def get_klass_info(klass, max_depth=0, cur_depth=0, requested=None,
1339 1339
     reverse_related_fields = []
1340 1340
     if restricted:
1341 1341
         for o in klass._meta.get_all_related_objects():
1342  
-            if o.field.unique and select_related_descend(o.field, restricted, requested, reverse=True):
  1342
+            if o.field.unique and select_related_descend(o.field, restricted, requested,
  1343
+                                                         only_load.get(o.model), reverse=True):
1343 1344
                 next = requested[o.field.related_query_name()]
1344 1345
                 klass_info = get_klass_info(o.model, max_depth=max_depth, cur_depth=cur_depth+1,
1345 1346
                                             requested=next, only_load=only_load, local_only=True)
13  django/db/models/query_utils.py
@@ -125,18 +125,19 @@ def _check_parent_chain(self, instance, name):
125 125
         return None
126 126
 
127 127
 
128  
-def select_related_descend(field, restricted, requested, reverse=False):
  128
+def select_related_descend(field, restricted, requested, load_fields, reverse=False):
129 129
     """
130 130
     Returns True if this field should be used to descend deeper for
131 131
     select_related() purposes. Used by both the query construction code
132 132
     (sql.query.fill_related_selections()) and the model instance creation code
133  
-    (query.get_cached_row()).
  133
+    (query.get_klass_info()).
134 134
 
135 135
     Arguments:
136 136
      * field - the field to be checked
137 137
      * restricted - a boolean field, indicating if the field list has been
138 138
        manually restricted using a requested clause)
139 139
      * requested - The select_related() dictionary.
  140
+     * load_fields - the set of fields to be loaded on this model
140 141
      * reverse - boolean, True if we are checking a reverse select related
141 142
     """
142 143
     if not field.rel:
@@ -150,6 +151,14 @@ def select_related_descend(field, restricted, requested, reverse=False):
150 151
             return False
151 152
     if not restricted and field.null:
152 153
         return False
  154
+    if load_fields:
  155
+        if field.name not in load_fields:
  156
+            if restricted and field.name in requested:
  157
+                raise InvalidQuery("Field %s.%s cannot be both deferred"
  158
+                                   " and traversed using select_related"
  159
+                                   " at the same time." %
  160
+                                   (field.model._meta.object_name, field.name))
  161
+            return False
153 162
     return True
154 163
 
155 164
 # This function is needed because data descriptors must be defined on a class
7  django/db/models/sql/compiler.py
@@ -596,6 +596,7 @@ def fill_related_selections(self, opts=None, root_alias=None, cur_depth=1,
596 596
         if avoid_set is None:
597 597
             avoid_set = set()
598 598
         orig_dupe_set = dupe_set
  599
+        only_load = self.query.get_loaded_field_names()
599 600
 
600 601
         # Setup for the case when only particular related fields should be
601 602
         # included in the related selection.
@@ -607,7 +608,8 @@ def fill_related_selections(self, opts=None, root_alias=None, cur_depth=1,
607 608
                 restricted = False
608 609
 
609 610
         for f, model in opts.get_fields_with_model():
610  
-            if not select_related_descend(f, restricted, requested):
  611
+            if not select_related_descend(f, restricted, requested,
  612
+                                          only_load.get(model or self.query.model)):
611 613
                 continue
612 614
             # The "avoid" set is aliases we want to avoid just for this
613 615
             # particular branch of the recursion. They aren't permanently
@@ -680,7 +682,8 @@ def fill_related_selections(self, opts=None, root_alias=None, cur_depth=1,
680 682
                 if o.field.unique
681 683
             ]
682 684
             for f, model in related_fields:
683  
-                if not select_related_descend(f, restricted, requested, reverse=True):
  685
+                if not select_related_descend(f, restricted, requested,
  686
+                                              only_load.get(model), reverse=True):
684 687
                     continue
685 688
                 # The "avoid" set is aliases we want to avoid just for this
686 689
                 # particular branch of the recursion. They aren't permanently
12  django/db/models/sql/query.py
@@ -1845,9 +1845,15 @@ def get_loaded_field_names(self):
1845 1845
 
1846 1846
         If no fields are marked for deferral, returns an empty dictionary.
1847 1847
         """
1848  
-        collection = {}
1849  
-        self.deferred_to_data(collection, self.get_loaded_field_names_cb)
1850  
-        return collection
  1848
+        # We cache this because we call this function multiple times
  1849
+        # (compiler.fill_related_selections, query.iterator)
  1850
+        try:
  1851
+            return self._loaded_field_names_cache
  1852
+        except AttributeError:
  1853
+            collection = {}
  1854
+            self.deferred_to_data(collection, self.get_loaded_field_names_cb)
  1855
+            self._loaded_field_names_cache = collection
  1856
+            return collection
1851 1857
 
1852 1858
     def get_loaded_field_names_cb(self, target, model, fields):
1853 1859
         """
11  docs/ref/models/querysets.txt
@@ -1083,11 +1083,13 @@ to ``defer()``::
1083 1083
     # Load all fields immediately.
1084 1084
     my_queryset.defer(None)
1085 1085
 
  1086
+.. versionchanged:: 1.5
  1087
+
1086 1088
 Some fields in a model won't be deferred, even if you ask for them. You can
1087 1089
 never defer the loading of the primary key. If you are using
1088 1090
 :meth:`select_related()` to retrieve related models, you shouldn't defer the
1089  
-loading of the field that connects from the primary model to the related one
1090  
-(at the moment, that doesn't raise an error, but it will eventually).
  1091
+loading of the field that connects from the primary model to the related
  1092
+one, doing so will result in an error.
1091 1093
 
1092 1094
 .. note::
1093 1095
 
@@ -1147,9 +1149,12 @@ logically::
1147 1149
     # existing set of fields).
1148 1150
     Entry.objects.defer("body").only("headline", "body")
1149 1151
 
  1152
+.. versionchanged:: 1.5
  1153
+
1150 1154
 All of the cautions in the note for the :meth:`defer` documentation apply to
1151 1155
 ``only()`` as well. Use it cautiously and only after exhausting your other
1152  
-options.
  1156
+options. Also note that using :meth:`only` and omitting a field requested
  1157
+using :meth:`select_related` is an error as well.
1153 1158
 
1154 1159
 using
1155 1160
 ~~~~~
20  tests/modeltests/defer/tests.py
... ...
@@ -1,6 +1,6 @@
1 1
 from __future__ import absolute_import
2 2
 
3  
-from django.db.models.query_utils import DeferredAttribute
  3
+from django.db.models.query_utils import DeferredAttribute, InvalidQuery
4 4
 from django.test import TestCase
5 5
 
6 6
 from .models import Secondary, Primary, Child, BigChild, ChildProxy
@@ -73,9 +73,19 @@ def test_defer(self):
73 73
         self.assert_delayed(qs.defer("name").get(pk=p1.pk), 1)
74 74
         self.assert_delayed(qs.only("name").get(pk=p1.pk), 2)
75 75
 
76  
-        # DOES THIS WORK?
77  
-        self.assert_delayed(qs.only("name").select_related("related")[0], 1)
78  
-        self.assert_delayed(qs.defer("related").select_related("related")[0], 0)
  76
+        # When we defer a field and also select_related it, the query is
  77
+        # invalid and raises an exception.
  78
+        with self.assertRaises(InvalidQuery):
  79
+            qs.only("name").select_related("related")[0]
  80
+        with self.assertRaises(InvalidQuery):
  81
+            qs.defer("related").select_related("related")[0]
  82
+
  83
+        # With a depth-based select_related, all deferred ForeignKeys are
  84
+        # deferred instead of traversed.
  85
+        with self.assertNumQueries(3):
  86
+            obj = qs.defer("related").select_related()[0]
  87
+            self.assert_delayed(obj, 1)
  88
+            self.assertEqual(obj.related.id, s1.pk)
79 89
 
80 90
         # Saving models with deferred fields is possible (but inefficient,
81 91
         # since every field has to be retrieved first).
@@ -155,7 +165,7 @@ def test_defer_proxy(self):
155 165
         children = ChildProxy.objects.all().select_related().only('id', 'name')
156 166
         self.assertEqual(len(children), 1)
157 167
         child = children[0]
158  
-        self.assert_delayed(child, 1)
  168
+        self.assert_delayed(child, 2)
159 169
         self.assertEqual(child.name, 'p1')
160 170
         self.assertEqual(child.value, 'xx')
161 171
 
4  tests/regressiontests/defer_regress/models.py
@@ -47,3 +47,7 @@ def __unicode__(self):
47 47
 
48 48
 class Feature(models.Model):
49 49
     item = models.ForeignKey(SimpleItem)
  50
+
  51
+class ItemAndSimpleItem(models.Model):
  52
+    item = models.ForeignKey(Item)
  53
+    simple = models.ForeignKey(SimpleItem)
26  tests/regressiontests/defer_regress/tests.py
@@ -9,7 +9,7 @@
9 9
 from django.test import TestCase
10 10
 
11 11
 from .models import (ResolveThis, Item, RelatedItem, Child, Leaf, Proxy,
12  
-    SimpleItem, Feature)
  12
+    SimpleItem, Feature, ItemAndSimpleItem)
13 13
 
14 14
 
15 15
 class DeferRegressionTest(TestCase):
@@ -109,6 +109,7 @@ def test_basic(self):
109 109
                 Child,
110 110
                 Feature,
111 111
                 Item,
  112
+                ItemAndSimpleItem,
112 113
                 Leaf,
113 114
                 Proxy,
114 115
                 RelatedItem,
@@ -125,12 +126,16 @@ def test_basic(self):
125 126
                 ),
126 127
             )
127 128
         )
  129
+        # FIXME: This is dependent on the order in which tests are run --
  130
+        # this test case has to be the first, otherwise a LOT more classes
  131
+        # appear.
128 132
         self.assertEqual(
129 133
             klasses, [
130 134
                 "Child",
131 135
                 "Child_Deferred_value",
132 136
                 "Feature",
133 137
                 "Item",
  138
+                "ItemAndSimpleItem",
134 139
                 "Item_Deferred_name",
135 140
                 "Item_Deferred_name_other_value_text",
136 141
                 "Item_Deferred_name_other_value_value",
@@ -139,7 +144,7 @@ def test_basic(self):
139 144
                 "Leaf",
140 145
                 "Leaf_Deferred_child_id_second_child_id_value",
141 146
                 "Leaf_Deferred_name_value",
142  
-                "Leaf_Deferred_second_child_value",
  147
+                "Leaf_Deferred_second_child_id_value",
143 148
                 "Leaf_Deferred_value",
144 149
                 "Proxy",
145 150
                 "RelatedItem",
@@ -174,3 +179,20 @@ def test_resolve_columns(self):
174 179
         qs = ResolveThis.objects.defer('num')
175 180
         self.assertEqual(1, qs.count())
176 181
         self.assertEqual('Foobar', qs[0].name)
  182
+
  183
+    def test_defer_with_select_related(self):
  184
+        item1 = Item.objects.create(name="first", value=47)
  185
+        item2 = Item.objects.create(name="second", value=42)
  186
+        simple = SimpleItem.objects.create(name="simple", value="23")
  187
+        related = ItemAndSimpleItem.objects.create(item=item1, simple=simple)
  188
+
  189
+        obj = ItemAndSimpleItem.objects.defer('item').select_related('simple').get()
  190
+        self.assertEqual(obj.item, item1)
  191
+        self.assertEqual(obj.item_id, item1.id)
  192
+
  193
+        obj.item = item2
  194
+        obj.save()
  195
+
  196
+        obj = ItemAndSimpleItem.objects.defer('item').select_related('simple').get()
  197
+        self.assertEqual(obj.item, item2)
  198
+        self.assertEqual(obj.item_id, item2.id)
2  tests/regressiontests/select_related_regress/tests.py
@@ -133,7 +133,7 @@ def test_regression_12851(self):
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.