RomanNum3ral
/
netbox
mirror of https://github.com/netbox-community/netbox.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Brian Tiemann 2026-07-14 14:19:08 -04:00
parent 48e08779d1
commit b61c232305
3 changed files with 51 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
netbox/dcim/tests/test_api.py
View File

@ -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):

71
netbox/netbox/api/viewsets/mixins.py
View File

@ -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:

10
netbox/utilities/testing/api.py
View File

@ -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])