From 81e1a35c364e5353d2bf99368ad30a4184fbb653 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Mon, 16 Mar 2015 19:28:53 +0000 Subject: [PATCH] Fixed #24495 -- Allowed unsaved model instance assignment check to be bypassed. --- django/contrib/contenttypes/fields.py | 4 +++- django/db/models/fields/related.py | 5 ++-- docs/ref/contrib/contenttypes.txt | 7 ++++++ docs/ref/models/fields.txt | 32 ++++++++++++++++++++++++- docs/releases/1.8.txt | 6 +++++ docs/topics/db/examples/many_to_one.txt | 4 ++++ docs/topics/db/examples/one_to_one.txt | 4 ++++ tests/contenttypes_tests/tests.py | 27 +++++++++++++++++++++ tests/many_to_one/tests.py | 25 +++++++++++++++++++ tests/one_to_one/tests.py | 27 ++++++++++++++++++++- 10 files changed, 136 insertions(+), 5 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 1dddbeb41c..f72cd49b26 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -40,6 +40,8 @@ class GenericForeignKey(object): one_to_one = False related_model = None + allow_unsaved_instance_assignment = False + def __init__(self, ct_field='content_type', fk_field='object_id', for_concrete_model=True): self.ct_field = ct_field self.fk_field = fk_field @@ -250,7 +252,7 @@ class GenericForeignKey(object): if value is not None: ct = self.get_content_type(obj=value) fk = value._get_pk_val() - if fk is None: + 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) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 931c117cb8..45c9b15081 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -513,7 +513,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 None in related_pk: + if not self.related.field.allow_unsaved_instance_assignment and None in related_pk: raise ValueError( 'Cannot assign "%r": "%s" instance isn\'t saved in the database.' % (value, instance._meta.object_name) @@ -684,7 +684,7 @@ class ReverseSingleRelatedObjectDescriptor(object): else: for lh_field, rh_field in self.field.related_fields: pk = value._get_pk_val() - if pk is None: + 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) @@ -1534,6 +1534,7 @@ class ForeignObject(RelatedField): one_to_many = False one_to_one = False + allow_unsaved_instance_assignment = False requires_unique_target = True related_accessor_class = ForeignRelatedObjectsDescriptor rel_class = ForeignObjectRel diff --git a/docs/ref/contrib/contenttypes.txt b/docs/ref/contrib/contenttypes.txt index f741e30a37..ef50f84ce8 100644 --- a/docs/ref/contrib/contenttypes.txt +++ b/docs/ref/contrib/contenttypes.txt @@ -299,6 +299,13 @@ 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 + `. + .. admonition:: Primary key type compatibility The "object_id" field doesn't have to be the same type as the diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index d1fc04d3b5..9fd9bd7fa3 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1291,6 +1291,30 @@ 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`` @@ -1388,7 +1412,7 @@ that control how the relationship functions. * ``_id``: the ``id`` of the model that the ``ManyToManyField`` points to. - If the ``ManyToManyField`` points from and to the same model, the following + If the ``ManyToManyField`` points from and to the same model, the following fields are generated: * ``id``: the primary key of the relation. @@ -1483,6 +1507,12 @@ 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.txt b/docs/releases/1.8.txt index bbbe97a881..a0a27ee89e 100644 --- a/docs/releases/1.8.txt +++ b/docs/releases/1.8.txt @@ -713,6 +713,12 @@ Now, an error will be raised to prevent data loss:: ... ValueError: Cannot assign "": "Author" instance isn't saved in the database. +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. + 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 c5c3e10034..e43809fd1c 100644 --- a/docs/topics/db/examples/many_to_one.txt +++ b/docs/topics/db/examples/many_to_one.txt @@ -60,6 +60,10 @@ raises ``ValueError``:: ... ValueError: 'Cannot assign "": "Reporter" instance isn't saved in the database.' +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 Previously, assigning unsaved objects did not raise an error and could diff --git a/docs/topics/db/examples/one_to_one.txt b/docs/topics/db/examples/one_to_one.txt index f1bdf3de9c..f2a3f54508 100644 --- a/docs/topics/db/examples/one_to_one.txt +++ b/docs/topics/db/examples/one_to_one.txt @@ -101,6 +101,10 @@ raises ``ValueError``:: ... ValueError: 'Cannot assign "": "Restaurant" instance isn't saved in the database.' +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 Previously, assigning unsaved objects did not raise an error and could diff --git a/tests/contenttypes_tests/tests.py b/tests/contenttypes_tests/tests.py index 3ae4774033..ef01a56301 100644 --- a/tests/contenttypes_tests/tests.py +++ b/tests/contenttypes_tests/tests.py @@ -257,6 +257,33 @@ class GenericForeignKeyTests(IsolatedModelsTestCase): 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/many_to_one/tests.py b/tests/many_to_one/tests.py index fdb77b4711..af7503fba8 100644 --- a/tests/many_to_one/tests.py +++ b/tests/many_to_one/tests.py @@ -159,6 +159,31 @@ 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)) diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index d7d5793b0a..2cda25b0aa 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, transaction +from django.db import IntegrityError, connection, models, transaction from django.test import TestCase from .models import ( @@ -145,6 +145,31 @@ class OneToOneTests(TestCase): % (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): """ Regression test for #9023: accessing the reverse relationship shouldn't