FETCH_PEERS can overwrite deferred-field changes and raise KeyError for deleted peers

[Patel Namraa]

When using FETCH_PEERS with deferred fields, DeferredAttribute.fetch_many() does not correctly handle peer instances whose state has changed since the queryset was evaluated.

There are two related problems.

1. Locally assigned values can be overwritten

Consider two objects loaded with a deferred field using FETCH_PEERS:

class Author(models.Model):
name = models.CharField(max_length=100)

authors = list(
Author.objects.defer("name").fetch_mode(FETCH_PEERS)
)

authors[0].name = "locally changed"

# Fetch the deferred field for another peer.

_ = authors[1].name

Fetching authors[1].name causes fetch_many() to populate the field on all peers. This also replaces authors[0].name with the value currently stored in the database.

The locally assigned value is therefore silently lost.

This does not happen with FETCH_ONE, where the locally assigned value is preserved.

2. A deleted peer can cause KeyError

A second problem occurs when one of the peer rows is deleted after the queryset has been evaluated.

fetch_many() uses in_bulk() to retrieve the deferred values and then indexes the returned dictionary for every peer:

value_by_pk = (
self.field.model._base_manager.using(db)
.values_list(attname, flat=True)
.in_bulk({instance.pk for instance in instances})
)

for instance in instances:
    setattr(instance, attname, value_by_pk[instance.pk])

in_bulk() does not include primary keys for rows that no longer exist. Consequently, if one peer has been deleted, accessing the deferred field on another existing peer can raise KeyError.

The missing peer should not cause an unrelated peer's deferred-field access to fail.

If the instance being accessed itself has been deleted, its behavior should remain consistent with FETCH_ONE rather than producing a KeyError caused by another peer.

Expected behavior:

FETCH_PEERS should not overwrite a peer's field if that value has already been assigned locally.

A peer whose database row no longer exists should not cause deferred-field access on other existing peers to fail.

The behavior for the instance whose own row has been deleted should remain consistent with FETCH_ONE.

I reproduced both behaviors against Django 6.1. The corresponding FETCH_ONE cases behave correctly.

This appears to be a missing state check in DeferredAttribute.fetch_many(): it unconditionally assigns the fetched value to every peer and assumes that every requested primary key is present in the result returned by in_bulk().

[Vishy]

Both issues were reviewed with the following tests,

tests/defer/tests.py

@@ -194 +194 @@
         with self.assertNumQueries(0):
             p2.value
 
+    def test_defer_fetch_mode_fetch_peers_preserves_locally_assigned_value(self):
+        p1, p2 = Primary.objects.fetch_mode(FETCH_PEERS).defer("value")
+        p1.value = "unsaved assignment"
+
+        # Accessing deferred field on p2 should not overwrite p1's local changes.
+        self.assertEqual(p2.value, "yy")
+        self.assertEqual(p1.value, "unsaved assignment")
+
+    def test_defer_fetch_mode_fetch_peers_deleted_peer(self):
+        p1, p2 = Primary.objects.fetch_mode(FETCH_PEERS).defer("value")
+
+        # Delete p1 from the database while its instance remains in memory.
+        Primary.objects.filter(pk=p1.pk).delete()
+
+        # Fetching deferred attribute on p2 should succeed and not raise
+        # KeyError for the missing p1.pk in in_bulk().
+        self.assertEqual(p2.value, "yy")
+
     def test_defer_fetch_mode_fetch_peers_single(self):
         p1 = Primary.objects.fetch_mode(FETCH_PEERS).defer("value").get(name="p1")
         with self.assertNumQueries(1):

[Jacob Walls]

Thanks for the reports. Next time, please present separate reports for separate bugs. Also, for the second issue, it would have been helpful to include the reproducer instead of just reasoning about the code.

I feel more sure about accepting the issue for the KeyError in in_bulk().

I suppose we can also accept the issue for the "don't fetch over an already set value", but I'll CC Adam to see if they have any opinions on that. The proposal is coherent with how .defer() works today -- accessing doesn't overwrite.

[Jacob Walls]

Sorry, I mixed up owner and cc fields.

[Jacob Walls]

Left a question on the PR about whether we truly want to raise DoesNotExist at the moment of accessing a deferred attribute if *any* peer has been deleted, rather than just for the one whose attribute is being accessed. It seems stricter than the "fetch peers" behavior for foreign keys.

I haven't checked GenericRelation.

[Jacob Walls]

My concern was the result of an incorrect test case.

[Jacob Walls <jacobtylerwalls@…>]

In 6b212c2:

Fixed #37436 -- Avoided overwriting when FETCH_PEERS fetches deferred fields.

If a deferred field has been plucked via refresh_from_db(fields=...)
or modified by assigning an attribute, it shouldn't be overwritten when
attribute access on a peer triggers a fetch_many().

The peer fetching should also tolerate deleted instances.

Co-authored-by: Vishy <vishyzero@…>

[Jacob Walls <jacobtylerwalls@…>]

In a92be5c4:

[6.1.x] Fixed #37436 -- Avoided overwriting when FETCH_PEERS fetches deferred fields.

If a deferred field has been plucked via refresh_from_db(fields=...)
or modified by assigning an attribute, it shouldn't be overwritten when
attribute access on a peer triggers a fetch_many().

The peer fetching should also tolerate deleted instances.

Co-authored-by: Vishy <vishyzero@…>

Backport of 6b212c29e7dfa94f92110d5be034033f449a0cf2 from main.

[Adam Johnson]

Thanks for the report and CCing me.

The overwriting fix looks good to me.

Raising DoesNotExist for now-missing rows feels a bit icky to me, but I guess it's better than continuing with partial state. Maybe the error message could be improved by including the PK of the missing row, because right now it might look like the initial instance has gone missing, rather than a peer?

[Jacob Walls]

Thanks for looking, Adam. I might have confused things in my earlier messages, and possibly in my editing of the commit message.

My latest understanding is that we did *not* introduce DoesNotExist for a deleted peer when fetching this.deferred. We prevented a KeyError after in_bulk(), and the manual raising of DoesNotExist is so that this.deferred doesn't misleadingly return None.