diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 4e5f4a919..34be0da6d 100644 --- a/netbox/dcim/tests/test_api.py +++ b/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 diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 2d80e6a68..ff419ec59 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/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] diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 4e77d0831..27873bf9e 100644 --- a/netbox/utilities/testing/api.py +++ b/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