From c4980e28e57f385d8ffed5e32ce373e52ce61049 Mon Sep 17 00:00:00 2001
From: Emad Mokhtar <emad.m.habib@gmail.com>
Date: Tue, 7 Jun 2016 14:24:19 +0300
Subject: [PATCH] Fixed #26290 -- Warned that paginating an unordered QuerySet
may result in inconsistent results.
---
django/core/paginator.py | 17 +++++++++++++++++
tests/admin_views/admin.py | 2 +-
tests/pagination/tests.py | 26 +++++++++++++++++---------
tests/sitemaps_tests/test_http.py | 2 +-
tests/sitemaps_tests/urls/http.py | 4 ++--
5 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/django/core/paginator.py b/django/core/paginator.py
index 29b8969b90..e64d9804c5 100644
--- a/django/core/paginator.py
+++ b/django/core/paginator.py
@@ -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()
self.per_page = int(per_page)
self.orphans = int(orphans)
self.allow_empty_first_page = allow_empty_first_page
@@ -94,6 +100,17 @@ class Paginator(object):
"""
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.
diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py
index 2146fa66f2..a987e146dc 100644
--- a/tests/admin_views/admin.py
+++ b/tests/admin_views/admin.py
@@ -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):
diff --git a/tests/pagination/tests.py b/tests/pagination/tests.py
index 8990c7fb5c..cc063c9eb8 100644
--- a/tests/pagination/tests.py
+++ b/tests/pagination/tests.py
@@ -5,6 +5,7 @@ from datetime import datetime
from django.core.paginator import (
EmptyPage, InvalidPage, PageNotAnInteger, Paginator,
+ UnorderedObjectListWarning,
)
from django.test import TestCase
from django.utils import six
@@ -256,7 +257,7 @@ class ModelPaginationTests(TestCase):
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 @@ class ModelPaginationTests(TestCase):
"<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,7 +277,7 @@ class ModelPaginationTests(TestCase):
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, [
@@ -286,9 +285,7 @@ class ModelPaginationTests(TestCase):
"<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 @@ class ModelPaginationTests(TestCase):
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 @@ class ModelPaginationTests(TestCase):
)
# 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)
diff --git a/tests/sitemaps_tests/test_http.py b/tests/sitemaps_tests/test_http.py
index aac4d69a20..33599e9ed9 100644
--- a/tests/sitemaps_tests/test_http.py
+++ b/tests/sitemaps_tests/test_http.py
@@ -194,7 +194,7 @@ class HTTPSitemapTests(SitemapTestsBase):
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)
diff --git a/tests/sitemaps_tests/urls/http.py b/tests/sitemaps_tests/urls/http.py
index 72e14d5e7f..e2fc991a29 100644
--- a/tests/sitemaps_tests/urls/http.py
+++ b/tests/sitemaps_tests/urls/http.py
@@ -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 @@ sitemaps_lastmod_descending = OrderedDict([
])
generic_sitemaps = {
- 'generic': GenericSitemap({'queryset': TestModel.objects.all()}),
+ 'generic': GenericSitemap({'queryset': TestModel.objects.order_by('pk').all()}),
}