Fixed #10811 -- Made assigning unsaved objects to FK, O2O, and GFK raise ValueError.

This prevents silent data loss.

Thanks Aymeric Augustin for the initial patch and Loic Bistuer for the review.

django/contrib/contenttypes/fields.py

@@ -223,6 +223,11 @@ class GenericForeignKey(object):
         if value is not None:
             ct = self.get_content_type(obj=value)
             fk = value._get_pk_val()
+            if fk is None:
+                raise ValueError(
+                    'Cannot assign "%r": "%s" instance isn\'t saved in the database.' %
+                    (value, value._meta.object_name)
+                )
 
         setattr(instance, self.ct_field, ct)
         setattr(instance, self.fk_field, fk)

django/db/models/fields/related.py

@@ -617,13 +617,20 @@ class ReverseSingleRelatedObjectDescriptor(object):
             if related is not None:
                 setattr(related, self.field.related.get_cache_name(), None)
 
-        # Set the value of the related field
+            for lh_field, rh_field in self.field.related_fields:
-        for lh_field, rh_field in self.field.related_fields:
-            try:
-                setattr(instance, lh_field.attname, getattr(value, rh_field.attname))
-            except AttributeError:
                 setattr(instance, lh_field.attname, None)
 
+        # Set the values of the related field.
+        else:
+            for lh_field, rh_field in self.field.related_fields:
+                val = getattr(value, rh_field.attname)
+                if val is None:
+                    raise ValueError(
+                        'Cannot assign "%r": "%s" instance isn\'t saved in the database.' %
+                        (value, self.field.rel.to._meta.object_name)
+                    )
+                setattr(instance, lh_field.attname, val)
+
         # 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.

docs/releases/1.8.txt

@@ -218,19 +218,47 @@ Backwards incompatible changes in 1.8
     deprecation timeline for a given feature, its removal may appear as a
     backwards incompatible change.
 
-* Some operations on related objects such as
+Related object operations are run in a transaction
-  :meth:`~django.db.models.fields.related.RelatedManager.add()` or
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-  :ref:`direct assignment<direct-assignment>` ran multiple data modifying
-  queries without wrapping them in transactions. To reduce the risk of data
-  corruption, all data modifying methods that affect multiple related objects
-  (i.e. ``add()``, ``remove()``, ``clear()``, and
-  :ref:`direct assignment<direct-assignment>`) now perform their data modifying
-  queries from within a transaction, provided your database supports
-  transactions.
 
-  This has one backwards incompatible side effect, signal handlers triggered
+Some operations on related objects such as
-  from these methods are now executed within the method's transaction and
+:meth:`~django.db.models.fields.related.RelatedManager.add()` or
-  any exception in a signal handler will prevent the whole operation.
+:ref:`direct assignment<direct-assignment>` ran multiple data modifying
+queries without wrapping them in transactions. To reduce the risk of data
+corruption, all data modifying methods that affect multiple related objects
+(i.e. ``add()``, ``remove()``, ``clear()``, and :ref:`direct assignment
+<direct-assignment>`) now perform their data modifying queries from within a
+transaction, provided your database supports transactions.
+
+This has one backwards incompatible side effect, signal handlers triggered from
+these methods are now executed within the method's transaction and any
+exception in a signal handler will prevent the whole operation.
+
+Unassigning unsaved objects to relations raises an error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Assigning unsaved objects to a :class:`~django.db.models.ForeignKey`,
+:class:`~django.contrib.contenttypes.fields.GenericForeignKey`, and
+:class:`~django.db.models.OneToOneField` now raises a :exc:`ValueError`.
+
+Previously, the assignment of an unsaved object would be silently ignored.
+For example::
+
+    >>> book = Book.objects.create(name="Django")
+    >>> book.author = Author(name="John")
+    >>> book.author.save()
+    >>> book.save()
+
+    >>> Book.objects.get(name="Django")
+    >>> book.author
+    >>>
+
+Now, an error will be raised to prevent data loss::
+
+    >>> book.author = Author(name="john")
+    Traceback (most recent call last):
+    ...
+    ValueError: Cannot assign "<Author: John>": "Author" instance isn't saved in the database.
 
 Miscellaneous
 ~~~~~~~~~~~~~

docs/topics/db/examples/many_to_one.txt

@@ -52,6 +52,21 @@ Create an Article::
     >>> a.reporter
     <Reporter: John Smith>
 
+Note that you must save an object before it can be assigned to a foreign key
+relationship. For example, creating an ``Article`` with unsaved ``Reporter``
+raises ``ValueError``::
+
+    >>> r3 = Reporter(first_name='John', last_name='Smith', email='john@example.com')
+    >>> Article(headline="This is a test", pub_date=date(2005, 7, 27), reporter=r3)
+    Traceback (most recent call last):
+    ...
+    ValueError: 'Cannot assign "<Reporter: John Smith>": "Reporter" instance isn't saved in the database.'
+
+.. versionchanged:: 1.8
+
+    Previously, assigning unsaved objects did not raise an error and could
+    result in silent data loss.
+
 Article objects have access to their related Reporter objects::
 
     >>> r = a.reporter

docs/topics/db/examples/one_to_one.txt

@@ -89,6 +89,25 @@ Set the place back again, using assignment in the reverse direction::
     >>> p1.restaurant
     <Restaurant: Demon Dogs the restaurant>
 
+Note that you must save an object before it can be assigned to a one-to-one
+relationship. For example, creating an ``Restaurant`` with unsaved ``Place``
+raises ``ValueError``::
+
+    >>> p3 = Place(name='Demon Dogs', address='944 W. Fullerton')
+    >>> Restaurant(place=p3, serves_hot_dogs=True, serves_pizza=False)
+    Traceback (most recent call last):
+    ...
+    ValueError: 'Cannot assign "<Place: Demon Dogs>": "Place" instance isn't saved in the database.'
+    >>> p.restaurant = Restaurant(place=p, serves_hot_dogs=True, serves_pizza=False)
+    Traceback (most recent call last):
+    ...
+    ValueError: 'Cannot assign "<Restaurant: Demon Dogs the restaurant>": "Restaurant" instance isn't saved in the database.'
+
+.. versionchanged:: 1.8
+
+    Previously, assigning unsaved objects did not raise an error and could
+    result in silent data loss.
+
 Restaurant.objects.all() just returns the Restaurants, not the Places.  Note
 that there are two restaurants - Ace Hardware the Restaurant was created in the
 call to r.place = p2::

tests/admin_util/tests.py

@@ -109,8 +109,9 @@ class UtilTests(SimpleTestCase):
 
         simple_function = lambda obj: SIMPLE_FUNCTION
 
+        site_obj = Site.objects.create(domain=SITE_NAME)
         article = Article(
-            site=Site(domain=SITE_NAME),
+            site=site_obj,
             title=TITLE_TEXT,
             created=CREATED_DATE,
         )

tests/contenttypes_tests/tests.py

@@ -209,6 +209,27 @@ class GenericForeignKeyTests(IsolatedModelsTestCase):
         errors = checks.run_checks()
         self.assertEqual(errors, ['performed!'])
 
+    def test_unsaved_instance_on_generic_foreign_key(self):
+        """
+        #10811 -- Assigning an unsaved object to GenericForeignKey
+        should raise an exception.
+        """
+        class Model(models.Model):
+            content_type = models.ForeignKey(ContentType, null=True)
+            object_id = models.PositiveIntegerField(null=True)
+            content_object = GenericForeignKey('content_type', 'object_id')
+
+        author = Author(name='Author')
+        model = Model()
+        model.content_object = None   # no error here as content_type allows None
+        with self.assertRaisesMessage(ValueError,
+                                    'Cannot assign "%r": "%s" instance isn\'t saved in the database.'
+                                    % (author, author._meta.object_name)):
+            model.content_object = author   # raised ValueError here as author is unsaved
+
+        author.save()
+        model.content_object = author   # no error because the instance is saved
+
 
 class GenericRelationshipTests(IsolatedModelsTestCase):
 

tests/many_to_one_regress/tests.py

@@ -66,8 +66,10 @@ class ManyToOneRegressionTests(TestCase):
 
         # Creation using keyword argument and unsaved related instance (#8070).
         p = Parent()
-        c = Child(parent=p)
+        with self.assertRaisesMessage(ValueError,
-        self.assertTrue(c.parent is p)
+                            'Cannot assign "%r": "%s" instance isn\'t saved in the database.'
+                            % (p, Child.parent.field.rel.to._meta.object_name)):
+            Child(parent=p)
 
         # Creation using attname keyword argument and an id will cause the
         # related object to be fetched.

tests/multiple_database/tests.py

@@ -476,8 +476,6 @@ class QueryTestCase(TestCase):
         dive = Book.objects.using('other').create(title="Dive into Python",
             published=datetime.date(2009, 5, 4))
 
-        mark = Person.objects.using('other').create(name="Mark Pilgrim")
-
         # Set a foreign key with an object from a different database
         with self.assertRaises(ValueError):
             dive.editor = marty
@@ -492,54 +490,6 @@ class QueryTestCase(TestCase):
             with transaction.atomic(using='default'):
                 marty.edited.add(dive)
 
-        # BUT! if you assign a FK object when the base object hasn't
-        # been saved yet, you implicitly assign the database for the
-        # base object.
-        chris = Person(name="Chris Mills")
-        html5 = Book(title="Dive into HTML5", published=datetime.date(2010, 3, 15))
-        # initially, no db assigned
-        self.assertEqual(chris._state.db, None)
-        self.assertEqual(html5._state.db, None)
-
-        # old object comes from 'other', so the new object is set to use 'other'...
-        dive.editor = chris
-        html5.editor = mark
-        self.assertEqual(chris._state.db, 'other')
-        self.assertEqual(html5._state.db, 'other')
-        # ... but it isn't saved yet
-        self.assertEqual(list(Person.objects.using('other').values_list('name', flat=True)),
-            ['Mark Pilgrim'])
-        self.assertEqual(list(Book.objects.using('other').values_list('title', flat=True)),
-            ['Dive into Python'])
-
-        # When saved (no using required), new objects goes to 'other'
-        chris.save()
-        html5.save()
-        self.assertEqual(list(Person.objects.using('default').values_list('name', flat=True)),
-            ['Marty Alchin'])
-        self.assertEqual(list(Person.objects.using('other').values_list('name', flat=True)),
-            ['Chris Mills', 'Mark Pilgrim'])
-        self.assertEqual(list(Book.objects.using('default').values_list('title', flat=True)),
-            ['Pro Django'])
-        self.assertEqual(list(Book.objects.using('other').values_list('title', flat=True)),
-            ['Dive into HTML5', 'Dive into Python'])
-
-        # This also works if you assign the FK in the constructor
-        water = Book(title="Dive into Water", published=datetime.date(2001, 1, 1), editor=mark)
-        self.assertEqual(water._state.db, 'other')
-        # ... but it isn't saved yet
-        self.assertEqual(list(Book.objects.using('default').values_list('title', flat=True)),
-            ['Pro Django'])
-        self.assertEqual(list(Book.objects.using('other').values_list('title', flat=True)),
-            ['Dive into HTML5', 'Dive into Python'])
-
-        # When saved, the new book goes to 'other'
-        water.save()
-        self.assertEqual(list(Book.objects.using('default').values_list('title', flat=True)),
-            ['Pro Django'])
-        self.assertEqual(list(Book.objects.using('other').values_list('title', flat=True)),
-            ['Dive into HTML5', 'Dive into Python', 'Dive into Water'])
-
     def test_foreign_key_deletion(self):
         "Cascaded deletions of Foreign Key relations issue queries on the right database"
         mark = Person.objects.using('other').create(name="Mark Pilgrim")
@@ -1148,6 +1098,7 @@ class RouterTestCase(TestCase):
         # old object comes from 'other', so the new object is set to use the
         # source of 'other'...
         self.assertEqual(dive._state.db, 'other')
+        chris.save()
         dive.editor = chris
         html5.editor = mark
 

tests/one_to_one/models.py

@@ -30,6 +30,10 @@ class Restaurant(models.Model):
         return "%s the restaurant" % self.place.name
 
 
+class Bar(models.Model):
+    place = models.OneToOneField(Place, null=True)
+
+
 @python_2_unicode_compatible
 class Waiter(models.Model):
     restaurant = models.ForeignKey(Restaurant)

tests/one_to_one/tests.py

@@ -3,7 +3,7 @@ from __future__ import unicode_literals
 from django.db import transaction, IntegrityError
 from django.test import TestCase
 
-from .models import (Place, Restaurant, Waiter, ManualPrimaryKey, RelatedModel,
+from .models import (Place, Restaurant, Bar, Waiter, ManualPrimaryKey, RelatedModel,
     MultiModel)
 
 
@@ -128,3 +128,20 @@ class OneToOneTests(TestCase):
         with self.assertRaises(IntegrityError):
             with transaction.atomic():
                 mm.save()
+
+    def test_unsaved_object(self):
+        """
+        #10811 -- Assigning an unsaved object to a OneToOneField
+        should raise an exception.
+        """
+        place = Place(name='User', address='London')
+        with self.assertRaisesMessage(ValueError,
+                            'Cannot assign "%r": "%s" instance isn\'t saved in the database.'
+                            % (place, Restaurant.place.field.rel.to._meta.object_name)):
+            Restaurant.objects.create(place=place, serves_hot_dogs=True, serves_pizza=False)
+        bar = Bar()
+        p = Place(name='User', address='London')
+        with self.assertRaisesMessage(ValueError,
+                            'Cannot assign "%r": "%s" instance isn\'t saved in the database.'
+                            % (bar, p._meta.object_name)):
+            p.bar = bar

tests/one_to_one_regress/tests.py

@@ -96,11 +96,6 @@ class OneToOneRegressionTests(TestCase):
         r = Restaurant(place=p)
         self.assertTrue(r.place is p)
 
-        # Creation using keyword argument and unsaved related instance (#8070).
-        p = Place()
-        r = Restaurant(place=p)
-        self.assertTrue(r.place is p)
-
         # Creation using attname keyword argument and an id will cause the related
         # object to be fetched.
         p = Place.objects.get(name="Demon Dogs")