Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#26916 closed Bug (fixed)

Prefetch's to_attr does not write into cached_properties in 1.10

Reported by: karyon Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.10
Severity: Release blocker Keywords:
Cc: karyon, Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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: prefetch and the 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.

Change History (7)

comment:1 Changed 3 years ago by Simon Charette

Cc: Simon Charette added
Component: UncategorizedDatabase layer (models, ORM)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 3 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:3 Changed 3 years ago by Simon Charette

Has patch: set

@karyon, please confirm this patch solves your issue.

comment:4 Changed 3 years ago by karyon

yes it does, thanks!

comment:5 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:6 Changed 3 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 271bfe65:

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.

comment:7 Changed 3 years ago by Simon Charette <charette.s@…>

In dcf0a35:

[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

Note: See TracTickets for help on using tickets.
Back to Top