Ensure a consistent error structure for both single and bulk requests

netbox/dcim/tests/test_api.py

@@ -467,6 +467,51 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase):
         response = self.client.patch(url, data, format='json', **self.header)
         self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
 
+    def test_bulk_update_objects_non_list_body(self):
+        """
+        PATCH a list endpoint with a body which is not a list. The response should identify the
+        problem with the request as a whole, as there are no entries to report against.
+        """
+        obj_perm = ObjectPermission(name='Test permission', actions=['change'])
+        obj_perm.save()
+        obj_perm.users.add(self.user)
+        obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
+
+        site = Site.objects.get(slug='site-1')
+        response = self.client.patch(
+            self._get_list_url(), {'id': site.pk, 'description': 'x'}, format='json', **self.header
+        )
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        self.assertIn('detail', response.data)
+        self.assertNotIn('errors', response.data)
+
+        site.refresh_from_db()
+        self.assertEqual(site.description, '')
+
+    def test_bulk_update_objects_non_numeric_id(self):
+        """
+        PATCH a set of objects where one entry carries a non-numeric ID. The failure must be
+        correlated by position, in the same structured form as every other bulk error.
+        """
+        obj_perm = ObjectPermission(name='Test permission', actions=['change'])
+        obj_perm.save()
+        obj_perm.users.add(self.user)
+        obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
+
+        site = Site.objects.get(slug='site-1')
+        data = [{'id': site.pk, 'description': 'x'}, {'id': 'not-a-number', 'description': 'y'}]
+        response = self.client.patch(self._get_list_url(), data, format='json', **self.header)
+
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(len(response.data['errors']), 1)
+        self.assertEqual(response.data['errors'][0]['index'], 1)
+        self.assertIn('id', response.data['errors'][0]['errors'])
+
+        # The valid entry must not have been applied
+        site.refresh_from_db()
+        self.assertEqual(site.description, '')
+
     def test_bulk_update_objects_duplicate_id_invalid_entry(self):
         """
         PATCH a set of objects in which one object is named twice, once with invalid data and once

netbox/netbox/api/viewsets/mixins.py

@@ -34,10 +34,74 @@ __all__ = (
     'SequentialBulkCreatesMixin',
     'discard_events_on_rollback',
     'get_duplicate_objects_response',
+    'get_invalid_entries_response',
     'get_missing_objects_response',
+    'get_non_list_response',
 )
 
 
+def get_non_list_response(data):
+    """
+    Return an error Response if the given request body is not a list of objects, or None if it is.
+
+    A bulk operation always addresses a list. The body reaching one is not necessarily a list,
+    however, as the router maps every PUT, PATCH, and DELETE on a list endpoint to a bulk action
+    regardless of what was sent. Rejecting a non-list body here keeps the per-entry errors reported
+    by the bulk actions correlated by position: those come from a serializer bound to a list, so
+    they are only positional if the body was a list to begin with.
+
+    The response carries only a `detail`, with no `errors`, as there are no entries to report
+    against.
+    """
+    if isinstance(data, list):
+        return None
+
+    return Response(
+        {
+            'detail': _('Expected a list of objects, but got {datatype}.').format(
+                datatype=type(data).__name__
+            ),
+        },
+        status=status.HTTP_400_BAD_REQUEST,
+    )
+
+
+def get_invalid_entries_response(entry_errors):
+    """
+    Return a structured error Response for the entries of a bulk request which could not be
+    interpreted, or None if every entry was interpretable.
+
+    The bulk update and delete actions first check that each entry identifies an object, before any
+    entry has been matched to one. A failure at that stage -- a missing or non-numeric `id`, or an
+    entry which is not an object at all -- is reported against the entry's position in the request
+    rather than against an object ID, since no object has been identified yet. This is the same
+    correlation bulk create uses throughout, for the same reason.
+
+    Passing this stage is what allows every later error to be correlated by `id` instead.
+
+    :param entry_errors: The `errors` of a BulkOperationSerializer bound to a list, which holds one
+        entry per object in the request (an empty dict where that object was interpretable).
+    """
+    errors = [
+        {'index': i, 'errors': item_errors}
+        for i, item_errors in enumerate(entry_errors)
+        if item_errors
+    ]
+    if not errors:
+        return None
+
+    return Response(
+        {
+            'detail': _('{failed_count} of {total} objects failed validation.').format(
+                failed_count=len(errors),
+                total=len(entry_errors),
+            ),
+            'errors': errors,
+        },
+        status=status.HTTP_400_BAD_REQUEST,
+    )
+
+
 def get_duplicate_objects_response(object_ids):
     """
     Return a structured error Response naming each of the given object IDs which appears more than
@@ -418,8 +482,14 @@ class BulkUpdateModelMixin:
         if (response := handle_background(request, action)) is not None:
             return response
 
+        if (response := get_non_list_response(request.data)) is not None:
+            return response
+
+        # Check that every entry identifies an object before matching any of them to one, so that
+        # a malformed entry is reported in the same form as every other bulk error
         serializer = BulkOperationSerializer(data=request.data, many=True)
-        serializer.is_valid(raise_exception=True)
+        if not serializer.is_valid():
+            return get_invalid_entries_response(serializer.errors)
 
         object_ids = [o['id'] for o in serializer.validated_data]
 
@@ -533,8 +603,14 @@ class BulkDestroyModelMixin:
         if (response := handle_background(request, 'bulk_destroy')) is not None:
             return response
 
+        if (response := get_non_list_response(request.data)) is not None:
+            return response
+
+        # Check that every entry identifies an object before matching any of them to one, so that
+        # a malformed entry is reported in the same form as every other bulk error
         serializer = BulkOperationSerializer(data=request.data, many=True)
-        serializer.is_valid(raise_exception=True)
+        if not serializer.is_valid():
+            return get_invalid_entries_response(serializer.errors)
 
         object_ids = [o['id'] for o in serializer.validated_data]
 

netbox/utilities/testing/api.py

@@ -661,6 +661,45 @@ class APIViewTestCases:
                         f'sibling ID',
                     )
 
+        def test_bulk_update_objects_malformed_entry(self):
+            """
+            PATCH a set of objects in which one entry does not identify an object. The failure must be
+            reported in the same structured form as a per-object failure, correlated by position.
+            """
+            if self.bulk_update_data is None:
+                self.skipTest('Bulk update data not set')
+
+            obj_perm = ObjectPermission(name='Test permission', actions=['change'])
+            obj_perm.save()
+            obj_perm.users.add(self.user)
+            obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
+
+            instance = self._get_queryset().first()
+
+            # The second entry omits the object ID, so it cannot be matched to an object
+            data = [{'id': instance.pk, **self.bulk_update_data}, {}]
+
+            response = self.client.patch(self._get_list_url(), data, format='json', **self.header)
+
+            self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+            self.assertIn('detail', response.data)
+            self.assertEqual(len(response.data['errors']), 1)
+
+            # Correlated by position, as no object was identified for this entry
+            self.assertEqual(response.data['errors'][0]['index'], 1)
+            self.assertIn('id', response.data['errors'][0]['errors'])
+
+            # The valid entry must not have been applied
+            instance_after = self._get_queryset().get(pk=instance.pk)
+            for field in self.bulk_update_data:
+                if field in ('changelog_message', 'add_tags', 'remove_tags'):
+                    continue
+                self.assertEqual(
+                    getattr(instance_after, field, None),
+                    getattr(instance, field, None),
+                    f'Field {field!r} of object {instance.pk} was modified despite a malformed sibling entry',
+                )
+
         def test_bulk_update_objects_duplicate_id(self):
             """
             PATCH a set of objects in which the same object is named twice. The request must be
@@ -828,6 +867,39 @@ class APIViewTestCases:
             # The objects named alongside the missing one must not have been deleted
             self.assertEqual(self._get_queryset().count(), initial_count)
 
+        def test_bulk_delete_objects_malformed_entry(self):
+            """
+            DELETE a set of objects in which one entry does not identify an object. The failure must be
+            reported in the same structured form as a per-object failure, correlated by position.
+            """
+            obj_perm = ObjectPermission(
+                name='Test permission',
+                actions=['delete']
+            )
+            obj_perm.save()
+            obj_perm.users.add(self.user)
+            obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
+
+            # Target the most recently created object to avoid triggering recursive deletions
+            instance = self._get_queryset().order_by('-id').first()
+
+            # The second entry omits the object ID, so it cannot be matched to an object
+            data = [{'id': instance.pk}, {}]
+
+            initial_count = self._get_queryset().count()
+            response = self.client.delete(self._get_list_url(), data, format='json', **self.header)
+
+            self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+            self.assertIn('detail', response.data)
+            self.assertEqual(len(response.data['errors']), 1)
+
+            # Correlated by position, as no object was identified for this entry
+            self.assertEqual(response.data['errors'][0]['index'], 1)
+            self.assertIn('id', response.data['errors'][0]['errors'])
+
+            # Nothing may have been deleted
+            self.assertEqual(self._get_queryset().count(), initial_count)
+
         def test_bulk_delete_objects_duplicate_id(self):
             """
             DELETE a set of objects in which the same object is named twice. The request must be