From 8c820924fb8fcc007a8355a3cd2a487c5bae58de Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 23 Jul 2025 12:23:13 -0500 Subject: [PATCH 1/6] 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 --- netbox/netbox/api/exceptions.py | 4 ++++ netbox/netbox/api/pagination.py | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/netbox/netbox/api/exceptions.py b/netbox/netbox/api/exceptions.py index f552b06b55a..c4d05d1a520 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 f1430a9fd82..698e0a8dda2 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 @@ def __init__(self): 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: From 75c726e86d8c490852b2bb3728f3d6606df3f762 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 23 Jul 2025 12:25:30 -0500 Subject: [PATCH 2/6] Don't try to reapply ordering if ordering is already present --- netbox/utilities/query.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/netbox/utilities/query.py b/netbox/utilities/query.py index b254df582fd..5eaff836f2b 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) From 6966c7ac2356bf7c349dd86ce0b00b39c1472f39 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 23 Jul 2025 12:26:23 -0500 Subject: [PATCH 3/6] 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. --- netbox/extras/api/views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 3f5bb172a02..facb1b17a59 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 From 8e39481c6b2791ac1b66b6c050deff14aa05720c Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 23 Jul 2025 12:32:12 -0500 Subject: [PATCH 4/6] Add default Token ordering behavior --- .../migrations/0010_add_token_meta_ordering.py | 17 +++++++++++++++++ netbox/users/models/tokens.py | 1 + 2 files changed, 18 insertions(+) create mode 100644 netbox/users/migrations/0010_add_token_meta_ordering.py 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 00000000000..bb2be6c45fa --- /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 2e704069935..3c1284bc9ce 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 From 7fa81f1498a5d04b9aee519c65afffb1026388db Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 29 Jul 2025 11:17:33 -0500 Subject: [PATCH 5/6] Adds basic tests for raising QuerySetNotOrdered --- netbox/netbox/tests/test_api.py | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/netbox/netbox/tests/test_api.py b/netbox/netbox/tests/test_api.py index d087910b5d7..61bbcd4c678 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 @@ def test_status(self): 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 From 808db8c8e855de0d6a5b4c7676d8e7f98fd7da58 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 29 Jul 2025 11:35:28 -0500 Subject: [PATCH 6/6] Note why ordering is not applied in TaggedItem.Meta --- netbox/extras/models/tags.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/netbox/extras/models/tags.py b/netbox/extras/models/tags.py index b40327265f5..7d52d9eb609 100644 --- a/netbox/extras/models/tags.py +++ b/netbox/extras/models/tags.py @@ -83,3 +83,6 @@ class Meta: 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.