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.

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