From 0f035927118182a9e243194d7c1eb59b4e6e7e4a Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Mon, 20 Mar 2023 08:27:09 +0100 Subject: [PATCH 1/8] Ensure CursorPagination respects nulls in the ordering field --- rest_framework/pagination.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index f5c5b913b5..87926b3816 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -10,6 +10,7 @@ from django.core.paginator import InvalidPage from django.core.paginator import Paginator as DjangoPaginator +from django.db.models import Q from django.template import loader from django.utils.encoding import force_str from django.utils.translation import gettext_lazy as _ @@ -631,7 +632,10 @@ def paginate_queryset(self, queryset, request, view=None): else: kwargs = {order_attr + '__gt': current_position} - queryset = queryset.filter(**kwargs) + # If some records contain a null for the ordering field, don't lose them. + filter_query = Q(**kwargs) | Q(**{order_attr + '__isnull': True}) + + queryset = queryset.filter(filter_query) # 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 From df5d07ac534ea8a96f6642024b84f95e8b292b68 Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Mon, 20 Mar 2023 08:36:06 +0100 Subject: [PATCH 2/8] Lint --- rest_framework/pagination.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 87926b3816..5106008f45 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -634,7 +634,7 @@ def paginate_queryset(self, queryset, request, view=None): # If some records contain a null for the ordering field, don't lose them. filter_query = Q(**kwargs) | Q(**{order_attr + '__isnull': True}) - + queryset = queryset.filter(filter_query) # If we have an offset cursor then offset the entire page by that amount. From e44d8de067d5e0e8a3de44d949ade72e2be4f20e Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Wed, 22 Mar 2023 14:52:43 +0100 Subject: [PATCH 3/8] Fix pagination tests --- rest_framework/pagination.py | 2 +- tests/test_pagination.py | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 5106008f45..ff2f3684f7 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -621,7 +621,7 @@ def paginate_queryset(self, queryset, request, view=None): 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: + if str(current_position) != 'None': order = self.ordering[0] is_reversed = order.startswith('-') order_attr = order.lstrip('-') diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 2812c4489e..82c6f5a3ac 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -951,17 +951,21 @@ class MockQuerySet: def __init__(self, items): self.items = items - def filter(self, created__gt=None, created__lt=None): + def filter(self, q): + q_args = dict(q.deconstruct()[1]) + created__gt = q_args.get('created__gt') + created__lt = q_args.get('created__lt') + if created__gt is not None: return MockQuerySet([ item for item in self.items - if item.created > int(created__gt) + if item.created is None or 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) + if item.created is None or item.created < int(created__lt) ]) def order_by(self, *ordering): From a1866b329ee3cf9b34b28b74aec72b98a8056c13 Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Thu, 23 Mar 2023 11:58:52 +0100 Subject: [PATCH 4/8] Add test_ascending with nulls --- rest_framework/pagination.py | 11 +++--- tests/test_pagination.py | 72 ++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index ff2f3684f7..2555d08402 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -633,8 +633,9 @@ def paginate_queryset(self, queryset, request, view=None): kwargs = {order_attr + '__gt': current_position} # If some records contain a null for the ordering field, don't lose them. - filter_query = Q(**kwargs) | Q(**{order_attr + '__isnull': True}) - + filter_query = Q(**kwargs) + if reverse: + filter_query |= Q(**{order_attr + '__isnull': True}) queryset = queryset.filter(filter_query) # If we have an offset cursor then offset the entire page by that amount. @@ -708,7 +709,7 @@ def get_next_link(self): # The item in this position and the item following it # have different positions. We can use this position as # our marker. - has_item_with_unique_position = True + has_item_with_unique_position = position is not None break # The item in this position has the same position as the item @@ -761,7 +762,7 @@ def get_previous_link(self): # The item in this position and the item following it # have different positions. We can use this position as # our marker. - has_item_with_unique_position = True + has_item_with_unique_position = position is not None break # The item in this position has the same position as the item @@ -887,7 +888,7 @@ def _get_position_from_instance(self, instance, ordering): attr = instance[field_name] else: attr = getattr(instance, field_name) - return str(attr) + return None if attr is None else str(attr) def get_paginated_response(self, data): return Response(OrderedDict([ diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 82c6f5a3ac..1a4667f266 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -1084,6 +1084,78 @@ def get_pages(self, url): return (previous, current, next, previous_url, next_url) +class NullableCursorPaginationModel(models.Model): + created = models.IntegerField(null=True) + + +class TestCursorPaginationWithNulls(TestCase): + """ + Unit tests for `pagination.CursorPagination` with ordering on a nullable field. + """ + + def setUp(self): + class ExamplePagination(pagination.CursorPagination): + page_size = 1 + ordering = 'created' + + self.pagination = ExamplePagination() + data = [ + None, None, 3, 4 + ] + for idx in data: + NullableCursorPaginationModel.objects.create(created=idx) + + self.queryset = NullableCursorPaginationModel.objects.all() + + get_pages = TestCursorPagination.get_pages + + def test_ascending(self): + (previous, current, next, previous_url, next_url) = self.get_pages('/') + + assert previous is None + assert current == [None] + assert next == [None] + assert previous_url is None + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [None] + assert current == [None] + assert next == [3] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [3] # [None] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L789 + assert current == [3] + assert next == [4] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [3] + assert current == [4] + assert next is None + assert next_url is None + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [None] + assert current == [3] + assert next == [4] + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [None] + assert current == [None] + assert next == [None] # [3] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L731 + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous is None + assert current == [None] + assert next == [None] + assert previous_url is None + + def test_get_displayed_page_numbers(): """ Test our contextual page display function. From b089edcd0683adb2cb54b02d0c8e4aa36d269636 Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Thu, 23 Mar 2023 14:11:55 +0100 Subject: [PATCH 5/8] Push tests for nulls --- tests/test_pagination.py | 53 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 1a4667f266..fbe064b354 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -1110,12 +1110,12 @@ class ExamplePagination(pagination.CursorPagination): get_pages = TestCursorPagination.get_pages def test_ascending(self): + """Test paginating one row at a time, current should go 1, 2, 3, 4, 3, 2, 1.""" (previous, current, next, previous_url, next_url) = self.get_pages('/') assert previous is None assert current == [None] assert next == [None] - assert previous_url is None (previous, current, next, previous_url, next_url) = self.get_pages(next_url) @@ -1153,7 +1153,56 @@ def test_ascending(self): assert previous is None assert current == [None] assert next == [None] - assert previous_url is None + + def test_decending(self): + """Test paginating one row at a time, current should go 4, 3, 2, 1, 2, 3, 4.""" + self.pagination.ordering = ('-created',) + (previous, current, next, previous_url, next_url) = self.get_pages('/') + + assert previous is None + assert current == [4] + assert next == [3] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [None] # [4] paging artifact + assert current == [3] + assert next == [None] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [None] # [3] paging artifact + assert current == [None] + assert next == [None] + + (previous, current, next, previous_url, next_url) = self.get_pages(next_url) + + assert previous == [None] + assert current == [None] + assert next is None + assert next_url is None + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [3] + assert current == [None] + assert next == [None] + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous == [None] + assert current == [3] + assert next == [3] # [4] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L731 + + # skip back artifact + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + (previous, current, next, previous_url, next_url) = self.get_pages(previous_url) + + assert previous is None + assert current == [4] + assert next == [3] def test_get_displayed_page_numbers(): From 0dd0873b4182ca448c30b0d3e8ea7614c3cd3dca Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Fri, 24 Mar 2023 10:32:02 +0100 Subject: [PATCH 6/8] Test pass --- rest_framework/pagination.py | 4 ++-- tests/test_pagination.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 2555d08402..50af70493c 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -632,9 +632,9 @@ def paginate_queryset(self, queryset, request, view=None): else: kwargs = {order_attr + '__gt': current_position} - # If some records contain a null for the ordering field, don't lose them. filter_query = Q(**kwargs) - if reverse: + # If some records contain a null for the ordering field, don't lose them. + if (reverse and not is_reversed) or is_reversed: filter_query |= Q(**{order_attr + '__isnull': True}) queryset = queryset.filter(filter_query) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index fbe064b354..b7633537ea 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -1154,7 +1154,7 @@ def test_ascending(self): assert current == [None] assert next == [None] - def test_decending(self): + def test_descending(self): """Test paginating one row at a time, current should go 4, 3, 2, 1, 2, 3, 4.""" self.pagination.ordering = ('-created',) (previous, current, next, previous_url, next_url) = self.get_pages('/') From c535420525eacead922815eb080b745c28543e8d Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Fri, 24 Mar 2023 10:49:30 +0100 Subject: [PATCH 7/8] Add comment --- rest_framework/pagination.py | 1 + 1 file changed, 1 insertion(+) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 50af70493c..34d71f828c 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -634,6 +634,7 @@ def paginate_queryset(self, queryset, request, view=None): filter_query = Q(**kwargs) # If some records contain a null for the ordering field, don't lose them. + # When reverse ordering, nulls will come last and need to be included. if (reverse and not is_reversed) or is_reversed: filter_query |= Q(**{order_attr + '__isnull': True}) queryset = queryset.filter(filter_query) From 8a16457a428a2a949aec0cc3b3afc5a89fb0b84f Mon Sep 17 00:00:00 2001 From: ddelange <14880945+ddelange@users.noreply.github.com> Date: Mon, 3 Apr 2023 09:29:14 +0200 Subject: [PATCH 8/8] Fix test for django30 --- tests/test_pagination.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index b7633537ea..8f9b20a0d4 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -953,6 +953,9 @@ def __init__(self, items): def filter(self, q): q_args = dict(q.deconstruct()[1]) + if not q_args: + # django 3.0.x artifact + q_args = dict(q.deconstruct()[2]) created__gt = q_args.get('created__gt') created__lt = q_args.get('created__lt')