Opened 7 years ago
Closed 6 years ago
#29244 closed Cleanup/optimization (fixed)
Paginator.count() may incorrectly silence AttributeError and TypeError
Reported by: | Rob Jauquet | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | 2.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
With this code:
from django.contrib.postgres.search import SearchQuery, Value, Func from django.core.paginator import Paginator from django.db.models import F, TextField, DateTimeField, Models from django.utils import timezone class Entry(Model): text = TextField() published_on = DateTimeField(default=timezone.now()) class Headline(Func): function = 'ts_headline' output_field = TextField() def __init__(self, text, query, options=None): extra = {} expressions = [text, query] if options: opt_str = '' for key, value in options.items(): opt_str += f'{key}={value},' expressions.append(Value(opt_str[:-1])) super().__init__(*expressions, **extra) entries = Entry.objects.annotate( text_highlighted=Headline( F('text'), SearchQuery('house'), options={'HighlightAll': False}, ) ).order_by('-published_on') paginator = Paginator(entries, 20) # evaluates the queryset paginator.page(1)
Passing options by dict
to the Func
subclass causes the count step for pagination to fail silently in paginator.py line 85 (1) which is in turn caused by a silenced TypeError during the hash in expressions.py line 372 (2).
Then, because of the fallback to len
on paginator.py line 90 (3) the entire queryset is evaluated instead of evaluating just the specified page (and in this specific case, calling ts_headline on every row instead of just the first page).
The correct way of passing options using **options
instead of options=None
fixes this case, but the silent error and fallback to calling len
on a queryset causes an unexpected expensive query when using pagination.
(1) https://github.com/django/django/blob/281c0223b376d6fa1a11e0726d824ed35cfe7524/django/core/paginator.py#L85
(2) https://github.com/django/django/blob/281c0223b376d6fa1a11e0726d824ed35cfe7524/django/db/models/expressions.py#L372
(3) https://github.com/django/django/blob/281c0223b376d6fa1a11e0726d824ed35cfe7524/django/core/paginator.py#L90
Change History (7)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
comment:3 by , 7 years ago
Thanks for the tip. I've updated the links.
For my use case, it would make sense to not call len
on a queryset in paginator.py and instead surface the exception. The len
fallback could be used only if the thing passed in is not a queryset.
comment:4 by , 7 years ago
Summary: | Passing dict to a Func subclass fails silently on hash step → Paginator.count() may incorrectly silence AttributeError and TypeError |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
It looks like the best solution would be to remove the try/except in Paginator.count()
. AttributeError
can be removed with hasattr(self.object_list, 'count')
and TypeError
with some checking that count is a method without any arguments. Perhaps a new function in django/utils/inspect.py
would help with that.
Do you have a proposal of how to address this? By the way, if you press the "y" key when viewing a GitHub page, you can get a link that includes a commit hash. Without that, the links can quickly go stale as line numbers change.