Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 1452d46

Browse files
committed
Fixed #6886: Tightened up ForeignKey and OneToOne field assignment. Specifically:
 * Raise a ValueError if you try to assign the wrong type of object. * Raise a ValueError if you try to assign None to a field not specified with null=True. * Cache the set value at set time instead of just at lookup time. This is a slightly backwards-incompatible change; see BackwardsIncompatibleChanges for more details. git-svn-id: http://code.djangoproject.com/svn/django/trunk@7574 bcc190cf-cafb-0310-a4f2-bffc1f526a37
1 parent d78f86a commit 1452d46

File tree

4 files changed

+118
-16
lines changed

4 files changed

+118
-16
lines changed

‎django/db/models/fields/related.py‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -182,14 +182,29 @@ def __get__(self, instance, instance_type=None):
182182
def __set__(self, instance, value):
183183
if instance is None:
184184
raise AttributeError, "%s must be accessed via instance" % self.related.opts.object_name
185+
186+
# The similarity of the code below to the code in
187+
# ReverseSingleRelatedObjectDescriptor is annoying, but there's a bunch
188+
# of small differences that would make a common base class convoluted.
189+
190+
# If null=True, we can assign null here, but otherwise the value needs
191+
# to be an instance of the related class.
192+
if value is None and self.related.field.null == False:
193+
raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' %
194+
(instance._meta.object_name, self.related.get_accessor_name()))
195+
elif value is not None and not isinstance(value, self.related.model):
196+
raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' %
197+
(value, instance._meta.object_name,
198+
self.related.get_accessor_name(), self.related.opts.object_name))
199+
185200
# Set the value of the related field
186201
setattr(value, self.related.field.rel.get_related_field().attname, instance)
187202

188-
# Clear the cache, if it exists
189-
try:
190-
delattr(value, self.related.field.get_cache_name())
191-
exceptAttributeError:
192-
pass
203+
# Since we already know what the related object is, seed the related
204+
# object caches now, too. This avoids another db hit if you get the
205+
# object you just set.
206+
setattr(instance, self.cache_name, value)
207+
setattr(value, self.related.field.get_cache_name(), instance)
193208

194209
class ReverseSingleRelatedObjectDescriptor(object):
195210
# This class provides the functionality that makes the related-object
@@ -225,18 +240,28 @@ def __get__(self, instance, instance_type=None):
225240
def __set__(self, instance, value):
226241
if instance is None:
227242
raise AttributeError, "%s must be accessed via instance" % self._field.name
243+
244+
# If null=True, we can assign null here, but otherwise the value needs
245+
# to be an instance of the related class.
246+
if value is None and self.field.null == False:
247+
raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' %
248+
(instance._meta.object_name, self.field.name))
249+
elif value is not None and not isinstance(value, self.field.rel.to):
250+
raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' %
251+
(value, instance._meta.object_name,
252+
self.field.name, self.field.rel.to._meta.object_name))
253+
228254
# Set the value of the related field
229255
try:
230256
val = getattr(value, self.field.rel.get_related_field().attname)
231257
except AttributeError:
232258
val = None
233259
setattr(instance, self.field.attname, val)
234260

235-
# Clear the cache, if it exists
236-
try:
237-
delattr(instance, self.field.get_cache_name())
238-
except AttributeError:
239-
pass
261+
# Since we already know what the related object is, seed the related
262+
# object cache now, too. This avoids another db hit if you get the
263+
# object you just set.
264+
setattr(instance, self.field.get_cache_name(), value)
240265

241266
class ForeignRelatedObjectsDescriptor(object):
242267
# This class provides the functionality that makes the related-object

‎tests/modeltests/one_to_one/models.py‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,8 @@ def __unicode__(self):
8080
>>> r.place
8181
<Place: Ace Hardware the place>
8282
83-
# Set the place back again, using assignment in the reverse direction. Need to
84-
# reload restaurant object first, because the reverse set can't update the
85-
# existing restaurant instance
83+
# Set the place back again, using assignment in the reverse direction.
8684
>>> p1.restaurant = r
87-
>>> r.save()
8885
>>> p1.restaurant
8986
<Restaurant: Demon Dogs the restaurant>
9087

‎tests/regressiontests/many_to_one_regress/models.py‎

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
"""
2+
Regression tests for a few FK bugs: #1578, #6886
3+
"""
4+
15
from django.db import models
26

37
# If ticket #1578 ever slips back in, these models will not be able to be
@@ -25,10 +29,48 @@ class Child(models.Model):
2529

2630

2731
__test__ = {'API_TESTS':"""
28-
>>> Third.AddManipulator().save(dict(id='3', name='An example', another=None))
32+
>>> Third.objects.create(id='3', name='An example')
2933
<Third: Third object>
3034
>>> parent = Parent(name = 'fred')
3135
>>> parent.save()
32-
>>> Child.AddManipulator().save(dict(name='bam-bam', parent=parent.id))
36+
>>> Child.objects.create(name='bam-bam', parent=parent)
3337
<Child: Child object>
38+
39+
#
40+
# Tests of ForeignKey assignment and the related-object cache (see #6886)
41+
#
42+
>>> p = Parent.objects.create(name="Parent")
43+
>>> c = Child.objects.create(name="Child", parent=p)
44+
45+
# Look up the object again so that we get a "fresh" object
46+
>>> c = Child.objects.get(name="Child")
47+
>>> p = c.parent
48+
49+
# Accessing the related object again returns the exactly same object
50+
>>> c.parent is p
51+
True
52+
53+
# But if we kill the cache, we get a new object
54+
>>> del c._parent_cache
55+
>>> c.parent is p
56+
False
57+
58+
# Assigning a new object results in that object getting cached immediately
59+
>>> p2 = Parent.objects.create(name="Parent 2")
60+
>>> c.parent = p2
61+
>>> c.parent is p2
62+
True
63+
64+
# Assigning None fails: Child.parent is null=False
65+
>>> c.parent = None
66+
Traceback (most recent call last):
67+
...
68+
ValueError: Cannot assign None: "Child.parent" does not allow null values.
69+
70+
# You also can't assign an object of the wrong type here
71+
>>> c.parent = First(id=1, second=1)
72+
Traceback (most recent call last):
73+
...
74+
ValueError: Cannot assign "<First: First object>": "Child.parent" must be a "Parent" instance.
75+
3476
"""}

‎tests/regressiontests/one_to_one_regress/models.py‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,42 @@ def __unicode__(self):
5050
<Restaurant: Demon Dogs the restaurant>
5151
>>> p1.bar
5252
<Bar: Demon Dogs the bar>
53+
54+
#
55+
# Regression test for #6886 (the related-object cache)
56+
#
57+
58+
# Look up the objects again so that we get "fresh" objects
59+
>>> p = Place.objects.get(name="Demon Dogs")
60+
>>> r = p.restaurant
61+
62+
# Accessing the related object again returns the exactly same object
63+
>>> p.restaurant is r
64+
True
65+
66+
# But if we kill the cache, we get a new object
67+
>>> del p._restaurant_cache
68+
>>> p.restaurant is r
69+
False
70+
71+
# Reassigning the Restaurant object results in an immediate cache update
72+
# We can't use a new Restaurant because that'll violate one-to-one, but
73+
# with a new *instance* the is test below will fail if #6886 regresses.
74+
>>> r2 = Restaurant.objects.get(pk=r.pk)
75+
>>> p.restaurant = r2
76+
>>> p.restaurant is r2
77+
True
78+
79+
# Assigning None fails: Place.restaurant is null=False
80+
>>> p.restaurant = None
81+
Traceback (most recent call last):
82+
...
83+
ValueError: Cannot assign None: "Place.restaurant" does not allow null values.
84+
85+
# You also can't assign an object of the wrong type here
86+
>>> p.restaurant = p
87+
Traceback (most recent call last):
88+
...
89+
ValueError: Cannot assign "<Place: Demon Dogs the place>": "Place.restaurant" must be a "Restaurant" instance.
90+
5391
"""}

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /