refresh_from_db() doesn't clear cached ForeignObject relations

[Jacob Walls]

Although it's an internal API ForeignObject is now ​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):

tests/composite_pk/test_models.py

@@ -155 +155 @@
         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)

---

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)

[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.)

[Gregory Mariani]

I'll have a look on it. In the interval if someone want to take the ticket go ahead

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

[Jacob Walls]

Thanks for picking this up. I think being able to set .user = None is important. Maybe we need to adjust this ​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.

[Gregory Mariani]

What's the behavior expected ?
Because comment.user = None will set the value None 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.

Here the model Comment l30
​https://github.com/django/django/blob/51cab4ad51616f8fdb050631be5c710b93685ec3/tests/composite_pk/models/tenant.py
the CASCADE relations are:

from_fields=("tenant_id", "user_id"),
to_fields=("tenant_id", "id")

So if we set user_id to None, the relations are destroyed for a ForeignObject.
Maybe I've misunderstood something

[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.

[Sarah Boyce]

This also fails on Django 5.1

tests/foreign_object/tests.py

@@ -437 +437 @@
         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.

[Jacob Walls]

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.

[Jacob Walls]

I opened a ​PR with my proposed alternative.

[Jacob Walls]

Thanks Gregory for advancing the investigation here. Since the first PR is now closed, I'm going to set myself in the owner field. Eager to hear any feedback you might have for me.

[moshe nahmias]

The test passes and actually test the behavior (so it fails on master but pass on the branch PR).
The code is similar to the surrounding code and looks good

[Clifford Gama]

Patch LGTM

The only remaining consideration is whether adding an entry to FORWARD_PROPERTIES without adding a corresponding test—despite limited test coverage for this set—is acceptable. Since:

1. The risk appears low (no tests fail without it, but stale cache could theoretically occur).
2. Testing this edge case robustly would likely require invasive model mutations post-setup.
3. Field-related entries in FORWARD_PROPERTIES are untested (You can remove all but one of them without seeing any test failures.

Leaving this to the merger’s discretion.

[Sarah Boyce <42296566+sarahboyce@…>]

In 69ab6e5:

Fixed #36207 -- Cleared cached ForeignObject relations via refresh_from_db().