#10811 closed Bug (fixed)
Assigning unsaved model to a ForeignKey leads to silent failures
Reported by: | Glenn Maynard | Owned by: | ANUBHAV JOSHI |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
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)
Change History (48)
comment:1 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:3 by , 13 years ago
UI/UX: | unset |
---|
comment:5 by , 12 years ago
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 by , 12 years ago
Owner: | changed from | to
---|
#11811 should be fixed in the same fashion at the same time.
comment:7 by , 11 years ago
I have a patch for this. Unfortunately it causes 6 unrelated test failures in the test suite, some of which look non-trivial.
by , 11 years ago
Attachment: | 10811.patch added |
---|
comment:10 by , 11 years ago
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 by , 11 years ago
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.
by , 11 years ago
Attachment: | 10811-2.patch added |
---|
comment:12 by , 11 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Patch needs improvement: | set |
Status: | new → assigned |
comment:13 by , 11 years ago
Owner: | removed |
---|---|
Status: | assigned → 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 by , 11 years ago
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 by , 11 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
Well I use Django 1.6 and there is no such problem, so I am closing it.
comment:16 by , 11 years ago
Resolution: | worksforme |
---|---|
Status: | closed → 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:18 by , 11 years ago
Cc: | 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.
by , 11 years ago
Attachment: | related.py added |
---|
comment:20 by , 11 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:21 by , 11 years ago
Actually I did something, please review: https://github.com/django/django/pull/2162
comment:22 by , 11 years ago
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 by , 10 years ago
#20554 was a duplicate. See there for some comments from Anssi including "This might be surprisingly hard to solve correctly."
comment:24 by , 10 years ago
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:26 by , 10 years ago
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.
comment:27 by , 10 years ago
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 by , 10 years ago
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 by , 10 years ago
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 by , 10 years ago
Cc: | added |
---|
comment:32 by , 10 years ago
Any reason why we can't do something like the save_with_refresh() function?
follow-up: 34 comment:33 by , 10 years ago
We won't add a new API for an edge case and let developers figure out which API to use in which context.
follow-up: 35 comment:34 by , 10 years ago
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 by , 10 years ago
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.
comment:36 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:38 by , 9 years ago
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).
comment:39 by , 9 years ago
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.
comment:40 by , 9 years ago
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.
follow-up: 42 comment:41 by , 9 years ago
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.
comment:42 by , 9 years ago
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 ?
comment:43 by , 9 years ago
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.
Change UI/UX from NULL to False.