Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#7478 closed (fixed)

Paginator could detect if the object_list is a QuerySet or a list

Reported by: batiste@… Owned by: nobody
Component: Core (Other) Version: master
Severity: Keywords: paginator
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

If the Paginator is accidentally used to count a large QuerySet (instead of a QuerySetPaginator), the generated SQL request could be inefficient (all content will be retrieved just to make len on it). Why don't we automatically detect if object_list has a count attribute? The paginator API will be simpler and it will be more difficult to introduce performance issues.

    def _get_count(self):
        if self._count is None:
            if hasattr(self.object_list, 'count'):
                self._count = self.object_list.count()
            else:
                self._count = len(self.object_list)
        return self._count
    count = property(_get_count)

And by the way the Paginator is far way to simple for my needs so I have sub class it. But I can't derivate from both QuerySetPaginator and Paginator. It's antoher reason why I think it's not a good idea to have 2 classes.

Change History (8)

comment:1 Changed 7 years ago by anonymous

  • Component changed from Uncategorized to Core framework
  • Keywords paginator added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by anonymous

Woops count is not the good attribute to test because lists also have it. "all" seems a safe choice to me.

comment:3 Changed 7 years ago by telenieko

type(xxx) == QuerySet ... ?

comment:4 Changed 7 years ago by anonymous

if isinstance(self.object_list, QuerySet):
    self._count = self.object_list.count()

comment:5 Changed 7 years ago by anonymous

I am using this one and it works nicely:

    def _get_count(self):
        if self._count is None:
            if isinstance(self.object_list, QuerySet):
                self._count = self.object_list.count()
            else:
                self._count = len(self.object_list)
        return self._count
    count = property(_get_count)

comment:6 Changed 7 years ago by mrts

  • milestone set to 1.0

This looks reasonable and in scope for 1.0.

comment:7 Changed 7 years ago by adrian

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

(In [7865]) Fixed #7478 -- Rolled QuerySetPaginator into the Paginator class, to simplify things. QuerySetPaginator still exists as an alias, for backwards compatibility. Thanks for the suggestion, batiste@…

comment:8 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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