Permalink
Browse files

Fixed #26290 -- Warned that paginating an unordered QuerySet may resu…

…lt in inconsistent results.
  • Loading branch information...
EmadMokhtar authored and timgraham committed Jun 7, 2016
1 parent 724dd20 commit c4980e28e57f385d8ffed5e32ce373e52ce61049
View
@@ -1,10 +1,15 @@
import collections
+import warnings
from math import ceil
from django.utils import six
from django.utils.functional import cached_property
+class UnorderedObjectListWarning(RuntimeWarning):
+ pass
+
+
class InvalidPage(Exception):
pass
@@ -22,6 +27,7 @@ class Paginator(object):
def __init__(self, object_list, per_page, orphans=0,
allow_empty_first_page=True):
self.object_list = object_list
+ self._check_object_list_is_ordered()

This comment has been minimized.

Show comment
Hide comment
@shtalinberg

shtalinberg Apr 21, 2017

This line hit to database when I just create new Pagination object with Django 1.11
https://travis-ci.org/shtalinberg/django-el-pagination/jobs/224166104
Django 1.8-1.10 don't have that bug - all tests success

@shtalinberg

shtalinberg Apr 21, 2017

This line hit to database when I just create new Pagination object with Django 1.11
https://travis-ci.org/shtalinberg/django-el-pagination/jobs/224166104
Django 1.8-1.10 don't have that bug - all tests success

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 21, 2017

Member

Please report issues at http://code.djangoproject.com rather than commenting on commits.

@timgraham

timgraham Apr 21, 2017

Member

Please report issues at http://code.djangoproject.com rather than commenting on commits.

This comment has been minimized.

Show comment
Hide comment
self.per_page = int(per_page)
self.orphans = int(orphans)
self.allow_empty_first_page = allow_empty_first_page
@@ -94,6 +100,17 @@ def page_range(self):
"""
return six.moves.range(1, self.num_pages + 1)
+ def _check_object_list_is_ordered(self):
+ """
+ Warn if self.object_list is unordered (typically a QuerySet).
+ """
+ if hasattr(self.object_list, 'ordered') and not self.object_list.ordered:
+ warnings.warn(
+ 'Pagination may yield inconsistent results with an unordered '
+ 'object_list: {!r}'.format(self.object_list),
+ UnorderedObjectListWarning
+ )
+
QuerySetPaginator = Paginator # For backwards-compatibility.
@@ -481,7 +481,7 @@ class FieldOverridePostAdmin(PostAdmin):
class CustomChangeList(ChangeList):
def get_queryset(self, request):
- return self.root_queryset.filter(pk=9999) # Does not exist
+ return self.root_queryset.order_by('pk').filter(pk=9999) # Doesn't exist
class GadgetAdmin(admin.ModelAdmin):
View
@@ -5,6 +5,7 @@
from django.core.paginator import (
EmptyPage, InvalidPage, PageNotAnInteger, Paginator,
+ UnorderedObjectListWarning,
)
from django.test import TestCase
from django.utils import six
@@ -256,7 +257,7 @@ def setUp(self):
a.save()
def test_first_page(self):
- paginator = Paginator(Article.objects.all(), 5)
+ paginator = Paginator(Article.objects.order_by('id'), 5)
p = paginator.page(1)
self.assertEqual("<Page 1 of 2>", six.text_type(p))
self.assertQuerysetEqual(p.object_list, [
@@ -265,9 +266,7 @@ def test_first_page(self):
"<Article: Article 3>",
"<Article: Article 4>",
"<Article: Article 5>"
- ],
- ordered=False
- )
+ ])
self.assertTrue(p.has_next())
self.assertFalse(p.has_previous())
self.assertTrue(p.has_other_pages())
@@ -278,17 +277,15 @@ def test_first_page(self):
self.assertEqual(5, p.end_index())
def test_last_page(self):
- paginator = Paginator(Article.objects.all(), 5)
+ paginator = Paginator(Article.objects.order_by('id'), 5)
p = paginator.page(2)
self.assertEqual("<Page 2 of 2>", six.text_type(p))
self.assertQuerysetEqual(p.object_list, [
"<Article: Article 6>",
"<Article: Article 7>",
"<Article: Article 8>",
"<Article: Article 9>"
- ],
- ordered=False
- )
+ ])
self.assertFalse(p.has_next())
self.assertTrue(p.has_previous())
self.assertTrue(p.has_other_pages())
@@ -303,7 +300,7 @@ def test_page_getitem(self):
Tests proper behavior of a paginator page __getitem__ (queryset
evaluation, slicing, exception raised).
"""
- paginator = Paginator(Article.objects.all(), 5)
+ paginator = Paginator(Article.objects.order_by('id'), 5)
p = paginator.page(1)
# Make sure object_list queryset is not evaluated by an invalid __getitem__ call.
@@ -323,3 +320,14 @@ def test_page_getitem(self):
)
# After __getitem__ is called, object_list is a list
self.assertIsInstance(p.object_list, list)
+
+ def test_paginating_unordered_queryset_raises_warning(self):
+ msg = (
+ "Pagination may yield inconsistent results with an unordered "
+ "object_list: <QuerySet [<Article: Article 1>, "
+ "<Article: Article 2>, <Article: Article 3>, <Article: Article 4>, "
+ "<Article: Article 5>, <Article: Article 6>, <Article: Article 7>, "
+ "<Article: Article 8>, <Article: Article 9>]>"
+ )
+ with self.assertRaisesMessage(UnorderedObjectListWarning, msg):
+ Paginator(Article.objects.all(), 5)
@@ -194,7 +194,7 @@ def test_sitemap_item(self):
Check to make sure that the raw item is included with each
Sitemap.get_url() url result.
"""
- test_sitemap = GenericSitemap({'queryset': TestModel.objects.all()})
+ test_sitemap = GenericSitemap({'queryset': TestModel.objects.order_by('pk').all()})
def is_testmodel(url):
return isinstance(url['item'], TestModel)
@@ -27,7 +27,7 @@ class SimpleI18nSitemap(Sitemap):
i18n = True
def items(self):
- return I18nTestModel.objects.all()
+ return I18nTestModel.objects.order_by('pk').all()
class EmptySitemap(Sitemap):
@@ -115,7 +115,7 @@ def testmodelview(request, id):
])
generic_sitemaps = {
- 'generic': GenericSitemap({'queryset': TestModel.objects.all()}),
+ 'generic': GenericSitemap({'queryset': TestModel.objects.order_by('pk').all()}),
}

0 comments on commit c4980e2

Please sign in to comment.