Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

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

Change History (19)

comment:1 by Erwin Junge, 8 years ago

Description: modified (diff)

comment:2 by Erwin Junge, 8 years ago

Owner: changed from nobody to Erwin Junge
Status: newassigned
Version: 1.11master

comment:3 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Amy Mok, 7 years ago

Owner: changed from Erwin Junge to Amy Mok

comment:5 by Tim Martin, 7 years ago

Cc: Tim Martin added
Patch needs improvement: set

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

comment:6 by robinh00d, 6 years ago

Owner: changed from Amy Mok to robinh00d

comment:7 by robinh00d, 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 Simon Charette, 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?

Last edited 6 years ago by Simon Charette (previous) (diff)

comment:9 by robinh00d, 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 robinh00d, 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 Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:12 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 266e7e0e:

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

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 519016e5:

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

Thanks Erwin Junge for the initial patch.

comment:15 by Jon Dufresne, 5 years ago

Cc: jon.dufresne@… 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 Mariusz Felisiak, 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 Jon Dufresne, 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 Jon Dufresne, 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.

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 4122d9d:

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

Regression in 519016e5f25d7c0a040015724f9920581551cab0.

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