Opened 2 years ago

Closed 22 months 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: master
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 dgl 2 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/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]

Changed 2 years ago by dgl

comment:2 Changed 2 years ago by dgl

  • Has patch set
  • Triage Stage changed from Accepted to 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 Changed 2 years ago by charettes

  • Triage Stage changed from Ready for checkin to Accepted

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

comment:4 Changed 2 years ago 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.

comment:5 Changed 2 years ago by charettes

  • Needs tests set
  • Patch needs improvement set
  • Version changed from 1.4 to master

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

comment:6 Changed 2 years ago by andrewjesaitis

  • Owner changed from nobody to andrewjesaitis
  • Status changed from new to assigned

comment:7 Changed 2 years ago by andrewjesaitis

  • 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 Changed 2 years ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

All pagination tests pass on py273, py323 SQLite.

comment:9 Changed 22 months ago by Claude Paroz <claude@…>

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

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