From c736ce3179a8bb7bbcbf29324a08a62efa8747b6 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 29 Jul 2025 11:49:36 -0500 Subject: [PATCH] Fixes #18900: raise QuerySetNotOrdered exception when trying to paginate unordered API querysets (#19943) * Fixes #18900: introduce/raise QuerySetNotOrdered exception Defines a new exception, `QuerySetNotOrdered`, and raises it in `OptionalLimitOffsetPagination.paginate_queryset` in the right conditions: - the iterable to be paginated is a QuerySet isinstance - the `queryset.ordered` flag is not truthy * Don't try to reapply ordering if ordering is already present * Add ordering for failing tagged-objects list API endpoint I chose to implement this here for TaggedItemViewSet, rather than on the model, because any meaningful ordering is going to be done on the related Tag instance and I didn't want to introduce potential, not well understood side-effects by applying a model-wide ordering via a related model field. * Add default Token ordering behavior * Adds basic tests for raising QuerySetNotOrdered * Note why ordering is not applied in TaggedItem.Meta --- netbox/extras/api/views.py | 4 +- netbox/extras/models/tags.py | 3 ++ netbox/netbox/api/exceptions.py | 4 ++ netbox/netbox/api/pagination.py | 7 ++++ netbox/netbox/tests/test_api.py | 42 +++++++++++++++++++ .../0010_add_token_meta_ordering.py | 17 ++++++++ netbox/users/models/tokens.py | 1 + netbox/utilities/query.py | 3 ++ 8 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 netbox/users/migrations/0010_add_token_meta_ordering.py diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 3f5bb172a..facb1b17a 100644 --- a/netbox/extras/api/views.py +++ b/netbox/extras/api/views.py @@ -185,7 +185,9 @@ class TagViewSet(NetBoxModelViewSet): class TaggedItemViewSet(RetrieveModelMixin, ListModelMixin, BaseViewSet): - queryset = TaggedItem.objects.prefetch_related('content_type', 'content_object', 'tag') + queryset = TaggedItem.objects.prefetch_related( + 'content_type', 'content_object', 'tag' + ).order_by('tag__weight', 'tag__name') serializer_class = serializers.TaggedItemSerializer filterset_class = filtersets.TaggedItemFilterSet diff --git a/netbox/extras/models/tags.py b/netbox/extras/models/tags.py index b40327265..7d52d9eb6 100644 --- a/netbox/extras/models/tags.py +++ b/netbox/extras/models/tags.py @@ -83,3 +83,6 @@ class TaggedItem(GenericTaggedItemBase): indexes = [models.Index(fields=["content_type", "object_id"])] verbose_name = _('tagged item') verbose_name_plural = _('tagged items') + # Note: while there is no ordering applied here (because it would basically be done on fields + # of the related `tag`), there is an ordering applied to extras.api.views.TaggedItemViewSet + # to allow for proper pagination. diff --git a/netbox/netbox/api/exceptions.py b/netbox/netbox/api/exceptions.py index f552b06b5..c4d05d1a5 100644 --- a/netbox/netbox/api/exceptions.py +++ b/netbox/netbox/api/exceptions.py @@ -12,3 +12,7 @@ class SerializerNotFound(Exception): class GraphQLTypeNotFound(Exception): pass + + +class QuerySetNotOrdered(Exception): + pass diff --git a/netbox/netbox/api/pagination.py b/netbox/netbox/api/pagination.py index f1430a9fd..698e0a8dd 100644 --- a/netbox/netbox/api/pagination.py +++ b/netbox/netbox/api/pagination.py @@ -1,6 +1,7 @@ from django.db.models import QuerySet from rest_framework.pagination import LimitOffsetPagination +from netbox.api.exceptions import QuerySetNotOrdered from netbox.config import get_config @@ -15,6 +16,12 @@ class OptionalLimitOffsetPagination(LimitOffsetPagination): def paginate_queryset(self, queryset, request, view=None): + if isinstance(queryset, QuerySet) and not queryset.ordered: + raise QuerySetNotOrdered( + "Paginating over an unordered queryset is unreliable. Ensure that a minimal " + "ordering has been applied to the queryset for this API endpoint." + ) + if isinstance(queryset, QuerySet): self.count = self.get_queryset_count(queryset) else: diff --git a/netbox/netbox/tests/test_api.py b/netbox/netbox/tests/test_api.py index d087910b5..61bbcd4c6 100644 --- a/netbox/netbox/tests/test_api.py +++ b/netbox/netbox/tests/test_api.py @@ -1,8 +1,13 @@ import uuid +from django.test import RequestFactory, TestCase from django.urls import reverse +from rest_framework.request import Request +from netbox.api.exceptions import QuerySetNotOrdered +from netbox.api.pagination import OptionalLimitOffsetPagination from utilities.testing import APITestCase +from users.models import Token class AppTest(APITestCase): @@ -26,3 +31,40 @@ class AppTest(APITestCase): response = self.client.get(f'{url}?format=api', **self.header) self.assertEqual(response.status_code, 200) + + +class OptionalLimitOffsetPaginationTest(TestCase): + + def setUp(self): + self.paginator = OptionalLimitOffsetPagination() + self.factory = RequestFactory() + + def _make_drf_request(self, path='/', query_params=None): + """Helper to create a proper DRF Request object""" + return Request(self.factory.get(path, query_params or {})) + + def test_raises_exception_for_unordered_queryset(self): + """Should raise QuerySetNotOrdered for unordered QuerySet""" + queryset = Token.objects.all().order_by() + request = self._make_drf_request() + + with self.assertRaises(QuerySetNotOrdered) as cm: + self.paginator.paginate_queryset(queryset, request) + + error_msg = str(cm.exception) + self.assertIn("Paginating over an unordered queryset is unreliable", error_msg) + self.assertIn("Ensure that a minimal ordering has been applied", error_msg) + + def test_allows_ordered_queryset(self): + """Should not raise exception for ordered QuerySet""" + queryset = Token.objects.all().order_by('created') + request = self._make_drf_request() + + self.paginator.paginate_queryset(queryset, request) # Should not raise exception + + def test_allows_non_queryset_iterables(self): + """Should not raise exception for non-QuerySet iterables""" + iterable = [1, 2, 3, 4, 5] + request = self._make_drf_request() + + self.paginator.paginate_queryset(iterable, request) # Should not raise exception diff --git a/netbox/users/migrations/0010_add_token_meta_ordering.py b/netbox/users/migrations/0010_add_token_meta_ordering.py new file mode 100644 index 000000000..bb2be6c45 --- /dev/null +++ b/netbox/users/migrations/0010_add_token_meta_ordering.py @@ -0,0 +1,17 @@ +# Generated by Django 5.2.4 on 2025-07-23 17:28 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0009_update_group_perms'), + ] + + operations = [ + migrations.AlterModelOptions( + name='token', + options={'ordering': ('-created',)}, + ), + ] diff --git a/netbox/users/models/tokens.py b/netbox/users/models/tokens.py index 2e7040699..3c1284bc9 100644 --- a/netbox/users/models/tokens.py +++ b/netbox/users/models/tokens.py @@ -74,6 +74,7 @@ class Token(models.Model): class Meta: verbose_name = _('token') verbose_name_plural = _('tokens') + ordering = ('-created',) def __str__(self): return self.key if settings.ALLOW_TOKEN_RETRIEVAL else self.partial diff --git a/netbox/utilities/query.py b/netbox/utilities/query.py index b254df582..5eaff836f 100644 --- a/netbox/utilities/query.py +++ b/netbox/utilities/query.py @@ -67,5 +67,8 @@ def reapply_model_ordering(queryset: QuerySet) -> QuerySet: # MPTT-based models are exempt from this; use caution when annotating querysets of these models if any(isinstance(manager, TreeManager) for manager in queryset.model._meta.local_managers): return queryset + elif queryset.ordered: + return queryset + ordering = queryset.model._meta.ordering return queryset.order_by(*ordering)