Opened 42 hours ago

Last modified 102 seconds ago

#36207 assigned Bug

refresh_from_db() doesn't refresh ForeignObject relations

Reported by: Jacob Walls Owned by: Gregory Mariani
Component: Database layer (models, ORM) Version: 5.2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:19207 build:success

Description (last modified by 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):

  • 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):  
    155155        self.assertEqual(4, token.permission_set.count())
    156156        self.assertEqual(4, user.permission_set.count())
    157157        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 Jacob Walls, 42 hours ago

Description: modified (diff)

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

comment:2 by Clifford Gama, 7 hours ago

Triage Stage: UnreviewedAccepted

comment:3 by Gregory Mariani, 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 Gregory Mariani, 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 Gregory Mariani, 52 minutes ago

Owner: set to Gregory Mariani
Status: newassigned

comment:6 by Gregory Mariani, 18 minutes ago

Has patch: set

comment:7 by Jacob Walls, 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.

Last edited 102 seconds ago by Jacob Walls (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top