From 3d8f8289d9476b13da2b9b2457423384a725c60f Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 8 Jul 2026 17:10:25 -0400 Subject: [PATCH 1/9] Closes #20054: Return per-object error details for failed bulk operations Bulk update (PATCH), sequential bulk create (POST), and bulk delete (DELETE) on list endpoints now collect per-object errors instead of aborting on the first failure. When any objects fail, the entire operation is rolled back atomically and a 400/409 response is returned with a structured payload: { "detail": "1 of 3 objects failed validation.", "results": [ {"id": 1, "status": "ok"}, {"id": 2, "status": "error", "errors": {"name": ["..."]}}, {"id": 3, "status": "ok"} ] } For bulk creates via SequentialBulkCreatesMixin the correlator is "index" (zero-based position in the request list) since no IDs exist yet. For bulk delete the status code remains 409 and the correlator is "id". Successful operations are unchanged (200/201/204). Co-Authored-By: Claude Sonnet 4.6 --- netbox/dcim/tests/test_api.py | 65 ++++++++++++++++ netbox/netbox/api/viewsets/mixins.py | 109 ++++++++++++++++++++++----- netbox/utilities/testing/api.py | 34 +++++++++ 3 files changed, 190 insertions(+), 18 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index ec0ef1a50..57a8c3cbd 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -151,6 +151,9 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): bulk_update_data = { 'status': 'planned', } + bulk_update_invalid_data = { + 'status': 'not-a-valid-status', + } graphql_filter_tests = ( GraphQLFilterTest( name='tenant__name__exact', @@ -467,6 +470,40 @@ 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_delete_objects_protected(self): + """ + DELETE a set of objects where one has a protected FK dependency. Verify the structured + per-object error response and that no objects are deleted (atomic rollback). + """ + 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)) + + # Site 1 has no dependent Device; Site 2 gets one (Device FK is on_delete=PROTECT) + site1 = Site.objects.get(slug='site-1') + site2 = Site.objects.get(slug='site-2') + create_test_device('Protected Device', site=site2) + + data = [{'id': site1.pk}, {'id': site2.pk}] + response = self.client.delete(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_409_CONFLICT) + self.assertIn('detail', response.data) + self.assertIn('results', response.data) + self.assertEqual(len(response.data['results']), 2) + # First site (no dependents) would have succeeded + self.assertEqual(response.data['results'][0]['id'], site1.pk) + self.assertEqual(response.data['results'][0]['status'], 'ok') + # Second site (has Device) should have failed + self.assertEqual(response.data['results'][1]['id'], site2.pk) + self.assertEqual(response.data['results'][1]['status'], 'error') + self.assertIn('errors', response.data['results'][1]) + + # 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') + self.assertTrue(Site.objects.filter(pk=site2.pk).exists(), 'Site 2 should not have been deleted') + class LocationTestCase(APIViewTestCases.APIViewTestCase): model = Location @@ -2162,6 +2199,34 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): response = self.client.post(url, {'config_template_id': override_template.pk}, format='json', **self.header) self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + def test_bulk_create_objects_validation_error(self): + """ + POST a set of Device objects where all fail validation. DeviceViewSet uses + SequentialBulkCreatesMixin, so the response should be the structured per-object error + format rather than DRF's default list-of-errors response. + """ + obj_perm = ObjectPermission(name='Test permission', actions=['add']) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model)) + + initial_count = self._get_queryset().count() + # Empty objects fail validation (required fields absent) + response = self.client.post(self._get_list_url(), [{}, {}], format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + self._get_queryset().count(), initial_count, + 'No objects should be created when any fail validation', + ) + self.assertIn('detail', response.data) + self.assertIn('results', response.data) + self.assertEqual(len(response.data['results']), 2) + for i, result in enumerate(response.data['results']): + self.assertEqual(result['index'], i) + self.assertEqual(result['status'], 'error') + self.assertIn('errors', result) + class ModuleTestCase(APIViewTestCases.APIViewTestCase): model = Module diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 0b6da06cd..cb4061b20 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -1,5 +1,6 @@ from django.core.exceptions import ObjectDoesNotExist from django.db import router, transaction +from django.db.models import ProtectedError, RestrictedError from django.http import Http404 from django.utils.translation import gettext_lazy as _ from rest_framework import status @@ -164,21 +165,39 @@ class SequentialBulkCreatesMixin: if (response := handle_background(request, 'create')) is not None: return response + if not isinstance(request.data, list): + # Creating a single object + return super().create(request, *args, **kwargs) + + # 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 = [] + return_data = [] 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) - - return_data = [] - for data in request.data: + for i, data in enumerate(request.data): serializer = self.get_serializer(data=data) - serializer.is_valid(raise_exception=True) - self.perform_create(serializer) - return_data.append(serializer.data) + if serializer.is_valid(): + self.perform_create(serializer) + return_data.append(serializer.data) + results.append({'index': i, 'status': 'ok'}) + else: + results.append({'index': i, 'status': 'error', 'errors': serializer.errors}) - headers = self.get_success_headers(serializer.data) + if any(r['status'] == 'error' for r in results): + transaction.set_rollback(True) - return Response(return_data, status=status.HTTP_201_CREATED, headers=headers) + if any(r['status'] == 'error' for r in results): + failed_count = sum(1 for r in results if r['status'] == 'error') + return Response( + { + 'detail': f'{failed_count} of {len(results)} objects failed validation.', + 'results': results, + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + headers = self.get_success_headers(return_data[-1]) if return_data else {} + return Response(return_data, status=status.HTTP_201_CREATED, headers=headers) class BulkUpdateModelMixin: @@ -226,7 +245,17 @@ class BulkUpdateModelMixin: obj.pop('id'): obj for obj in request.data } - object_pks = self.perform_bulk_update(qs, update_data, partial=partial) + object_pks, results = self.perform_bulk_update(qs, update_data, partial=partial) + + if results: + failed_count = sum(1 for r in results if r['status'] == 'error') + return Response( + { + 'detail': f'{failed_count} of {len(results)} objects failed validation.', + 'results': results, + }, + status=status.HTTP_400_BAD_REQUEST, + ) # Prefetch related objects for all updated instances qs = self.get_queryset().filter(pk__in=object_pks) @@ -236,17 +265,31 @@ class BulkUpdateModelMixin: def perform_bulk_update(self, objects, update_data, partial): updated_pks = [] + results = [] with transaction.atomic(using=router.db_for_write(self.queryset.model)): + # Pass 1: validate all objects without writing to the database + prepared = [] for obj in objects: data = update_data.get(obj.id) if hasattr(obj, 'snapshot'): obj.snapshot() serializer = self.get_serializer(obj, data=data, partial=partial) - serializer.is_valid(raise_exception=True) - self.perform_update(serializer) - updated_pks.append(obj.pk) + prepared.append((obj, serializer, serializer.is_valid())) - return updated_pks + if any(not valid for _, _, valid in prepared): + results = [ + {'id': obj.pk, 'status': 'error', 'errors': ser.errors} if not valid + else {'id': obj.pk, 'status': 'ok'} + for obj, ser, valid in prepared + ] + transaction.set_rollback(True) + else: + # Pass 2: all objects are valid — perform updates + for obj, serializer, _ in prepared: + self.perform_update(serializer) + updated_pks.append(obj.pk) + + return updated_pks, results def get_bulk_update_serializer_class(self, *, partial=False): return get_bulk_update_serializer_class( @@ -302,18 +345,48 @@ class BulkDestroyModelMixin: o['id']: o.get('changelog_message') for o in serializer.validated_data } - self.perform_bulk_destroy(qs, changelog_messages) + results = self.perform_bulk_destroy(qs, changelog_messages) + + if results and any(r['status'] == 'error' for r in results): + failed_count = sum(1 for r in results if r['status'] == 'error') + return Response( + { + 'detail': f'{failed_count} of {len(results)} objects could not be deleted.', + 'results': results, + }, + status=status.HTTP_409_CONFLICT, + ) return Response(status=status.HTTP_204_NO_CONTENT) def perform_bulk_destroy(self, objects, changelog_messages=None): changelog_messages = changelog_messages or {} + results = [] with transaction.atomic(using=router.db_for_write(self.queryset.model)): for obj in objects: if hasattr(obj, 'snapshot'): obj.snapshot() obj._changelog_message = changelog_messages.get(obj.pk) - self.perform_destroy(obj) + pk = obj.pk # Django sets obj.pk = None after deletion; capture it first + try: + self.perform_destroy(obj) + results.append({'id': pk, 'status': 'ok'}) + except (ProtectedError, RestrictedError) as e: + protected = list( + e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects + ) + n = len(protected) + objects_str = ', '.join(f'{o} ({o.pk})' for o in protected[:10]) + if n > 10: + objects_str += f', and {n - 10} more' + results.append({ + 'id': obj.pk, + 'status': 'error', + 'errors': {'detail': f'Unable to delete. {n} dependent object(s): {objects_str}'}, + }) + if any(r['status'] == 'error' for r in results): + transaction.set_rollback(True) + return results class ObjectValidationMixin: diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 9f4fa645b..6aaee1f45 100644 --- a/netbox/utilities/testing/api.py +++ b/netbox/utilities/testing/api.py @@ -395,6 +395,7 @@ class APIViewTestCases: class UpdateObjectViewTestCase(APITestCase): update_data = {} bulk_update_data = None + bulk_update_invalid_data = None validation_excluded_fields = [] def test_update_object_without_permission(self): @@ -546,6 +547,39 @@ class APIViewTestCases: self.assertObjectChange(oc, action=ObjectChangeActionChoices.ACTION_UPDATE, message=changelog_message) + def test_bulk_update_objects_validation_error(self): + """ + PATCH a set of objects where one fails validation. Verify the structured per-object error + response and that no objects are modified (atomic rollback). + """ + if self.bulk_update_data is None or self.bulk_update_invalid_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)) + + id_list = list(self._get_queryset().values_list('id', flat=True)[:2]) + self.assertEqual(len(id_list), 2, 'Insufficient number of objects to test bulk update validation error') + + # First object: valid data; second: invalid data that must fail validation + data = [ + {'id': id_list[0], **self.bulk_update_data}, + {'id': id_list[1], **self.bulk_update_invalid_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.assertIn('results', response.data) + self.assertEqual(len(response.data['results']), 2) + self.assertEqual(response.data['results'][0]['id'], id_list[0]) + self.assertEqual(response.data['results'][0]['status'], 'ok') + self.assertEqual(response.data['results'][1]['id'], id_list[1]) + self.assertEqual(response.data['results'][1]['status'], 'error') + self.assertIn('errors', response.data['results'][1]) + class DeleteObjectViewTestCase(APITestCase): def test_delete_object_without_permission(self): From d8506f178e5dd22b08c103ed65e172f5609ea761 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 8 Jul 2026 18:04:44 -0400 Subject: [PATCH 2/9] Address PR review feedback for #20054 bulk error correlation - Use pre-captured `pk` consistently in perform_bulk_destroy error path - Add comment clarifying the `if results:` sentinel in bulk_update - Add per-field atomicity assertion to test_bulk_update_objects_validation_error - Use ID-keyed dict instead of positional index in test_bulk_delete_objects_protected Co-Authored-By: Claude Sonnet 4.6 --- netbox/dcim/tests/test_api.py | 17 ++++++++++------- netbox/netbox/api/viewsets/mixins.py | 4 +++- netbox/utilities/testing/api.py | 15 +++++++++++++++ 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 57a8c3cbd..1b8ab8498 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -492,13 +492,16 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): self.assertIn('detail', response.data) self.assertIn('results', response.data) self.assertEqual(len(response.data['results']), 2) - # First site (no dependents) would have succeeded - self.assertEqual(response.data['results'][0]['id'], site1.pk) - self.assertEqual(response.data['results'][0]['status'], 'ok') - # Second site (has Device) should have failed - self.assertEqual(response.data['results'][1]['id'], site2.pk) - self.assertEqual(response.data['results'][1]['status'], 'error') - self.assertIn('errors', response.data['results'][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 + self.assertEqual(results_by_id[site1.pk]['status'], 'ok') + # Site 2 (has Device) should have failed + self.assertEqual(results_by_id[site2.pk]['status'], 'error') + self.assertIn('errors', results_by_id[site2.pk]) # 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') diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index cb4061b20..138096449 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -247,6 +247,8 @@ class BulkUpdateModelMixin: object_pks, results = self.perform_bulk_update(qs, update_data, partial=partial) + # perform_bulk_update returns an empty list on full success; non-empty means at least one + # object failed validation and the full results list (with per-object status) is populated. if results: failed_count = sum(1 for r in results if r['status'] == 'error') return Response( @@ -380,7 +382,7 @@ class BulkDestroyModelMixin: if n > 10: objects_str += f', and {n - 10} more' results.append({ - 'id': obj.pk, + 'id': pk, 'status': 'error', 'errors': {'detail': f'Unable to delete. {n} dependent object(s): {objects_str}'}, }) diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 6aaee1f45..99ea69a39 100644 --- a/netbox/utilities/testing/api.py +++ b/netbox/utilities/testing/api.py @@ -568,6 +568,10 @@ class APIViewTestCases: {'id': id_list[0], **self.bulk_update_data}, {'id': id_list[1], **self.bulk_update_invalid_data}, ] + + # Snapshot field values before the request so we can verify atomicity afterward + instance0_before = self._get_queryset().get(pk=id_list[0]) + response = self.client.patch(self._get_list_url(), data, format='json', **self.header) self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) @@ -580,6 +584,17 @@ class APIViewTestCases: self.assertEqual(response.data['results'][1]['status'], 'error') self.assertIn('errors', response.data['results'][1]) + # Verify atomicity: object 0 passed validation but must not have been modified + instance0_after = self._get_queryset().get(pk=id_list[0]) + for field in self.bulk_update_data: + if field in ('changelog_message', 'add_tags', 'remove_tags'): + continue + self.assertEqual( + getattr(instance0_after, field, None), + getattr(instance0_before, field, None), + f'Field {field!r} of object {id_list[0]} was modified — atomic rollback may be broken', + ) + class DeleteObjectViewTestCase(APITestCase): def test_delete_object_without_permission(self): From 94197efcfb2247d972a6f668bb53d7fdca2f2a07 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 8 Jul 2026 18:11:34 -0400 Subject: [PATCH 3/9] Improve test_bulk_create_objects_validation_error with mixed ok/error case Use a valid first item (create_data[0]) alongside an invalid second item ({}) so the test exercises both the 'ok' result shape and the atomic rollback of an item that would otherwise have been persisted. Co-Authored-By: Claude Sonnet 4.6 --- netbox/dcim/tests/test_api.py | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 1b8ab8498..a9797b59e 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -2204,31 +2204,42 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): def test_bulk_create_objects_validation_error(self): """ - POST a set of Device objects where all fail validation. DeviceViewSet uses - SequentialBulkCreatesMixin, so the response should be the structured per-object error - format rather than DRF's default list-of-errors response. + 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). """ obj_perm = ObjectPermission(name='Test permission', actions=['add']) obj_perm.save() obj_perm.users.add(self.user) obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model)) + self.add_related_view_permissions(self.create_data[0]) + initial_count = self._get_queryset().count() - # Empty objects fail validation (required fields absent) - response = self.client.post(self._get_list_url(), [{}, {}], format='json', **self.header) + # First item is valid; second is empty (missing required fields) and will fail + response = self.client.post( + self._get_list_url(), + [self.create_data[0], {}], + format='json', + **self.header, + ) self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) self.assertEqual( self._get_queryset().count(), initial_count, - 'No objects should be created when any fail validation', + '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) - for i, result in enumerate(response.data['results']): - self.assertEqual(result['index'], i) - self.assertEqual(result['status'], 'error') - self.assertIn('errors', result) + # First item passed validation + self.assertEqual(response.data['results'][0]['index'], 0) + self.assertEqual(response.data['results'][0]['status'], 'ok') + # Second item failed validation + self.assertEqual(response.data['results'][1]['index'], 1) + self.assertEqual(response.data['results'][1]['status'], 'error') + self.assertIn('errors', response.data['results'][1]) class ModuleTestCase(APIViewTestCases.APIViewTestCase): From d1310ed58053987da6cb63ccd225afb96c37808d Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Wed, 8 Jul 2026 18:42:05 -0400 Subject: [PATCH 4/9] Address PR #22646 review findings from automated reviewer - Security: remove object names/PKs from ProtectedError detail; report count only to avoid exposing objects the caller may lack permission to view - i18n: wrap new error detail strings with _().format() to match codebase convention - Redundancy: remove superfluous `results and` guard in bulk_destroy (any() on an empty list already returns False) - Comment: explain that SequentialBulkCreatesMixin continues provisionally creating after a failure so cross-object validators see a realistic state Co-Authored-By: Claude Sonnet 4.6 --- netbox/netbox/api/viewsets/mixins.py | 31 +++++++++++++++++++++------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 138096449..99be26087 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -177,6 +177,9 @@ class SequentialBulkCreatesMixin: for i, data in enumerate(request.data): serializer = self.get_serializer(data=data) if serializer.is_valid(): + # Provisionally create even when a prior item failed, so subsequent + # cross-object validators (e.g. rack space checks) see a realistic state. + # 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, 'status': 'ok'}) @@ -190,7 +193,10 @@ class SequentialBulkCreatesMixin: failed_count = sum(1 for r in results if r['status'] == 'error') return Response( { - 'detail': f'{failed_count} of {len(results)} objects failed validation.', + 'detail': _('{failed_count} of {total} objects failed validation.').format( + failed_count=failed_count, + total=len(results), + ), 'results': results, }, status=status.HTTP_400_BAD_REQUEST, @@ -253,7 +259,10 @@ class BulkUpdateModelMixin: failed_count = sum(1 for r in results if r['status'] == 'error') return Response( { - 'detail': f'{failed_count} of {len(results)} objects failed validation.', + 'detail': _('{failed_count} of {total} objects failed validation.').format( + failed_count=failed_count, + total=len(results), + ), 'results': results, }, status=status.HTTP_400_BAD_REQUEST, @@ -349,11 +358,14 @@ class BulkDestroyModelMixin: results = self.perform_bulk_destroy(qs, changelog_messages) - if results and any(r['status'] == 'error' for r in results): + if any(r['status'] == 'error' for r in results): failed_count = sum(1 for r in results if r['status'] == 'error') return Response( { - 'detail': f'{failed_count} of {len(results)} objects could not be deleted.', + 'detail': _('{failed_count} of {total} objects could not be deleted.').format( + failed_count=failed_count, + total=len(results), + ), 'results': results, }, status=status.HTTP_409_CONFLICT, @@ -378,13 +390,16 @@ class BulkDestroyModelMixin: e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects ) n = len(protected) - objects_str = ', '.join(f'{o} ({o.pk})' for o in protected[:10]) - if n > 10: - objects_str += f', and {n - 10} more' + # Report only the count — not names or PKs — to avoid exposing objects + # the caller may not have permission to view. results.append({ 'id': pk, 'status': 'error', - 'errors': {'detail': f'Unable to delete. {n} dependent object(s): {objects_str}'}, + 'errors': { + 'detail': _( + 'Unable to delete: {n} dependent object(s) prevent deletion.' + ).format(n=n), + }, }) if any(r['status'] == 'error' for r in results): transaction.set_rollback(True) From 3c77972f59d9b0da5a84ab2e08311391990655f7 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 9 Jul 2026 15:08:00 -0400 Subject: [PATCH 5/9] Address review feedback on bulk operation mixins - Move single-object create back inside transaction.atomic() (comment 1) - Replace repeated result-list iterations with local error_count counters in create(), perform_bulk_update(), and perform_bulk_destroy() (comments 3, 4, 6) - Rewrite perform_bulk_update() from two-pass (validate-all, save-all) to sequential per-object validate+save, matching SequentialBulkCreatesMixin; subsequent validators now see DB state from prior saves so cross-object uniqueness conflicts are caught at validation time (comment 5) - Update bulk_update() and bulk_destroy() callers to unpack new return tuples and use the counters directly Co-Authored-By: Claude Sonnet 4.6 --- netbox/netbox/api/viewsets/mixins.py | 67 +++++++++++++--------------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 99be26087..c1a3145f6 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -165,15 +165,16 @@ class SequentialBulkCreatesMixin: if (response := handle_background(request, 'create')) is not None: return response - if not isinstance(request.data, list): - # Creating a single object - return super().create(request, *args, **kwargs) - # 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 = [] 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) + for i, data in enumerate(request.data): serializer = self.get_serializer(data=data) if serializer.is_valid(): @@ -185,16 +186,16 @@ class SequentialBulkCreatesMixin: results.append({'index': i, 'status': 'ok'}) else: results.append({'index': i, 'status': 'error', 'errors': serializer.errors}) + error_count += 1 - if any(r['status'] == 'error' for r in results): + if error_count: transaction.set_rollback(True) - if any(r['status'] == 'error' for r in results): - failed_count = sum(1 for r in results if r['status'] == 'error') + if error_count: return Response( { 'detail': _('{failed_count} of {total} objects failed validation.').format( - failed_count=failed_count, + failed_count=error_count, total=len(results), ), 'results': results, @@ -251,16 +252,13 @@ class BulkUpdateModelMixin: obj.pop('id'): obj for obj in request.data } - object_pks, results = self.perform_bulk_update(qs, update_data, partial=partial) + object_pks, results, error_count = self.perform_bulk_update(qs, update_data, partial=partial) - # perform_bulk_update returns an empty list on full success; non-empty means at least one - # object failed validation and the full results list (with per-object status) is populated. - if results: - failed_count = sum(1 for r in results if r['status'] == 'error') + if error_count: return Response( { 'detail': _('{failed_count} of {total} objects failed validation.').format( - failed_count=failed_count, + failed_count=error_count, total=len(results), ), 'results': results, @@ -277,30 +275,26 @@ class BulkUpdateModelMixin: def perform_bulk_update(self, objects, update_data, partial): updated_pks = [] results = [] + error_count = 0 with transaction.atomic(using=router.db_for_write(self.queryset.model)): - # Pass 1: validate all objects without writing to the database - prepared = [] + # 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 + # will fail validation rather than raising an integrity error on save). for obj in objects: data = update_data.get(obj.id) if hasattr(obj, 'snapshot'): obj.snapshot() serializer = self.get_serializer(obj, data=data, partial=partial) - prepared.append((obj, serializer, serializer.is_valid())) - - if any(not valid for _, _, valid in prepared): - results = [ - {'id': obj.pk, 'status': 'error', 'errors': ser.errors} if not valid - else {'id': obj.pk, 'status': 'ok'} - for obj, ser, valid in prepared - ] - transaction.set_rollback(True) - else: - # Pass 2: all objects are valid — perform updates - for obj, serializer, _ in prepared: + if serializer.is_valid(): self.perform_update(serializer) updated_pks.append(obj.pk) - - return updated_pks, results + results.append({'id': obj.pk, 'status': 'ok'}) + else: + results.append({'id': obj.pk, 'status': 'error', 'errors': serializer.errors}) + error_count += 1 + if error_count: + transaction.set_rollback(True) + return updated_pks, results, error_count def get_bulk_update_serializer_class(self, *, partial=False): return get_bulk_update_serializer_class( @@ -356,14 +350,13 @@ class BulkDestroyModelMixin: o['id']: o.get('changelog_message') for o in serializer.validated_data } - results = self.perform_bulk_destroy(qs, changelog_messages) + results, error_count = self.perform_bulk_destroy(qs, changelog_messages) - if any(r['status'] == 'error' for r in results): - failed_count = sum(1 for r in results if r['status'] == 'error') + if error_count: return Response( { 'detail': _('{failed_count} of {total} objects could not be deleted.').format( - failed_count=failed_count, + failed_count=error_count, total=len(results), ), 'results': results, @@ -376,6 +369,7 @@ class BulkDestroyModelMixin: def perform_bulk_destroy(self, objects, changelog_messages=None): changelog_messages = changelog_messages or {} results = [] + error_count = 0 with transaction.atomic(using=router.db_for_write(self.queryset.model)): for obj in objects: if hasattr(obj, 'snapshot'): @@ -401,9 +395,10 @@ class BulkDestroyModelMixin: ).format(n=n), }, }) - if any(r['status'] == 'error' for r in results): + error_count += 1 + if error_count: transaction.set_rollback(True) - return results + return results, error_count class ObjectValidationMixin: From 48e08779d1c2fecf3d7fac0d1181a75c6c1c7707 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 9 Jul 2026 15:24:09 -0400 Subject: [PATCH 6/9] Drop explicit status key from bulk operation results Success is now inferred from the absence of an errors key, matching Jeremy's suggestion. Error entries carry only {id/index, errors}; successful entries carry only {id/index}. Update all tests accordingly. Co-Authored-By: Claude Sonnet 4.6 --- netbox/dcim/tests/test_api.py | 14 ++++++-------- netbox/netbox/api/viewsets/mixins.py | 11 +++++------ netbox/utilities/testing/api.py | 3 +-- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index a9797b59e..be4de06eb 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -497,10 +497,9 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): 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 - self.assertEqual(results_by_id[site1.pk]['status'], 'ok') - # Site 2 (has Device) should have failed - self.assertEqual(results_by_id[site2.pk]['status'], 'error') + # 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]) # Verify that no sites were actually deleted (transaction rolled back) @@ -2233,12 +2232,11 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): self.assertIn('detail', response.data) self.assertIn('results', response.data) self.assertEqual(len(response.data['results']), 2) - # First item passed validation + # First item passed validation — no errors key self.assertEqual(response.data['results'][0]['index'], 0) - self.assertEqual(response.data['results'][0]['status'], 'ok') - # Second item failed validation + self.assertNotIn('errors', response.data['results'][0]) + # Second item failed validation — errors key present self.assertEqual(response.data['results'][1]['index'], 1) - self.assertEqual(response.data['results'][1]['status'], 'error') self.assertIn('errors', response.data['results'][1]) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index c1a3145f6..0ad7c5295 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -183,9 +183,9 @@ 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, 'status': 'ok'}) + results.append({'index': i}) else: - results.append({'index': i, 'status': 'error', 'errors': serializer.errors}) + results.append({'index': i, 'errors': serializer.errors}) error_count += 1 if error_count: @@ -288,9 +288,9 @@ class BulkUpdateModelMixin: if serializer.is_valid(): self.perform_update(serializer) updated_pks.append(obj.pk) - results.append({'id': obj.pk, 'status': 'ok'}) + results.append({'id': obj.pk}) else: - results.append({'id': obj.pk, 'status': 'error', 'errors': serializer.errors}) + results.append({'id': obj.pk, 'errors': serializer.errors}) error_count += 1 if error_count: transaction.set_rollback(True) @@ -378,7 +378,7 @@ class BulkDestroyModelMixin: pk = obj.pk # Django sets obj.pk = None after deletion; capture it first try: self.perform_destroy(obj) - results.append({'id': pk, 'status': 'ok'}) + results.append({'id': pk}) except (ProtectedError, RestrictedError) as e: protected = list( e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects @@ -388,7 +388,6 @@ class BulkDestroyModelMixin: # the caller may not have permission to view. results.append({ 'id': pk, - 'status': 'error', 'errors': { 'detail': _( 'Unable to delete: {n} dependent object(s) prevent deletion.' diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 99ea69a39..1a651b163 100644 --- a/netbox/utilities/testing/api.py +++ b/netbox/utilities/testing/api.py @@ -579,9 +579,8 @@ class APIViewTestCases: self.assertIn('results', response.data) self.assertEqual(len(response.data['results']), 2) self.assertEqual(response.data['results'][0]['id'], id_list[0]) - self.assertEqual(response.data['results'][0]['status'], 'ok') + self.assertNotIn('errors', response.data['results'][0]) self.assertEqual(response.data['results'][1]['id'], id_list[1]) - self.assertEqual(response.data['results'][1]['status'], 'error') self.assertIn('errors', response.data['results'][1]) # Verify atomicity: object 0 passed validation but must not have been modified From b61c232305a34374f2aa5cc27e41fe6ee157aa96 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Tue, 14 Jul 2026 14:19:08 -0400 Subject: [PATCH 7/9] 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]) From 3df0bc8e62bf10c969eae6ac05455f024e0a0599 Mon Sep 17 00:00:00 2001 From: bctiemann Date: Tue, 14 Jul 2026 22:12:28 -0400 Subject: [PATCH 8/9] Update netbox/netbox/api/viewsets/mixins.py Co-authored-by: Jeremy Stretch --- netbox/netbox/api/viewsets/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 336630e11..85b140b82 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -389,7 +389,7 @@ class BulkDestroyModelMixin: 'errors': { '__all__': _( 'Unable to delete: {n} dependent object(s) prevent deletion.' - ).format(n=n), + ).format(n=len(protected)), }, }) if errors: From 63984e693ca9014fccbf699d9fb2251b8def38ed Mon Sep 17 00:00:00 2001 From: bctiemann Date: Tue, 14 Jul 2026 22:13:05 -0400 Subject: [PATCH 9/9] Update netbox/netbox/api/viewsets/mixins.py Co-authored-by: Jeremy Stretch --- netbox/netbox/api/viewsets/mixins.py | 1 - 1 file changed, 1 deletion(-) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 85b140b82..0f36f2b34 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -378,7 +378,6 @@ class BulkDestroyModelMixin: 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 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