Saving parent object after setting on child leads to unexpected data loss

[Erwin Junge]

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

[Tim Martin]

The current PR has merge conflicts that need to be resolved.

[robinh00d]

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

[Simon Charette]

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?

[robinh00d]

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

[robinh00d]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 266e7e0e:

Refs #28147 -- Added test for saving nullable ForeignKey with to_field attribute after saving parent.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 519016e5:

Fixed #28147 -- Fixed loss of assigned parent when saving child after parent.

Thanks Erwin Junge for the initial patch.

[Jon Dufresne]

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.

[Mariusz Felisiak]

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?

[Jon Dufresne]

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.

[Jon Dufresne]

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4122d9d:

Refs #28147 -- Fixed setting of OneToOne and Foreign Key fields to None when using attnames.

Regression in 519016e5f25d7c0a040015724f9920581551cab0.