Opened 42 hours ago
Last modified 102 seconds ago
#36207 assigned Bug
refresh_from_db() doesn't refresh ForeignObject relations
Description (last modified by ) ¶
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):
-
TabularUnified tests/composite_pk/test_models.py
diff --git a/tests/composite_pk/test_models.py b/tests/composite_pk/test_models.py index 27157a52ad..7cd97a31a9 100644
a b class CompositePKModelsTests(TestCase): 155 155 self.assertEqual(4, token.permission_set.count()) 156 156 self.assertEqual(4, user.permission_set.count()) 157 157 self.assertEqual(4, comment.permission_set.count()) 158 159 def test_refresh_foreign_object(self): 160 self.comment_1.user = None 161 self.comment_1.refresh_from_db() 162 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)
According to the ticket's flags, the next step(s) to move this issue forward are:
- For anyone except the patch author to review the patch using the patch review checklist and either mark the ticket as "Ready for checkin" if everything looks good, or leave comments for improvement and mark the ticket as "Patch needs improvement".
Change History (7)
comment:1 by , 42 hours ago
Description: | modified (diff) |
---|
comment:2 by , 7 hours ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 6 hours ago
I'll have a look on it. In the interval if someone want to take the ticket go ahead
comment:4 by , 53 minutes ago
@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)
comment:5 by , 52 minutes ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 18 minutes ago
Has patch: | set |
---|
comment:7 by , 2 minutes ago
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.
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.)