Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#19261 closed Cleanup/optimization (fixed)

Page.__getitem__ usage causes queryset to evaluate when doing method lookups

Reported by: trbs Owned by: nobody
Component: Template system Version: 1.4
Severity: Normal Keywords: page, type_error, __getitem__, pagination
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The code for Page.getitem forces an evaluation (query to the database) of the queryset when it's actually looking up the name the name of class functions.

Python: 2.7 (think 2.6 as well)
Django: 1.4.x

The function looks like this:

    def __getitem__(self, index):
        # The object_list is converted to a list so that if it was a QuerySe    t
        # it won't be a database hit per __getitem__.
        return super(Page, self).__getitem__(self, index)

Now consider this template code that runs from a generic class based view:

{% if page_obj.has_previous %}
...
{% endif %}

Here page_obj.has_previous generates a call to getitem with 'has_previous' as its parameter index.

This translates to:

return list(self.object_list)['has_previous']

Which raises a TypeError: list indices must be integers, not unicode`.
And as per http://docs.python.org/2/reference/datamodel.html#object.__getitem this is nicely ignored and getitem returns the correct attribute.

However before the type error, during list(self.object_list) the queryset is executed. This should not happen if it's not necessary. It defeats caching in some cases, the query could be expensive and is generally not what we would expect when calling a Page instance's methods.

To avoid this for the (quite common) usage of getitem, I suggest something in the lines of:

   def __getitem__(self, index):
        # The object_list is converted to a list so that if it was a QuerySet
        # it won't be a database hit per __getitem__.
        if isinstance(index, (int,long)):
            return list(self.object_list)[index]
        return super(Page, self).__getitem__(self, index)

(Maybe we don't need to check for long ? and is there maybe some other type that we should check ?)

Patch does not yet includes tests for this. However I'm not really sure what test would actually check this. As the type error is actually swallowed by Python itself.

(btw: i set easy-pickings, as finding it out was not necessarily 'easy' but fixing it should hopefully be)

Attachments (2)

Change History (8)

comment:1 by Claude Paroz, 11 years ago

Component: Core (Other)Template system
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

FWIW, TypeError swallowing does not occur in Python, but in the template variable evaluation (see django/template/base.py, class Variable).

I think that in most of the use cases, the page object will get evaluated anyway somewhere sooner or later, but well, I still think that your suggestion makes sense as an optimization for corner cases.

comment:2 by trbs, 11 years ago

Ah i see. I thought it was strange. I confused getattribute which would be a lookup when accessing a member. Sorry my bad.

So are you suggestion that the 'proper' fix would be that the template system should not do slicing operation when a function which the same exists on the object ?

I guess that would break backwards compatibility...

Putting the optimization in here is much easier for this specific case. (t.b.h. I think calling it a corner case does not really do the problem justice as the problem itself is not so much a corner case but applies any method on page_obj called from the template. But I agree that most people that would have problems with this query will most likely also have a problem with the COUNT() query coming out of pagination. At least on PostgreSQL)

Last but not least :) I updated the patch... also slices should be honored in getitem.

in reply to:  2 comment:3 by Claude Paroz, 11 years ago

Needs tests: set

Replying to trbs:

So are you suggestion that the 'proper' fix would be that the template system should not do slicing operation when a function which the same exists on the object ?

No, I think your approach is fine. You can see something similar in QuerySet.__getitem__. It might even be better to copy the exact same two lines (if not isinstance(k, (slice,) + six.integer_types): raise TypeError). Then we need tests, of course.

comment:4 by trbs, 11 years ago

Added a new patch which uses the same construct as QuerySet.getitem and adds tests.

Last edited 11 years ago by trbs (previous) (diff)

comment:5 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 1b307d6c8fe9a897da2daa189b7d5f59120e4410:

Fixed #19261 -- Delayed Queryset evaluation in paginators

Thanks trbs for the report and the patch.

comment:6 by Claude Paroz <claude@…>, 11 years ago

In dc95791e61750024a610b6c5cf4d32b7325fcb51:

[1.5.x] Fixed #19261 -- Delayed Queryset evaluation in paginators

Thanks trbs for the report and the patch.
Backport of 1b307d6c8f from master.

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