diff --git a/docs/integrations/rest-api.md b/docs/integrations/rest-api.md index 6005cdb7f..c31d58212 100644 --- a/docs/integrations/rest-api.md +++ b/docs/integrations/rest-api.md @@ -592,6 +592,9 @@ http://netbox/api/dcim/sites/ \ ] ``` +!!! note + The bulk creation of objects is an all-or-none operation, meaning that if NetBox fails to successfully create any of the specified objects (e.g. due to a validation error), the entire operation will be aborted and none of the objects will be created. + ### Updating an Object To modify an object which has already been created, make a `PATCH` request to the model's _detail_ endpoint specifying its unique numeric ID. Include any data which you wish to update on the object. As with object creation, the `Authorization` and `Content-Type` headers must also be specified. diff --git a/netbox/dcim/api/views.py b/netbox/dcim/api/views.py index e02ea7cf6..79dc4b6dc 100644 --- a/netbox/dcim/api/views.py +++ b/netbox/dcim/api/views.py @@ -17,7 +17,6 @@ from netbox.api.authentication import IsAuthenticatedOrLoginNotRequired from netbox.api.metadata import ContentTypeMetadata from netbox.api.pagination import StripCountAnnotationsPaginator from netbox.api.viewsets import NetBoxModelViewSet, NetBoxReadOnlyModelViewSet -from netbox.api.viewsets.mixins import SequentialBulkCreatesMixin from utilities.api import get_serializer_for_model from utilities.query import count_related from utilities.query_functions import CollateAsChar @@ -428,7 +427,7 @@ class PlatformViewSet(NetBoxModelViewSet): # Devices/modules # -class DeviceViewSet(SequentialBulkCreatesMixin, ConfigContextQuerySetMixin, RenderConfigMixin, NetBoxModelViewSet): +class DeviceViewSet(ConfigContextQuerySetMixin, RenderConfigMixin, NetBoxModelViewSet): queryset = Device.objects.prefetch_related( 'device_type__manufacturer', # Referenced by Device.__str__() for unnamed devices 'parent_bay', # Referenced by DeviceSerializer.get_parent_device() diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index f76f92a71..406803d91 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -467,6 +467,33 @@ 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_create_objects_conflicting(self): + """ + POST a set of objects in which two conflict with one another. Objects are created one at a + time, so the second must fail validation against the first rather than passing validation + and then raising an IntegrityError on save. + """ + 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() + data = [ + {'name': 'Site 10', 'slug': 'site-10'}, + {'name': 'Site 11', 'slug': 'site-11'}, + {'name': 'Site 10', 'slug': 'site-10'}, # Duplicates the first item + ] + response = self.client.post(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + self.assertEqual(self._get_queryset().count(), initial_count) + + # Only the third item failed; the first two were provisionally created and rolled back + self.assertEqual(len(response.data['errors']), 1) + self.assertEqual(response.data['errors'][0]['index'], 2) + self.assertIn('slug', response.data['errors'][0]['errors']) + def test_bulk_delete_objects_protected(self): """ DELETE a set of objects where one has a protected FK dependency. Verify the structured @@ -2383,10 +2410,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 report only the - failed object, and no objects should be created despite the first item passing - (atomic rollback). + POST a set of Device objects where the first passes and the second fails validation. 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() diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 42814a91d..b5a25d9ae 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -306,7 +306,7 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase): role = DeviceRole.objects.create(name='Device Role 1', slug='device-role-1') site = Site.objects.create(name='Site 1', slug='site-1') - # DeviceViewSet uses SequentialBulkCreatesMixin, so each valid object is provisionally + # Bulk creates are performed one object at a time, so each valid object is provisionally # created (and its event queued) before a later object fails validation. event_rule = EventRule.objects.get(name='Event Rule 1') event_rule.object_types.set([ObjectType.objects.get_for_model(Device)]) diff --git a/netbox/netbox/api/viewsets/__init__.py b/netbox/netbox/api/viewsets/__init__.py index 7f82a141d..c814fbf47 100644 --- a/netbox/netbox/api/viewsets/__init__.py +++ b/netbox/netbox/api/viewsets/__init__.py @@ -152,6 +152,7 @@ class NetBoxReadOnlyModelViewSet( class NetBoxModelViewSet( ETagMixin, mixins.BackgroundOperationMixin, + mixins.BulkCreateModelMixin, mixins.BulkUpdateModelMixin, mixins.BulkDestroyModelMixin, mixins.ObjectValidationMixin, @@ -250,31 +251,27 @@ class NetBoxModelViewSet( if (response := self._handle_background_request(request, 'create')) is not None: return response + # Creating multiple objects, which are validated and saved one at a time in order to + # collect per-object errors (see BulkCreateModelMixin) + if isinstance(request.data, list): + return self.bulk_create(request, *args, **kwargs) + serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) - bulk_create = getattr(serializer, 'many', False) self.perform_create(serializer) - # After creating the instance(s), re-initialize the serializer with a queryset + # After creating the instance, re-initialize the serializer with a queryset # to ensure related objects are prefetched. - if bulk_create: - instance_pks = [obj.pk for obj in serializer.instance] - # Order by PK to ensure that the ordering of objects in the response - # matches the ordering of those in the request. - qs = self.get_queryset().filter(pk__in=instance_pks).order_by('pk') - else: - qs = self.get_queryset().get(pk=serializer.instance.pk) + qs = self.get_queryset().get(pk=serializer.instance.pk) - # Re-serialize the instance(s) with prefetched data - serializer = self.get_serializer(qs, many=bulk_create) + # Re-serialize the instance with prefetched data + serializer = self.get_serializer(qs) headers = self.get_success_headers(serializer.data) response = Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers) - # Add ETag for single-object creation only (bulk returns a list, no single ETag) - if not bulk_create: - if etag := self._get_etag(qs): - response['ETag'] = etag + if etag := self._get_etag(qs): + response['ETag'] = etag return response diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 0f4933094..e31cbcc7e 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -1,3 +1,4 @@ +import warnings from contextlib import contextmanager from django.core.exceptions import ObjectDoesNotExist @@ -9,6 +10,7 @@ from rest_framework import status from rest_framework.exceptions import ValidationError from rest_framework.response import Response from rest_framework.reverse import reverse +from rest_framework.settings import api_settings from core.models import ObjectType from core.signals import clear_events @@ -22,6 +24,7 @@ from utilities.rqworker import any_workers_for_queue __all__ = ( 'BackgroundOperationMixin', + 'BulkCreateModelMixin', 'BulkDestroyModelMixin', 'BulkUpdateModelMixin', 'CustomFieldsMixin', @@ -232,64 +235,108 @@ class ExportTemplatesMixin: return super().list(request, *args, **kwargs) -class SequentialBulkCreatesMixin: +class BulkCreateModelMixin: """ - Perform bulk creation of new objects sequentially, rather than all at once. This ensures that any validation - which depends on the evaluation of existing objects (such as checking for free space within a rack) functions - appropriately. + Support the creation of multiple objects using the list endpoint for a model. Accepts a POST action with a list + of one or more JSON objects, each specifying the attributes of an object to be created. For example: + + POST /api/dcim/sites/ + [ + {"name": "Site 1", "slug": "site-1"}, + {"name": "Site 2", "slug": "site-2"} + ] """ - def create(self, request, *args, **kwargs): - # If background processing was requested for a bulk (list) create, enqueue a job and - # return immediately. _handle_background_request() comes from BackgroundOperationMixin; - # fall back to "no background" so this mixin remains usable on its own (e.g. in custom - # viewsets). - handle_background = getattr(self, '_handle_background_request', lambda *a, **kw: None) - if (response := handle_background(request, 'create')) is not None: - return response - - # 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. - errors = [] - return_data = [] - using = router.db_for_write(self.queryset.model) - with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): - 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 not serializer.is_valid(): - errors.append({'index': i, 'errors': serializer.errors}) - continue - try: - # 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) - except AbortRequest as e: - errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}}) - else: - return_data.append(serializer.data) - - if errors: - transaction.set_rollback(True) + def bulk_create(self, request, *args, **kwargs): + created_pks, errors = self.perform_bulk_create(request.data) if errors: return Response( { 'detail': _('{failed_count} of {total} objects failed validation.').format( failed_count=len(errors), - total=total, + total=len(request.data), ), 'errors': errors, }, 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) + # Re-fetch the new objects to serialize them with their related objects prefetched. Order by PK + # to ensure that the ordering of objects in the response matches the ordering of those in the + # request (the objects were created in the order given, so PK order is request order). + qs = self.get_queryset().filter(pk__in=created_pks).order_by('pk') + serializer = self.get_serializer(qs, many=True) + + return Response(serializer.data, status=status.HTTP_201_CREATED) + + def perform_bulk_create(self, data): + created_pks = [] + errors = [] + using = router.db_for_write(self.queryset.model) + with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): + # Validate and save each object in turn, rather than validating the entire batch up front, + # so that validation which depends on the state left by prior saves is evaluated correctly. + # This covers both validation against other existing objects (e.g. checking for free space + # within a rack) and uniqueness: two objects in one batch which conflict with one another + # would otherwise both validate against the pre-batch state and then fail on save, raising + # an unhandled IntegrityError. + for i, item in enumerate(data): + if not isinstance(item, dict): + # Checked explicitly because get_serializer() infers many=True from a list, so a + # nested list would otherwise be validated as a batch of its own + errors.append({ + 'index': i, + 'errors': { + api_settings.NON_FIELD_ERRORS_KEY: [ + _('Invalid data. Expected a dictionary, but got {datatype}.').format( + datatype=type(item).__name__ + ), + ], + }, + }) + continue + serializer = self.get_serializer(data=item) + if not serializer.is_valid(): + errors.append({'index': i, 'errors': serializer.errors}) + continue + try: + # Provisionally create even when a prior item failed, so subsequent + # cross-object validators see a realistic state. All creates are rolled + # back together if any item in the batch fails. + self.perform_create(serializer) + except AbortRequest as e: + # Raised by a signal receiver rather than by validation (e.g. assigning a tag + # which is restricted to other object types). perform_create() wraps its write + # in its own atomic block, so the connection is rolled back to that savepoint + # and the remaining objects in the batch can still be evaluated. The message is + # coerced to a string because a few receivers pass an exception rather than text. + errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}}) + else: + created_pks.append(serializer.instance.pk) + if errors: + transaction.set_rollback(True) + return created_pks, errors + + +# TODO: Remove this in NetBox v5.0 +class SequentialBulkCreatesMixin: + """ + Deprecated no-op mixin retained for backward compatibility. + + Historically this was applied to individual ViewSets to make their bulk creates run one object + at a time. All ViewSets derived from NetBoxModelViewSet now do this unconditionally (see + BulkCreateModelMixin), so this mixin is a transparent pass-through and may be removed in a + future release. Plugins should stop inheriting from it. + """ + + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + warnings.warn( + "SequentialBulkCreatesMixin is deprecated and no longer does anything; all bulk " + f"creates are now performed sequentially. Remove it from {cls.__name__}.", + DeprecationWarning, + stacklevel=2, + ) class BulkUpdateModelMixin: diff --git a/netbox/utilities/testing/api.py b/netbox/utilities/testing/api.py index 61e6b7ddb..a978c306e 100644 --- a/netbox/utilities/testing/api.py +++ b/netbox/utilities/testing/api.py @@ -388,6 +388,40 @@ class APIViewTestCases: self.assertObjectChange(oc, action=ObjectChangeActionChoices.ACTION_CREATE, message=changelog_message) + def test_bulk_create_objects_invalid_item(self): + """ + POST a set of objects in which one item is invalid. The failure must be correlated to + that item's position in the request, and the entire batch must be rolled back. + """ + 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() + + # A non-dictionary is used as the invalid item because it is guaranteed to fail for every + # model, whereas which *fields* are required varies from one model to the next. + response = self.client.post( + self._get_list_url(), + [self.create_data[0], 'this is not an object'], + 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 sibling fails validation' + ) + self.assertIn('detail', response.data) + self.assertEqual(len(response.data['errors']), 1) + self.assertEqual(response.data['errors'][0]['index'], 1) + self.assertIn('errors', response.data['errors'][0]) + class UpdateObjectViewTestCase(APITestCase): update_data = {} bulk_update_data = None