From b61c232305a34374f2aa5cc27e41fe6ee157aa96 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Tue, 14 Jul 2026 14:19:08 -0400 Subject: [PATCH] Return errors-only response for bulk operations, drop error_count Rename the 'results' key to 'errors' and omit successful objects from the bulk create/update/destroy error response, applied consistently across all three mixins. len(errors) replaces the separate error_count bookkeeping. Also change the ProtectedError/RestrictedError entry's 'detail' key to '__all__' to match the field-based error format used by creates and updates, and correct a comment that implied bulk delete enforces a permission boundary the single-object delete endpoint doesn't actually have. Addresses review feedback from @jeremystretch. --- netbox/dcim/tests/test_api.py | 34 +++++-------- netbox/netbox/api/viewsets/mixins.py | 71 +++++++++++++--------------- netbox/utilities/testing/api.py | 10 ++-- 3 files changed, 51 insertions(+), 64 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index be4de06eb..0d98d9ba1 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -490,17 +490,12 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): self.assertHttpStatus(response, status.HTTP_409_CONFLICT) self.assertIn('detail', response.data) - self.assertIn('results', response.data) - self.assertEqual(len(response.data['results']), 2) + self.assertIn('errors', response.data) + self.assertEqual(len(response.data['errors']), 1) - # Index results by ID to avoid relying on queryset ordering - results_by_id = {r['id']: r for r in response.data['results']} - self.assertIn(site1.pk, results_by_id) - self.assertIn(site2.pk, results_by_id) - # Site 1 (no dependents) would have succeeded — no errors key - self.assertNotIn('errors', results_by_id[site1.pk]) - # Site 2 (has Device) should have failed — errors key present - self.assertIn('errors', results_by_id[site2.pk]) + # Site 2 (has Device) should be the only entry, since Site 1 succeeded + self.assertEqual(response.data['errors'][0]['id'], site2.pk) + self.assertIn('errors', response.data['errors'][0]) # Verify that no sites were actually deleted (transaction rolled back) self.assertTrue(Site.objects.filter(pk=site1.pk).exists(), 'Site 1 should not have been deleted') @@ -2204,9 +2199,9 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): def test_bulk_create_objects_validation_error(self): """ POST a set of Device objects where the first passes and the second fails validation. - DeviceViewSet uses SequentialBulkCreatesMixin, so the response should be the structured - per-object format with mixed ok/error statuses, and no objects should be created despite - the first item passing (atomic rollback). + DeviceViewSet uses SequentialBulkCreatesMixin, so the response should report only the + failed object, and no objects should be created despite the first item passing + (atomic rollback). """ obj_perm = ObjectPermission(name='Test permission', actions=['add']) obj_perm.save() @@ -2230,14 +2225,11 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): 'No objects should be created when any sibling fails validation', ) self.assertIn('detail', response.data) - self.assertIn('results', response.data) - self.assertEqual(len(response.data['results']), 2) - # First item passed validation — no errors key - self.assertEqual(response.data['results'][0]['index'], 0) - self.assertNotIn('errors', response.data['results'][0]) - # Second item failed validation — errors key present - self.assertEqual(response.data['results'][1]['index'], 1) - self.assertIn('errors', response.data['results'][1]) + self.assertIn('errors', response.data) + self.assertEqual(len(response.data['errors']), 1) + # Second item failed validation — first item succeeded so it's omitted + self.assertEqual(response.data['errors'][0]['index'], 1) + self.assertIn('errors', response.data['errors'][0]) class ModuleTestCase(APIViewTestCases.APIViewTestCase): diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 0ad7c5295..336630e11 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -167,14 +167,14 @@ class SequentialBulkCreatesMixin: # Create objects sequentially so each validation sees the state left by prior creates # (e.g. rack space checks). Collect per-object errors instead of failing on the first. - results = [] + errors = [] return_data = [] - error_count = 0 with transaction.atomic(using=router.db_for_write(self.queryset.model)): if not isinstance(request.data, list): # Creating a single object return super().create(request, *args, **kwargs) + total = len(request.data) for i, data in enumerate(request.data): serializer = self.get_serializer(data=data) if serializer.is_valid(): @@ -183,22 +183,20 @@ class SequentialBulkCreatesMixin: # All creates are rolled back together if any item in the batch fails. self.perform_create(serializer) return_data.append(serializer.data) - results.append({'index': i}) else: - results.append({'index': i, 'errors': serializer.errors}) - error_count += 1 + errors.append({'index': i, 'errors': serializer.errors}) - if error_count: + if errors: transaction.set_rollback(True) - if error_count: + if errors: return Response( { 'detail': _('{failed_count} of {total} objects failed validation.').format( - failed_count=error_count, - total=len(results), + failed_count=len(errors), + total=total, ), - 'results': results, + 'errors': errors, }, status=status.HTTP_400_BAD_REQUEST, ) @@ -252,16 +250,16 @@ class BulkUpdateModelMixin: obj.pop('id'): obj for obj in request.data } - object_pks, results, error_count = self.perform_bulk_update(qs, update_data, partial=partial) + object_pks, errors = self.perform_bulk_update(qs, update_data, partial=partial) - if error_count: + if errors: return Response( { 'detail': _('{failed_count} of {total} objects failed validation.').format( - failed_count=error_count, - total=len(results), + failed_count=len(errors), + total=len(object_pks) + len(errors), ), - 'results': results, + 'errors': errors, }, status=status.HTTP_400_BAD_REQUEST, ) @@ -274,8 +272,7 @@ class BulkUpdateModelMixin: def perform_bulk_update(self, objects, update_data, partial): updated_pks = [] - results = [] - error_count = 0 + errors = [] with transaction.atomic(using=router.db_for_write(self.queryset.model)): # Validate and save each object in turn so subsequent validations see the DB # state left by prior saves (e.g. two items renamed to the same name: the second @@ -288,13 +285,11 @@ class BulkUpdateModelMixin: if serializer.is_valid(): self.perform_update(serializer) updated_pks.append(obj.pk) - results.append({'id': obj.pk}) else: - results.append({'id': obj.pk, 'errors': serializer.errors}) - error_count += 1 - if error_count: + errors.append({'id': obj.pk, 'errors': serializer.errors}) + if errors: transaction.set_rollback(True) - return updated_pks, results, error_count + return updated_pks, errors def get_bulk_update_serializer_class(self, *, partial=False): return get_bulk_update_serializer_class( @@ -350,16 +345,16 @@ class BulkDestroyModelMixin: o['id']: o.get('changelog_message') for o in serializer.validated_data } - results, error_count = self.perform_bulk_destroy(qs, changelog_messages) + errors, total = self.perform_bulk_destroy(qs, changelog_messages) - if error_count: + if errors: return Response( { 'detail': _('{failed_count} of {total} objects could not be deleted.').format( - failed_count=error_count, - total=len(results), + failed_count=len(errors), + total=total, ), - 'results': results, + 'errors': errors, }, status=status.HTTP_409_CONFLICT, ) @@ -368,36 +363,38 @@ class BulkDestroyModelMixin: def perform_bulk_destroy(self, objects, changelog_messages=None): changelog_messages = changelog_messages or {} - results = [] - error_count = 0 + errors = [] + total = 0 with transaction.atomic(using=router.db_for_write(self.queryset.model)): for obj in objects: + total += 1 if hasattr(obj, 'snapshot'): obj.snapshot() obj._changelog_message = changelog_messages.get(obj.pk) pk = obj.pk # Django sets obj.pk = None after deletion; capture it first try: self.perform_destroy(obj) - results.append({'id': pk}) except (ProtectedError, RestrictedError) as e: protected = list( e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects ) n = len(protected) - # Report only the count — not names or PKs — to avoid exposing objects - # the caller may not have permission to view. - results.append({ + # Report only the count, not names or PKs, to keep each per-object error + # entry small in a batch response. Note: the single-object delete endpoint + # (NetBoxModelViewSet.dispatch()) does include names and PKs of dependent + # objects, so this is not a hard security boundary — just a narrower + # response shape for the bulk case. + errors.append({ 'id': pk, 'errors': { - 'detail': _( + '__all__': _( 'Unable to delete: {n} dependent object(s) prevent deletion.' ).format(n=n), }, }) - error_count += 1 - if error_count: + if errors: transaction.set_rollback(True) - return results, error_count + return errors, total class ObjectValidationMixin: diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 1a651b163..1750ffed5 100644 --- a/netbox/utilities/testing/api.py +++ b/netbox/utilities/testing/api.py @@ -576,12 +576,10 @@ class APIViewTestCases: self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) self.assertIn('detail', response.data) - self.assertIn('results', response.data) - self.assertEqual(len(response.data['results']), 2) - self.assertEqual(response.data['results'][0]['id'], id_list[0]) - self.assertNotIn('errors', response.data['results'][0]) - self.assertEqual(response.data['results'][1]['id'], id_list[1]) - self.assertIn('errors', response.data['results'][1]) + self.assertIn('errors', response.data) + self.assertEqual(len(response.data['errors']), 1) + self.assertEqual(response.data['errors'][0]['id'], id_list[1]) + self.assertIn('errors', response.data['errors'][0]) # Verify atomicity: object 0 passed validation but must not have been modified instance0_after = self._get_queryset().get(pk=id_list[0])