Skip to content

Commit

Permalink
Fixed #24495 -- Allowed unsaved model instance assignment check to be…
Browse files Browse the repository at this point in the history
… bypassed.
  • Loading branch information
kaedroho authored and timgraham committed Mar 18, 2015
1 parent 02d78bb commit 81e1a35
Show file tree
Hide file tree
Showing 10 changed files with 136 additions and 5 deletions.
4 changes: 3 additions & 1 deletion django/contrib/contenttypes/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -250,7 +252,7 @@ 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:
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)
Expand Down
5 changes: 3 additions & 2 deletions django/db/models/fields/related.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ def __set__(self, instance, value):
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)
Expand Down Expand Up @@ -684,7 +684,7 @@ def __set__(self, instance, value):
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)
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions docs/ref/contrib/contenttypes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
<django.db.models.ForeignKey.allow_unsaved_instance_assignment>`.

.. admonition:: Primary key type compatibility

The "object_id" field doesn't have to be the same type as the
Expand Down
32 changes: 31 additions & 1 deletion docs/ref/models/fields.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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``
Expand Down Expand Up @@ -1388,7 +1412,7 @@ that control how the relationship functions.
* ``<other_model>_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.
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions docs/releases/1.8.txt
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,12 @@ Now, an error will be raised to prevent data loss::
...
ValueError: Cannot assign "<Author: John>": "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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
4 changes: 4 additions & 0 deletions docs/topics/db/examples/many_to_one.txt
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ raises ``ValueError``::
...
ValueError: 'Cannot assign "<Reporter: John Smith>": "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
Expand Down
4 changes: 4 additions & 0 deletions docs/topics/db/examples/one_to_one.txt
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ raises ``ValueError``::
...
ValueError: 'Cannot assign "<Restaurant: Demon Dogs the restaurant>": "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
Expand Down
27 changes: 27 additions & 0 deletions tests/contenttypes_tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,33 @@ class Model(models.Model):
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):

Expand Down
25 changes: 25 additions & 0 deletions tests/many_to_one/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,31 @@ def test_assign(self):
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))
Expand Down
27 changes: 26 additions & 1 deletion tests/one_to_one/tests.py
Original file line number Diff line number Diff line change
@@ -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 (
Expand Down Expand Up @@ -145,6 +145,31 @@ def test_unsaved_object(self):
% (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
Expand Down

0 comments on commit 81e1a35

Please sign in to comment.