Fixed #34925 -- Prevented Model.refresh_from_db() from mutating list of fields.

[trontelj]

Resolve the issue of skipping elements during list iteration and subsequently removing them from the list.

[github-actions]

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

[felixxm]

@trontelj Thanks 👍 Regression tests are required.

[trontelj]

The failed tests are not related to the changes I made in code.

[shangxiao]

The failed tests are not related to the changes I made in code.

There's no test for the compilemessages patch though … we can't just add patches willy-nilly without confirming correct behaviour 💀

Remember… it might look correct but it's not confirmed to be correct without a red->green test.

[timgraham]

I'd omit the change to compilemessages and make it a separate bug/PR (or at least a separate commit in this PR).

[rsingh37]

GTM

[nessita]

@trontelj Thank you for this work, but as Tim mentioned, we should make a separated PR for the changes related to compilemessages.

[cordery]

Thank you @trontelj for your PR! Please update it to remove the changes to django/core/management/commands/compilemessages.py as I'd like this patch to make it in to a release as it is functionality that I (alone apparently as this has been broken a long time!) am attempting to use :) If you like I'll take care of the PR for compilemessages.

[shangxiao]

as I'd like this patch to make it in to a release as it is functionality that I (alone apparently as this has been broken a long time!) am attempting to use :)

Sorry for the bad news but unfortunately this won't make 5.0 as it's not categorised as a release blocker.

[cordery]

Ah well, easy enough to continue to work around by manipulating _prefetched_objects_cache directly.

[trontelj]

sorry for late reply, family matters. I'll handle it over the weekend

[nessita]

sorry for late reply, family matters. I'll handle it over the weekend

No problem, we understand that contributors collaborate when they can. FYI, the fixing of the compilemessages code is being address in PR #17452, so in this PR you just need to remove the changes in that command and we'll continue with the review.

[nessita]

@trontelj Hi! Could you please rebase onto main, and re-push? The other related PR has been merged now.

[felixxm]

@trontelj Thanks 👍 Welcome aboard ⛵

I simplified tests to avoid creating a new model.

[charettes]

I think the merges solution only addresses one of the problems.

Since the provided fields passed by referenced is altered directly instead of creating a copy before doing so it could cause code of this form to unexpectedly not refresh at all.

fields = ["title"]
Book.objects.filter(pk=book.pk).update(title="foo")
book.refresh_from_db(fields=fields)
logger.log("Refreshed fields %s", fields)  # Will log an empty list
assert boo.title == "foo"
Book.objects.filter(pk=book.pk).update(title="bar")
book.refresh_from_db(fields=fields)
assert boo.title == "bar"  # Will fail because fields == [] by the point `refresh_from_db` was called

Any objection to adding the following changes as well?

diff --git a/django/db/models/base.py b/django/db/models/base.py
index e2becf8baf..6eaa600f10 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -691,6 +691,7 @@ def refresh_from_db(self, using=None, fields=None):
             self._prefetched_objects_cache = {}
         else:
             prefetched_objects_cache = getattr(self, "_prefetched_objects_cache", ())
+            fields = list(fields)
             for field in list(fields):
                 if field in prefetched_objects_cache:
                     del prefetched_objects_cache[field]
@@ -711,7 +712,6 @@ def refresh_from_db(self, using=None, fields=None):
         # Use provided fields, if not set then reload all non-deferred fields.
         deferred_fields = self.get_deferred_fields()
         if fields is not None:
-            fields = list(fields)
             db_instance_qs = db_instance_qs.only(*fields)
         elif deferred_fields:
             fields = [
diff --git a/tests/basic/tests.py b/tests/basic/tests.py
index c6ad1faefd..ad82cffe8c 100644
--- a/tests/basic/tests.py
+++ b/tests/basic/tests.py
@@ -951,7 +951,9 @@ def test_prefetched_cache_cleared(self):
         # Relation is added and prefetch cache is stale.
         self.assertCountEqual(a2_prefetched.selfref_set.all(), [])
         self.assertCountEqual(a2_prefetched.cited.all(), [])
-        a2_prefetched.refresh_from_db(fields=["selfref_set", "cited"])
+        fields = ["selfref_set", "cited"]
+        a2_prefetched.refresh_from_db(fields=fields)
+        self.assertEqual(fields, ["selfref_set", "cited"])
         # Cache was cleared and new results are available.
         self.assertCountEqual(a2_prefetched.selfref_set.all(), [s])
         self.assertCountEqual(a2_prefetched.cited.all(), [s])

[felixxm]

I think the merges solution only addresses one of the problems.

Good catch 🎯