From 3c77972f59d9b0da5a84ab2e08311391990655f7 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 9 Jul 2026 15:08:00 -0400 Subject: [PATCH] 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: