From e4b813c0e723e774c5c7386a29f10bc807a47e47 Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Fri, 24 Jul 2015 07:51:40 -0400 Subject: [PATCH] [1.8.x] Fixed #25160 -- Moved unsaved model instance data loss check to Model.save() This mostly reverts 5643a3b51be338196d0b292d5626ad43648448d3 and 81e1a35c364e5353d2bf99368ad30a4184fbb653. Thanks Carl Meyer for review. Backport of 5980b05c1fad69eef907e0076aa2dc837edab529 from master --- django/contrib/contenttypes/fields.py | 6 +-- django/db/models/base.py | 24 ++++++++++++ django/db/models/fields/related.py | 9 +---- docs/ref/contrib/contenttypes.txt | 7 ---- docs/ref/models/fields.txt | 30 --------------- docs/releases/1.8.4.txt | 4 ++ docs/releases/1.8.txt | 18 ++++++++- docs/topics/db/examples/many_to_one.txt | 16 ++++---- docs/topics/db/examples/one_to_one.txt | 20 ++++------ tests/admin_utils/tests.py | 7 ++-- tests/contenttypes_tests/tests.py | 48 ------------------------ tests/generic_relations/tests.py | 10 +++++ tests/many_to_one/tests.py | 39 +++----------------- tests/model_fields/test_uuid.py | 15 +++++++- tests/one_to_one/tests.py | 49 +++++++------------------ 15 files changed, 107 insertions(+), 195 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index be19352fdb1..3865e9836de 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -32,6 +32,7 @@ class GenericForeignKey(object): one_to_one = False related_model = None + # For backwards compatibility; ignored as of Django 1.8.4. allow_unsaved_instance_assignment = False def __init__(self, ct_field="content_type", fk_field="object_id", for_concrete_model=True): @@ -243,11 +244,6 @@ class GenericForeignKey(object): if value is not None: ct = self.get_content_type(obj=value) fk = value._get_pk_val() - if not self.allow_unsaved_instance_assignment and 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/base.py b/django/db/models/base.py index 86ba01ffcc0..c237889af19 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -661,6 +661,30 @@ class Model(six.with_metaclass(ModelBase)): that the "save" must be an SQL insert or update (or equivalent for non-SQL backends), respectively. Normally, they should not be set. """ + # Ensure that a model instance without a PK hasn't been assigned to + # a ForeignKey or OneToOneField on this model. If the field is + # nullable, allowing the save() would result in silent data loss. + for field in self._meta.concrete_fields: + if field.is_relation: + # If the related field isn't cached, then an instance hasn't + # been assigned and there's no need to worry about this check. + try: + getattr(self, field.get_cache_name()) + except AttributeError: + continue + obj = getattr(self, field.name, None) + # A pk may have been assigned manually to a model instance not + # saved to the database (or auto-generated in a case like + # UUIDField), but we allow the save to proceed and rely on the + # database to raise an IntegrityError if applicable. If + # constraints aren't supported by the database, there's the + # unavoidable risk of data corruption. + if obj and obj.pk is None: + raise ValueError( + "save() prohibited to prevent data loss due to " + "unsaved related object '%s'." % field.name + ) + using = using or router.db_for_write(self.__class__, instance=self) if force_insert and (force_update or update_fields): raise ValueError("Cannot force both insert and updating in model saving.") diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index c0680eea81d..91bc7f00ba4 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -506,7 +506,7 @@ class SingleRelatedObjectDescriptor(object): raise ValueError('Cannot assign "%r": the current database router prevents this relation.' % value) related_pk = tuple(getattr(instance, field.attname) for field in self.related.field.foreign_related_fields) - if not self.related.field.allow_unsaved_instance_assignment and None in related_pk: + if None in related_pk: raise ValueError( 'Cannot assign "%r": "%s" instance isn\'t saved in the database.' % (value, instance._meta.object_name) @@ -669,12 +669,6 @@ class ReverseSingleRelatedObjectDescriptor(object): # Set the values of the related field. else: for lh_field, rh_field in self.field.related_fields: - pk = value._get_pk_val() - if not self.field.allow_unsaved_instance_assignment and pk 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, getattr(value, rh_field.attname)) # Since we already know what the related object is, seed the related @@ -1491,6 +1485,7 @@ class ForeignObject(RelatedField): one_to_many = False one_to_one = False + # For backwards compatibility; ignored as of Django 1.8.4. allow_unsaved_instance_assignment = False requires_unique_target = True related_accessor_class = ForeignRelatedObjectsDescriptor diff --git a/docs/ref/contrib/contenttypes.txt b/docs/ref/contrib/contenttypes.txt index 7dce890b155..510c6b2dc49 100644 --- a/docs/ref/contrib/contenttypes.txt +++ b/docs/ref/contrib/contenttypes.txt @@ -299,13 +299,6 @@ model: is ``True``. This mirrors the ``for_concrete_model`` argument to :meth:`~django.contrib.contenttypes.models.ContentTypeManager.get_for_model`. - .. attribute:: GenericForeignKey.allow_unsaved_instance_assignment - - .. versionadded:: 1.8 - - Works analogously to :attr:`ForeignKey.allow_unsaved_instance_assignment - `. - .. deprecated:: 1.7 This class used to be defined in ``django.contrib.contenttypes.generic``. diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index 8f66fc5a53a..7e2bde56c37 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1349,30 +1349,6 @@ The possible values for :attr:`~ForeignKey.on_delete` are found in If in doubt, leave it to its default of ``True``. -.. attribute:: ForeignKey.allow_unsaved_instance_assignment - - .. versionadded:: 1.8 - - This flag was added for backwards compatibility as older versions of - Django always allowed assigning unsaved model instances. - - Django prevents unsaved model instances from being assigned to a - ``ForeignKey`` field to prevent accidental data loss (unsaved foreign keys - are silently ignored when saving a model instance). - - If you require allowing the assignment of unsaved instances and aren't - concerned about the data loss possibility (e.g. you never save the objects - to the database), you can disable this check by creating a subclass of the - field class and setting its ``allow_unsaved_instance_assignment`` attribute - to ``True``. For example:: - - class UnsavedForeignKey(models.ForeignKey): - # A ForeignKey which can point to an unsaved object - allow_unsaved_instance_assignment = True - - class Book(models.Model): - author = UnsavedForeignKey(Author) - .. _ref-manytomany: ``ManyToManyField`` @@ -1575,12 +1551,6 @@ that control how the relationship functions. If in doubt, leave it to its default of ``True``. -.. attribute:: ManyToManyField.allow_unsaved_instance_assignment - - .. versionadded:: 1.8 - - Works analogously to :attr:`ForeignKey.allow_unsaved_instance_assignment`. - :class:`ManyToManyField` does not support :attr:`~Field.validators`. :attr:`~Field.null` has no effect since there is no way to require a diff --git a/docs/releases/1.8.4.txt b/docs/releases/1.8.4.txt index 77ca647ff0b..b9d7c12c538 100644 --- a/docs/releases/1.8.4.txt +++ b/docs/releases/1.8.4.txt @@ -27,3 +27,7 @@ Bugfixes * Fixed the recording of squashed migrations when running the ``migrate`` command (:ticket:`25231`). + +* Moved the :ref:`unsaved model instance assignment data loss check + ` to ``Model.save()`` to allow easier usage + of in-memory models (:ticket:`25160`). diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt index 1901d7fba4a..9f91d463402 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -684,9 +684,23 @@ 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. +.. _unsaved-model-instance-check-18: + Assigning unsaved objects to relations raises an error ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +.. note:: + + To more easily allow in-memory usage of models, this change was reverted in + Django 1.8.4 and replaced with a check during ``model.save()``. For example:: + + >>> book = Book.objects.create(name="Django") + >>> book.author = Author(name="John") + >>> book.save() + Traceback (most recent call last): + ... + ValueError: save() prohibited to prevent data loss due to unsaved related object 'author'. + 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`. @@ -713,8 +727,8 @@ Now, an error will be raised to prevent data loss:: If you require allowing the assignment of unsaved instances (the old behavior) and aren't concerned about the data loss possibility (e.g. you never save the objects to the database), you can disable this check by using the -:attr:`~django.db.models.ForeignKey.allow_unsaved_instance_assignment` -attribute. +``ForeignKey.allow_unsaved_instance_assignment`` attribute. (This attribute was +removed in 1.8.4 as it's no longer relevant.) Management commands that only accept positional arguments ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/topics/db/examples/many_to_one.txt b/docs/topics/db/examples/many_to_one.txt index e43809fd1c0..63b700c5a5d 100644 --- a/docs/topics/db/examples/many_to_one.txt +++ b/docs/topics/db/examples/many_to_one.txt @@ -55,19 +55,17 @@ 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) + >>> Article.objects.create(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.' + ValueError: save() prohibited to prevent data loss due to unsaved related object 'reporter'. -If you want to disable the unsaved instance check, you can use the -:attr:`~django.db.models.ForeignKey.allow_unsaved_instance_assignment` -attribute. +.. versionchanged:: 1.8.4 -.. versionchanged:: 1.8 - - Previously, assigning unsaved objects did not raise an error and could - result in silent data loss. + Previously, saving an object with unsaved related objects did not raise an + error and could result in silent data loss. In 1.8-1.8.3, unsaved model + instances couldn't be assigned to related fields, but this restriction was + removed to allow easier usage of in-memory models. Article objects have access to their related Reporter objects:: diff --git a/docs/topics/db/examples/one_to_one.txt b/docs/topics/db/examples/one_to_one.txt index f2a3f54508e..1cb70734375 100644 --- a/docs/topics/db/examples/one_to_one.txt +++ b/docs/topics/db/examples/one_to_one.txt @@ -92,23 +92,17 @@ 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) + >>> Restaurant.objects.create(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.' + ValueError: save() prohibited to prevent data loss due to unsaved related object 'place'. -If you want to disable the unsaved instance check, you can use the -:attr:`~django.db.models.ForeignKey.allow_unsaved_instance_assignment` -attribute. +.. versionchanged:: 1.8.4 -.. versionchanged:: 1.8 - - Previously, assigning unsaved objects did not raise an error and could - result in silent data loss. + Previously, saving an object with unsaved related objects did not raise an + error and could result in silent data loss. In 1.8-1.8.3, unsaved model + instances couldn't be assigned to related fields, but this restriction was + removed to allow easier usage of in-memory models. Restaurant.objects.all() just returns the Restaurants, not the Places. Note that there are two restaurants - Ace Hardware the Restaurant was created in the diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py index 5720fbb64de..1bc158acc0f 100644 --- a/tests/admin_utils/tests.py +++ b/tests/admin_utils/tests.py @@ -12,7 +12,7 @@ from django.contrib.admin.utils import ( ) from django.contrib.admin.views.main import EMPTY_CHANGELIST_VALUE from django.db import DEFAULT_DB_ALIAS, models -from django.test import TestCase +from django.test import SimpleTestCase, TestCase from django.utils import six from django.utils.formats import localize from django.utils.safestring import mark_safe @@ -94,7 +94,8 @@ class NestedObjectsTests(TestCase): n.collect([Vehicle.objects.first()]) -class UtilsTests(TestCase): +class UtilsTests(SimpleTestCase): + def test_values_from_lookup_field(self): """ Regression test for #12654: lookup_field @@ -112,7 +113,7 @@ class UtilsTests(TestCase): simple_function = lambda obj: SIMPLE_FUNCTION - site_obj = Site.objects.create(domain=SITE_NAME) + site_obj = Site(domain=SITE_NAME) article = Article( site=site_obj, title=TITLE_TEXT, diff --git a/tests/contenttypes_tests/tests.py b/tests/contenttypes_tests/tests.py index 350dc831624..8e27cf42901 100644 --- a/tests/contenttypes_tests/tests.py +++ b/tests/contenttypes_tests/tests.py @@ -212,54 +212,6 @@ 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 - - def test_unsaved_instance_on_generic_foreign_key_allowed_when_wanted(self): - """ - #24495 - Assigning an unsaved object to a GenericForeignKey - should be allowed when the allow_unsaved_instance_assignment - attribute has been set to True. - """ - class UnsavedGenericForeignKey(GenericForeignKey): - # A GenericForeignKey which can point to an unsaved object - allow_unsaved_instance_assignment = True - - class Band(models.Model): - name = models.CharField(max_length=50) - - class BandMember(models.Model): - band_ct = models.ForeignKey(ContentType) - band_id = models.PositiveIntegerField() - band = UnsavedGenericForeignKey('band_ct', 'band_id') - first_name = models.CharField(max_length=50) - last_name = models.CharField(max_length=50) - - beatles = Band(name='The Beatles') - john = BandMember(first_name='John', last_name='Lennon') - # This should not raise an exception as the GenericForeignKey between - # member and band has allow_unsaved_instance_assignment=True. - john.band = beatles - self.assertEqual(john.band, beatles) - class GenericRelationshipTests(IsolatedModelsTestCase): diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py index f886d3dc9de..3bd147b0c35 100644 --- a/tests/generic_relations/tests.py +++ b/tests/generic_relations/tests.py @@ -4,6 +4,7 @@ from django import forms from django.contrib.contenttypes.forms import generic_inlineformset_factory from django.contrib.contenttypes.models import ContentType from django.core.exceptions import FieldError +from django.db import IntegrityError from django.db.models import Q from django.test import TestCase from django.utils import six @@ -408,6 +409,15 @@ class GenericRelationsTests(TestCase): with self.assertRaisesMessage(FieldError, msg): TaggedItem.objects.get(content_object='') + def test_unsaved_instance_on_generic_foreign_key(self): + """ + Assigning an unsaved object to GenericForeignKey should raise an + exception on model.save(). + """ + quartz = Mineral(name="Quartz", hardness=7) + with self.assertRaises(IntegrityError): + TaggedItem.objects.create(tag="shiny", content_object=quartz) + class CustomWidget(forms.TextInput): pass diff --git a/tests/many_to_one/tests.py b/tests/many_to_one/tests.py index 9031d03e92a..8ea7f773cab 100644 --- a/tests/many_to_one/tests.py +++ b/tests/many_to_one/tests.py @@ -120,31 +120,6 @@ class ManyToOneTests(TestCase): self.assertFalse(hasattr(self.r2.article_set, 'remove')) self.assertFalse(hasattr(self.r2.article_set, 'clear')) - def test_assign_unsaved_check_override(self): - """ - #24495 - Assigning an unsaved object to a ForeignKey - should be allowed when the allow_unsaved_instance_assignment - attribute has been set to True. - """ - class UnsavedForeignKey(models.ForeignKey): - # A ForeignKey which can point to an unsaved object - allow_unsaved_instance_assignment = True - - class Band(models.Model): - name = models.CharField(max_length=50) - - class BandMember(models.Model): - band = UnsavedForeignKey(Band) - first_name = models.CharField(max_length=50) - last_name = models.CharField(max_length=50) - - beatles = Band(name='The Beatles') - john = BandMember(first_name='John', last_name='Lennon') - # This should not raise an exception as the ForeignKey between member - # and band has allow_unsaved_instance_assignment=True. - john.band = beatles - self.assertEqual(john.band, beatles) - def test_selects(self): self.r.article_set.create(headline="John's second story", pub_date=datetime.date(2005, 7, 29)) @@ -524,15 +499,13 @@ class ManyToOneTests(TestCase): # Creation using keyword argument and unsaved related instance (#8070). p = Parent() - 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) + msg = "save() prohibited to prevent data loss due to unsaved related object 'parent'." + with self.assertRaisesMessage(ValueError, msg): + Child.objects.create(parent=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)): - ToFieldChild(parent=p) + msg = "save() prohibited to prevent data loss due to unsaved related object 'parent'." + with self.assertRaisesMessage(ValueError, msg): + ToFieldChild.objects.create(parent=p) # Creation using attname keyword argument and an id will cause the # related object to be fetched. diff --git a/tests/model_fields/test_uuid.py b/tests/model_fields/test_uuid.py index 21c2869edf9..6ed74bd62b1 100644 --- a/tests/model_fields/test_uuid.py +++ b/tests/model_fields/test_uuid.py @@ -2,8 +2,8 @@ import json import uuid from django.core import exceptions, serializers -from django.db import models -from django.test import TestCase +from django.db import IntegrityError, models +from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature from .models import ( NullableUUIDModel, PrimaryKeyUUIDModel, RelatedToUUIDModel, UUIDGrandchild, @@ -151,3 +151,14 @@ class TestAsPrimaryKey(TestCase): def test_two_level_foreign_keys(self): # exercises ForeignKey.get_db_prep_value() UUIDGrandchild().save() + + +class TestAsPrimaryKeyTransactionTests(TransactionTestCase): + # Need a TransactionTestCase to avoid deferring FK constraint checking. + available_apps = ['model_fields'] + + @skipUnlessDBFeature('supports_foreign_keys') + def test_unsaved_fk(self): + u1 = PrimaryKeyUUIDModel() + with self.assertRaises(IntegrityError): + RelatedToUUIDModel.objects.create(uuid_fk=u1) diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index 420d55da387..94a6ad957ea 100644 --- a/tests/one_to_one/tests.py +++ b/tests/one_to_one/tests.py @@ -1,6 +1,6 @@ from __future__ import unicode_literals -from django.db import IntegrityError, connection, models, transaction +from django.db import IntegrityError, connection, transaction from django.test import TestCase from .models import ( @@ -134,41 +134,9 @@ class OneToOneTests(TestCase): 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)): + msg = "save() prohibited to prevent data loss due to unsaved related object 'place'." + with self.assertRaisesMessage(ValueError, msg): Restaurant.objects.create(place=place, serves_hot_dogs=True, serves_pizza=False) - bar = UndergroundBar() - 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.undergroundbar = bar - - def test_unsaved_object_check_override(self): - """ - #24495 - Assigning an unsaved object to a OneToOneField - should be allowed when the allow_unsaved_instance_assignment - attribute has been set to True. - """ - class UnsavedOneToOneField(models.OneToOneField): - # A OneToOneField which can point to an unsaved object - allow_unsaved_instance_assignment = True - - class Band(models.Model): - name = models.CharField(max_length=50) - - class BandManager(models.Model): - band = UnsavedOneToOneField(Band) - first_name = models.CharField(max_length=50) - last_name = models.CharField(max_length=50) - - band = Band(name='The Beatles') - manager = BandManager(first_name='Brian', last_name='Epstein') - # This should not raise an exception as the OneToOneField between - # manager and band has allow_unsaved_instance_assignment=True. - manager.band = band - self.assertEqual(manager.band, band) def test_reverse_relationship_cache_cascade(self): """ @@ -249,6 +217,11 @@ class OneToOneTests(TestCase): r = Restaurant(place=p) self.assertIs(r.place, 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") @@ -392,8 +365,12 @@ class OneToOneTests(TestCase): """ p = Place() b = UndergroundBar.objects.create() + msg = ( + 'Cannot assign "": "Place" ' + 'instance isn\'t saved in the database.' + ) with self.assertNumQueries(0): - with self.assertRaises(ValueError): + with self.assertRaisesMessage(ValueError, msg): p.undergroundbar = b def test_nullable_o2o_delete(self):