Assigning unsaved model to a ForeignKey leads to silent failures

[Glenn Maynard]

Assigning an object to a ForeignKey field without first saving it leads to silent, unobvious failures:

class Author(models.Model):
    name = models.CharField(max_length=50)

class Book(models.Model):
    author = models.ForeignKey(Author, null = True)

book = Book.objects.create()
book.author = Author(name="john")
book.author.save()
book.save()

This raises no errors, and saves book with a null author, because the author's PK field was still None at the time it was assigned.

Ideally, this would store the author, pull out the PK at save time rather than assignment time, and raise an error if the author still isn't saved when book.save() is called (and even more ideally, book would hook the author's save and set self.author_id as soon as the author is saved). Minimally, though, it should raise an error at assignment time to avoid the silent, confusing failure.

(Of course, using create() instead of the object constructor avoids this by saving immediately, but that's not always what's wanted.)

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Aymeric Augustin]

I prefer failing early and loudly, by raising an exception when an unsaved object is assigned to a related field.

I could listen to an argument for trying to re-fetch the pk from the cached related instance in save(), but that feels like action-at-a-distance: the actual problem usually happened earlier.

Automatically saving related objects is too magic; I'm sure it would be considered unexpected and undesirable in some circumstances.

---

This is a bug that may lead to data loss ; it's pretty bad. Given that it hasn't been fixed in three years I'm strongly in favor of the simplest solution, raising an exception.

It would also be consistent with 3190abcd75b1fcd660353da4001885ef82cbc596 — a fix for the same problem for reverse one-to-one relations.

[Aymeric Augustin]

#11811 should be fixed in the same fashion at the same time.

[Aymeric Augustin]

I have a patch for this. Unfortunately it causes 6 unrelated test failures in the test suite, some of which look non-trivial.

[Aymeric Augustin]

Note the existence of the opposite ticket: #8070 :'(

[Aymeric Augustin]

Honza points out that he takes advantage of this ability in tests.

[Aymeric Augustin]

I'm attaching a new patch that fixes a few test failures:

• by saving the objects before assigning them to foreign keys,
• by removing explicit tests for the behavior this ticket wants to prevent, introduced (among other more reasonable changes) in #8070.

With this patch, there are still two tests failures related to admin inlines.

======================================================================
ERROR: test_create_inlines_on_inherited_model (admin_inlines.tests.TestInline)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/myk/Documents/dev/django/tests/admin_inlines/tests.py", line 192, in test_create_inlines_on_inherited_model
    response = self.client.post('/admin/admin_inlines/extraterrestrial/add/', data)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 460, in post
    response = super(Client, self).post(path, data=data, content_type=content_type, **extra)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 289, in post
    return self.generic('POST', path, post_data, content_type, **extra)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 339, in generic
    return self.request(**r)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 421, in request
    six.reraise(*exc_info)
  File "/Users/myk/Documents/dev/django/django/utils/six.py", line 491, in reraise
    raise value
  File "/Users/myk/Documents/dev/django/django/core/handlers/base.py", line 114, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py", line 467, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 99, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/sites.py", line 198, in inner
    return view(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 29, in _wrapper
    return bound_func(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 99, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 25, in bound_func
    return func(self, *args2, **kwargs2)
  File "/Users/myk/Documents/dev/django/django/db/transaction.py", line 360, in inner
    return func(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py", line 1161, in add_view
    if all_valid(formsets) and form_validated:
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 415, in all_valid
    if not formset.is_valid():
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 292, in is_valid
    err = self.errors
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 267, in errors
    self.full_clean()
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 315, in full_clean
    self._errors.append(form.errors)
  File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 121, in errors
    self.full_clean()
  File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 275, in full_clean
    self._post_clean()
  File "/Users/myk/Documents/dev/django/django/forms/models.py", line 387, in _post_clean
    self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
  File "/Users/myk/Documents/dev/django/django/forms/models.py", line 57, in construct_instance
    f.save_form_data(instance, cleaned_data[f.name])
  File "/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py", line 637, in save_form_data
    setattr(instance, self.name, data)
  File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line 337, in __set__
    (value, self.field.rel.to._meta.object_name))
ValueError: Cannot assign "<ExtraTerrestrial: ExtraTerrestrial object>": "ExtraTerrestrial" instance isn't saved in the database.

======================================================================
ERROR: testInline (admin_views.tests.AdminInheritedInlinesTest)
Ensure that inline models which inherit from a common parent are correctly handled by admin.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/myk/Documents/dev/django/tests/admin_views/tests.py", line 2278, in testInline
    response = self.client.post('/test_admin/admin/admin_views/persona/add/', post_data)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 460, in post
    response = super(Client, self).post(path, data=data, content_type=content_type, **extra)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 289, in post
    return self.generic('POST', path, post_data, content_type, **extra)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 339, in generic
    return self.request(**r)
  File "/Users/myk/Documents/dev/django/django/test/client.py", line 421, in request
    six.reraise(*exc_info)
  File "/Users/myk/Documents/dev/django/django/utils/six.py", line 491, in reraise
    raise value
  File "/Users/myk/Documents/dev/django/django/core/handlers/base.py", line 114, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py", line 467, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 99, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/sites.py", line 198, in inner
    return view(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 29, in _wrapper
    return bound_func(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 99, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/utils/decorators.py", line 25, in bound_func
    return func(self, *args2, **kwargs2)
  File "/Users/myk/Documents/dev/django/django/db/transaction.py", line 360, in inner
    return func(*args, **kwargs)
  File "/Users/myk/Documents/dev/django/django/contrib/admin/options.py", line 1161, in add_view
    if all_valid(formsets) and form_validated:
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 415, in all_valid
    if not formset.is_valid():
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 292, in is_valid
    err = self.errors
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 267, in errors
    self.full_clean()
  File "/Users/myk/Documents/dev/django/django/forms/formsets.py", line 315, in full_clean
    self._errors.append(form.errors)
  File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 121, in errors
    self.full_clean()
  File "/Users/myk/Documents/dev/django/django/forms/forms.py", line 275, in full_clean
    self._post_clean()
  File "/Users/myk/Documents/dev/django/django/forms/models.py", line 387, in _post_clean
    self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
  File "/Users/myk/Documents/dev/django/django/forms/models.py", line 57, in construct_instance
    f.save_form_data(instance, cleaned_data[f.name])
  File "/Users/myk/Documents/dev/django/django/db/models/fields/__init__.py", line 637, in save_form_data
    setattr(instance, self.name, data)
  File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line 337, in __set__
    (value, self.field.rel.to._meta.object_name))
ValueError: Cannot assign "<Persona: Test Name>": "Persona" instance isn't saved in the database.

During the validation phase, in BaseModelForm._post_clean, the model formset attemps to build an in memory representation of the object created by the inline and to attach the parent. This fails as soon as you can't attach unsaved objects to foreign keys.

---

Russell helped me track this down inside InlineModelAdmin, but I couldn't find a fix. Here's what we understood.

1) To create the formset for the admin inline, Django calls InlineModelAdmin.get_formset without a fields keyword argument.

2) get_formset build the list of fields for the form based on the fieldsets defined for the InlineModelAdmin: fields = flatten_fieldsets(self.get_fieldsets(request, obj))

3) When the InlineModelAdmin doesn't contain an fieldsets attribute, BaseModelAdmin.get_fieldsets returns [(None, {'fields': self.get_fields(request, obj)})]

4) InlineModelAdmin.get_fields attempts to obtain the list of fields from its formset's form, which it obtains with form = self.get_formset(request, obj, fields=None).form.

5) At this point we're extremely close to an infinite recursion, but passing the fields keyword argument avoids it. This looks very fragile and I wouldn't be surprised if there was a design error in this area. Since fields is explicitly set to None, the inner get_formset uses forms.ALL_FIELDS instead.

6) InlineModelAdmin.get_fields returns form.base_fields which contains all fields if the InlineModelAdmin.form wasn't set to a particular form class.

6) We're back in the outer InlineModelAdmin.get_formset where this list of fields gets passed explicitly to inlineformset_factory.

Thus, the field representing the foreign key to the parent ends up being explicitly included in the formset's form, which is the root cause of these test failures.

[Aymeric Augustin]

Jacob thinks that wontfixing on the basis that a) the problem is complicated b) the current behavior might be useful isn't the answer, because it's still a bug that can result in data loss, no matter how much we document it.

[Aymeric Augustin]

I'm admitting defeat. If you want to tackle this, my advice in to untangle the loop in InlineModelAdmin.

The patch also needs release notes as it's probably going to break at least a few people's code. People who relied on this possibility in tests should switch to mocking.

[anonymous]

Let the following code:

a = A.objects.create(...)
a.b = B()
a.b.save()
a.save()

Isn't it as simple as checking if a.b is not None and a.b_id is None? If this happens, raise an exception. For instance (surely it must use getattr and setattr):

if a.b is not None and a.b_id is None:
    if a.b.id is not None:
        a.b_id = b.id
    else:
        raise Exception()

[ANUBHAV JOSHI]

Well I use Django 1.6 and there is no such problem, so I am closing it.

[Aymeric Augustin]

Given that the problem still existed 4 months ago, that I failed to fix it after a non-negligible amount of effort (meaning it isn't trivial), and that I'm not aware of any changes in this area since then (and I would probably have noticed), I tend to think it isn't fixed.

How did you determine the issue had vanished?

[anonymous]

ok i realized that i did something wrong, the problem still exists.

[ANUBHAV JOSHI]

Well I found a way to raise warning in Django 1.6.
If it is correct then please tell me how to do the same in dev.

[ANUBHAV JOSHI]

See line 439.

[ANUBHAV JOSHI]

Actually I did something, please review: ​https://github.com/django/django/pull/2162

[Tim Graham]

It looks like you got to roughly the same place as Aymeric did in his attempt to solve this (running into problems with InlineModelAdmin). I'm not really inclined to look into it myself since Aymeric already tried and it's not clear fixing this ticket will yield much benefit -- from an earlier comment: "Jacob thinks that wontfixing on the basis that a) the problem is complicated b) the current behavior might be useful isn't the answer, because it's still a bug that can result in data loss, no matter how much we document it."

[Tim Graham]

#20554 was a duplicate. See there for some comments from Anssi including "This might be surprisingly hard to solve correctly."

[ANUBHAV JOSHI]

Well I went through the entire problem today thoroughly.

The problem is that when in the inline test(which causes errors), we are trying to create an object on the fly and then since it has no id so it cannot be used. Following Aymeric's comments above line by line its true that form gets fields: ['et', 'place'] out of which et which is FK creates problem in form validation step in _post_clean when construct_instance is called.
We need a saved object of ExtraTerrestrial class for its id to be saved in Sighting.

As far as I think, there are other tests in same test file with models using the FK, why not try to implement the error causing test case in a similar way/ or create an object before and send it(dunno if the second part is possible)
Or
Is it possible to write a check in _post_clean and prevent calling of construct_instance?

Need some guidance here.

A similarity between failing tests: Both are dealing with inherited models. Although I don't think that might be a problem here but still I thought it better to post.

[Tim Graham]

Looks like #8892 is another duplicate.

[altlist]

One could do this and it does what I expect in Django 1.6

book.author.save()
book.author = book.author  # refresh id
book.save()

So I extended the original patch in #8892 (ticket) and wrote a save_with_refresh(obj) function.

book.author.save()
refresh.save_with_refresh(book)

One could modify models.base.save() to accept a "refresh" keyword that would do the above.

[altlist]

save_with_refresh

[ANUBHAV JOSHI]

I have found out some things regarding the failure of tests, and it is due to inherited models.

In case of admin_inline test(look into ​https://github.com/django/django/blob/master/tests/admin_inlines/tests.py#L191):

the Extraterrestrial object is never assigned a pk, this failure of tests is not due to the fix of #10811 but there is some other problem because a similar test(see ​https://github.com/django/django/blob/master/tests/admin_inlines/tests.py#L65) works fine.

If you look at models, there is:

class Lifeform():
     pass
class ExtraTe...(Lifeform):
     pass
class Sighting():
     some_fk = ExtraTerr...

In database instead of id in ExtraTerrestrial there is a field lifeform_ptr_id, which never gets a value. I have tried out printing out id of the ET object before and after save is called but it comes out None. So the problem lies there.

[ANUBHAV JOSHI]

Even before the fix for this bug was written, I don't know how the tests passes because the id has always been None, so I do not understand how were the tests passing.
Are ids pulled out again at a later point??

[ANUBHAV JOSHI]

There is some problem in saving objects in inherited models in case of inlines as far as I think. It might very well be related to #19524.

[ANUBHAV JOSHI]

Patch complete: ​https://github.com/django/django/pull/2697

[altlist]

Any reason why we can't do something like the save_with_refresh() function?

[Aymeric Augustin]

We won't add a new API for an edge case and let developers figure out which API to use in which context.

[altlist]

Replying to aaugustin:

We won't add a new API for an edge case and let developers figure out which API to use in which context.

I was suggesting an extension to the save() API, where one could pass a "refresh=True" option. In the meantime, using my own routine.

[ANUBHAV JOSHI]

Replying to altlist:

I was suggesting an extension to the save() API, where one could pass a "refresh=True" option. In the meantime, using my own routine.

Raising an error and notifying that the action is wrong is better approach than allowing the user to do wrong and correct later.

[Tim Graham <timograham@…>]

In 5643a3b51be338196d0b292d5626ad43648448d3:

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.

[Tim Graham <timograham@…>]

In fd5897193f0bd8fa211885be5726f8e5613f3c08:

Fixed problem with refs #10811.

When 'to_field' is specified with a FK, then we need to check the pk value
the object.

[Piotr Maliński]

Just a note that this change broke factory_boy factories a lot with Django 1.8 - now .build() or .attributes() can't be used if model has a subfactory (FK or OneToOne relation).

[Ryan Hiebert]

I wonder if it would have been possible and better to raise the error when the model was saved rather than when the related model was assigned. It would both prevent the data loss issue and allow completely unsaved models to be used, such as how .build() and .attributes() work for factory_boy factories.

[Tim Graham]

I believe the discussion of whether or not the ORM should be usable with completely in-memory models was answered as "no", but I don't recall the reasoning and can't cite any discussion. Feel free to start a thread on the DevelopersMailingList as that's a better place to have a discussion about this.

[Anssi Kääriäinen]

I must say I believe doing the check at save() time is better than disallowing assignment completely. This should result in the same data-loss-preventation effect, while giving better backwards compatibility for users.

The check in save() would essentially be "if 'fk' in self.dict and self.fk.id is None -> raise Error" (with suitable replacements to get the from and to fields dynamically). I believe this is safe to backpatch to 1.8, as currently assigning anything with pk = None to fk will raise an error. So, there shouldn't be any code where the check is True in released 1.8 versions.

[Michael]

Replying to akaariai:

I must say I believe doing the check at save() time is better than disallowing assignment completely. This should result in the same data-loss-preventation effect, while giving better backwards compatibility for users.

The check in save() would essentially be "if 'fk' in self.dict and self.fk.id is None -> raise Error" (with suitable replacements to get the from and to fields dynamically). I believe this is safe to backpatch to 1.8, as currently assigning anything with pk = None to fk will raise an error. So, there shouldn't be any code where the check is True in released 1.8 versions.

I agree that moving the check to the save() method seems the best solution. This change really caused our unittests to run much slower since now we need to create objects in the database to test properties unrelated to the fk's. How can we get this in 1.9 ?

[Tim Graham]

Writing a patch would be a good start. Note that we added a flag to bypass the check in #24495. I guess that would be reverted then.

[Tim Graham]

Created #25160 for moving the check to save().