Prefetch with to_attr silently ignored if matching property exists

[dc-strahlkraft]

Minimal reproduction ​https://gitlab.com/Dan_Strahlkraft/django-prefetch

Given these models:

class Product(models.Model):
    @property
    def enabled_images(self):
        return self.images.filter(enabled=True)


class ProductImage(models.Model):
    product = models.ForeignKey(Product, models.PROTECT, related_name="images")
    enabled = models.BooleanField()

I want to eagerly load the enabled_images property so that it doesn't hit the database on access. So I write: Product.objects.prefetch_related(Prefetch("images", ProductImage.objects.filter(enabled=True), to_attr="enabled_images")). But that doesn't prefetch anything. The Prefetch is silently ignored.

The simplest workaround I can think of is:

class Product(models.Model):
    @property
    def enabled_images(self):
        try:
            return self.prefetched_enabled_images
        except AttributeError:
            return self.images.filter(enabled=True)

Then I can write Product.objects.prefetch_related(Prefetch("images", ProductImage.objects.filter(enabled=True), to_attr="prefetched_enabled_images")) and now accessing product.enabled_images won't hit the database, but I think it's uglier.

[Simon Charette]

It's likely something we should document explicitly but the mere presence of an attribute conflicting with a specified to_attr is assumed to denote that the relationship is already fetched and this it is ignore by design.

I guess we could adapt the logic added in #26916 for cached_property to allow any property with a setter (not only cached_property) and fail loudly if pointed at one that doesn't have one.

It the mean time your best bet is likely to use cached_property instead of property.

[Simon Charette]

Re-hashing this issue I wonder if it will even be possible to implement support for property when callable(prop.fset) as the prefetching algorithm requires a form of bookeeping for previously fetched attributes.

In the case where to_attr is not used the already-prefetched sentinel is the presence of the relationship name in _prefetched_objects_cache and if to_attr is used to target a cached_property it's the presence of the to_attr in instance.__dict__. The latter works because cached_property is known to use __dict__ to store a value assigned to it but property implementations are opaque to Django so it can't assume the same.

The various approaches I can think of are

1. Adjust the prefetching logic to stop doing already-prefetched bookkeeping using attribute presence as alluded to in ticket:26916#comment:1
2. Special case property by setting an extra attribute (e.g. _prefetched_to_attr_{name} = True) to denote already-fetched
3. Fail loudly when detecting a property (and potentially any descriptor not recognized by Django)

[zky]

Replying to Simon Charette:

Re-hashing this issue I wonder if it will even be possible to implement support for property when callable(prop.fset) as the prefetching algorithm requires a form of bookeeping for previously fetched attributes.

In the case where to_attr is not used the already-prefetched sentinel is the presence of the relationship name in _prefetched_objects_cache and if to_attr is used to target a cached_property it's the presence of the to_attr in instance.__dict__. The latter works because cached_property is known to use __dict__ to store a value assigned to it but property implementations are opaque to Django so it can't assume the same.

The various approaches I can think of are

1. Adjust the prefetching logic to stop doing already-prefetched bookkeeping using attribute presence as alluded to in ticket:26916#comment:1
2. Special case property by setting an extra attribute (e.g. _prefetched_to_attr_{name} = True) to denote already-fetched
3. Fail loudly when detecting a property (and potentially any descriptor not recognized by Django)

Hi Simon,

Structurally, I think adjusting the prefetching logic to stop relying on attribute presence for bookkeeping is the best way forward. Moving the prefetch state to an internal state (like obj._state.prefetched_to_attrs) cleanly decouples data loading from class-level descriptors. This fixes the root cause rather than just patching specific descriptors.

However, should we avoid raising a hard exception directly? Since some users might currently have this incorrect usage in their code, would it be better to emit a warning instead?

[Simon Charette]

Hey zky,

Structurally, I think adjusting the prefetching logic to stop relying on attribute presence for bookkeeping is the best way forward. Moving the prefetch state to an internal state (like obj._state.prefetched_to_attrs) cleanly decouples data loading from class-level descriptors.

Agreed.

However, should we avoid raising a hard exception directly? Since some users might currently have this incorrect usage in their code, would it be better to emit a warning instead?

I think we should consider going ​the deprecation route to eventually turns into an exception. In practice today any property usage is silently broken because the bookeeping logic triggers the getter so even if we don't add proper support I think we should consider deprecating towards an exception that suggests using cached_property instead.

---

Thirumalesh,

The proposed patch ignores the second part of the conversation regarding assignment bookeeping; we can't use to_attr in obj.__dict__ as an heuristic for reasons explained in comment:3. Please avoid submitting patches for tickets already claimed by other contributors, zky in this case.

[zky]

Replying to Simon Charette:

Hey zky,

Structurally, I think adjusting the prefetching logic to stop relying on attribute presence for bookkeeping is the best way forward. Moving the prefetch state to an internal state (like obj._state.prefetched_to_attrs) cleanly decouples data loading from class-level descriptors.

Agreed.

However, should we avoid raising a hard exception directly? Since some users might currently have this incorrect usage in their code, would it be better to emit a warning instead?

I think we should consider going ​the deprecation route to eventually turns into an exception. In practice today any property usage is silently broken because the bookeeping logic triggers the getter so even if we don't add proper support I think we should consider deprecating towards an exception that suggests using cached_property instead.

---

Thirumalesh,

The proposed patch ignores the second part of the conversation regarding assignment bookeeping; we can't use to_attr in obj.__dict__ as an heuristic for reasons explained in comment:3. Please avoid submitting patches for tickets already claimed by other contributors, zky in this case.

Awesome, I'll start working on the implementation.