[Django] #36207: refresh_from_db() doesn't refresh ForeignObject relations

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Although it's an internal API `ForeignObject` is now
[https://docs.djangoproject.com/en/5.2/topics/composite-primary-key
/#composite-primary-keys-and-relations quasi-public]. When trying it out
in a non-composite-pk situation I found that `refresh_from_db()` didn't
clear out a related ForeignObject.

Here is a rough test using the composite_pk models:
{{{#!diff
diff --git a/tests/composite_pk/test_models.py
b/tests/composite_pk/test_models.py
index 27157a52ad..fdf58d1621 100644
--- a/tests/composite_pk/test_models.py
+++ b/tests/composite_pk/test_models.py
@@ -155,3 +155,9 @@ class CompositePKModelsTests(TestCase):
self.assertEqual(4, token.permission_set.count())
self.assertEqual(4, user.permission_set.count())
self.assertEqual(4, comment.permission_set.count())
+
+ def test_refresh_foreign_object(self):
+ self.comment_1.user = self.comment_2.user
+ self.assertEqual(self.comment_1.user, self.comment_2.user)
+ self.comment_1.refresh_from_db()
+ self.assertNotEqual(self.comment_1.user, self.comment_2.user)
}}}
----
{{{#!py
======================================================================
FAIL: test_refresh_foreign_object
(composite_pk.test_models.CompositePKModelsTests.test_refresh_foreign_object)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/.../django/tests/composite_pk/test_models.py", line 163, in
test_refresh_foreign_object
self.assertNotEqual(self.comment_1.user, self.comment_2.user)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: <User: User object ((1, 1))> == <User: User object ((1,
1))>

----------------------------------------------------------------------
Ran 1 test in 0.006s

FAILED (failures=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36207>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Old description:

New description:

Although it's an internal API `ForeignObject` is now
[https://docs.djangoproject.com/en/5.2/topics/composite-primary-key
/#composite-primary-keys-and-relations quasi-public]. When trying it out
in a non-composite-pk situation I found that `refresh_from_db()` didn't
clear out a related ForeignObject.

Here is a rough test using the composite_pk models (EDIT: prior version of
test wasn't demonstrating anything):

{{{#!diff
diff --git a/tests/composite_pk/test_models.py
b/tests/composite_pk/test_models.py

index 27157a52ad..7cd97a31a9 100644
--- a/tests/composite_pk/test_models.py
+++ b/tests/composite_pk/test_models.py
@@ -155,3 +155,8 @@ class CompositePKModelsTests(TestCase):

self.assertEqual(4, token.permission_set.count())
self.assertEqual(4, user.permission_set.count())
self.assertEqual(4, comment.permission_set.count())
+
+ def test_refresh_foreign_object(self):

+ self.comment_1.user = None
+ self.comment_1.refresh_from_db()
+ self.assertEqual(self.comment_1.user, self.user_1)
}}}
----
{{{#!py
E
======================================================================
ERROR: test_refresh_foreign_object

(composite_pk.test_models.CompositePKModelsTests.test_refresh_foreign_object)
----------------------------------------------------------------------
Traceback (most recent call last):

File "/Users/.../django/tests/composite_pk/test_models.py", line 161, in
test_refresh_foreign_object
self.comment_1.refresh_from_db()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/.../py313/lib/python3.13/site-
packages/django/db/models/base.py", line 737, in refresh_from_db
db_instance = db_instance_qs.get()
File "/Users/.../py313/lib/python3.13/site-
packages/django/db/models/query.py", line 633, in get
raise self.model.DoesNotExist(
"%s matching query does not exist." % self.model._meta.object_name
)
composite_pk.models.tenant.Comment.DoesNotExist: Comment matching query
does not exist.

----------------------------------------------------------------------
Ran 1 test in 0.008s

FAILED (errors=1)
}}}

--
Comment (by Jacob Walls):

The variant where I found this didn't cause `refresh_from_db()` to throw,
it just silently kept the old value. (This was with a nullable
ForeignObject.)
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:1>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted

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

Changes (by Clifford Gama):

* stage: Unreviewed => Accepted

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

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Gregory Mariani):

I'll have a look on it. In the interval if someone want to take the ticket
go ahead
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:3>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Gregory Mariani):

@Jacob
Does this test is also correct with the logic you expect ?


{{{
def test_refresh_foreign_object(self):
# Retrieve the comment via the DB to ensure from_db() is called and
_original_pk is set
comment = Comment.objects.get(pk=self.comment_1.pk)
# Remove the underlying 'user_id' from __dict__ to simulate a cleared
ForeignObject cache
comment.__dict__.pop("user_id", None)
# Call refresh_from_db() to reload the original values from the
database
comment.refresh_from_db()
# Assert that refresh_from_db() has restored 'user' to its original
value
self.assertEqual(comment.user, self.user_1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:4>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Gregory Mariani):

* owner: (none) => Gregory Mariani
* status: new => assigned

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

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | 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 Gregory Mariani):

* has_patch: 0 => 1

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

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Thanks for picking this up. I think being able to set `.user = None` is
important. Maybe we need to adjust this
[https://github.com/django/django/blob/51cab4ad51616f8fdb050631be5c710b93685ec3/django/db/models/fields/related_descriptors.py#L207
query[ from `.get()` to `.filter(...).first()` to support that? The test
model I was reusing was not nullable, but you can imagine another test
model having a nullable ForeignObject.
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:7>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Gregory Mariani):

What's the behavior expected ?
Because comment.user = None will set the value and obviously
self.assertEqual(comment.user, self.user_1) will raise with my update:


{{{
composite_pk.models.tenant.Comment.user.RelatedObjectDoesNotExist: Comment
has no user.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:8>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

> Because comment.user = None will set the value None

But the test case I sketched doesn't save that change to the database, so
my expectation was that `refresh_from_db()` would return the same thing
that a fresh query would return. Or at least this was my expectation based
on how `ForeignKey` works.

> So if we set user_id to None, the relations are destroyed for a
ForeignObject.

If this is how ForeignObject is supposed to work, then I feel we need to
document it, because from the composite PK guide that I linked it's not
obvious. However my impression is that it's ''not'' supposed to work like
`.add()`, which immediately alters data.
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:9>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | 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 Jacob Walls):

* needs_better_patch: 0 => 1

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

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(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 Sarah Boyce):

* cc: Csirmaz Bendegúz, Lily Foote (added)
* severity: Release blocker => Normal

Comment:

This also fails on Django 5.1

{{{#!diff
--- a/tests/foreign_object/tests.py
+++ b/tests/foreign_object/tests.py
@@ -437,6 +437,15 @@ class MultiColumnFKTests(TestCase):
normal_groups_lists = [list(p.groups.all()) for p in
Person.objects.all()]
self.assertEqual(groups_lists, normal_groups_lists)

+ def test_refresh_foreign_object(self):
+ member = Membership.objects.create(
+ membership_country=self.usa, person=self.bob, group_id=None
+ )
+ self.assertEqual(member.person, self.bob)
+ member.person = None
+ member.refresh_from_db()
+ self.assertEqual(member.person, self.bob)
+
@translation.override("fi")
def test_translations(self):
a1 = Article.objects.create(pub_date=datetime.date.today())
}}}

We documented that this is an internal API, and therefore not supported by
our deprecation policy.
I don't think this is supported by our "new feature in 5.2" policy of back
porting bug fixes. I am therefore demoting this from a release blocker to
"normal".

If this note implies this should work "perfectly", we should remove the
suggestion to use `ForeignObject`.

#35956 aims to add ForeignKey support.
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:11>

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(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 Gregory Mariani):

* needs_better_patch: 1 => 0

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

[Django]

#36207: refresh_from_db() doesn't refresh ForeignObject relations
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Gregory
| Mariani
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(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 Jacob Walls):

* needs_better_patch: 0 => 1

Comment:

I think I confounded things by involving `None` in my draft test. I didn't
notice that both sides of the relation involved CompositePrimaryKey.
Django doesn't support setting a model's PK to None and then refreshing
from db--and I'm not suggesting to add support for that.

I left a review on the PR improve the assertion and suggest an alternative
approach. It works for me, but some further investigation/validation would
be very welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/36207#comment:13>