[Django] #19938: django.core.paginator.Page.__getitem__ being called multiple times when used from a template.
Django
used from a template.
-----------------------------------+-----------------------
Reporter: joshua.fialkoff@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.4
Severity: Normal | Keywords: paginator
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-----------------------------------+-----------------------
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__ }}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.4
Keywords: paginator | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Bug => Cleanup/optimization
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:1>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: Core (Other) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: paginator | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by dgl):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:2>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.4
Keywords: paginator | Triage Stage: Accepted
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* stage: Ready for checkin => Accepted
Comment:
Please don't mark your own ticket to ''Ready for checkin'', wait/ask for
someone else to review it.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:3>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.4
Keywords: paginator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by claudep):
I'm not sure the proposed patch addresses the original issue, where
`object_list` was apparently not a `QuerySet`, but a more "costly"
generator.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:4>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: paginator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
* version: 1.4 => master
* needs_tests: 0 => 1
Comment:
claudep I think your initial approach is the correct way of handling this.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:5>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner:
Type: | andrewjesaitis
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: paginator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by andrewjesaitis):
* owner: nobody => andrewjesaitis
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:6>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner:
Type: | andrewjesaitis
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: paginator | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by andrewjesaitis):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
Added a patch for the issue as suggested by claudep.
https://github.com/django/django/pull/919
Test is incorporated into {{{test_page_getitem}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:7>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner:
Type: | andrewjesaitis
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by charettes):
* stage: Accepted => Ready for checkin
Comment:
All pagination tests pass on py273, py323 SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:8>
Django
used from a template.
Reporter: joshua.fialkoff@… | Owner:
Type: | andrewjesaitis
Component: Core (Other) | Version: master
Keywords: paginator | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"31f6421b134e4e83a459d2faa1009b33fefd6276"]:
{{{
#!CommitTicketReference repository=""
revision="31f6421b134e4e83a459d2faa1009b33fefd6276"
Fixed #19938 -- Consumed iterator only once in paginator's Page
Thanks Joshua Fialkoff for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19938#comment:9>