From 7f18ec1b536a90b1fc194ef6140a6dcd8b605051 Mon Sep 17 00:00:00 2001 From: Max Muoto Date: Sat, 27 Apr 2024 06:07:05 -0500 Subject: [PATCH] Revert "Ensure CursorPagination respects nulls in the ordering field (#8912)" (#9381) This reverts commit b1cec517ff33d633d3ebcf5794a5f0f0583fabe6. --- rest_framework/pagination.py | 16 ++--- tests/test_pagination.py | 134 +---------------------------------- 2 files changed, 8 insertions(+), 142 deletions(-) diff --git a/rest_framework/pagination.py b/rest_framework/pagination.py index 2b20e76af..a543ceeb5 100644 --- a/rest_framework/pagination.py +++ b/rest_framework/pagination.py @@ -11,7 +11,6 @@ from urllib import parse 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 +630,7 @@ class CursorPagination(BasePagination): queryset = queryset.order_by(*self.ordering) # If we have a cursor with a fixed position then filter by that. - if str(current_position) != 'None': + if current_position is not None: order = self.ordering[0] is_reversed = order.startswith('-') order_attr = order.lstrip('-') @@ -642,12 +641,7 @@ class CursorPagination(BasePagination): else: kwargs = {order_attr + '__gt': current_position} - 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) + queryset = queryset.filter(**kwargs) # 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 @@ -720,7 +714,7 @@ class CursorPagination(BasePagination): # 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 = position is not None + has_item_with_unique_position = True break # The item in this position has the same position as the item @@ -773,7 +767,7 @@ class CursorPagination(BasePagination): # 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 = position is not None + has_item_with_unique_position = True break # The item in this position has the same position as the item @@ -896,7 +890,7 @@ class CursorPagination(BasePagination): attr = instance[field_name] else: attr = getattr(instance, field_name) - return None if attr is None else str(attr) + return str(attr) def get_paginated_response(self, data): return Response({ diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 090eb0d81..02d443ade 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -972,24 +972,17 @@ class TestCursorPagination(CursorPaginationTestsMixin): def __init__(self, items): self.items = 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') - + 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 is None or item.created > int(created__gt) + if item.created > int(created__gt) ]) assert created__lt is not None return MockQuerySet([ item for item in self.items - if item.created is None or item.created < int(created__lt) + if item.created < int(created__lt) ]) def order_by(self, *ordering): @@ -1108,127 +1101,6 @@ class TestCursorPaginationWithValueQueryset(CursorPaginationTestsMixin, TestCase 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): - """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] - - (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] - - 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('/') - - 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(): """ Test our contextual page display function.