#28147 closed Bug (fixed)
Saving parent object after setting on child leads to unexpected data loss
Reported by: | Erwin Junge | Owned by: | robinh00d |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Tim Martin, jon.dufresne@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
When saving a parent object after setting it on a child object and then saving the child object, no error is thrown but the FK relation is saved with a NULL value.
Failing testcase:
# Create parent and child, save parent, save child, parent_id should be set p = Parent() c = Child(parent=p) p.save() c.save() c.refresh_from_db() self.assertIs(c.parent, p)
Patch available: https://github.com/django/django/pull/8434
Change History (19)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Version: | 1.11 → master |
comment:3 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 7 years ago
Owner: | changed from | to
---|
comment:5 by , 7 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
comment:6 by , 6 years ago
Owner: | changed from | to
---|
comment:7 by , 6 years ago
Running the failed testcase mentioned in the description raises an integrity error.
django.db.utils.IntegrityError: NOT NULL constraint failed: many_to_one_child.parent_id
.
This is working as intended.
I believe this can be closed as it was fixed by #29896
comment:8 by , 6 years ago
That makes sense. I went through the PR proposed here and I noticed the .pk
usage was probably missing to_field
usages.
I guess we could add a regression test for the nullable foreign key case though
e.g.
author = Author() book = Book(nullable_author=author) author.save() book.save() book.refresh_from_db() self.assertEqual(book.nullable_author, author)
Thoughts on that robinh00d?
comment:9 by , 5 years ago
I went through the PR proposed here and I noticed the .pk usage was probably missing to_field usages.
In the context of this ticket,
If there was a case where a foreign key had a custom to_field
, current behavior will depend on the DB to raise an exception if the value is not assigned OR assigned to a non-existing reference.
Similarly to the usage of .pk
, we can potentially add logic to check if the value has been assigned or not. And if it hasn't been assigned, we can raise the save() prohibited to prevent data loss due to...
exception message. The advantage of this is that we're raising the exception in code which means no unnecessary DB call is required.
In the case where the value is assigned, but to a non-existing reference, I guess we really have no other choice but to let the DB handle it.
Whether or not a patch to handle missing to_field
is required is beyond me.
I agree with adding the regression tests, I will add it in later when I'm free :)
comment:10 by , 5 years ago
Patch needs improvement: | unset |
---|
Disregard my comment above, I just realised I tested everything wrong. class child
in many_to_one/models
does not have a nullable parent. I've ran the test again and the outcome is as described in the description.
I have made updates to the previously proposed PR to rectify the issue.
I have submitted the PR:
PR
comment:11 by , 5 years ago
Patch needs improvement: | set |
---|
comment:12 by , 5 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:15 by , 5 years ago
Cc: | added |
---|
This change caused a regression for a project I work on. When assigning a FK ID field to None
, the FK is now restored on save. Here is a test to demonstrate that could be added to many_to_one
tests.
def test_assign_fk_id_none(self): parent = Parent.objects.create(name='jeff') child = Child.objects.create(name='frank', parent=parent) parent.bestchild = child parent.save() parent.bestchild_id = None parent.save() self.assertIsNone(parent.bestchild_id) self.assertIsNone(parent.bestchild)
====================================================================== FAIL: test_assign_fk_id_none (many_to_one.test_regression.MyTestCase) ---------------------------------------------------------------------- Traceback (most recent call last): File ".../django/tests/many_to_one/test_regression.py", line 14, in test_assign_fk_id_none self.assertIsNone(parent.bestchild_id) AssertionError: 1 is not None
Bisected to 519016e5f25d7c0a040015724f9920581551cab0.
comment:16 by , 5 years ago
Jon, thanks for the report. I agree that this behavior has changed, but I'm not sure how we should deal with this. Docs says that "your code should never have to deal with the database column name". I'm not sure if will be able to fix this without reverting this patch and whether we should. Any ideas?
comment:17 by , 5 years ago
Thanks. I'm happy to fix my own code to assign to the FK instead of the FK ID, that is no problem on my end. So no need to revert.
However, I interact with the FK ID as a readonly field all the time and think it is fine practice to do so. So I'm not sure I fully agree with linked note. If one simply wants to check the FK is not NULL
, using the _id
field can potentially avoid an unnecessary query. In that use case, I think the interaction is fine. For example, the following can avoid a query:
if child.parent_id: # has parent
Perhaps the _id
field should be improved and enforced to be a readonly property. If one tries to assign to a _id
, an error would be thrown. If one reads from the field, the field is read as normal. This would be similar to how the many-to-many field handles direct assignment. At the very least, it would avoid project bugs due to silently restored FKs from this change in behavior.
comment:18 by , 5 years ago
A potential fix for the regression: https://github.com/django/django/pull/11587
The PR keeps this change intact while allowing direct assignment to _id
fields. It includes the test from above.
The current PR has merge conflicts that need to be resolved.