[Django] #26916: Prefetch's to_attr does not write into cached_properties in 1.10
Django
-------------------------------+--------------------
Reporter: karyon | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
consider code like this
{{{
Model A:
some_attribute = models.ManyToManyField(B, ...)
@cached_property
def filtered_attribute(self):
return self.some_attribute.filter(...)
# this manually fills some_attribute for all objects with a single
additional query.
all_of_a = A.objects.prefetch_related(Prefetch("some_attribute",
queryset=B.objects.filter(...), to_attr="filtered_attribute"))
for a in all_of_a:
# since we have cached some_attribute, no additional db hits here.
do_stuff_with(a.some_attribute)
}}}
untested code, i hope the intent is clear. actual code:
[https://github.com/fsr-
itse/EvaP/blob/b6222413f15fc7256622a31f7045046f7be1e466/evap/staff/views.py#L58
prefetch] and the [https://github.com/fsr-
itse/EvaP/blob/b6222413f15fc7256622a31f7045046f7be1e466/evap/evaluation/models.py#L387
property]
i thought this "hack" made sense since you can assign cached properties
with the = operator just fine, so why not let prefetch_related do exactly
that?
this used to work with django 1.9, whereas in django 1.10, the cached
property does not seem to be written to.
this first regressed in bdbe50a491ca41e7d4ebace47bfe8abe50a58211, example
stacktrace (code is in the "prefetch" link above):
{{{
...
File "/vagrant/evap/staff/views.py", line 80, in semester_view
courses = get_courses_with_prefetched_data(semester)
File "/vagrant/evap/staff/views.py", line 65, in
get_courses_with_prefetched_data
course.general_contribution = course.general_contribution[0]
File "/vagrant/src/django/django/utils/functional.py", line 35, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File "/vagrant/evap/evaluation/models.py", line 390, in
general_contribution
return self.contributions.get(contributor=None)
File "/vagrant/src/django/django/db/models/manager.py", line 85, in
manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
AttributeError: 'list' object has no attribute 'get'
}}}
it actually executes the cached_property although i expected it to return
the cached value. what the actual problem is i don't know.
error message changed in 53a5fb3cc0137bebeebc0d4d321dbfe20397b065, stack
trace:
{{{
...
File "/vagrant/evap/staff/views.py", line 80, in semester_view
courses = get_courses_with_prefetched_data(semester)
File "/vagrant/evap/staff/views.py", line 65, in
get_courses_with_prefetched_data
course.general_contribution = course.general_contribution[0]
TypeError: 'Contribution' object does not support indexing
}}}
here it seems to execute the cached_property successfully, returning a
single element, and then failing at the [0]. again i expected it to return
the cached value, which would be a list or queryset.
--
Ticket URL: <https://code.djangoproject.com/ticket/26916>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: karyon | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | 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 charettes):
* severity: Normal => Release blocker
* cc: charettes (added)
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
The underlying issue is how the prefetch related algorithm for the
`to_attr` case detects whether or not a relationship is already fetched by
using `hasattr(instance, to_attr)` on the first object of the returned
queryset.
In the reported case the target attribute is a `cached_property` which
makes the detection fail as `hasattr(instance, to_attr)` returns `True`
even if relationship is not fetched yet.
Ideally the recursive algorithm should pass around its prefetching state
between steps instead of relying on attribute and key (when the related
manager is used directly) existence but I suspect this would be too
invasive to backport to 1.10 at this point.
Instead I suggest special casing the `is_fetched` detection for the
`cache_property` case by looking of `to_attr in instance.__dict__` if
`getattr(instance.__class__, to_attr, None)` is an instance of
`cached_property`.
The state passing refactor could be handled in another ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:1>
Django
Reporter: karyon | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* status: new => assigned
* owner: nobody => charettes
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:2>
Django
Reporter: karyon | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
@karyon, please confirm [https://github.com/django/django/pull/6936 this
patch] solves your issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:3>
Django
Reporter: karyon | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by karyon):
yes it does, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:4>
Django
Reporter: karyon | Owner: charettes
Type: Bug | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:5>
Django
Reporter: karyon | Owner: charettes
Component: Database layer | Version: 1.10
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"271bfe65d986f5ecbaeb7a70a3092356c0a9e222" 271bfe65]:
{{{
#!CommitTicketReference repository=""
revision="271bfe65d986f5ecbaeb7a70a3092356c0a9e222"
Fixed #26916 -- Fixed prefetch_related when using a cached_property as
to_attr.
Thanks Trac alias karyon for the report and Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:6>
Django
Reporter: karyon | Owner: charettes
Type: Bug | Status: closed
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"dcf0a35b088ba9ce8907d8f34178bd6dffc27022" dcf0a35]:
{{{
#!CommitTicketReference repository=""
revision="dcf0a35b088ba9ce8907d8f34178bd6dffc27022"
[1.10.x] Fixed #26916 -- Fixed prefetch_related when using a
cached_property as to_attr.
Thanks Trac alias karyon for the report and Tim for the review.
Backport of 271bfe65d986f5ecbaeb7a70a3092356c0a9e222 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26916#comment:7>