From 1452d46240609ff39dacf9ea2f759ed600c2f09f Mon Sep 17 00:00:00 2001 From: Jacob Kaplan-Moss Date: Thu, 5 Jun 2008 00:39:32 +0000 Subject: [PATCH] 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 --- django/db/models/fields/related.py | 45 ++++++++++++++---- tests/modeltests/one_to_one/models.py | 5 +- .../many_to_one_regress/models.py | 46 ++++++++++++++++++- .../one_to_one_regress/models.py | 38 +++++++++++++++ 4 files changed, 118 insertions(+), 16 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index e3677c2f55..4ea0c3fb06 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -182,14 +182,29 @@ class SingleRelatedObjectDescriptor(object): def __set__(self, instance, value): if instance is None: raise AttributeError, "%s must be accessed via instance" % self.related.opts.object_name + + # The similarity of the code below to the code in + # ReverseSingleRelatedObjectDescriptor is annoying, but there's a bunch + # of small differences that would make a common base class convoluted. + + # If null=True, we can assign null here, but otherwise the value needs + # to be an instance of the related class. + if value is None and self.related.field.null == False: + raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' % + (instance._meta.object_name, self.related.get_accessor_name())) + elif value is not None and not isinstance(value, self.related.model): + raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' % + (value, instance._meta.object_name, + self.related.get_accessor_name(), self.related.opts.object_name)) + # Set the value of the related field setattr(value, self.related.field.rel.get_related_field().attname, instance) - # Clear the cache, if it exists - try: - delattr(value, self.related.field.get_cache_name()) - except AttributeError: - pass + # Since we already know what the related object is, seed the related + # object caches now, too. This avoids another db hit if you get the + # object you just set. + setattr(instance, self.cache_name, value) + setattr(value, self.related.field.get_cache_name(), instance) class ReverseSingleRelatedObjectDescriptor(object): # This class provides the functionality that makes the related-object @@ -225,6 +240,17 @@ class ReverseSingleRelatedObjectDescriptor(object): def __set__(self, instance, value): if instance is None: raise AttributeError, "%s must be accessed via instance" % self._field.name + + # If null=True, we can assign null here, but otherwise the value needs + # to be an instance of the related class. + if value is None and self.field.null == False: + raise ValueError('Cannot assign None: "%s.%s" does not allow null values.' % + (instance._meta.object_name, self.field.name)) + elif value is not None and not isinstance(value, self.field.rel.to): + raise ValueError('Cannot assign "%r": "%s.%s" must be a "%s" instance.' % + (value, instance._meta.object_name, + self.field.name, self.field.rel.to._meta.object_name)) + # Set the value of the related field try: val = getattr(value, self.field.rel.get_related_field().attname) @@ -232,11 +258,10 @@ class ReverseSingleRelatedObjectDescriptor(object): val = None setattr(instance, self.field.attname, val) - # Clear the cache, if it exists - try: - delattr(instance, self.field.get_cache_name()) - except AttributeError: - pass + # Since we already know what the related object is, seed the related + # object cache now, too. This avoids another db hit if you get the + # object you just set. + setattr(instance, self.field.get_cache_name(), value) class ForeignRelatedObjectsDescriptor(object): # This class provides the functionality that makes the related-object diff --git a/tests/modeltests/one_to_one/models.py b/tests/modeltests/one_to_one/models.py index 800ccddac2..6fa4dd8c18 100644 --- a/tests/modeltests/one_to_one/models.py +++ b/tests/modeltests/one_to_one/models.py @@ -80,11 +80,8 @@ DoesNotExist: Restaurant matching query does not exist. >>> r.place -# Set the place back again, using assignment in the reverse direction. Need to -# reload restaurant object first, because the reverse set can't update the -# existing restaurant instance +# Set the place back again, using assignment in the reverse direction. >>> p1.restaurant = r ->>> r.save() >>> p1.restaurant diff --git a/tests/regressiontests/many_to_one_regress/models.py b/tests/regressiontests/many_to_one_regress/models.py index 57bbcd8489..4e49df1555 100644 --- a/tests/regressiontests/many_to_one_regress/models.py +++ b/tests/regressiontests/many_to_one_regress/models.py @@ -1,3 +1,7 @@ +""" +Regression tests for a few FK bugs: #1578, #6886 +""" + from django.db import models # If ticket #1578 ever slips back in, these models will not be able to be @@ -25,10 +29,48 @@ class Child(models.Model): __test__ = {'API_TESTS':""" ->>> Third.AddManipulator().save(dict(id='3', name='An example', another=None)) +>>> Third.objects.create(id='3', name='An example') >>> parent = Parent(name = 'fred') >>> parent.save() ->>> Child.AddManipulator().save(dict(name='bam-bam', parent=parent.id)) +>>> Child.objects.create(name='bam-bam', parent=parent) + +# +# Tests of ForeignKey assignment and the related-object cache (see #6886) +# +>>> p = Parent.objects.create(name="Parent") +>>> c = Child.objects.create(name="Child", parent=p) + +# Look up the object again so that we get a "fresh" object +>>> c = Child.objects.get(name="Child") +>>> p = c.parent + +# Accessing the related object again returns the exactly same object +>>> c.parent is p +True + +# But if we kill the cache, we get a new object +>>> del c._parent_cache +>>> c.parent is p +False + +# Assigning a new object results in that object getting cached immediately +>>> p2 = Parent.objects.create(name="Parent 2") +>>> c.parent = p2 +>>> c.parent is p2 +True + +# Assigning None fails: Child.parent is null=False +>>> c.parent = None +Traceback (most recent call last): + ... +ValueError: Cannot assign None: "Child.parent" does not allow null values. + +# You also can't assign an object of the wrong type here +>>> c.parent = First(id=1, second=1) +Traceback (most recent call last): + ... +ValueError: Cannot assign "": "Child.parent" must be a "Parent" instance. + """} diff --git a/tests/regressiontests/one_to_one_regress/models.py b/tests/regressiontests/one_to_one_regress/models.py index c68fdfc780..99022882f2 100644 --- a/tests/regressiontests/one_to_one_regress/models.py +++ b/tests/regressiontests/one_to_one_regress/models.py @@ -50,4 +50,42 @@ __test__ = {'API_TESTS':""" >>> p1.bar + +# +# Regression test for #6886 (the related-object cache) +# + +# Look up the objects again so that we get "fresh" objects +>>> p = Place.objects.get(name="Demon Dogs") +>>> r = p.restaurant + +# Accessing the related object again returns the exactly same object +>>> p.restaurant is r +True + +# But if we kill the cache, we get a new object +>>> del p._restaurant_cache +>>> p.restaurant is r +False + +# Reassigning the Restaurant object results in an immediate cache update +# We can't use a new Restaurant because that'll violate one-to-one, but +# with a new *instance* the is test below will fail if #6886 regresses. +>>> r2 = Restaurant.objects.get(pk=r.pk) +>>> p.restaurant = r2 +>>> p.restaurant is r2 +True + +# Assigning None fails: Place.restaurant is null=False +>>> p.restaurant = None +Traceback (most recent call last): + ... +ValueError: Cannot assign None: "Place.restaurant" does not allow null values. + +# You also can't assign an object of the wrong type here +>>> p.restaurant = p +Traceback (most recent call last): + ... +ValueError: Cannot assign "": "Place.restaurant" must be a "Restaurant" instance. + """}