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

[trbs]

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)

[Claude Paroz]

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.

[trbs]

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.

[Claude Paroz]

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.

[trbs]

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

[Claude Paroz <claude@…>]

In 1b307d6c8fe9a897da2daa189b7d5f59120e4410:

Fixed #19261 -- Delayed Queryset evaluation in paginators

Thanks trbs for the report and the patch.

[Claude Paroz <claude@…>]

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.