Code

Opened 20 months ago

Closed 20 months ago

Last modified 20 months 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 Changed 20 months ago by claudep

  • Component changed from Core (Other) to Template system
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/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 follow-up: Changed 20 months ago by 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.

comment:3 in reply to: ↑ 2 Changed 20 months ago by claudep

  • 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 Changed 20 months ago by trbs

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

Last edited 20 months ago by trbs (previous) (diff)

comment:5 Changed 20 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 1b307d6c8fe9a897da2daa189b7d5f59120e4410:

Fixed #19261 -- Delayed Queryset evaluation in paginators

Thanks trbs for the report and the patch.

comment:6 Changed 20 months ago by 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.