Opened 11 years ago

Closed 11 years ago

#19938 closed Cleanup/optimization (fixed)

django.core.paginator.Page.__getitem__ being called multiple times when used from a template.

Reported by: joshua.fialkoff@… Owned by: andrewjesaitis
Component: Core (Other) Version: dev
Severity: Normal Keywords: paginator
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Template code (using generic ListView):

{{ page_obj.number }}
{{ page_obj.number }}
{{ page_obj.number }}

When paired with a generic ListView, this will result in three calls to page_obj.__getitem__ (presumably, because the template first tries to use 'number' as an index). Each call to __getitem__ performs a list(self.object_list) . In many cases, this will be quick - which, I suppose, is why it has escaped notice. In my specific case, object_list is a generator, and each iteration performs a variety of functions (specifically, looking up other values that are of interest). So, this ultimately takes a while to run.

My workaround was simply to set page_obj.object_list = list(page_obj.object_list) prior to it being used in the template. It seems the actual fix here would just be to perform this operation in __init__ .

Attachments (1)

ticket19938.diff (1.9 KB ) - added by Dmitry Gladkov 11 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I'd rather not consume object_list in __init__, because it should still be possible to instanciate a Page without querying the database.
But maybe something like that (untested):

         # The object_list is converted to a list so that if it was a QuerySet
         # it won't be a database hit per __getitem__.
-        return list(self.object_list)[index]
+        if not isinstance(self.object_list, list):
+            self.object_list = list(self.object_list)
+        return self.object_list[index]

by Dmitry Gladkov, 11 years ago

Attachment: ticket19938.diff added

comment:2 by Dmitry Gladkov, 11 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

As the comment in the actual function suggests, converting self.object_list to list only makes sense if we are paginating QuerySet, so I changed the example code above to check if object_list is instance of QuerySet.

comment:3 by Simon Charette, 11 years ago

Triage Stage: Ready for checkinAccepted

Please don't mark your own ticket to Ready for checkin, wait/ask for someone else to review it.

comment:4 by Claude Paroz, 11 years ago

I'm not sure the proposed patch addresses the original issue, where object_list was apparently not a QuerySet, but a more "costly" generator.

comment:5 by Simon Charette, 11 years ago

Needs tests: set
Patch needs improvement: set
Version: 1.4master

claudep I think your initial approach is the correct way of handling this.

comment:6 by andrewjesaitis, 11 years ago

Owner: changed from nobody to andrewjesaitis
Status: newassigned

comment:7 by andrewjesaitis, 11 years ago

Needs tests: unset
Patch needs improvement: unset

Added a patch for the issue as suggested by claudep. https://github.com/django/django/pull/919

Test is incorporated into test_page_getitem

comment:8 by Simon Charette, 11 years ago

Triage Stage: AcceptedReady for checkin

All pagination tests pass on py273, py323 SQLite.

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

Resolution: fixed
Status: assignedclosed

In 31f6421b134e4e83a459d2faa1009b33fefd6276:

Fixed #19938 -- Consumed iterator only once in paginator's Page

Thanks Joshua Fialkoff for the report.

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