Opened 12 years ago
Closed 12 years ago
#19938 closed Cleanup/optimization (fixed)
django.core.paginator.Page.__getitem__ being called multiple times when used from a template.
Reported by: | 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)
Change History (10)
comment:1 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Bug → Cleanup/optimization |
by , 12 years ago
Attachment: | ticket19938.diff added |
---|
comment:2 by , 12 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready 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 , 12 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Please don't mark your own ticket to Ready for checkin, wait/ask for someone else to review it.
comment:4 by , 12 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 , 12 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Version: | 1.4 → master |
claudep I think your initial approach is the correct way of handling this.
comment:6 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 12 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 , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
All pagination tests pass on py273, py323 SQLite.
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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):