[Django] #37201: Prefetch with to_attr silently ignored if matching property exists

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37201>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------

Reporter: dc-strahlkraft | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:1>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------

Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by zky):

* owner: (none) => zky
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:2>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:3>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by zky):

* has_patch: 0 => 1

Comment:

https://github.com/django/django/pull/21576
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:4>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by zky):

* has_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:5>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by zky):

Replying to [comment:3 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:6>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Thirumalesh):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:7>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

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
[https://docs.djangoproject.com/en/6.0/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:8>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by zky):

Replying to [comment:8 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
[https://docs.djangoproject.com/en/6.0/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:9>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by zky):

https://github.com/django/django/pull/21680
--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:10>

[Django]

#37201: Prefetch with to_attr silently ignored if matching property exists
-------------------------------------+-------------------------------------
Reporter: dc-strahlkraft | Owner: zky
Type: Bug | Status: assigned
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by zky):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/37201#comment:11>