Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #10811 -- Assigning unsaved objects to FK, O2O and GFK raises ValueError. #2697

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

coder9042 commented May 21, 2014

Thanks @timgraham @loic for help and reviewing.

Contributor

coder9042 commented May 21, 2014

When the fix for this was written, the problem that came up was that tests in admin_inline and admin_views failed. The reason for their failure was in case of inlines the value of FK is set at a later point of time in save_new()(function overridden for the same purpose of assigning FK) rather than in construct_instance.
InlineForeignKeyField was already being excluded, but after construct_instance is called, I excluded it before the call to construct_instance, because there is no meaning to use setattr when there is going to be None there, so the problem of failing tests in admin_inline and admin_views was over.
But now there was one another problem, whenever there was new object creation in BaseInlineFormSet everything worked fine, as no ValueError raised because of fix for 10811 and save_new() does its work, but when an existing object was edited, problem occured as this time it used to get value in construct_instance, but since we had excluded InlineForeignKeyField, so similar to save_new(), I had to override save_existing()

What happened uptil now:

  • adding new_obj -> construct_instance(FK field set to None, in case when parent class object not created or FK field set if parent class obj existed) -> save_new(FK field set here again, overriden to avoid case where not set in construct_instance)
  • changing old_obj -> construct_instance(FK field set) -> save_existing(not overidden)

What is done here:

In fix #2697 , added check for FK in __set__ which led to raising of errors when that case occured as described above, so now InlineForeignKeyField does not gets passed in construct_instance as it is excluded before call to construct_instance rather than after.
Reason because this can be done for:

  • new_objs: The FK is set in overriden save_new() regardless of if its set to None or proper value in construct_instance, so it can be excluded.
  • old_objs: Problem as its FK field get value in construct_instance and save_existing() not overriden to do the same at later point of time, so did the same thing as in save_new()

In this way, our fix works fine and no original functionality altered.

Problem Solved!!
(Had a complete discussion on this with @timgraham )

Contributor

coder9042 commented May 21, 2014

The tests for this fix already included in edited tests by using with self.assertRaises(ValueError):

@aaugustin aaugustin and 1 other commented on an outdated diff May 21, 2014

django/forms/models.py
@@ -888,6 +889,19 @@ def save_new(self, form, commit=True):
form.save_m2m()
return obj
+ def save_existing(self, form, instance, commit=True):
@aaugustin

aaugustin May 21, 2014

Owner

The bar for introducing new features in the Model API is very high. Has this been discussed on the mailing list?

@aaugustin

aaugustin May 21, 2014

Owner

In fact, this new API doesn't appear to be used anywhere in the PR and it isn't documented. Why is it included in the patch?

@aaugustin

aaugustin May 21, 2014

Owner

Err -- ignore me, this is in model forms... It's too late to review patches correctly!

@coder9042

coder9042 May 21, 2014

Contributor

save_new is already being overridden for same purpose, i overrided save_existing and excluded inlineforeignkeyfield from being set in construct_instance as there it would result in None when new obj is created or else pk set explicitly(the reason for overriding save_new

I am not introducing any new thing, just using the idea which was used for new objects for the existing objects by overriding save_existing in the same way as save_new.
Overriding done in BaseInlineFormSet

Please see my initial comment in this PR. I have updated it and tried to explain the entire situation there

@coder9042 coder9042 closed this May 21, 2014

@coder9042 coder9042 reopened this May 21, 2014

Contributor

coder9042 commented May 21, 2014

Sorry, closed by mistake..pressed the other button....its too late for me too...

@charettes charettes commented on an outdated diff May 24, 2014

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)
@charettes

charettes May 24, 2014

Member

Remove the spaces around =.

@loic loic and 1 other commented on an outdated diff May 24, 2014

tests/model_formsets/tests.py
@@ -614,9 +614,6 @@ def test_inline_formsets_save_as_new(self):
'book_set-2-title': '',
}
- formset = AuthorBooksFormSet(data, instance=Author(), save_as_new=True)
- self.assertTrue(formset.is_valid())
@loic

loic May 24, 2014

Member

Why does this fail? Shouldn't the failure only happen when the formset is actually saved?

@coder9042

coder9042 May 24, 2014

Contributor

Removed by mistake

@loic loic and 1 other commented on an outdated diff May 24, 2014

tests/multiple_database/tests.py
@@ -495,30 +495,24 @@ def test_foreign_key_cross_database_protection(self):
# BUT! if you assign a FK object when the base object hasn't
@loic

loic May 24, 2014

Member

Does this comment need updating?

@loic

loic May 24, 2014

Member

I even wonder if this part of the test should be kept at all, it tests specifically for the "feature" you are removing.

@coder9042

coder9042 May 24, 2014

Contributor

Will remove this part

@loic loic commented on an outdated diff May 24, 2014

docs/releases/1.8.txt
@@ -219,6 +219,9 @@ Backwards incompatible changes in 1.8
from these methods are now executed within the method's transaction and
any exception in a signal handler will prevent the whole operation.
+* A check has been introduced to prevent assigning unsaved objects to
@loic

loic May 24, 2014

Member

I wouldn't formulate this as a "check" since this means something different with the new "check framework". Maybe just Assigning unsaved objects toForeignKeynow raises aValueError..

@loic loic and 3 others commented on an outdated diff May 24, 2014

django/db/models/fields/related.py
@@ -620,7 +620,12 @@ def __set__(self, instance, value):
# 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))
+ val = getattr(value, rh_field.attname)
@loic

loic May 24, 2014

Member

I haven't had time to look at this in details but my gut feeling is that this is fragile. I'm also curious where the AttributeError (and its None fallback) fits into the new paradigm. @akaariai care to comment?

@loic

loic Jun 3, 2014

Member

It looks like we already used this approach in SingleRelatedObjectDescriptor (raise a ValueError on unsaved objects) so I guess it's fine to use it here as well. I'm still unsure how this interacts with the AttributeError case but I guess it can be revisited later if necessary.

@aaugustin

aaugustin Jun 3, 2014

Owner

Indeed, my last iteration of the patch before @coder9042 took handled AttributeError like None, see https://code.djangoproject.com/attachment/ticket/10811/10811-2.patch.

@loic

loic Jun 3, 2014

Member

Can't figure out when the AttributeError is supposed to happen, I'm gonna try to remove its handling and see what breaks in the test suite.

@loic

loic Jun 3, 2014

Member

Alright so the AttributeError is when value is None, wouldn't it be better to handle None directly? I'd say it would have the merit of being explicit, some feel strongly about "asking forgiveness" rather than "asking permission" but here the "forgiveness" is rather indirect.

See https://gist.github.com/57028547e85f7fea928e

@loic

loic Jun 4, 2014

Member

Replaced the snippet in the comment above with a gist, I think this diff makes the code much clearer wrt its intentions. Test suite passes without problem.

@coder9042

coder9042 Jun 4, 2014

Contributor

Ok I understand this. I will update the PR soon.
Thanks

@loic

loic Jun 4, 2014

Member

@coder9042 Hang on, let's see if @timgraham and @aaugustin agree with the change.

@timgraham

timgraham Jun 4, 2014

Owner

Makes sense to me.

Contributor

coder9042 commented May 26, 2014

Is anthing else required?

@timgraham timgraham and 1 other commented on an outdated diff May 26, 2014

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)
@timgraham

timgraham May 26, 2014

Owner

wrong spacing around =
site_obj = Site.objects.create(domain=SITE_NAME)

@coder9042

coder9042 May 26, 2014

Contributor

I was just told above to remove it. Which one should I keep

@timgraham

timgraham May 26, 2014

Owner

use flake8, it will tell you how to fix it.

Owner

timgraham commented May 26, 2014

I think the documentation should probably be beefed up a bit, e.g. say why we made the change so that assigning unsaved objects to a FK raises an error. Also, mention this restriction in the model topic or reference doc.

Contributor

coder9042 commented May 27, 2014

@timgraham @apollo13
I have added the test, please see if that is what is required.

@timgraham timgraham commented on an outdated diff May 27, 2014

tests/model_forms/tests.py
@@ -160,8 +160,38 @@ class Meta:
fields = '__all__'
model = CustomErrorMessage
+#Form for testing #13776
@timgraham

timgraham May 27, 2014

Owner

inline comments should have a space after #

@timgraham timgraham commented on an outdated diff May 27, 2014

tests/model_forms/tests.py
@@ -160,8 +160,38 @@ class Meta:
fields = '__all__'
model = CustomErrorMessage
+#Form for testing #13776
+class FormForTestingIsValid(forms.ModelForm):
+ class Meta:
+ model = Student
+ fields = '__all__'
+
+ def __init__(self, *a, **k):
@timgraham

timgraham May 27, 2014

Owner

please use *args, **kwargs

@timgraham timgraham commented on an outdated diff May 27, 2014

tests/model_forms/tests.py
class ModelFormBaseTest(TestCase):
+ def setUp(self):
@timgraham

timgraham May 27, 2014

Owner

why add a setUp method when the user is only used in the new test?

@timgraham timgraham commented on an outdated diff May 27, 2014

tests/model_forms/models.py
@@ -401,3 +401,15 @@ class Character(models.Model):
class StumpJoke(models.Model):
most_recently_fooled = models.ForeignKey(Character, limit_choices_to=today_callable_dict, related_name="+")
has_fooled_today = models.ManyToManyField(Character, limit_choices_to=today_callable_q, related_name="+")
+
+#Models for #13776
+
@timgraham

timgraham May 27, 2014

Owner

remove newlines

@timgraham timgraham and 1 other commented on an outdated diff May 27, 2014

django/forms/models.py
@@ -417,6 +414,9 @@ def _post_clean(self):
if isinstance(field, InlineForeignKeyField):
exclude.append(name)
+ # Update the model instance with self.cleaned_data.
+ self.instance = construct_instance(self, self.instance, opts.fields, exclude)
@timgraham

timgraham May 27, 2014

Owner

concerned that opts.fields -> exclude change here may break something.

@coder9042

coder9042 May 27, 2014

Contributor

It does not as the conditions that _get_validation_excludes checks are also checked in construct_instance.
We can see here: http://dpaste.com/2C7JXK2

@timgraham

timgraham Jun 2, 2014

Owner

Okay, I've reviewed this part and it makes sense.

@coder9042 coder9042 changed the title from Fixed #10811 -- Assigning unsaved objects to FK raises ValueError to Fixed #10811, #13776 -- Assigning unsaved objects to FK raises ValueError, is_valid in ModelForm does not raises ValueError May 27, 2014

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/models.py
@@ -401,3 +401,14 @@ class Character(models.Model):
class StumpJoke(models.Model):
most_recently_fooled = models.ForeignKey(Character, limit_choices_to=today_callable_dict, related_name="+")
has_fooled_today = models.ManyToManyField(Character, limit_choices_to=today_callable_q, related_name="+")
+
+
+#Models for #13776
@timgraham

timgraham May 30, 2014

Owner

space after #

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/models.py
@@ -401,3 +401,14 @@ class Character(models.Model):
class StumpJoke(models.Model):
most_recently_fooled = models.ForeignKey(Character, limit_choices_to=today_callable_dict, related_name="+")
has_fooled_today = models.ManyToManyField(Character, limit_choices_to=today_callable_q, related_name="+")
+
+
+#Models for #13776
+class User(models.Model):
+ name = models.CharField(max_length=30)
+ phone = models.IntegerField(max_length=11, null=True)
+
+
+class Student(models.Model):
@timgraham

timgraham May 30, 2014

Owner

try to avoid adding new models where possible (slows down test suite). I wonder if we could use the StumpJoke model (it has a FK with null=False). If not, we could probably at least make the FK on the new model to an existing model instead of adding a Student model.

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/models.py
@@ -401,3 +401,14 @@ class Character(models.Model):
class StumpJoke(models.Model):
most_recently_fooled = models.ForeignKey(Character, limit_choices_to=today_callable_dict, related_name="+")
has_fooled_today = models.ManyToManyField(Character, limit_choices_to=today_callable_q, related_name="+")
+
+
+#Models for #13776
+class User(models.Model):
+ name = models.CharField(max_length=30)
+ phone = models.IntegerField(max_length=11, null=True)
+
+
+class Student(models.Model):
+ user = models.ForeignKey(User, on_delete=models.PROTECT)
@timgraham

timgraham May 30, 2014

Owner

try to keep model as simple as possible. In this case, I don't think on_delete=models.PROTECT is necessary to reproduce the bug.

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/tests.py
@@ -161,6 +161,18 @@ class Meta:
model = CustomErrorMessage
+# Form for testing #13776
+class FormForTestingIsValid(forms.ModelForm):
+ class Meta:
+ model = Student
+ fields = '__all__'
+
+ def __init__(self, *args, **kwargs):
+ super(FormForTestingIsValid, self).__init__(*args, **kwargs)
+ for field in self.fields:
@timgraham

timgraham May 30, 2014

Owner

keep it as simple as possible and make it explicit: self.fields['user'].required=False
I don't think setting other fields to required=False is important for this bug, right?

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/tests.py
@@ -199,6 +211,23 @@ def test_empty_fields_to_construct_instance(self):
instance = construct_instance(form, Person(), fields=())
self.assertEqual(instance.name, '')
+ def test_blank_with_null_foreign_key_field(self):
+ user = User.objects.create(name='Username')
@timgraham

timgraham May 30, 2014

Owner

Add a docstring briefly explaining what we're testing and the ticket # so it can be referenced for further info.

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/tests.py
@@ -161,6 +161,18 @@ class Meta:
model = CustomErrorMessage
+# Form for testing #13776
+class FormForTestingIsValid(forms.ModelForm):
@timgraham

timgraham May 30, 2014

Owner

can you put the test in the test method? I think that's preferable so it's not split up.

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/tests.py
@@ -199,6 +211,23 @@ def test_empty_fields_to_construct_instance(self):
instance = construct_instance(form, Person(), fields=())
self.assertEqual(instance.name, '')
+ def test_blank_with_null_foreign_key_field(self):
+ user = User.objects.create(name='Username')
+ data = {'std': 'Engineering'}
+ data2 = {'std': 'Engineering', 'user': user.pk}
+
+ f1 = FormForTestingIsValid()
+ self.assertTrue(not f1.is_valid()) # form not valid because its not bound
@timgraham

timgraham May 30, 2014

Owner

or ... self.assertFalse(f1.is_valid()) although I'm not sure this assertion is really necessary... testing that unbound forms aren't valid is probably tested elsewhere.

@timgraham timgraham commented on an outdated diff May 30, 2014

tests/model_forms/tests.py
@@ -199,6 +211,23 @@ def test_empty_fields_to_construct_instance(self):
instance = construct_instance(form, Person(), fields=())
self.assertEqual(instance.name, '')
+ def test_blank_with_null_foreign_key_field(self):
+ user = User.objects.create(name='Username')
+ data = {'std': 'Engineering'}
+ data2 = {'std': 'Engineering', 'user': user.pk}
+
+ f1 = FormForTestingIsValid()
+ self.assertTrue(not f1.is_valid()) # form not valid because its not bound
+ self.assertEqual(f1.errors, {})
+
+ f2 = FormForTestingIsValid(data) # form valid because required=False
+ self.assertTrue(f2.is_valid())
+
+ f3 = FormForTestingIsValid(data2)
+ self.assertTrue(f3.is_valid()) # form valid because required=False
@timgraham

timgraham May 30, 2014

Owner

# form valid because required=False --> but all the fields are required so not sure that comment really makes sense?

@timgraham timgraham commented on an outdated diff May 30, 2014

docs/releases/1.8.txt
@@ -219,6 +219,13 @@ Backwards incompatible changes in 1.8
from these methods are now executed within the method's transaction and
any exception in a signal handler will prevent the whole operation.
+* Assigning unsaved objects to :class:`~django.db.models.ForeignKey` now raises a ``ValueError``.
@timgraham

timgraham May 30, 2014

Owner

to a ForeignKey

@timgraham timgraham commented on an outdated diff May 30, 2014

docs/ref/models/fields.txt
@@ -1062,6 +1062,10 @@ avoid the overhead of an index if you are creating a foreign key for
consistency rather than joins, or if you will be creating an alternative index
like a partial or multiple column index.
+.. versionchanged:: 1.8
@timgraham

timgraham May 30, 2014

Owner

This is really not the best place for this advice. Try to find somewhere where it talks about assigned objects to foreign keys, maybe topics/db/examples/many_to_one.txt

Contributor

coder9042 commented May 30, 2014

The suggested changes have been done.
The merged build is failing in some test in command_sql for dbs other than sqlite. In my system its failing even on master with mysql and python2.7 with exact same error.
Do you think its probably due to cb9c9a7 ?

@timgraham timgraham and 1 other commented on an outdated diff Jun 2, 2014

tests/model_forms/models.py
@@ -401,3 +401,9 @@ class Character(models.Model):
class StumpJoke(models.Model):
most_recently_fooled = models.ForeignKey(Character, limit_choices_to=today_callable_dict, related_name="+")
has_fooled_today = models.ManyToManyField(Character, limit_choices_to=today_callable_q, related_name="+")
+
+
+# Model for #13776
+class Student(models.Model):
+ user = models.ForeignKey(Character)
@timgraham

timgraham Jun 2, 2014

Owner

call this field character to make it less confusing.

Also, could you open a PR for just the changes required to fix #13776? I think that is a subset of this PR which we can merge first.

@coder9042

coder9042 Jun 3, 2014

Contributor

@timgraham
Here #2757 .
I will convert this PR back to only for #10811 after the above is merged.

Member

loic commented Jun 3, 2014

@coder9042 Could you add a test case to verify that assigning unsaved objects to OneToOneField and GenericForeignKey aren't also affected by this issue? We need to ensure that things remain consistent across the different relations.

@coder9042 coder9042 closed this Jun 3, 2014

@coder9042 coder9042 reopened this Jun 3, 2014

Contributor

coder9042 commented Jun 3, 2014

Sorry closed again by mistake.
@loic
Added test for O2O.

@loic loic and 1 other commented on an outdated diff Jun 3, 2014

django/db/models/fields/related.py
@@ -620,7 +620,12 @@ def __set__(self, instance, value):
# 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))
+ val = getattr(value, rh_field.attname)
+ if val is None:
+ raise ValueError("The '%s' instance isn\'t saved in the database." %
Contributor

coder9042 commented Jun 3, 2014

@loic
Improved test for OneToOne.
Fix and test for GenericForeignKey added.
@timgraham
Made the changes you suggested.
I think we need to add more into the docs regarding O2O and GFK now. I could not find a suitable place for GFK. Please if you could point that out.

Contributor

coder9042 commented Jun 4, 2014

@loic @timgraham @aaugustin
I have updated #2757 . I think that now it is good to go.
Those changes haven't been done here. So for those changes please refer to the other PR.

@coder9042 coder9042 changed the title from Fixed #10811, #13776 -- Assigning unsaved objects to FK raises ValueError, is_valid in ModelForm does not raises ValueError to Fixed #10811 -- Assigning unsaved objects to FK, O2O and GFK raises V… … …alueError. Jun 4, 2014

@coder9042 coder9042 changed the title from Fixed #10811 -- Assigning unsaved objects to FK, O2O and GFK raises V… … …alueError. to Fixed #10811 -- Assigning unsaved objects to FK, O2O and GFK raises ValueError. Jun 4, 2014

Contributor

coder9042 commented Jun 4, 2014

@timgraham @loic
Since the other PR is now merged, I have rebased this one.
Looking to forward to this being merged as well.

@timgraham timgraham commented on an outdated diff Jun 4, 2014

docs/releases/1.8.txt
@@ -230,6 +230,15 @@ Backwards incompatible changes in 1.8
from these methods are now executed within the method's transaction and
any exception in a signal handler will prevent the whole operation.
+* Assigning unsaved objects to a :class:`~django.db.models.ForeignKey`,
+ :class:`~django.contrib.contenttypes.fields.GenericForeignKey` and
@timgraham

timgraham Jun 4, 2014

Owner

comma after GFK
(when you have a list like: A, B, (and|or, etc.) C please always include a comma before the conjunction)

@timgraham timgraham commented on an outdated diff Jun 4, 2014

tests/contenttypes_tests/tests.py
@@ -209,6 +209,22 @@ class Model(models.Model):
errors = checks.run_checks()
self.assertEqual(errors, ['performed!'])
+ def test_unsaved_instance_on_generic_foreign_key(self):
+ 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.assertRaises(ValueError):
@timgraham

timgraham Jun 4, 2014

Owner

please use self.assertRaisesMessage or six.assertRaisesRegex rather than self.assertRaises to check the exception's message as well

@timgraham timgraham commented on an outdated diff Jun 4, 2014

tests/one_to_one/tests.py
@@ -128,3 +128,13 @@ def test_multiple_o2o(self):
with self.assertRaises(IntegrityError):
with transaction.atomic():
mm.save()
+
+ def test_unsaved_object(self):
+ # Refs. 10811
@timgraham

timgraham Jun 4, 2014

Owner

"""
#10811 -- Assigning an unsaved object to a OneToOneField should raise an exception.
"""

@timgraham timgraham commented on an outdated diff Jun 4, 2014

tests/one_to_one/tests.py
@@ -128,3 +128,13 @@ def test_multiple_o2o(self):
with self.assertRaises(IntegrityError):
with transaction.atomic():
mm.save()
+
+ def test_unsaved_object(self):
+ # Refs. 10811
+ place = Place(name='User', address='London')
+ with self.assertRaises(ValueError):
+ Restaurant.objects.create(place=place, serves_hot_dogs=True, serves_pizza=False)
+ bar = Bar()
+ with self.assertRaises(ValueError):
+ p = Place(name='User', address='London')
@timgraham

timgraham Jun 4, 2014

Owner

move this line outside assertRaises (keep assertRaises blocks as small as possible so it's clear what is expected to throw the error)

@timgraham timgraham commented on an outdated diff Jun 4, 2014

tests/one_to_one_regress/tests.py
@@ -98,8 +98,8 @@ def test_related_object_cache(self):
# Creation using keyword argument and unsaved related instance (#8070).
p = Place()
- r = Restaurant(place=p)
- self.assertTrue(r.place is p)
+ with self.assertRaises(ValueError):
@timgraham

timgraham Jun 4, 2014

Owner

this seems redundant with what you added in tests/one_to_one

@timgraham timgraham commented on an outdated diff Jun 4, 2014

docs/topics/db/examples/many_to_one.txt
@@ -52,6 +52,14 @@ Create an Article::
>>> a.reporter
<Reporter: John Smith>
+Creating an Article from unsaved Reporter raises ValueError::
@timgraham

timgraham Jun 4, 2014

Owner

Add: "Note that you must save an object before it can be assigned to a foreign key relationship. For example, creating..."

with -> from
`` around Article, Reporter, and ValueError

Contributor

coder9042 commented Jun 4, 2014

@timgraham @loic
Pushed all the changes.
Anything else required or its good?

@timgraham timgraham commented on an outdated diff Jun 5, 2014

tests/many_to_one_regress/tests.py
@@ -66,8 +66,8 @@ 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.assertRaises(ValueError):
@timgraham

timgraham Jun 5, 2014

Owner

assertRaisesMessage

@timgraham timgraham commented on an outdated diff Jun 5, 2014

tests/one_to_one/tests.py
@@ -128,3 +128,15 @@ 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.assertRaises(ValueError):
@timgraham

timgraham Jun 5, 2014

Owner

assertRaisesMessage

@timgraham timgraham commented on an outdated diff Jun 5, 2014

tests/one_to_one/tests.py
@@ -128,3 +128,15 @@ 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.assertRaises(ValueError):
+ Restaurant.objects.create(place=place, serves_hot_dogs=True, serves_pizza=False)
+ bar = Bar()
+ p = Place(name='User', address='London')
+ with self.assertRaises(ValueError):
@timgraham

timgraham Jun 5, 2014

Owner

assertRaisesMessage

Owner

timgraham commented Jun 5, 2014

buildbot, test this please.

@timgraham timgraham commented on an outdated diff Jun 5, 2014

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, ct)
@timgraham

timgraham Jun 5, 2014

Owner

I don't think ct is the correct value to use in this error message. Shouldn't it be the type of the value?

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/ref/contrib/contenttypes.txt
@@ -359,6 +359,9 @@ normal field object, these examples will *not* work::
Likewise, :class:`~django.contrib.contenttypes.fields.GenericForeignKey`\s
does not appear in :class:`~django.forms.ModelForm`\s.
+Assigning unsaved instance to ``GenericForeignKey`` will raise ``ValueError`` similar
@timgraham

timgraham Jun 5, 2014

Owner

This seems a bit out of place, I think we can probably just omit it actually.

@timgraham timgraham commented on the diff Jun 5, 2014

tests/multiple_database/tests.py
@@ -492,54 +492,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
@timgraham

timgraham Jun 5, 2014

Owner

please check flake8, mark is no longer used.

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/many_to_one.txt
@@ -52,6 +52,15 @@ 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')
+ >>> a = Article(id=None, headline="This is a test", pub_date=date(2005, 7, 27), reporter=r3)
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Reporter: Reporter object>": "Reporter" instance isn't saved in the database.'
@timgraham

timgraham Jun 5, 2014

Owner

<Reporter: John>

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/many_to_one.txt
@@ -52,6 +52,15 @@ 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')
+ >>> a = Article(id=None, headline="This is a test", pub_date=date(2005, 7, 27), reporter=r3)
@timgraham

timgraham Jun 5, 2014

Owner

remove id=None

@timgraham timgraham commented on the diff Jun 5, 2014

docs/topics/db/examples/one_to_one.txt
@@ -89,6 +89,19 @@ 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``::
+
+ >>> p = Place(name='Demon Dogs', address='944 W. Fullerton')
+ >>> r = Restaurant(place=p, serves_hot_dogs=True, serves_pizza=False)
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Place: Place object>": "Place" instance isn't saved in the database.'
@timgraham

timgraham Jun 5, 2014

Owner

<Place: Demon Dogs>

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/many_to_one.txt
@@ -52,6 +52,15 @@ 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.
@timgraham

timgraham Jun 5, 2014

Owner

wrap text at 80 characters

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/one_to_one.txt
@@ -89,6 +89,19 @@ 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.
@timgraham

timgraham Jun 5, 2014

Owner

wrap text at 80 characters

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/one_to_one.txt
@@ -89,6 +89,19 @@ 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``::
+
+ >>> p = Place(name='Demon Dogs', address='944 W. Fullerton')
+ >>> r = Restaurant(place=p, serves_hot_dogs=True, serves_pizza=False)
@timgraham

timgraham Jun 5, 2014

Owner

remove "r = " to prevent confusion with the next line?

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/one_to_one.txt
@@ -89,6 +89,19 @@ 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``::
+
+ >>> p = Place(name='Demon Dogs', address='944 W. Fullerton')
+ >>> r = Restaurant(place=p, serves_hot_dogs=True, serves_pizza=False)
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Place: Place object>": "Place" instance isn't saved in the database.'
+ >>> p.restaurant = r
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Restaurant: Restaurant object>": "Restaurant" instance isn't saved in the database.'
@timgraham

timgraham Jun 5, 2014

Owner

<Restaurant: Demon Dogs the restaurant>

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/many_to_one.txt
@@ -52,6 +52,15 @@ 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')
+ >>> a = Article(id=None, headline="This is a test", pub_date=date(2005, 7, 27), reporter=r3)
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Reporter: Reporter object>": "Reporter" instance isn't saved in the database.'
+
@timgraham

timgraham Jun 5, 2014

Owner

.. versionchanged:: 1.8

Previously, assigning unsaved objects did not raise an error and could result in silent data loss.

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/topics/db/examples/one_to_one.txt
@@ -89,6 +89,19 @@ 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``::
+
+ >>> p = Place(name='Demon Dogs', address='944 W. Fullerton')
+ >>> r = Restaurant(place=p, serves_hot_dogs=True, serves_pizza=False)
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Place: Place object>": "Place" instance isn't saved in the database.'
+ >>> p.restaurant = r
+ Traceback (most recent call last):
+ ...
+ ValueError: 'Cannot assign "<Restaurant: Restaurant object>": "Restaurant" instance isn't saved in the database.'
+
@timgraham

timgraham Jun 5, 2014

Owner

.. versionchanged:: 1.8

Previously, assigning unsaved objects did not raise an error and could result in silent data loss.

@timgraham timgraham commented on an outdated diff Jun 5, 2014

docs/releases/1.8.txt
@@ -230,6 +230,15 @@ Backwards incompatible changes in 1.8
from these methods are now executed within the method's transaction and
any exception in a signal handler will prevent the whole operation.
+* Assigning unsaved objects to a :class:`~django.db.models.ForeignKey`,
@timgraham

timgraham Jun 5, 2014

Owner

my revisions to the release notes: http://dpaste.com/34QQKDN/

Owner

timgraham commented Jun 5, 2014

Add to commit message: "Thanks Aymeric Augustin for the initial patch and Loic Bistuer for the review."

@coder9042 coder9042 Fixed #10811 -- Assigning unsaved objects to FK, O2O, and GFK raises …
…ValueError

Thanks Aymeric Augustin for the initial patch and Loic Bistuer for the review.
ec14999
Owner

timgraham commented Jun 5, 2014

merged in 5643a3b.

@timgraham timgraham closed this Jun 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment