Fixes #23447: Ignore ordering by a count field left out of the REST API response (#23515)

* Fixes #23447: Ignore ordering by a count field left out of the REST API response

* Fixes #23447: Ignore ordering by a count field left out of the REST API response

* review fixes

* fix sort

netbox/dcim/tests/test_api.py

@@ -1425,6 +1425,18 @@ class RackTestCase(APIViewTestCases.APIViewTestCase):
             },
         ]
 
+    def test_ordering_by_count_without_field(self):
+        """Ordering by a count left out of the response still sorts by it."""
+        self.add_permissions('dcim.view_rack')
+        rack = Rack.objects.get(name='Rack 3')
+        create_test_device('Device 1', site=rack.site, rack=rack)
+        response = self.client.get(
+            f'{self._get_list_url()}?ordering=-device_count,name&fields=id,name', **self.header
+        )
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual([row['name'] for row in response.data['results']], ['Rack 3', 'Rack 1', 'Rack 2'])
+        self.assertNotIn('device_count', response.data['results'][0])
+
     def test_get_rack_elevation(self):
         """
         GET a single rack elevation.
@@ -4336,6 +4348,45 @@ class InterfaceTestCase(Mixins.FieldPrefetchMixin, Mixins.ComponentTraceMixin, A
             filters.append(f'device_id={device.pk}')
         return [filters[0], '&'.join(filters)]
 
+    def test_ordering_by_count_annotation(self):
+        """Ordering by a count annotation sorts the response whether or not it renders the count."""
+        self.add_permissions('dcim.view_interface', 'extras.view_exporttemplate')
+        interfaces = {name: Interface.objects.get(name=name) for name in ('Interface 2', 'Interface 3')}
+        IPAddress.objects.bulk_create((
+            IPAddress(address='192.0.2.1/24', assigned_object=interfaces['Interface 2']),
+            IPAddress(address='192.0.2.2/24', assigned_object=interfaces['Interface 2']),
+            IPAddress(address='192.0.2.3/24', assigned_object=interfaces['Interface 3']),
+        ))
+        export_template = ExportTemplate.objects.create(
+            name='Interfaces', template_code='{% for interface in queryset %}{{ interface.name }},{% endfor %}'
+        )
+        export_template.object_types.set([ObjectType.objects.get_for_model(Interface)])
+        url = f'{self._get_list_url()}?ordering=-count_ipaddresses'
+
+        # The count's source isn't a valid ordering field
+        response = self.client.get(f'{self._get_list_url()}?ordering=-_count_ipaddresses', **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(
+            [row['name'] for row in response.data['results']], ['Interface 1', 'Interface 2', 'Interface 3']
+        )
+
+        # Responses without the count annotate it for the ordering alone
+        for params in ('', '&fields=id,name,count_ipaddresses', '&brief=1', '&fields=id,name', '&export=Interfaces'):
+            with self.subTest(params=params):
+                response = self.client.get(url + params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                if 'export' in params:
+                    self.assertEqual(response.content.decode(), 'Interface 2,Interface 3,Interface 1,')
+                else:
+                    self.assertEqual(
+                        [row['name'] for row in response.data['results']],
+                        ['Interface 2', 'Interface 3', 'Interface 1'],
+                    )
+                    self.assertEqual(
+                        'count_ipaddresses' in response.data['results'][0],
+                        params in ('', '&fields=id,name,count_ipaddresses'),
+                    )
+
     def test_bulk_delete_child_interfaces(self):
         interface1 = Interface.objects.get(name='Interface 1')
         device = interface1.device

netbox/netbox/api/filters.py

@@ -0,0 +1,57 @@
+from rest_framework import filters
+
+from netbox.api.fields import RelatedObjectCountField
+from utilities.api import get_annotations_for_serializer
+
+__all__ = (
+    'OrderingFilter',
+)
+
+
+class OrderingFilter(filters.OrderingFilter):
+    """
+    Extends DRF's OrderingFilter to order by a RelatedObjectCountField under its field name rather than its source,
+    annotating the count when the response leaves it out, since its annotation is otherwise applied only when the field
+    is included in the response.
+    """
+    @staticmethod
+    def _get_serializer_class(view):
+        if hasattr(view, 'get_serializer_class'):
+            return view.get_serializer_class()
+        return view.serializer_class
+
+    def _get_count_sources(self, view):
+        """Map the name of each RelatedObjectCountField on the view's serializer to its source."""
+        return {
+            field_name: field.source or field_name
+            for field_name, field in self._get_serializer_class(view)._declared_fields.items()
+            if isinstance(field, RelatedObjectCountField)
+        }
+
+    def get_default_valid_fields(self, queryset, view, context=None):
+        field_names = {source: field_name for field_name, source in self._get_count_sources(view).items()}
+        return [
+            (field_names.get(key, key), label)
+            for key, label in super().get_default_valid_fields(queryset, view, context)
+        ]
+
+    def filter_queryset(self, request, queryset, view):
+        ordering = self.get_ordering(request, queryset, view)
+        if not ordering:
+            return queryset
+
+        count_sources = self._get_count_sources(view)
+        terms = []
+        missing = []
+        for term in ordering:
+            if isinstance(term, str) and (field_name := term.lstrip('-')) in count_sources:
+                source = count_sources[field_name]
+                if source not in queryset.query.annotations:
+                    missing.append(field_name)
+                term = f'-{source}' if term.startswith('-') else source
+            terms.append(term)
+        if missing:
+            serializer_class = self._get_serializer_class(view)
+            queryset = queryset.annotate(**get_annotations_for_serializer(serializer_class, fields=missing))
+
+        return queryset.order_by(*terms)

netbox/netbox/api/viewsets/__init__.py

@@ -188,7 +188,7 @@ class BaseViewSet(GenericViewSet):
         """Return the names of the count fields annotated for reads only."""
         return frozenset(
             name for name, field in self.get_serializer_class()._declared_fields.items()
-            if type(field) is RelatedObjectCountField and field.fallback_source
+            if isinstance(field, RelatedObjectCountField) and field.fallback_source
         )
 
 

netbox/netbox/settings.py

@@ -793,7 +793,7 @@ REST_FRAMEWORK = {
     ),
     'DEFAULT_FILTER_BACKENDS': (
         'django_filters.rest_framework.DjangoFilterBackend',
-        'rest_framework.filters.OrderingFilter',
+        'netbox.api.filters.OrderingFilter',
     ),
     'DEFAULT_METADATA_CLASS': 'netbox.api.metadata.BulkOperationMetadata',
     'DEFAULT_PAGINATION_CLASS': 'netbox.api.pagination.NetBoxPagination',

netbox/utilities/api.py

@@ -251,7 +251,7 @@ def get_annotations_for_serializer(serializer_class, fields=None, omit=None, exc
 
     annotations = {}
     for field_name, field in serializer_class._declared_fields.items():
-        if field_name in effective_fields and field_name not in exclude and type(field) is RelatedObjectCountField:
+        if field_name in effective_fields and field_name not in exclude and isinstance(field, RelatedObjectCountField):
             related_field = getattr(model, field.relation).field
             if isinstance(related_field, GenericRelation):
                 count = count_related(

netbox/virtualization/tests/test_api.py

@@ -1138,6 +1138,37 @@ class VMInterfaceTestCase(APIViewTestCases.APIViewTestCase):
         # Unannotated instances count through the relations
         self.assertEqual((vm_interface.count_ipaddresses, vm_interface.count_fhrp_groups), (2, 1))
 
+    def test_ordering_by_count_annotation(self):
+        """Ordering by a count annotation sorts the response whether or not it renders the count."""
+        self.add_permissions('virtualization.view_vminterface')
+        groups = FHRPGroup.objects.bulk_create((
+            FHRPGroup(protocol=FHRPGroupProtocolChoices.PROTOCOL_VRRP2, group_id=1),
+            FHRPGroup(protocol=FHRPGroupProtocolChoices.PROTOCOL_VRRP2, group_id=2),
+        ))
+        interfaces = {name: VMInterface.objects.get(name=name) for name in ('Interface 2', 'Interface 3')}
+        FHRPGroupAssignment.objects.bulk_create((
+            FHRPGroupAssignment(group=groups[0], interface=interfaces['Interface 2'], priority=100),
+            FHRPGroupAssignment(group=groups[1], interface=interfaces['Interface 2'], priority=100),
+            FHRPGroupAssignment(group=groups[0], interface=interfaces['Interface 3'], priority=100),
+        ))
+        url = f'{self._get_list_url()}?ordering=-count_fhrp_groups'
+
+        # The count's source isn't a valid ordering field
+        response = self.client.get(f'{self._get_list_url()}?ordering=-_count_fhrp_groups', **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+        self.assertEqual(
+            [row['name'] for row in response.data['results']], ['Interface 1', 'Interface 2', 'Interface 3']
+        )
+
+        # Responses without the count annotate it for the ordering alone
+        for params in ('', '&brief=1', '&fields=id,name'):
+            with self.subTest(params=params):
+                response = self.client.get(url + params, **self.header)
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                self.assertEqual(
+                    [row['name'] for row in response.data['results']], ['Interface 2', 'Interface 3', 'Interface 1']
+                )
+
     def test_export_template_prefetches_related_objects(self):
         """An API export renders model instances, so it prefetches each counted relation once."""
         self.add_permissions('virtualization.view_vminterface', 'extras.view_exporttemplate')