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

jacobtylerwalls · 2025-03-09T15:32:08Z

Trac ticket number

Branch description

Before, model fields using the private API models.ForeignObject to relate foreign objects without enforcing a foreign key (as modeled in the composite pk docs) didn't have their relations cleared via refresh_from_db().

Alternative to #19207

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
[n/a] I have added or updated relevant docs, including release notes if applicable.
[n/a] I have attached screenshots in both light and dark modes for any UI changes.

moshe742

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

cliff688 · 2025-04-27T05:47:16Z

django/db/models/options.py

+    @cached_property
+    def virtual_relations(self):
+        """
+        Return many-to-one related objects that may be virtual, i.e. using
+        ForeignObjectRel without ManyToOneRel (ForeignKey).
+
+        Private API intended only to be used by Django itself.
+        """


Does this need to be added to FORWARD_PROPERTIES so the cache can be expired? I think it does because it invokes _get_fields().

Just to add context to my earlier note — I did a deeper dive into how FORWARD_PROPERTIES affects cache invalidation in practice.

Removing most field-related entries from the set didn’t cause any test failures. This means missing test coverage, no?

The only field-related entry whose removal did cause test failures was _forward_fields_map, which broke two tests: schema.tests.SchemaTests.test_m2m_repoint_* with RuntimeWarning: Model 'schema.localbookwithm2m_tags' was already registered. Reloading models is not advised as it can lead to inconsistencies, most notably with related models.. I haven’t pinned down the exact reasons, but it likely stems from _get_field(), which builds the forward field map being invoked during the final stages of model preparation, via Options._prepare().

Cache expiration (for field-related properties) is done by Options.add_field(), but this is only called during model class creation, via Field.contribute_to_class(), which is probably why this is mostly untested/ difficult to test.

By contrast, removing manager-related entries did break tests, confirming that those properties are actively used at runtime and need proper invalidation.

While omitting virtual_relations doesn’t break anything now, it does call _get_fields() and could theoretically return stale results if the model were mutated. That said, it’s hard to write a test for this without doing post-setup mutation (). Still, we might want to explore better test coverage for this area overall.

Thanks, great catch. We also appear to be missing db_returning_fields, possibly others. Would you be willing to open a ticket with your findings about the test coverage gap & need for a testing strategy?

Added "virtual_relations" to FORWARD_PROPERTIES and squashed it in.

Thanks @jacobtylerwalls for the updates!

Would you be willing to open a ticket with your findings about the test coverage gap & need for a testing strategy?

I'm happy to open a ticket to highlight these findings, though I don't yet have a concrete testing strategy.

Given the uncertainty around testing FORWARD_PROPERTIES, I think this is ready for a merger's review - marking as RFC. It's up to their discretion whether adding an entry to this (largely untested) set is acceptable, despite the testing challenges.

Opened ticket-36369

sarahboyce · 2025-05-08T15:00:45Z

django/db/models/base.py

@@ -753,7 +753,7 @@ def refresh_from_db(self, using=None, fields=None, from_queryset=None):

        db_instance = db_instance_qs.get()
        non_loaded_fields = db_instance.get_deferred_fields()
-        for field in self._meta.concrete_fields:
+        for field in chain(self._meta.concrete_fields, self._meta.virtual_relations):


Suggested change

for field in chain(self._meta.concrete_fields, self._meta.virtual_relations):

for field in self._meta.fields:

I was wondering why we can't do the above. Virtual fields to me are non-concrete fields, so this would include some virtual fields that are not relations but not sure if that's an issue?

Ah, I should have worked out that those two were complements of each other. Nice!

And extra nice to avoid adding this new property, since I now see it was removed once already in 933dc62 😅

However, I noticed while playing with this model -- if there are multiple foreign objects pointing to the same column (e.g. through inheritance), multiple identical queries will be issued on refresh. I'm going to see if that's what the "cached relations" bit just below this is supposed to be handling.

Added a little test to demonstrate. Guarding the setattr seems to prevent new behavior changes.

How should I see the multiple queries issue? I can see that when I revert the guard, then I get an error running the new test:

raise FieldDoesNotExist( django.core.exceptions.FieldDoesNotExist: BasePerson has no field named 'abstract_non_concrete_id'

But not sure where the multiple queries come to play (this is SQLite).

I'm hoping instead of adding a test to where we are testing model meta, perhaps we can modify the Membership model instead:

--- a/tests/foreign_object/models/person.py +++ b/tests/foreign_object/models/person.py @@ -68,6 +68,13 @@ class Membership(models.Model): to_fields=["id", "group_country"], on_delete=models.CASCADE, ) + not_concrete_country = models.ForeignObject( + Country, + on_delete=models.CASCADE, + from_fields=["non_concrete_id"], # Field does not exist. + to_fields=["id"], + related_name="non_concrete_country", + ) class Meta: ordering = ("date_joined", "invite_reason")

This is enough of a change for the same error, but still have an open question on the queries to know if this is "enough".

How should I see the multiple queries issue?

Ah, I forgot to push up the edits I made to those models. I had to add the missing fields, e.g. the one you marked here as "does not exist". I think this wouldn't function otherwise? I suppose they're missing because that file just has SimpleTestCases and isn't exercising queries?

I can just move the self.assertNumQueries to my original test and have coverage without touching any of these models.

…om_db().

sarahboyce

Thank you! This looks great ⭐

moshe742 approved these changes Apr 26, 2025

View reviewed changes

cliff688 reviewed Apr 27, 2025

View reviewed changes

jacobtylerwalls force-pushed the fix-refresh-foreign-object branch from 32f5bbc to 324cf32 Compare May 3, 2025 12:47

sarahboyce reviewed May 8, 2025

View reviewed changes

jacobtylerwalls force-pushed the fix-refresh-foreign-object branch 2 times, most recently from ffd305f to cec9568 Compare May 9, 2025 01:01

Fixed #36207 -- Cleared cached ForeignObject relations via refresh_fr…

8000

dc2b083

…om_db().

jacobtylerwalls force-pushed the fix-refresh-foreign-object branch from cec9568 to dc2b083 Compare May 9, 2025 10:53

sarahboyce approved these changes May 9, 2025

View reviewed changes

sarahboyce merged commit 69ab6e5 into django:main May 9, 2025
29 of 30 checks passed

jacobtylerwalls deleted the fix-refresh-foreign-object branch May 9, 2025 12:15

This was referenced May 20, 2025

Ticket 36369 #19483

Closed

Fixed #36369: Added tests for FORWARD_PROPERTIES and REVERSE_PROPERTIES. #19484

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

	for field in chain(self._meta.concrete_fields, self._meta.virtual_relations):
	for field in self._meta.fields:

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

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

Conversation

Trac ticket number

Branch description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment