[Django] #28147: Saving parent object after setting on child leads to unexpected data loss

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------
Reporter: Erwin | Owner: nobody
Junge |
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28147>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Erwin Junge:

Old description:

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

New description:

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

--

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:1>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: Erwin
| Junge
Type: Bug | Status: assigned
Component: Database layer | Version: master

(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Erwin Junge):

* status: new => assigned
* owner: nobody => Erwin Junge
* version: 1.11 => master

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:2>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: Erwin
| Junge
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:3>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: Amy Mok

Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Amy Mok):

* owner: Erwin Junge => Amy Mok

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:4>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: Amy Mok
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Martin):

* cc: Tim Martin (added)
* needs_better_patch: 0 => 1

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:5>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d

Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by robinh00d):

* owner: Amy Mok => robinh00d

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:6>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:7>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

That makes sense. I went through the
[https://github.com/django/django/pull/8434/files#diff-
507b415116b409afa4f723e41a759a9eR665 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.

{{{#!python
author = Author()
book = Book(nullable_author=author)
author.save()
book.save()
}}}

Thoughts on that robinh00d?

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:8>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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 :)

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:9>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by robinh00d):

* needs_better_patch: 1 => 0

Comment:

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:
[https://github.com/django/django/pull/11383 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:10>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:11>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:12>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"266e7e0eccf1d109e8c64c7bc5e612a538ad618a" 266e7e0e]:
{{{
#!CommitTicketReference repository=""
revision="266e7e0eccf1d109e8c64c7bc5e612a538ad618a"
Refs #28147 -- Added test for saving nullable ForeignKey with to_field
attribute after saving parent.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:13>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d

Type: Bug | Status: closed

Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"519016e5f25d7c0a040015724f9920581551cab0" 519016e5]:
{{{
#!CommitTicketReference repository=""
revision="519016e5f25d7c0a040015724f9920581551cab0"
Fixed #28147 -- Fixed loss of assigned parent when saving child after
parent.

Thanks Erwin Junge for the initial patch.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:14>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jon Dufresne):

* cc: jon.dufresne@… (added)

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:15>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by felixxm):

Jon, thanks for the report. I agree that this behavior has changed, but
I'm not sure how we should deal with this.
[https://docs.djangoproject.com/en/2.2/ref/models/fields/#database-
representation 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?

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:16>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:17>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:18>

[Django]

#28147: Saving parent object after setting on child leads to unexpected data loss
-------------------------------------+-------------------------------------

Reporter: Erwin Junge | Owner: robinh00d
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"4122d9d3f1983eea612f236e941d937bd8589a0d" 4122d9d]:
{{{
#!CommitTicketReference repository=""
revision="4122d9d3f1983eea612f236e941d937bd8589a0d"
Refs #28147 -- Fixed setting of OneToOne and Foreign Key fields to None
when using attnames.

Regression in 519016e5f25d7c0a040015724f9920581551cab0.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/28147#comment:19>