From e16f3852b4e64a037cdbf143253ebc8e3678aff6 Mon Sep 17 00:00:00 2001 From: Stanislav Khlud Date: Fri, 15 Sep 2023 12:19:15 +0700 Subject: [PATCH] Fix ordering duplication on non-unique field Co-authored-by: Roman Gorbil --- rest_framework/filters.py | 40 +++++++++++++++++++++++++++++++++++++-- tests/test_pagination.py | 8 ++++---- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/rest_framework/filters.py b/rest_framework/filters.py index 065e72f94..97f0501cd 100644 --- a/rest_framework/filters.py +++ b/rest_framework/filters.py @@ -233,16 +233,52 @@ class OrderingFilter(BaseFilterBackend): The `ordering` query parameter can be overridden by setting the `ordering_param` value on the OrderingFilter or by specifying an `ORDERING_PARAM` value in the API settings. + + Always add "pk" as last parameter for ordering. + + For example, we have following QS + | pk | name | + |----|------| + | 1 | Bob | + | 2 | Dan | + | 3 | Bob | + | 4 | Joe | + + And we want to order it by name and take 2nd element. + We can get following results: + `qs.order_by('name')` + | pk | name | + |----|------| + | 1 | Bob | + | 3 | Bob | <- pk 3 is 2nd element + | 2 | Dan | + | 4 | Joe | + + Or we can get: + | pk | name | + |----|------| + | 3 | Bob | + | 1 | Bob | <- pk 1 is 2nd element + | 2 | Dan | + | 4 | Joe | + + As you see, QS is correctly ordered, but order is not consistent, and + pagination is also inconsistent since we are using fields which doesn't + have unique restriction. So we always add "pk" as last ordering param. + """ params = request.query_params.get(self.ordering_param) if params: fields = [param.strip() for param in params.split(',')] ordering = self.remove_invalid_fields(queryset, fields, view, request) if ordering: - return ordering + return ordering + ["pk"] # No ordering was included, or all the ordering fields were invalid - return self.get_default_ordering(view) + default_ordering = self.get_default_ordering(view) + if not default_ordering: + return default_ordering + return default_ordering + ("pk",) def get_default_ordering(self, view): ordering = getattr(view, 'ordering', None) diff --git a/tests/test_pagination.py b/tests/test_pagination.py index 090eb0d81..b79a54409 100644 --- a/tests/test_pagination.py +++ b/tests/test_pagination.py @@ -624,15 +624,15 @@ class CursorPaginationTestsMixin: request = Request(factory.get('/', {'ordering': 'username'})) ordering = self.pagination.get_ordering(request, [], MockView()) - assert ordering == ('username',) + assert ordering == ('username', 'pk') request = Request(factory.get('/', {'ordering': '-username'})) ordering = self.pagination.get_ordering(request, [], MockView()) - assert ordering == ('-username',) + assert ordering == ('-username', 'pk') request = Request(factory.get('/', {'ordering': 'invalid'})) ordering = self.pagination.get_ordering(request, [], MockView()) - assert ordering == ('created',) + assert ordering == ('created', 'pk') def test_use_with_ordering_filter_without_ordering_default_value(self): class MockView: @@ -646,7 +646,7 @@ class CursorPaginationTestsMixin: request = Request(factory.get('/', {'ordering': 'username'})) ordering = self.pagination.get_ordering(request, [], MockView()) - assert ordering == ('username',) + assert ordering == ('username', "pk") request = Request(factory.get('/', {'ordering': 'invalid'})) ordering = self.pagination.get_ordering(request, [], MockView())