From 4919492582547d227a22852ad2339fa73739cc94 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Sat, 17 Jan 2015 00:10:43 +0000 Subject: [PATCH 01/12] First pass at cursor pagination --- rest_framework/pagination.py | 51 +++++++++++++++++++++ tests/test_pagination.py | 88 ++++++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 1b7524c6c..89d6f9f4f 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -3,10 +3,12 @@ Pagination serializers determine the structure of the output that should be used for paginated responses. """ from __future__ import unicode_literals +from base64 import b64encode, b64decode from collections import namedtuple from django.core.paginator import InvalidPage, Paginator as DjangoPaginator from django.template import Context, loader from django.utils import six +from django.utils.six.moves.urllib import parse as urlparse from django.utils.translation import ugettext as _ from rest_framework.compat import OrderedDict from rest_framework.exceptions import NotFound @@ -377,3 +379,52 @@ class LimitOffsetPagination(BasePagination): template = loader.get_template(self.template) context = Context(self.get_html_context()) return template.render(context) + + +class CursorPagination(BasePagination): + # reverse + # limit + # multiple orderings + cursor_query_param = 'cursor' + page_size = 5 + + def paginate_queryset(self, queryset, request, view=None): + self.base_url = request.build_absolute_uri() + self.ordering = self.get_ordering() + encoded = request.query_params.get(self.cursor_query_param) + + if encoded is None: + cursor = None + else: + cursor = self.decode_cursor(encoded, self.ordering) + + if cursor is not None: + kwargs = {self.ordering + '__gt': cursor} + queryset = queryset.filter(**kwargs) + + results = list(queryset[:self.page_size + 1]) + self.page = results[:self.page_size] + self.has_next = len(results) > len(self.page) + return self.page + + def get_next_link(self): + if not self.has_next: + return None + last_item = self.page[-1] + cursor = self.get_cursor_from_instance(last_item, self.ordering) + encoded = self.encode_cursor(cursor, self.ordering) + return replace_query_param(self.base_url, self.cursor_query_param, encoded) + + def get_ordering(self): + return 'created' + + def get_cursor_from_instance(self, instance, ordering): + return getattr(instance, ordering) + + def decode_cursor(self, encoded, ordering): + items = urlparse.parse_qs(b64decode(encoded)) + return items.get(ordering)[0] + + def encode_cursor(self, cursor, ordering): + items = [(ordering, cursor)] + return b64encode(urlparse.urlencode(items, doseq=True)) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 7cc923472..7f18b446b 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -422,6 +422,94 @@ class TestLimitOffset: assert queryset == [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] +class TestCursorPagination: + """ + Unit tests for `pagination.CursorPagination`. + """ + + def setup(self): + class MockObject(object): + def __init__(self, idx): + self.created = idx + + class MockQuerySet(object): + def __init__(self, items): + self.items = items + + def filter(self, created__gt): + return [ + item for item in self.items + if item.created > int(created__gt) + ] + + def __getitem__(self, sliced): + return self.items[sliced] + + self.pagination = pagination.CursorPagination() + self.queryset = MockQuerySet( + [MockObject(idx) for idx in range(1, 21)] + ) + + def paginate_queryset(self, request): + return list(self.pagination.paginate_queryset(self.queryset, request)) + + # def get_paginated_content(self, queryset): + # response = self.pagination.get_paginated_response(queryset) + # return response.data + + # def get_html_context(self): + # return self.pagination.get_html_context() + + def test_following_cursor(self): + request = Request(factory.get('/')) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 2, 3, 4, 5] + + next_url = self.pagination.get_next_link() + assert next_url + + request = Request(factory.get(next_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [6, 7, 8, 9, 10] + + next_url = self.pagination.get_next_link() + assert next_url + + request = Request(factory.get(next_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [11, 12, 13, 14, 15] + + next_url = self.pagination.get_next_link() + assert next_url + + request = Request(factory.get(next_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [16, 17, 18, 19, 20] + + next_url = self.pagination.get_next_link() + assert next_url is None + + # assert content == { + # 'results': [1, 2, 3, 4, 5], + # 'previous': None, + # 'next': 'http://testserver/?limit=5&offset=5', + # 'count': 100 + # } + # assert context == { + # 'previous_url': None, + # 'next_url': 'http://testserver/?limit=5&offset=5', + # 'page_links': [ + # PageLink('http://testserver/?limit=5', 1, True, False), + # PageLink('http://testserver/?limit=5&offset=5', 2, False, False), + # PageLink('http://testserver/?limit=5&offset=10', 3, False, False), + # PAGE_BREAK, + # PageLink('http://testserver/?limit=5&offset=95', 20, False, False), + # ] + # } + # assert self.pagination.display_page_controls + # assert isinstance(self.pagination.to_html(), type('')) + + def test_get_displayed_page_numbers(): """ Test our contextual page display function. From 492f3c410d3a91a3f37218e93485a693d9078000 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Sat, 17 Jan 2015 00:59:02 +0000 Subject: [PATCH 02/12] Cleaning up cursor implementation --- rest_framework/pagination.py | 51 +++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 89d6f9f4f..3984da13e 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -381,10 +381,33 @@ class LimitOffsetPagination(BasePagination): return template.render(context) +Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) + + +def decode_cursor(encoded): + tokens = urlparse.parse_qs(b64decode(encoded)) + try: + offset = int(tokens['offset'][0]) + reverse = bool(int(tokens['reverse'][0])) + position = tokens['position'][0] + except (TypeError, ValueError): + return None + + return Cursor(offset=offset, reverse=reverse, position=position) + + +def encode_cursor(cursor): + tokens = { + 'offset': str(cursor.offset), + 'reverse': '1' if cursor.reverse else '0', + 'position': cursor.position + } + return b64encode(urlparse.urlencode(tokens, doseq=True)) + + class CursorPagination(BasePagination): # reverse # limit - # multiple orderings cursor_query_param = 'cursor' page_size = 5 @@ -396,10 +419,11 @@ class CursorPagination(BasePagination): if encoded is None: cursor = None else: - cursor = self.decode_cursor(encoded, self.ordering) + cursor = decode_cursor(encoded) + # TODO: Invalid cursors should 404 if cursor is not None: - kwargs = {self.ordering + '__gt': cursor} + kwargs = {self.ordering + '__gt': cursor.position} queryset = queryset.filter(**kwargs) results = list(queryset[:self.page_size + 1]) @@ -411,20 +435,21 @@ class CursorPagination(BasePagination): if not self.has_next: return None last_item = self.page[-1] - cursor = self.get_cursor_from_instance(last_item, self.ordering) - encoded = self.encode_cursor(cursor, self.ordering) + position = self.get_position_from_instance(last_item, self.ordering) + cursor = Cursor(offset=0, reverse=False, position=position) + encoded = encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) def get_ordering(self): return 'created' - def get_cursor_from_instance(self, instance, ordering): - return getattr(instance, ordering) + def get_position_from_instance(self, instance, ordering): + return str(getattr(instance, ordering)) - def decode_cursor(self, encoded, ordering): - items = urlparse.parse_qs(b64decode(encoded)) - return items.get(ordering)[0] + # def decode_cursor(self, encoded, ordering): + # items = urlparse.parse_qs(b64decode(encoded)) + # return items.get(ordering)[0] - def encode_cursor(self, cursor, ordering): - items = [(ordering, cursor)] - return b64encode(urlparse.urlencode(items, doseq=True)) + # def encode_cursor(self, cursor, ordering): + # items = [(ordering, cursor)] + # return b64encode(urlparse.urlencode(items, doseq=True)) From dbb684117f6fe0f9c34f98d5e914fc106090cdbc Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Mon, 19 Jan 2015 09:24:42 +0000 Subject: [PATCH 03/12] Add offset support for cursor pagination --- rest_framework/pagination.py | 67 ++++++++++++++++++++++++++---------- tests/test_pagination.py | 64 ++++++++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 22 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 3984da13e..f56f55ce1 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -1,3 +1,4 @@ +# coding: utf-8 """ Pagination serializers determine the structure of the output that should be used for paginated responses. @@ -385,7 +386,7 @@ Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) def decode_cursor(encoded): - tokens = urlparse.parse_qs(b64decode(encoded)) + tokens = urlparse.parse_qs(b64decode(encoded), keep_blank_values=True) try: offset = int(tokens['offset'][0]) reverse = bool(int(tokens['reverse'][0])) @@ -406,8 +407,7 @@ def encode_cursor(cursor): class CursorPagination(BasePagination): - # reverse - # limit + # TODO: reverse cursors cursor_query_param = 'cursor' page_size = 5 @@ -417,26 +417,63 @@ class CursorPagination(BasePagination): encoded = request.query_params.get(self.cursor_query_param) if encoded is None: - cursor = None + self.cursor = None else: - cursor = decode_cursor(encoded) + self.cursor = decode_cursor(encoded) # TODO: Invalid cursors should 404 - if cursor is not None: - kwargs = {self.ordering + '__gt': cursor.position} + if self.cursor is not None and self.cursor.position != '': + kwargs = {self.ordering + '__gt': self.cursor.position} queryset = queryset.filter(**kwargs) - results = list(queryset[:self.page_size + 1]) + # The offset is used in order to deal with cases where we have + # items with an identical position. This allows the cursors + # to gracefully deal with non-unique fields as the ordering. + offset = 0 if (self.cursor is None) else self.cursor.offset + + # We fetch an extra item in order to determine if there is a next page. + results = list(queryset[offset:offset + self.page_size + 1]) self.page = results[:self.page_size] self.has_next = len(results) > len(self.page) + self.next_item = results[-1] if self.has_next else None return self.page def get_next_link(self): if not self.has_next: return None - last_item = self.page[-1] - position = self.get_position_from_instance(last_item, self.ordering) - cursor = Cursor(offset=0, reverse=False, position=position) + + compare = self.get_position_from_instance(self.next_item, self.ordering) + offset = 0 + for item in reversed(self.page): + position = self.get_position_from_instance(item, self.ordering) + if position != compare: + # The item in this position and the item following it + # have different positions. We can use this position as + # our marker. + break + + # The item in this postion has the same position as the item + # following it, we can't use it as a marker position, so increment + # the offset and keep seeking to the previous item. + compare = position + offset += 1 + + else: + if self.cursor is None: + # There were no unique positions in the page, and we were + # on the first page, ie. there was no existing cursor. + # Our cursor will have an offset equal to the page size, + # but no position to filter against yet. + offset = self.page_size + position = '' + else: + # There were no unique positions in the page. + # Use the position from the existing cursor and increment + # it's offset by the page size. + offset = self.cursor.offset + self.page_size + position = self.cursor.position + + cursor = Cursor(offset=offset, reverse=False, position=position) encoded = encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) @@ -445,11 +482,3 @@ class CursorPagination(BasePagination): def get_position_from_instance(self, instance, ordering): return str(getattr(instance, ordering)) - - # def decode_cursor(self, encoded, ordering): - # items = urlparse.parse_qs(b64decode(encoded)) - # return items.get(ordering)[0] - - # def encode_cursor(self, cursor, ordering): - # items = [(ordering, cursor)] - # return b64encode(urlparse.urlencode(items, doseq=True)) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 7f18b446b..f04079a72 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -447,7 +447,7 @@ class TestCursorPagination: self.pagination = pagination.CursorPagination() self.queryset = MockQuerySet( - [MockObject(idx) for idx in range(1, 21)] + [MockObject(idx) for idx in range(1, 16)] ) def paginate_queryset(self, request): @@ -479,16 +479,74 @@ class TestCursorPagination: queryset = self.paginate_queryset(request) assert [item.created for item in queryset] == [11, 12, 13, 14, 15] + next_url = self.pagination.get_next_link() + assert next_url is None + + +class TestCrazyCursorPagination: + """ + Unit tests for `pagination.CursorPagination`. + """ + + def setup(self): + class MockObject(object): + def __init__(self, idx): + self.created = idx + + class MockQuerySet(object): + def __init__(self, items): + self.items = items + + def filter(self, created__gt): + return [ + item for item in self.items + if item.created > int(created__gt) + ] + + def __getitem__(self, sliced): + return self.items[sliced] + + self.pagination = pagination.CursorPagination() + self.queryset = MockQuerySet([ + MockObject(idx) for idx in [ + 1, 1, 1, 1, 1, + 1, 1, 1, 1, 1, + 1, 1, 2, 3, 4, + 5, 6, 7, 8, 9 + ] + ]) + + def paginate_queryset(self, request): + return list(self.pagination.paginate_queryset(self.queryset, request)) + + def test_following_cursor_identical_items(self): + request = Request(factory.get('/')) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + next_url = self.pagination.get_next_link() assert next_url request = Request(factory.get(next_url)) queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [16, 17, 18, 19, 20] + assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + + next_url = self.pagination.get_next_link() + assert next_url + + request = Request(factory.get(next_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 1, 2, 3, 4] + + next_url = self.pagination.get_next_link() + assert next_url + + request = Request(factory.get(next_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [5, 6, 7, 8, 9] next_url = self.pagination.get_next_link() assert next_url is None - # assert content == { # 'results': [1, 2, 3, 4, 5], # 'previous': None, From cae9528c54ea13863ea056d40168e8d8df68b276 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 10:28:19 +0000 Subject: [PATCH 04/12] Add support for reverse cursors --- rest_framework/pagination.py | 126 +++++++++++++++++++++++++++++------ tests/test_pagination.py | 6 ++ 2 files changed, 112 insertions(+), 20 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 5482788a3..9e22a8bf3 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -407,45 +407,84 @@ def encode_cursor(cursor): class CursorPagination(BasePagination): - # TODO: reverse cursors + # TODO: handle queries with '' as a legitimate position cursor_query_param = 'cursor' page_size = 5 def paginate_queryset(self, queryset, request, view=None): self.base_url = request.build_absolute_uri() self.ordering = self.get_ordering() - encoded = request.query_params.get(self.cursor_query_param) + # Determine if we have a cursor, and if so then decode it. + encoded = request.query_params.get(self.cursor_query_param) if encoded is None: self.cursor = None + (offset, reverse, current_position) = (0, False, '') else: self.cursor = decode_cursor(encoded) + (offset, reverse, current_position) = self.cursor # TODO: Invalid cursors should 404 - if self.cursor is not None and self.cursor.position != '': - kwargs = {self.ordering + '__gt': self.cursor.position} + # Cursor pagination always enforces an ordering. + if reverse: + queryset = queryset.order_by('-' + self.ordering) + else: + queryset = queryset.order_by(self.ordering) + + # If we have a cursor with a fixed position then filter by that. + if current_position != '': + if self.cursor.reverse: + kwargs = {self.ordering + '__lt': current_position} + else: + kwargs = {self.ordering + '__gt': current_position} queryset = queryset.filter(**kwargs) - # The offset is used in order to deal with cases where we have - # items with an identical position. This allows the cursors - # to gracefully deal with non-unique fields as the ordering. - offset = 0 if (self.cursor is None) else self.cursor.offset - - # We fetch an extra item in order to determine if there is a next page. + # If we have an offset cursor then offset the entire page by that amount. + # We also always fetch an extra item in order to determine if there is a + # page following on from this one. results = list(queryset[offset:offset + self.page_size + 1]) self.page = results[:self.page_size] - self.has_next = len(results) > len(self.page) - self.next_item = results[-1] if self.has_next else None + + # Determine the position of the final item following the page. + if len(results) > len(self.page): + has_following_postion = True + following_position = self._get_position_from_instance(results[-1], self.ordering) + else: + has_following_postion = False + following_position = None + + # If we have a reverse queryset, then the query ordering was in reverse + # so we need to reverse the items again before returning them to the user. + if reverse: + self.page = reversed(self.page) + + if reverse: + # Determine next and previous positions for reverse cursors. + self.has_next = current_position != '' or offset > 0 + self.has_previous = has_following_postion + if self.has_next: + self.next_position = current_position + if self.has_previous: + self.previous_position = following_position + else: + # Determine next and previous positions for forward cursors. + self.has_next = has_following_postion + self.has_previous = current_position != '' or offset > 0 + if self.has_next: + self.next_position = following_position + if self.has_previous: + self.previous_position = current_position + return self.page def get_next_link(self): if not self.has_next: return None - compare = self.get_position_from_instance(self.next_item, self.ordering) + compare = self.next_position offset = 0 for item in reversed(self.page): - position = self.get_position_from_instance(item, self.ordering) + position = self._get_position_from_instance(item, self.ordering) if position != compare: # The item in this position and the item following it # have different positions. We can use this position as @@ -459,26 +498,73 @@ class CursorPagination(BasePagination): offset += 1 else: - if self.cursor is None: - # There were no unique positions in the page, and we were - # on the first page, ie. there was no existing cursor. + # There were no unique positions in the page. + if not self.has_previous: + # We are on the first page. # Our cursor will have an offset equal to the page size, # but no position to filter against yet. offset = self.page_size position = '' + elif self.cursor.reverse: + # The change in direction will introduce a paging artifact, + # where we end up skipping forward a few extra items. + offset = 0 + position = self.previous_position else: - # There were no unique positions in the page. # Use the position from the existing cursor and increment # it's offset by the page size. offset = self.cursor.offset + self.page_size - position = self.cursor.position + position = self.previous_position cursor = Cursor(offset=offset, reverse=False, position=position) encoded = encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) + def get_previous_link(self): + if not self.has_previous: + return None + + compare = self.previous_position + offset = 0 + for item in self.page: + position = self._get_position_from_instance(item, self.ordering) + if position != compare: + # The item in this position and the item following it + # have different positions. We can use this position as + # our marker. + break + + # The item in this postion has the same position as the item + # following it, we can't use it as a marker position, so increment + # the offset and keep seeking to the previous item. + compare = position + offset += 1 + + else: + # There were no unique positions in the page. + if not self.has_next: + # We are on the final page. + # Our cursor will have an offset equal to the page size, + # but no position to filter against yet. + offset = self.page_size + position = '' + elif self.cursor.reverse: + # Use the position from the existing cursor and increment + # it's offset by the page size. + offset = self.cursor.offset + self.page_size + position = self.next_position + else: + # The change in direction will introduce a paging artifact, + # where we end up skipping back a few extra items. + offset = 0 + position = self.next_position + + cursor = Cursor(offset=offset, reverse=True, position=position) + encoded = encode_cursor(cursor) + return replace_query_param(self.base_url, self.cursor_query_param, encoded) + def get_ordering(self): return 'created' - def get_position_from_instance(self, instance, ordering): + def _get_position_from_instance(self, instance, ordering): return str(getattr(instance, ordering)) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index f04079a72..47019671f 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -442,6 +442,9 @@ class TestCursorPagination: if item.created > int(created__gt) ] + def order_by(self, ordering): + return self + def __getitem__(self, sliced): return self.items[sliced] @@ -503,6 +506,9 @@ class TestCrazyCursorPagination: if item.created > int(created__gt) ] + def order_by(self, ordering): + return self + def __getitem__(self, sliced): return self.items[sliced] From f1af603fb05fce236a4258e18df8af8888043247 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 10:51:04 +0000 Subject: [PATCH 05/12] Tests for reverse pagination --- rest_framework/pagination.py | 2 + tests/test_pagination.py | 98 ++++++++++++++++++++++++++---------- 2 files changed, 73 insertions(+), 27 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 9e22a8bf3..d5af2ac81 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -408,6 +408,8 @@ def encode_cursor(cursor): class CursorPagination(BasePagination): # TODO: handle queries with '' as a legitimate position + # Support case where ordering is already negative + # Support tuple orderings cursor_query_param = 'cursor' page_size = 5 diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 47019671f..4907a0807 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -436,13 +436,22 @@ class TestCursorPagination: def __init__(self, items): self.items = items - def filter(self, created__gt): - return [ + def filter(self, created__gt=None, created__lt=None): + if created__gt is not None: + return MockQuerySet([ + item for item in self.items + if item.created > int(created__gt) + ]) + + assert created__lt is not None + return MockQuerySet([ item for item in self.items - if item.created > int(created__gt) - ] + if item.created < int(created__lt) + ]) def order_by(self, ordering): + if ordering.startswith('-'): + return MockQuerySet(reversed(self.items)) return self def __getitem__(self, sliced): @@ -485,6 +494,25 @@ class TestCursorPagination: next_url = self.pagination.get_next_link() assert next_url is None + # Now page back again + + previous_url = self.pagination.get_previous_link() + assert previous_url + + request = Request(factory.get(previous_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [6, 7, 8, 9, 10] + + previous_url = self.pagination.get_previous_link() + assert previous_url + + request = Request(factory.get(previous_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 2, 3, 4, 5] + + previous_url = self.pagination.get_previous_link() + assert previous_url is None + class TestCrazyCursorPagination: """ @@ -500,13 +528,22 @@ class TestCrazyCursorPagination: def __init__(self, items): self.items = items - def filter(self, created__gt): - return [ + def filter(self, created__gt=None, created__lt=None): + if created__gt is not None: + return MockQuerySet([ + item for item in self.items + if item.created > int(created__gt) + ]) + + assert created__lt is not None + return MockQuerySet([ item for item in self.items - if item.created > int(created__gt) - ] + if item.created < int(created__lt) + ]) def order_by(self, ordering): + if ordering.startswith('-'): + return MockQuerySet(reversed(self.items)) return self def __getitem__(self, sliced): @@ -553,25 +590,32 @@ class TestCrazyCursorPagination: next_url = self.pagination.get_next_link() assert next_url is None - # assert content == { - # 'results': [1, 2, 3, 4, 5], - # 'previous': None, - # 'next': 'http://testserver/?limit=5&offset=5', - # 'count': 100 - # } - # assert context == { - # 'previous_url': None, - # 'next_url': 'http://testserver/?limit=5&offset=5', - # 'page_links': [ - # PageLink('http://testserver/?limit=5', 1, True, False), - # PageLink('http://testserver/?limit=5&offset=5', 2, False, False), - # PageLink('http://testserver/?limit=5&offset=10', 3, False, False), - # PAGE_BREAK, - # PageLink('http://testserver/?limit=5&offset=95', 20, False, False), - # ] - # } - # assert self.pagination.display_page_controls - # assert isinstance(self.pagination.to_html(), type('')) + + # Now page back again + + previous_url = self.pagination.get_previous_link() + assert previous_url + + request = Request(factory.get(previous_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 1, 2, 3, 4] + + previous_url = self.pagination.get_previous_link() + assert previous_url + + request = Request(factory.get(previous_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + + previous_url = self.pagination.get_previous_link() + assert previous_url + + request = Request(factory.get(previous_url)) + queryset = self.paginate_queryset(request) + assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + + previous_url = self.pagination.get_previous_link() + assert previous_url is None def test_get_displayed_page_numbers(): From 94b5f7a86e401e46f14fb8982afaa7a8c61847c9 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 12:14:52 +0000 Subject: [PATCH 06/12] Tidy up cursor tests and make more comprehensive --- rest_framework/pagination.py | 30 ++++- tests/test_pagination.py | 242 ++++++++++++++--------------------- 2 files changed, 122 insertions(+), 150 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index d5af2ac81..618352391 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -171,6 +171,8 @@ class PageNumberPagination(BasePagination): template = 'rest_framework/pagination/numbers.html' + invalid_page_message = _('Invalid page "{page_number}": {message}.') + def _handle_backwards_compat(self, view): """ Prior to version 3.1, pagination was handled in the view, and the @@ -203,7 +205,7 @@ class PageNumberPagination(BasePagination): try: self.page = paginator.page(page_number) except InvalidPage as exc: - msg = _('Invalid page "{page_number}": {message}.').format( + msg = self.invalid_page_message.format( page_number=page_number, message=six.text_type(exc) ) raise NotFound(msg) @@ -386,8 +388,8 @@ Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) def decode_cursor(encoded): - tokens = urlparse.parse_qs(b64decode(encoded), keep_blank_values=True) try: + tokens = urlparse.parse_qs(b64decode(encoded), keep_blank_values=True) offset = int(tokens['offset'][0]) reverse = bool(int(tokens['reverse'][0])) position = tokens['position'][0] @@ -411,7 +413,8 @@ class CursorPagination(BasePagination): # Support case where ordering is already negative # Support tuple orderings cursor_query_param = 'cursor' - page_size = 5 + page_size = api_settings.PAGINATE_BY + invalid_cursor_message = _('Invalid cursor') def paginate_queryset(self, queryset, request, view=None): self.base_url = request.build_absolute_uri() @@ -424,8 +427,9 @@ class CursorPagination(BasePagination): (offset, reverse, current_position) = (0, False, '') else: self.cursor = decode_cursor(encoded) + if self.cursor is None: + raise NotFound(self.invalid_cursor_message) (offset, reverse, current_position) = self.cursor - # TODO: Invalid cursors should 404 # Cursor pagination always enforces an ordering. if reverse: @@ -458,7 +462,7 @@ class CursorPagination(BasePagination): # If we have a reverse queryset, then the query ordering was in reverse # so we need to reverse the items again before returning them to the user. if reverse: - self.page = reversed(self.page) + self.page = list(reversed(self.page)) if reverse: # Determine next and previous positions for reverse cursors. @@ -483,8 +487,14 @@ class CursorPagination(BasePagination): if not self.has_next: return None - compare = self.next_position + if self.cursor and self.cursor.reverse and self.cursor.offset != 0: + # If we're reversing direction and we have an offset cursor + # then we cannot use the first position we find as a marker. + compare = self._get_position_from_instance(self.page[-1], self.ordering) + else: + compare = self.next_position offset = 0 + for item in reversed(self.page): position = self._get_position_from_instance(item, self.ordering) if position != compare: @@ -526,8 +536,14 @@ class CursorPagination(BasePagination): if not self.has_previous: return None - compare = self.previous_position + if self.cursor and not self.cursor.reverse and self.cursor.offset != 0: + # If we're reversing direction and we have an offset cursor + # then we cannot use the first position we find as a marker. + compare = self._get_position_from_instance(self.page[0], self.ordering) + else: + compare = self.previous_position offset = 0 + for item in self.page: position = self._get_position_from_instance(item, self.ordering) if position != compare: diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 4907a0807..e32dd0288 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -451,171 +451,127 @@ class TestCursorPagination: def order_by(self, ordering): if ordering.startswith('-'): - return MockQuerySet(reversed(self.items)) + return MockQuerySet(list(reversed(self.items))) return self def __getitem__(self, sliced): return self.items[sliced] - self.pagination = pagination.CursorPagination() - self.queryset = MockQuerySet( - [MockObject(idx) for idx in range(1, 16)] - ) + class ExamplePagination(pagination.CursorPagination): + page_size = 5 - def paginate_queryset(self, request): - return list(self.pagination.paginate_queryset(self.queryset, request)) - - # def get_paginated_content(self, queryset): - # response = self.pagination.get_paginated_response(queryset) - # return response.data - - # def get_html_context(self): - # return self.pagination.get_html_context() - - def test_following_cursor(self): - request = Request(factory.get('/')) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 2, 3, 4, 5] - - next_url = self.pagination.get_next_link() - assert next_url - - request = Request(factory.get(next_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [6, 7, 8, 9, 10] - - next_url = self.pagination.get_next_link() - assert next_url - - request = Request(factory.get(next_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [11, 12, 13, 14, 15] - - next_url = self.pagination.get_next_link() - assert next_url is None - - # Now page back again - - previous_url = self.pagination.get_previous_link() - assert previous_url - - request = Request(factory.get(previous_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [6, 7, 8, 9, 10] - - previous_url = self.pagination.get_previous_link() - assert previous_url - - request = Request(factory.get(previous_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 2, 3, 4, 5] - - previous_url = self.pagination.get_previous_link() - assert previous_url is None - - -class TestCrazyCursorPagination: - """ - Unit tests for `pagination.CursorPagination`. - """ - - def setup(self): - class MockObject(object): - def __init__(self, idx): - self.created = idx - - class MockQuerySet(object): - def __init__(self, items): - self.items = items - - def filter(self, created__gt=None, created__lt=None): - if created__gt is not None: - return MockQuerySet([ - item for item in self.items - if item.created > int(created__gt) - ]) - - assert created__lt is not None - return MockQuerySet([ - item for item in self.items - if item.created < int(created__lt) - ]) - - def order_by(self, ordering): - if ordering.startswith('-'): - return MockQuerySet(reversed(self.items)) - return self - - def __getitem__(self, sliced): - return self.items[sliced] - - self.pagination = pagination.CursorPagination() + self.pagination = ExamplePagination() self.queryset = MockQuerySet([ MockObject(idx) for idx in [ 1, 1, 1, 1, 1, - 1, 1, 1, 1, 1, - 1, 1, 2, 3, 4, - 5, 6, 7, 8, 9 + 1, 2, 3, 4, 4, + 4, 4, 5, 6, 7, + 7, 7, 7, 7, 7, + 7, 7, 7, 8, 9, + 9, 9, 9, 9, 9 ] ]) - def paginate_queryset(self, request): - return list(self.pagination.paginate_queryset(self.queryset, request)) + def get_pages(self, url): + """ + Given a URL return a tuple of: - def test_following_cursor_identical_items(self): - request = Request(factory.get('/')) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + (previous page, current page, next page, previous url, next url) + """ + request = Request(factory.get(url)) + queryset = self.pagination.paginate_queryset(self.queryset, request) + current = [item.created for item in queryset] next_url = self.pagination.get_next_link() - assert next_url - - request = Request(factory.get(next_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 1, 1, 1] - - next_url = self.pagination.get_next_link() - assert next_url - - request = Request(factory.get(next_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 2, 3, 4] - - next_url = self.pagination.get_next_link() - assert next_url - - request = Request(factory.get(next_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [5, 6, 7, 8, 9] - - next_url = self.pagination.get_next_link() - assert next_url is None - - # Now page back again - previous_url = self.pagination.get_previous_link() - assert previous_url - request = Request(factory.get(previous_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 2, 3, 4] + if next_url is not None: + request = Request(factory.get(next_url)) + queryset = self.pagination.paginate_queryset(self.queryset, request) + next = [item.created for item in queryset] + else: + next = None - previous_url = self.pagination.get_previous_link() - assert previous_url + if previous_url is not None: + request = Request(factory.get(previous_url)) + queryset = self.pagination.paginate_queryset(self.queryset, request) + previous = [item.created for item in queryset] + else: + previous = None - request = Request(factory.get(previous_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + return (previous, current, next, previous_url, next_url) - previous_url = self.pagination.get_previous_link() - assert previous_url + def test_invalid_cursor(self): + request = Request(factory.get('/', {'cursor': '123'})) + with pytest.raises(exceptions.NotFound): + self.pagination.paginate_queryset(self.queryset, request) - request = Request(factory.get(previous_url)) - queryset = self.paginate_queryset(request) - assert [item.created for item in queryset] == [1, 1, 1, 1, 1] + def test_cursor_pagination(self): + (previous, current, next, previous_url, next_url) = self.get_pages('/') - previous_url = self.pagination.get_previous_link() - assert previous_url is None + assert previous is None + assert current == [1, 1, 1, 1, 1] + assert next == [1, 2, 3, 4, 4] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [1, 1, 1, 1, 1] + assert current == [1, 2, 3, 4, 4] + assert next == [4, 4, 5, 6, 7] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [1, 2, 3, 4, 4] + assert current == [4, 4, 5, 6, 7] + assert next == [7, 7, 7, 7, 7] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [4, 4, 4, 5, 6] # Paging artifact + assert current == [7, 7, 7, 7, 7] + assert next == [7, 7, 7, 8, 9] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [7, 7, 7, 7, 7] + assert current == [7, 7, 7, 8, 9] + assert next == [9, 9, 9, 9, 9] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [7, 7, 7, 8, 9] + assert current == [9, 9, 9, 9, 9] + assert next is None + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [7, 7, 7, 7, 7] + assert current == [7, 7, 7, 8, 9] + assert next == [9, 9, 9, 9, 9] + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [4, 4, 5, 6, 7] + assert current == [7, 7, 7, 7, 7] + assert next == [8, 9, 9, 9, 9] # Paging artifact + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [1, 2, 3, 4, 4] + assert current == [4, 4, 5, 6, 7] + assert next == [7, 7, 7, 7, 7] + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [1, 1, 1, 1, 1] + assert current == [1, 2, 3, 4, 4] + assert next == [4, 4, 5, 6, 7] + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous is None + assert current == [1, 1, 1, 1, 1] + assert next == [1, 2, 3, 4, 4] def test_get_displayed_page_numbers(): From ca372ef6ef1cf95eb9282a484782e1a3721cb72b Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 13:50:51 +0000 Subject: [PATCH 07/12] Fix for python 3 --- rest_framework/pagination.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 618352391..0c5abccb7 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -389,7 +389,7 @@ Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) def decode_cursor(encoded): try: - tokens = urlparse.parse_qs(b64decode(encoded), keep_blank_values=True) + tokens = urlparse.parse_qs(b64decode(encoded).decode('ascii'), keep_blank_values=True) offset = int(tokens['offset'][0]) reverse = bool(int(tokens['reverse'][0])) position = tokens['position'][0] @@ -405,7 +405,7 @@ def encode_cursor(cursor): 'reverse': '1' if cursor.reverse else '0', 'position': cursor.position } - return b64encode(urlparse.urlencode(tokens, doseq=True)) + return b64encode(urlparse.urlencode(tokens, doseq=True).encode('ascii')) class CursorPagination(BasePagination): From 38a2ed6f62adcdcb2eba94f6133d4dd976a53af1 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 14:04:25 +0000 Subject: [PATCH 08/12] Python 3 fixes for cursor pagination --- rest_framework/pagination.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 0c5abccb7..cf1f1afa1 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -389,7 +389,8 @@ Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) def decode_cursor(encoded): try: - tokens = urlparse.parse_qs(b64decode(encoded).decode('ascii'), keep_blank_values=True) + querystring = b64decode(encoded.encode('ascii')).decode('ascii') + tokens = urlparse.parse_qs(querystring, keep_blank_values=True) offset = int(tokens['offset'][0]) reverse = bool(int(tokens['reverse'][0])) position = tokens['position'][0] @@ -405,7 +406,8 @@ def encode_cursor(cursor): 'reverse': '1' if cursor.reverse else '0', 'position': cursor.position } - return b64encode(urlparse.urlencode(tokens, doseq=True).encode('ascii')) + querystring = urlparse.urlencode(tokens, doseq=True) + return b64encode(querystring.encode('ascii')).decode('ascii') class CursorPagination(BasePagination): From 83a82b44a56a303d43a16dd675fae116e51b9d85 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 15:07:01 +0000 Subject: [PATCH 09/12] Support for tuple ordering in cursor pagination --- rest_framework/pagination.py | 113 +++++++++++++++++++++-------------- tests/test_pagination.py | 2 +- 2 files changed, 68 insertions(+), 47 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index cf1f1afa1..58223f49b 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -20,12 +20,12 @@ from rest_framework.utils.urls import ( ) -def _strict_positive_int(integer_string, cutoff=None): +def _positive_int(integer_string, strict=False, cutoff=None): """ Cast a string to a strictly positive integer. """ ret = int(integer_string) - if ret <= 0: + if ret < 0 or (ret == 0 and strict): raise ValueError() if cutoff: ret = min(ret, cutoff) @@ -126,6 +126,47 @@ def _get_page_links(page_numbers, current, url_func): return page_links +def _decode_cursor(encoded): + """ + Given a string representing an encoded cursor, return a `Cursor` instance. + """ + try: + querystring = b64decode(encoded.encode('ascii')).decode('ascii') + tokens = urlparse.parse_qs(querystring, keep_blank_values=True) + offset = _positive_int(tokens['offset'][0]) + reverse = bool(int(tokens['reverse'][0])) + position = tokens.get('position', [None])[0] + except (TypeError, ValueError): + return None + + return Cursor(offset=offset, reverse=reverse, position=position) + + +def _encode_cursor(cursor): + """ + Given a Cursor instance, return an encoded string representation. + """ + tokens = { + 'offset': str(cursor.offset), + 'reverse': '1' if cursor.reverse else '0', + } + if cursor.position is not None: + tokens['position'] = cursor.position + + querystring = urlparse.urlencode(tokens, doseq=True) + return b64encode(querystring.encode('ascii')).decode('ascii') + + +def _reverse_ordering(ordering_tuple): + """ + Given an order_by tuple such as `('-created', 'uuid')` reverse the + ordering and return a new tuple, eg. `('created', '-uuid')`. + """ + invert = lambda x: x[1:] if (x.startswith('-')) else '-' + x + return tuple([invert(item) for item in ordering_tuple]) + + +Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) PageLink = namedtuple('PageLink', ['url', 'number', 'is_active', 'is_break']) PAGE_BREAK = PageLink(url=None, number=None, is_active=False, is_break=True) @@ -228,8 +269,9 @@ class PageNumberPagination(BasePagination): def get_page_size(self, request): if self.paginate_by_param: try: - return _strict_positive_int( + return _positive_int( request.query_params[self.paginate_by_param], + strict=True, cutoff=self.max_paginate_by ) except (KeyError, ValueError): @@ -312,7 +354,7 @@ class LimitOffsetPagination(BasePagination): def get_limit(self, request): if self.limit_query_param: try: - return _strict_positive_int( + return _positive_int( request.query_params[self.limit_query_param], cutoff=self.max_limit ) @@ -323,7 +365,7 @@ class LimitOffsetPagination(BasePagination): def get_offset(self, request): try: - return _strict_positive_int( + return _positive_int( request.query_params[self.offset_query_param], ) except (KeyError, ValueError): @@ -384,36 +426,10 @@ class LimitOffsetPagination(BasePagination): return template.render(context) -Cursor = namedtuple('Cursor', ['offset', 'reverse', 'position']) - - -def decode_cursor(encoded): - try: - querystring = b64decode(encoded.encode('ascii')).decode('ascii') - tokens = urlparse.parse_qs(querystring, keep_blank_values=True) - offset = int(tokens['offset'][0]) - reverse = bool(int(tokens['reverse'][0])) - position = tokens['position'][0] - except (TypeError, ValueError): - return None - - return Cursor(offset=offset, reverse=reverse, position=position) - - -def encode_cursor(cursor): - tokens = { - 'offset': str(cursor.offset), - 'reverse': '1' if cursor.reverse else '0', - 'position': cursor.position - } - querystring = urlparse.urlencode(tokens, doseq=True) - return b64encode(querystring.encode('ascii')).decode('ascii') - - class CursorPagination(BasePagination): - # TODO: handle queries with '' as a legitimate position # Support case where ordering is already negative # Support tuple orderings + # Determine how/if True, False and None positions work cursor_query_param = 'cursor' page_size = api_settings.PAGINATE_BY invalid_cursor_message = _('Invalid cursor') @@ -426,25 +442,26 @@ class CursorPagination(BasePagination): encoded = request.query_params.get(self.cursor_query_param) if encoded is None: self.cursor = None - (offset, reverse, current_position) = (0, False, '') + (offset, reverse, current_position) = (0, False, None) else: - self.cursor = decode_cursor(encoded) + self.cursor = _decode_cursor(encoded) if self.cursor is None: raise NotFound(self.invalid_cursor_message) (offset, reverse, current_position) = self.cursor # Cursor pagination always enforces an ordering. if reverse: - queryset = queryset.order_by('-' + self.ordering) + queryset = queryset.order_by(_reverse_ordering(self.ordering)) else: queryset = queryset.order_by(self.ordering) # If we have a cursor with a fixed position then filter by that. - if current_position != '': + if current_position is not None: + primary_ordering_attr = self.ordering[0].lstrip('-') if self.cursor.reverse: - kwargs = {self.ordering + '__lt': current_position} + kwargs = {primary_ordering_attr + '__lt': current_position} else: - kwargs = {self.ordering + '__gt': current_position} + kwargs = {primary_ordering_attr + '__gt': current_position} queryset = queryset.filter(**kwargs) # If we have an offset cursor then offset the entire page by that amount. @@ -468,7 +485,7 @@ class CursorPagination(BasePagination): if reverse: # Determine next and previous positions for reverse cursors. - self.has_next = current_position != '' or offset > 0 + self.has_next = (current_position is not None) or (offset > 0) self.has_previous = has_following_postion if self.has_next: self.next_position = current_position @@ -477,7 +494,7 @@ class CursorPagination(BasePagination): else: # Determine next and previous positions for forward cursors. self.has_next = has_following_postion - self.has_previous = current_position != '' or offset > 0 + self.has_previous = (current_position is not None) or (offset > 0) if self.has_next: self.next_position = following_position if self.has_previous: @@ -518,7 +535,7 @@ class CursorPagination(BasePagination): # Our cursor will have an offset equal to the page size, # but no position to filter against yet. offset = self.page_size - position = '' + position = None elif self.cursor.reverse: # The change in direction will introduce a paging artifact, # where we end up skipping forward a few extra items. @@ -531,7 +548,7 @@ class CursorPagination(BasePagination): position = self.previous_position cursor = Cursor(offset=offset, reverse=False, position=position) - encoded = encode_cursor(cursor) + encoded = _encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) def get_previous_link(self): @@ -567,7 +584,7 @@ class CursorPagination(BasePagination): # Our cursor will have an offset equal to the page size, # but no position to filter against yet. offset = self.page_size - position = '' + position = None elif self.cursor.reverse: # Use the position from the existing cursor and increment # it's offset by the page size. @@ -580,11 +597,15 @@ class CursorPagination(BasePagination): position = self.next_position cursor = Cursor(offset=offset, reverse=True, position=position) - encoded = encode_cursor(cursor) + encoded = _encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) def get_ordering(self): - return 'created' + """ + Return a tuple of strings, that may be used in an `order_by` method. + """ + return ('created',) def _get_position_from_instance(self, instance, ordering): - return str(getattr(instance, ordering)) + attr = getattr(instance, ordering[0]) + return six.text_type(attr) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index e32dd0288..fffdcbe92 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -450,7 +450,7 @@ class TestCursorPagination: ]) def order_by(self, ordering): - if ordering.startswith('-'): + if ordering[0].startswith('-'): return MockQuerySet(list(reversed(self.items))) return self From 408261ee02b176732b7f840f7042e7c24f3ecd27 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 15:15:52 +0000 Subject: [PATCH 10/12] Support ordering attribute either on view or on pagination class for CursorPagination --- rest_framework/pagination.py | 24 +++++++++++++++++++----- tests/test_pagination.py | 1 + 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 58223f49b..7b28b47f0 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -427,16 +427,16 @@ class LimitOffsetPagination(BasePagination): class CursorPagination(BasePagination): - # Support case where ordering is already negative - # Support tuple orderings + # Support usage with OrderingFilter # Determine how/if True, False and None positions work cursor_query_param = 'cursor' page_size = api_settings.PAGINATE_BY invalid_cursor_message = _('Invalid cursor') + ordering = None def paginate_queryset(self, queryset, request, view=None): self.base_url = request.build_absolute_uri() - self.ordering = self.get_ordering() + self.ordering = self.get_ordering(view) # Determine if we have a cursor, and if so then decode it. encoded = request.query_params.get(self.cursor_query_param) @@ -600,11 +600,25 @@ class CursorPagination(BasePagination): encoded = _encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) - def get_ordering(self): + def get_ordering(self, view): """ Return a tuple of strings, that may be used in an `order_by` method. """ - return ('created',) + ordering = getattr(view, 'ordering', getattr(self, 'ordering', None)) + + assert ordering is not None, ( + 'Using cursor pagination, but no ordering attribute was declared ' + 'on the view or on the pagination class.' + ) + assert isinstance(ordering, (six.string_types, list, tuple)), ( + 'Invalid ordering. Expected string or tuple, but got {type}'.format( + type=type(ordering).__name__ + ) + ) + + if isinstance(ordering, six.string_types): + return (ordering,) + return ordering def _get_position_from_instance(self, instance, ordering): attr = getattr(instance, ordering[0]) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index fffdcbe92..c05b4abab 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -459,6 +459,7 @@ class TestCursorPagination: class ExamplePagination(pagination.CursorPagination): page_size = 5 + ordering = 'created' self.pagination = ExamplePagination() self.queryset = MockQuerySet([ From 0822c9e55820f8e4737329e38abc2e21718af9e5 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 16:12:05 +0000 Subject: [PATCH 11/12] Cursor pagination now works with OrderingFilter --- rest_framework/filters.py | 24 ++++++++++----------- rest_framework/pagination.py | 41 +++++++++++++++++++++++++++--------- tests/test_pagination.py | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 23 deletions(-) diff --git a/rest_framework/filters.py b/rest_framework/filters.py index d188a2d1e..2bcf36991 100644 --- a/rest_framework/filters.py +++ b/rest_framework/filters.py @@ -114,7 +114,7 @@ class OrderingFilter(BaseFilterBackend): ordering_param = api_settings.ORDERING_PARAM ordering_fields = None - def get_ordering(self, request): + def get_ordering(self, request, queryset, view): """ Ordering is set by a comma delimited ?ordering=... query parameter. @@ -124,7 +124,13 @@ class OrderingFilter(BaseFilterBackend): """ params = request.query_params.get(self.ordering_param) if params: - return [param.strip() for param in params.split(',')] + fields = [param.strip() for param in params.split(',')] + ordering = self.remove_invalid_fields(queryset, fields, view) + if ordering: + return ordering + + # No ordering was included, or all the ordering fields were invalid + return self.get_default_ordering(view) def get_default_ordering(self, view): ordering = getattr(view, 'ordering', None) @@ -132,7 +138,7 @@ class OrderingFilter(BaseFilterBackend): return (ordering,) return ordering - def remove_invalid_fields(self, queryset, ordering, view): + def remove_invalid_fields(self, queryset, fields, view): valid_fields = getattr(view, 'ordering_fields', self.ordering_fields) if valid_fields is None: @@ -152,18 +158,10 @@ class OrderingFilter(BaseFilterBackend): valid_fields = [field.name for field in queryset.model._meta.fields] valid_fields += queryset.query.aggregates.keys() - return [term for term in ordering if term.lstrip('-') in valid_fields] + return [term for term in fields if term.lstrip('-') in valid_fields] def filter_queryset(self, request, queryset, view): - ordering = self.get_ordering(request) - - if ordering: - # Skip any incorrect parameters - ordering = self.remove_invalid_fields(queryset, ordering, view) - - if not ordering: - # Use 'ordering' attribute by default - ordering = self.get_default_ordering(view) + ordering = self.get_ordering(request, queryset, view) if ordering: return queryset.order_by(*ordering) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 7b28b47f0..1b4174bc6 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -427,8 +427,9 @@ class LimitOffsetPagination(BasePagination): class CursorPagination(BasePagination): - # Support usage with OrderingFilter - # Determine how/if True, False and None positions work + # Determine how/if True, False and None positions work - do the string + # encodings work with Django queryset filters? + # Consider a max offset cap. cursor_query_param = 'cursor' page_size = api_settings.PAGINATE_BY invalid_cursor_message = _('Invalid cursor') @@ -436,7 +437,7 @@ class CursorPagination(BasePagination): def paginate_queryset(self, queryset, request, view=None): self.base_url = request.build_absolute_uri() - self.ordering = self.get_ordering(view) + self.ordering = self.get_ordering(request, queryset, view) # Determine if we have a cursor, and if so then decode it. encoded = request.query_params.get(self.cursor_query_param) @@ -600,16 +601,36 @@ class CursorPagination(BasePagination): encoded = _encode_cursor(cursor) return replace_query_param(self.base_url, self.cursor_query_param, encoded) - def get_ordering(self, view): + def get_ordering(self, request, queryset, view): """ Return a tuple of strings, that may be used in an `order_by` method. """ - ordering = getattr(view, 'ordering', getattr(self, 'ordering', None)) + ordering_filters = [ + filter_cls for filter_cls in getattr(view, 'filter_backends', []) + if hasattr(filter_cls, 'get_ordering') + ] + + if ordering_filters: + # If a filter exists on the view that implements `get_ordering` + # then we defer to that filter to determine the ordering. + filter_cls = ordering_filters[0] + filter_instance = filter_cls() + ordering = filter_instance.get_ordering(request, queryset, view) + assert ordering is not None, ( + 'Using cursor pagination, but filter class {filter_cls} ' + 'returned a `None` ordering.'.format( + filter_cls=filter_cls.__name__ + ) + ) + else: + # The default case is to check for an `ordering` attribute, + # first on the view instance, and then on this pagination instance. + ordering = getattr(view, 'ordering', getattr(self, 'ordering', None)) + assert ordering is not None, ( + 'Using cursor pagination, but no ordering attribute was declared ' + 'on the view or on the pagination class.' + ) - assert ordering is not None, ( - 'Using cursor pagination, but no ordering attribute was declared ' - 'on the view or on the pagination class.' - ) assert isinstance(ordering, (six.string_types, list, tuple)), ( 'Invalid ordering. Expected string or tuple, but got {type}'.format( type=type(ordering).__name__ @@ -618,7 +639,7 @@ class CursorPagination(BasePagination): if isinstance(ordering, six.string_types): return (ordering,) - return ordering + return tuple(ordering) def _get_position_from_instance(self, instance, ordering): attr = getattr(instance, ordering[0]) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index c05b4abab..338be610c 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -77,6 +77,20 @@ class TestPaginationIntegration: 'count': 50 } + def test_setting_page_size_to_zero(self): + """ + When page_size parameter is invalid it should return to the default. + """ + request = factory.get('/', {'page_size': 0}) + response = self.view(request) + assert response.status_code == status.HTTP_200_OK + assert response.data == { + 'results': [2, 4, 6, 8, 10], + 'previous': None, + 'next': 'http://testserver/?page=2&page_size=0', + 'count': 50 + } + def test_additional_query_params_are_preserved(self): request = factory.get('/', {'page': 2, 'filter': 'even'}) response = self.view(request) @@ -88,6 +102,14 @@ class TestPaginationIntegration: 'count': 50 } + def test_404_not_found_for_zero_page(self): + request = factory.get('/', {'page': '0'}) + response = self.view(request) + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.data == { + 'detail': 'Invalid page "0": That page number is less than 1.' + } + def test_404_not_found_for_invalid_page(self): request = factory.get('/', {'page': 'invalid'}) response = self.view(request) @@ -507,6 +529,24 @@ class TestCursorPagination: with pytest.raises(exceptions.NotFound): self.pagination.paginate_queryset(self.queryset, request) + def test_use_with_ordering_filter(self): + class MockView: + filter_backends = (filters.OrderingFilter,) + ordering_fields = ['username', 'created'] + ordering = 'created' + + request = Request(factory.get('/', {'ordering': 'username'})) + ordering = self.pagination.get_ordering(request, [], MockView()) + assert ordering == ('username',) + + request = Request(factory.get('/', {'ordering': '-username'})) + ordering = self.pagination.get_ordering(request, [], MockView()) + assert ordering == ('-username',) + + request = Request(factory.get('/', {'ordering': 'invalid'})) + ordering = self.pagination.get_ordering(request, [], MockView()) + assert ordering == ('created',) + def test_cursor_pagination(self): (previous, current, next, previous_url, next_url) = self.get_pages('/') From 43d983fae82ab23ca94f52deb29e938eb2a40e88 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Thu, 22 Jan 2015 17:25:12 +0000 Subject: [PATCH 12/12] Add paging controls --- rest_framework/pagination.py | 66 ++++++++++++++----- .../rest_framework/css/bootstrap-tweaks.css | 12 +++- .../pagination/previous_and_next.html | 12 ++++ tests/test_pagination.py | 5 +- 4 files changed, 78 insertions(+), 17 deletions(-) create mode 100644 rest_framework/templates/rest_framework/pagination/previous_and_next.html diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 1b4174bc6..b3658acad 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -133,9 +133,14 @@ def _decode_cursor(encoded): try: querystring = b64decode(encoded.encode('ascii')).decode('ascii') tokens = urlparse.parse_qs(querystring, keep_blank_values=True) - offset = _positive_int(tokens['offset'][0]) - reverse = bool(int(tokens['reverse'][0])) - position = tokens.get('position', [None])[0] + + offset = tokens.get('o', ['0'])[0] + offset = _positive_int(offset) + + reverse = tokens.get('r', ['0'])[0] + reverse = bool(int(reverse)) + + position = tokens.get('p', [None])[0] except (TypeError, ValueError): return None @@ -146,12 +151,13 @@ def _encode_cursor(cursor): """ Given a Cursor instance, return an encoded string representation. """ - tokens = { - 'offset': str(cursor.offset), - 'reverse': '1' if cursor.reverse else '0', - } + tokens = {} + if cursor.offset != 0: + tokens['o'] = str(cursor.offset) + if cursor.reverse: + tokens['r'] = '1' if cursor.position is not None: - tokens['position'] = cursor.position + tokens['p'] = cursor.position querystring = urlparse.urlencode(tokens, doseq=True) return b64encode(querystring.encode('ascii')).decode('ascii') @@ -430,10 +436,12 @@ class CursorPagination(BasePagination): # Determine how/if True, False and None positions work - do the string # encodings work with Django queryset filters? # Consider a max offset cap. + # Tidy up the `get_ordering` API (eg remove queryset from it) cursor_query_param = 'cursor' page_size = api_settings.PAGINATE_BY invalid_cursor_message = _('Invalid cursor') ordering = None + template = 'rest_framework/pagination/previous_and_next.html' def paginate_queryset(self, queryset, request, view=None): self.base_url = request.build_absolute_uri() @@ -452,17 +460,22 @@ class CursorPagination(BasePagination): # Cursor pagination always enforces an ordering. if reverse: - queryset = queryset.order_by(_reverse_ordering(self.ordering)) + queryset = queryset.order_by(*_reverse_ordering(self.ordering)) else: - queryset = queryset.order_by(self.ordering) + queryset = queryset.order_by(*self.ordering) # If we have a cursor with a fixed position then filter by that. if current_position is not None: - primary_ordering_attr = self.ordering[0].lstrip('-') - if self.cursor.reverse: - kwargs = {primary_ordering_attr + '__lt': current_position} + order = self.ordering[0] + is_reversed = order.startswith('-') + order_attr = order.lstrip('-') + + # Test for: (cursor reversed) XOR (queryset reversed) + if self.cursor.reverse != is_reversed: + kwargs = {order_attr + '__lt': current_position} else: - kwargs = {primary_ordering_attr + '__gt': current_position} + kwargs = {order_attr + '__gt': current_position} + queryset = queryset.filter(**kwargs) # If we have an offset cursor then offset the entire page by that amount. @@ -501,6 +514,11 @@ class CursorPagination(BasePagination): if self.has_previous: self.previous_position = current_position + # Display page controls in the browsable API if there is more + # than one page. + if self.has_previous or self.has_next: + self.display_page_controls = True + return self.page def get_next_link(self): @@ -642,5 +660,23 @@ class CursorPagination(BasePagination): return tuple(ordering) def _get_position_from_instance(self, instance, ordering): - attr = getattr(instance, ordering[0]) + attr = getattr(instance, ordering[0].lstrip('-')) return six.text_type(attr) + + def get_paginated_response(self, data): + return Response(OrderedDict([ + ('next', self.get_next_link()), + ('previous', self.get_previous_link()), + ('results', data) + ])) + + def get_html_context(self): + return { + 'previous_url': self.get_previous_link(), + 'next_url': self.get_next_link() + } + + def to_html(self): + template = loader.get_template(self.template) + context = Context(self.get_html_context()) + return template.render(context) diff --git a/rest_framework/static/rest_framework/css/bootstrap-tweaks.css b/rest_framework/static/rest_framework/css/bootstrap-tweaks.css index 15b42178f..04f12ed3d 100644 --- a/rest_framework/static/rest_framework/css/bootstrap-tweaks.css +++ b/rest_framework/static/rest_framework/css/bootstrap-tweaks.css @@ -63,10 +63,20 @@ a single block in the template. .pagination>.disabled>a, .pagination>.disabled>a:hover, .pagination>.disabled>a:focus { - cursor: default; + cursor: not-allowed; pointer-events: none; } +.pager>.disabled>a, +.pager>.disabled>a:hover, +.pager>.disabled>a:focus { + pointer-events: none; +} + +.pager .next { + margin-left: 10px; +} + /*=== dabapps bootstrap styles ====*/ html { diff --git a/rest_framework/templates/rest_framework/pagination/previous_and_next.html b/rest_framework/templates/rest_framework/pagination/previous_and_next.html new file mode 100644 index 000000000..eacbfff45 --- /dev/null +++ b/rest_framework/templates/rest_framework/pagination/previous_and_next.html @@ -0,0 +1,12 @@ + diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 338be610c..13bfb6272 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -1,3 +1,4 @@ +# coding: utf-8 from __future__ import unicode_literals from rest_framework import exceptions, generics, pagination, serializers, status, filters from rest_framework.request import Request @@ -471,7 +472,7 @@ class TestCursorPagination: if item.created < int(created__lt) ]) - def order_by(self, ordering): + def order_by(self, *ordering): if ordering[0].startswith('-'): return MockQuerySet(list(reversed(self.items))) return self @@ -614,6 +615,8 @@ class TestCursorPagination: assert current == [1, 1, 1, 1, 1] assert next == [1, 2, 3, 4, 4] + assert isinstance(self.pagination.to_html(), type('')) + def test_get_displayed_page_numbers(): """