Opened 6 years ago

Closed 9 months ago

Last modified 8 months ago

#10811 closed Bug (fixed)

Assigning unsaved model to a ForeignKey leads to silent failures

Reported by: Glenn Owned by: anubhav9042
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: anubhav9042@…, altlist@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.)

Attachments (4)

10811.patch (3.0 KB) - added by aaugustin 18 months ago.
10811-2.patch (5.6 KB) - added by aaugustin 18 months ago.
related.py (62.7 KB) - added by anubhav9042 14 months ago.
refresh.py (1.6 KB) - added by altlist 10 months ago.
save_with_refresh

Download all attachments as: .zip

Change History (41)

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:3 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 2 years ago by aaugustin

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.

comment:6 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

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

comment:7 Changed 18 months ago by aaugustin

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

Changed 18 months ago by aaugustin

comment:8 Changed 18 months ago by aaugustin

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

comment:9 Changed 18 months ago by aaugustin

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

comment:10 Changed 18 months ago by aaugustin

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.

comment:11 Changed 18 months ago by aaugustin

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.

Changed 18 months ago by aaugustin

comment:12 Changed 18 months ago by aaugustin

  • Has patch set
  • Needs documentation set
  • Patch needs improvement set
  • Status changed from new to assigned

comment:13 Changed 18 months ago by aaugustin

  • Owner aaugustin deleted
  • Status changed from assigned to new

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.

comment:14 Changed 16 months ago by 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()

comment:15 Changed 14 months ago by anubhav9042

  • Resolution set to worksforme
  • Status changed from new to closed

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

comment:16 Changed 14 months ago by aaugustin

  • Resolution worksforme deleted
  • Status changed from closed to new

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?

comment:17 Changed 14 months ago by anonymous

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

comment:18 Changed 14 months ago by anubhav9042

  • Cc anubhav9042@… added

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.

Changed 14 months ago by anubhav9042

comment:19 Changed 14 months ago by anubhav9042

See line 439.

comment:20 Changed 14 months ago by anubhav9042

  • Owner set to anubhav9042
  • Status changed from new to assigned

comment:21 Changed 14 months ago by anubhav9042

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

comment:22 Changed 13 months ago by timo

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."

comment:23 Changed 10 months ago by timo

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

comment:24 Changed 10 months ago by anubhav9042

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.

comment:25 Changed 10 months ago by timo

Looks like #8892 is another duplicate.

comment:26 Changed 10 months ago by 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)
book.save()

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

Version 0, edited 10 months ago by altlist (next)

Changed 10 months ago by altlist

save_with_refresh

comment:27 Changed 10 months ago by anubhav9042

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.

comment:28 Changed 10 months ago by anubhav9042

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??

comment:29 Changed 10 months ago by anubhav9042

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.

comment:31 Changed 9 months ago by altlist

  • Cc altlist@… added

comment:32 Changed 9 months ago by altlist

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

comment:33 follow-up: Changed 9 months ago by aaugustin

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

comment:34 in reply to: ↑ 33 ; follow-up: Changed 9 months ago by 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.

comment:35 in reply to: ↑ 34 Changed 9 months ago by anubhav9042

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.

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

Last edited 9 months ago by anubhav9042 (previous) (diff)

comment:36 Changed 9 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:37 Changed 8 months ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top