review fixes

netbox/dcim/tests/test_api.py

@@ -1426,11 +1426,16 @@ class RackTestCase(APIViewTestCases.APIViewTestCase):
         ]
 
     def test_ordering_by_count_without_field(self):
-        """Ordering by a count left out of the response is ignored rather than failing the request."""
+        """Ordering by a count left out of the response still sorts by it."""
         self.add_permissions('dcim.view_rack')
-        response = self.client.get(f'{self._get_list_url()}?ordering=-device_count&fields=id,name', **self.header)
+        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 1', 'Rack 2', 'Rack 3'])
+        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):
         """
@@ -4344,7 +4349,7 @@ class InterfaceTestCase(Mixins.FieldPrefetchMixin, Mixins.ComponentTraceMixin, A
         return [filters[0], '&'.join(filters)]
 
     def test_ordering_by_count_annotation(self):
-        """Ordering by a count annotation sorts a response that renders it and is ignored by one that does not."""
+        """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((
@@ -4358,25 +4363,21 @@ class InterfaceTestCase(Mixins.FieldPrefetchMixin, Mixins.ComponentTraceMixin, A
         export_template.object_types.set([ObjectType.objects.get_for_model(Interface)])
         url = f'{self._get_list_url()}?ordering=-_count_ipaddresses'
 
-        for params in ('', '&fields=id,name,count_ipaddresses'):
-            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']
-                )
-
-        # Responses without the count carry no annotation to order by, so they keep the default ordering
-        for params in ('&brief=1', '&fields=id,name', '&export=Interfaces'):
+        # 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 1,Interface 2,Interface 3,')
+                    self.assertEqual(response.content.decode(), 'Interface 2,Interface 3,Interface 1,')
                 else:
                     self.assertEqual(
                         [row['name'] for row in response.data['results']],
-                        ['Interface 1', 'Interface 2', 'Interface 3'],
+                        ['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):

netbox/netbox/api/filters.py

@@ -1,6 +1,7 @@
 from rest_framework import filters
 
 from netbox.api.fields import RelatedObjectCountField
+from utilities.api import get_annotations_for_serializer
 
 __all__ = (
     'OrderingFilter',
@@ -9,21 +10,28 @@ __all__ = (
 
 class OrderingFilter(filters.OrderingFilter):
     """
-    Extends DRF's OrderingFilter to accept a RelatedObjectCountField as an ordering key only when its annotation has
-    been applied to the queryset, which happens only when the field is included in the response.
+    Extends DRF's OrderingFilter to annotate a RelatedObjectCountField named by the ordering when the response leaves
+    it out, since its annotation is otherwise applied only when the field is included in the response.
     """
-    def get_default_valid_fields(self, queryset, view, context=None):
-        valid_fields = 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
+
         if hasattr(view, 'get_serializer_class'):
             serializer_class = view.get_serializer_class()
         else:
             serializer_class = view.serializer_class
-        count_sources = {
-            field.source or field_name
+        count_fields = {
+            field.source or field_name: field_name
             for field_name, field in serializer_class._declared_fields.items()
-            if type(field) is RelatedObjectCountField
+            if isinstance(field, RelatedObjectCountField)
         }
-        return [
-            (source, label) for source, label in valid_fields
-            if source not in count_sources or source in queryset.query.annotations
+        missing = [
+            count_fields[source] for source in (term.lstrip('-') for term in ordering)
+            if source in count_fields and source not in queryset.query.annotations
         ]
+        if missing:
+            queryset = queryset.annotate(**get_annotations_for_serializer(serializer_class, fields=missing))
+
+        return queryset.order_by(*ordering)

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/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

@@ -1139,7 +1139,7 @@ class VMInterfaceTestCase(APIViewTestCases.APIViewTestCase):
         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 a response that renders it and is ignored by one that does not."""
+        """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),
@@ -1153,19 +1153,13 @@ class VMInterfaceTestCase(APIViewTestCases.APIViewTestCase):
         ))
         url = f'{self._get_list_url()}?ordering=-_count_fhrp_groups'
 
-        response = self.client.get(url, **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']
-        )
-
-        # Responses without the count carry no annotation to order by, so they keep the default ordering
-        for params in ('&brief=1', '&fields=id,name'):
+        # 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 1', 'Interface 2', 'Interface 3']
+                    [row['name'] for row in response.data['results']], ['Interface 2', 'Interface 3', 'Interface 1']
                 )
 
     def test_export_template_prefetches_related_objects(self):