From d6ba2ae51e8f1ce322f1055785a34995c215bdc7 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 12 Aug 2026 13:48:34 -0400 Subject: [PATCH] Misc cleanup --- netbox/netbox/api/viewsets/mixins.py | 84 +++++++--------------- netbox/netbox/tests/test_api_background.py | 37 ++++++++++ 2 files changed, 62 insertions(+), 59 deletions(-) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 094d65c18..ddddbb573 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -1,4 +1,3 @@ -import warnings from collections import Counter from contextlib import contextmanager @@ -32,7 +31,6 @@ __all__ = ( 'CustomFieldsMixin', 'ExportTemplatesMixin', 'ObjectValidationMixin', - 'SequentialBulkCreatesMixin', 'discard_events_on_rollback', 'get_duplicate_objects_response', 'get_invalid_entries_response', @@ -51,9 +49,6 @@ BULK_ERROR_STATUSES = ( status.HTTP_400_BAD_REQUEST, ) -# Reported for an object whose write was undone because the object it produced falls outside the -# queryset permitted to the requesting user (see ObjectValidationMixin._validate_objects). Which -# constraint was violated is deliberately not disclosed, consistent with the single-object endpoints. PERMISSION_DENIED_MESSAGE = _("You do not have permission to perform this action on this object.") @@ -106,8 +101,7 @@ def get_non_list_response(data): def _as_field_errors(item_errors): """ - Return the errors reported for one entry of a bulk request as a mapping of field name to - messages. + Return the errors reported for one entry of a bulk request as a mapping of field name to messages. """ if isinstance(item_errors, dict): return item_errors @@ -321,9 +315,11 @@ class BackgroundOperationMixin: raise RQWorkerNotRunningException() model = self.queryset.model - verb = _("delete") if action == 'bulk_destroy' else ( - _("create") if action == 'create' else _("update") - ) + verb = { + 'create': _("create"), + 'bulk_create': _("create"), + 'bulk_destroy': _("delete"), + }.get(action, _("update")) job_name = _("Bulk {verb} {object_type}").format( verb=verb, object_type=model._meta.verbose_name_plural, @@ -406,6 +402,12 @@ class BulkCreateModelMixin: ] """ def bulk_create(self, request, *args, **kwargs): + # If background processing was requested, enqueue a job and return immediately (before + # any validation, which is deferred to the worker). + handle_background = getattr(self, '_handle_background_request', lambda *a, **kw: None) + if (response := handle_background(request, 'bulk_create')) is not None: + return response + created_pks, errors, error_status = self.perform_bulk_create(request.data) if errors: @@ -433,27 +435,20 @@ class BulkCreateModelMixin: Validate and create each of the given objects, rolling the entire batch back if any one of them could not be created. - Returns the PKs of the objects created, the per-object errors, and the status code with - which to report them (None if there were none). See resolve_bulk_error_status(). + Returns the PKs of the objects created, the per-object errors (if any), and the status code + with which to report them (None if there were none). """ created_pks = [] errors = [] error_statuses = set() 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. + # 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. 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. This mirrors - # the message REST framework itself reports for a non-dictionary item, hence its - # key rather than a literal -- which NON_FIELD_ERRORS_KEY is configured to match - # anyway, so that the API has a single key for non-field errors (see settings). + # 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': { @@ -472,23 +467,18 @@ class BulkCreateModelMixin: error_statuses.add(status.HTTP_400_BAD_REQUEST) 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. + # 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. + # Raised by a signal receiver rather than by validation (e.g. assigning a tag which is restricted + # to other object types). errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}}) error_statuses.add(status.HTTP_400_BAD_REQUEST) except PermissionDenied: - # Raised by perform_create() when the object it saved falls outside the queryset - # permitted to the requesting user. Reported per object so that the offending - # entry is named, but still as a 403, which is what the single-object endpoint - # returns for the same rejection. + # Raised by perform_create() when the object it saved falls outside the queryset permitted to the + # requesting user. Reported per object so that the offending entry is named, but still as a 403, + # which is what the single-object endpoint returns for the same rejection. errors.append({'index': i, 'errors': {'__all__': [PERMISSION_DENIED_MESSAGE]}}) error_statuses.add(status.HTTP_403_FORBIDDEN) else: @@ -498,27 +488,6 @@ class BulkCreateModelMixin: return created_pks, errors, resolve_bulk_error_status(error_statuses) -# 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: """ Support bulk modification of objects using the list endpoint for a model. Accepts a PATCH action with a list of one @@ -593,9 +562,6 @@ class BulkUpdateModelMixin: { 'detail': _('{failed_count} of {total} objects could not be updated.').format( failed_count=len(errors), - # Every object named was matched and attempted, the duplicate and missing-ID - # checks above having rejected the batch otherwise, so this equals the number - # of objects submitted. total=len(object_pks) + len(errors), ), 'errors': errors, diff --git a/netbox/netbox/tests/test_api_background.py b/netbox/netbox/tests/test_api_background.py index f562d92ec..76def67f7 100644 --- a/netbox/netbox/tests/test_api_background.py +++ b/netbox/netbox/tests/test_api_background.py @@ -14,10 +14,12 @@ from unittest.mock import patch from django.contrib.contenttypes.models import ContentType from django.test import RequestFactory from rest_framework import status +from rest_framework.test import APIRequestFactory from core.choices import JobStatusChoices from core.exceptions import JobFailed from core.models import Job, ObjectChange +from dcim.api.views import RegionViewSet from dcim.models import DeviceType, Manufacturer, Region from users.models import ObjectPermission from utilities.request import copy_safe_request @@ -120,6 +122,41 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase): self.assertTrue(job.error) self.assertFalse(Region.objects.filter(slug='region-a').exists()) + def test_background_bulk_create_direct_invocation(self): + """ + bulk_create() honors ?background=true itself, as bulk_update() and bulk_destroy() do, so a + caller which reaches it without passing through NetBoxModelViewSet.create() (e.g. a custom + viewset) still gets background processing rather than a synchronous write. + """ + self.grant('add', 'view') + payload = [{'name': 'Region A', 'slug': 'region-a'}] + + # Apply the same minimal scaffolding as AsyncAPIJob does when it invokes an action directly + viewset = RegionViewSet() + viewset.action_map = {'post': 'bulk_create'} + viewset.kwargs = {} + viewset.args = () + viewset.format_kwarg = None + request = viewset.initialize_request( + APIRequestFactory().post('/api/dcim/regions/?background=true', payload, format='json') + ) + request.user = self.user + request.id = uuid.uuid4() # Ordinarily set by NetBox's middleware; recorded on the changelog + viewset.request = request + + response = viewset.bulk_create(request) + + self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED) + job = Job.objects.get(pk=response.data['job']['id']) + self.assertEqual(job.name, 'Bulk create regions') + + # The worker re-invokes this same action against a request carrying no query string, so the + # work is performed there rather than being enqueued a second time + self.assertEqual(job.status, JobStatusChoices.STATUS_COMPLETED) + self.assertEqual(job.data['status_code'], status.HTTP_201_CREATED) + self.assertTrue(Region.objects.filter(slug='region-a').exists()) + self.assertEqual(Job.objects.count(), 1) + # ------------------------------------------------------------------ update def test_background_bulk_update_patch(self):