From 5643a3b51be338196d0b292d5626ad43648448d3 Mon Sep 17 00:00:00 2001 From: Anubhav Joshi Date: Mon, 19 May 2014 14:15:55 +0530 Subject: [PATCH] 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 | 5 +++ django/db/models/fields/related.py | 17 +++++--- docs/releases/1.8.txt | 52 +++++++++++++++++++------ docs/topics/db/examples/many_to_one.txt | 15 +++++++ docs/topics/db/examples/one_to_one.txt | 19 +++++++++ tests/admin_util/tests.py | 3 +- tests/contenttypes_tests/tests.py | 21 ++++++++++ tests/many_to_one_regress/tests.py | 6 ++- tests/multiple_database/tests.py | 51 +----------------------- tests/one_to_one/models.py | 4 ++ tests/one_to_one/tests.py | 19 ++++++++- tests/one_to_one_regress/tests.py | 5 --- 12 files changed, 141 insertions(+), 76 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 6ea225191a..a94ed3282c 100644 --- a/django/contrib/contenttypes/fields.py +++ b/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) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index ceae2cd563..9452b06dbd 100644 --- a/django/db/models/fields/related.py +++ b/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: - 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. diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index 087098b4ba..31f4fb2cbb 100644 --- a/docs/releases/1.8.txt +++ b/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` 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`) now perform their data modifying - queries from within a transaction, provided your database supports - transactions. +Related object operations are run in a transaction +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - 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. +Some operations on related objects such as +:meth:`~django.db.models.fields.related.RelatedManager.add()` or +:ref:`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 +`) 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" instance isn't saved in the database. Miscellaneous ~~~~~~~~~~~~~ diff --git a/docs/topics/db/examples/many_to_one.txt b/docs/topics/db/examples/many_to_one.txt index 1101865755..7de8e6f51f 100644 --- a/docs/topics/db/examples/many_to_one.txt +++ b/docs/topics/db/examples/many_to_one.txt @@ -52,6 +52,21 @@ Create an Article:: >>> a.reporter +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" 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 diff --git a/docs/topics/db/examples/one_to_one.txt b/docs/topics/db/examples/one_to_one.txt index b66d52d369..beb4aa01d9 100644 --- a/docs/topics/db/examples/one_to_one.txt +++ b/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 +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" 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" 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:: diff --git a/tests/admin_util/tests.py b/tests/admin_util/tests.py index 20980efffa..bab6e7799c 100644 --- a/tests/admin_util/tests.py +++ b/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, ) diff --git a/tests/contenttypes_tests/tests.py b/tests/contenttypes_tests/tests.py index 18c542bf4b..2bec2927bd 100644 --- a/tests/contenttypes_tests/tests.py +++ b/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): diff --git a/tests/many_to_one_regress/tests.py b/tests/many_to_one_regress/tests.py index adb51b6879..a19991fd3b 100644 --- a/tests/many_to_one_regress/tests.py +++ b/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) - 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. diff --git a/tests/multiple_database/tests.py b/tests/multiple_database/tests.py index f3b540e31a..88da4072ae 100644 --- a/tests/multiple_database/tests.py +++ b/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 diff --git a/tests/one_to_one/models.py b/tests/one_to_one/models.py index b54cabbaa3..f9c0a40ee0 100644 --- a/tests/one_to_one/models.py +++ b/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) diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index d5d955b08e..f0a7a175a7 100644 --- a/tests/one_to_one/tests.py +++ b/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 diff --git a/tests/one_to_one_regress/tests.py b/tests/one_to_one_regress/tests.py index f14d1eeabd..2f4c5fe6bf 100644 --- a/tests/one_to_one_regress/tests.py +++ b/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")