Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #10811 -- Made assigning unsaved objects to FK, O2O, and GFK ra…

…ise ValueError.

This prevents silent data loss.

Thanks Aymeric Augustin for the initial patch and Loic Bistuer for the review.
commit 5643a3b51be338196d0b292d5626ad43648448d3 1 parent 4f72e5f
@coder9042 coder9042 authored timgraham committed
View
5 django/contrib/contenttypes/fields.py
@@ -223,6 +223,11 @@ def __set__(self, instance, value):
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)
View
17 django/db/models/fields/related.py
@@ -617,13 +617,20 @@ def __set__(self, instance, value):
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:
- try:
- setattr(instance, lh_field.attname, getattr(value, rh_field.attname))
- except AttributeError:
+ for lh_field, rh_field in self.field.related_fields:
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.
View
54 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
- :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
- from these methods are now executed within the method's transaction and
- any exception in a signal handler will prevent the whole operation.
+Related object operations are run in a transaction
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Some operations on related objects such as
+: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 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
~~~~~~~~~~~~~
View
15 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
View
19 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::
View
3  tests/admin_util/tests.py
@@ -109,8 +109,9 @@ def get_admin_value(self, obj):
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,
)
View
21 tests/contenttypes_tests/tests.py
@@ -209,6 +209,27 @@ class Model(models.Model):
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):
View
6 tests/many_to_one_regress/tests.py
@@ -66,8 +66,10 @@ def test_fk_assignment_and_related_object_cache(self):
# Creation using keyword argument and unsaved related instance (#8070).
p = Parent()
- c = Child(parent=p)
- self.assertTrue(c.parent is p)
+ with self.assertRaisesMessage(ValueError,
+ '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.
View
51 tests/multiple_database/tests.py
@@ -476,8 +476,6 @@ def test_foreign_key_cross_database_protection(self):
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 @@ def test_foreign_key_cross_database_protection(self):
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 @@ def test_foreign_key_cross_database_protection(self):
# 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
View
4 tests/one_to_one/models.py
@@ -30,6 +30,10 @@ def __str__(self):
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)
View
19 tests/one_to_one/tests.py
@@ -3,7 +3,7 @@
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 @@ def test_multiple_o2o(self):
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
View
5 tests/one_to_one_regress/tests.py
@@ -96,11 +96,6 @@ def test_related_object_cache(self):
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")
Please sign in to comment.
Something went wrong with that request. Please try again.