Commit 1452d462 authored by Jacob Kaplan-Moss's avatar Jacob Kaplan-Moss
Browse files

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
parent d78f86a2

django/db/models/fields/related.py

+35 −10
Original line number Diff line number Diff line
@@ -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

tests/modeltests/one_to_one/models.py

+1 −4
Original line number Diff line number Diff line
@@ -80,11 +80,8 @@ DoesNotExist: Restaurant matching query does not exist. 
>>> r.place

<Place: Ace Hardware the 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

<Restaurant: Demon Dogs the restaurant>

tests/regressiontests/many_to_one_regress/models.py

+44 −2
Original line number Diff line number Diff line 
"""

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')

<Third: Third object>

>>> parent = Parent(name = 'fred')

>>> parent.save()

>>> Child.AddManipulator().save(dict(name='bam-bam', parent=parent.id))

>>> Child.objects.create(name='bam-bam', parent=parent)

<Child: Child object>



#

# 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 "<First: First object>": "Child.parent" must be a "Parent" instance.



"""}

tests/regressiontests/one_to_one_regress/models.py

+38 −0
Original line number Diff line number Diff line
@@ -50,4 +50,42 @@ __test__ = {'API_TESTS':""" 
<Restaurant: Demon Dogs the restaurant>

>>> p1.bar

<Bar: Demon Dogs the 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: Demon Dogs the place>": "Place.restaurant" must be a "Restaurant" instance.



"""}