diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index ea19b98f6..f76f92a71 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -1,7 +1,7 @@ import json from django.conf import settings -from django.test import tag +from django.test import override_settings, tag from django.urls import reverse from django.utils.translation import gettext as _ from rest_framework import status @@ -553,6 +553,73 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): 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') + def test_bulk_update_objects_abort_request(self): + """ + PATCH a set of objects where a signal receiver raises AbortRequest for more than one of + them. Each failure must be correlated to its own object (proving the batch continues past + the first abort) and no object may be modified. + """ + # This tag may only be assigned to Regions, so assigning it to a Site raises AbortRequest + # from extras.signals.validate_assigned_tags. + restricted_tag = Tag.objects.create(name='Regions Only', slug='regions-only') + restricted_tag.object_types.set([ObjectType.objects.get_for_model(Region)]) + + site1 = Site.objects.get(slug='site-1') + site2 = Site.objects.get(slug='site-2') + + 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)) + self.add_permissions('extras.view_tag') + + data = [ + {'id': site1.pk, 'tags': [{'name': 'Regions Only'}]}, + {'id': site2.pk, 'tags': [{'name': 'Regions Only'}]}, + ] + 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.assertEqual([e['id'] for e in response.data['errors']], [site1.pk, site2.pk]) + for error in response.data['errors']: + # Reported as a non-field error, in the same list-of-messages form as field errors + self.assertIsInstance(error['errors']['__all__'], list) + + # Neither site may have been tagged (whole batch rolled back) + self.assertFalse(site1.tags.exists(), 'Site 1 should not have been tagged') + self.assertFalse(site2.tags.exists(), 'Site 2 should not have been tagged') + + def test_bulk_delete_objects_abort_request(self): + """ + DELETE a set of objects where a protection rule blocks more than one of them. Each failure + must be correlated to its own object and no object may be deleted. + """ + site1 = Site.objects.get(slug='site-1') + site2 = Site.objects.get(slug='site-2') + + 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)) + + # Neither site has a description, so the rule blocks both deletions via AbortRequest raised + # from core.signals.handle_deleted_object. + protection_rules = {'dcim.site': [{'description': {'required': True}}]} + data = [{'id': site1.pk}, {'id': site2.pk}] + with override_settings(PROTECTION_RULES=protection_rules): + 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.assertEqual([e['id'] for e in response.data['errors']], [site1.pk, site2.pk]) + for error in response.data['errors']: + self.assertIsInstance(error['errors']['__all__'], list) + + # Neither site may have been deleted + 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 @@ -2347,6 +2414,41 @@ class DeviceTestCase(APIViewTestCases.APIViewTestCase): self.assertEqual(response.data['errors'][0]['index'], 1) self.assertIn('errors', response.data['errors'][0]) + def test_bulk_create_objects_abort_request(self): + """ + POST a set of Device objects where a signal receiver raises AbortRequest for more than one + of them. Each failure must be correlated to its position in the request (proving the batch + continues past the first abort) and no object may be created. + """ + # This tag may only be assigned to Regions, so assigning it to a Device raises AbortRequest + # from extras.signals.validate_assigned_tags. + restricted_tag = Tag.objects.create(name='Regions Only', slug='regions-only') + restricted_tag.object_types.set([ObjectType.objects.get_for_model(Region)]) + + 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_permissions('extras.view_tag') + + initial_count = self._get_queryset().count() + data = [ + {**self.create_data[0], 'tags': [{'name': 'Regions Only'}]}, + {**self.create_data[1], 'tags': [{'name': 'Regions Only'}]}, + ] + response = self.client.post(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + self.assertIn('detail', response.data) + self.assertEqual([e['index'] for e in response.data['errors']], [0, 1]) + for error in response.data['errors']: + self.assertIsInstance(error['errors']['__all__'], list) + + self.assertEqual( + self._get_queryset().count(), initial_count, + 'No objects should be created when any sibling is aborted', + ) + class ModuleTestCase(APIViewTestCases.APIViewTestCase): model = Module diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index 1132ad2d1..42814a91d 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -599,9 +599,9 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase): def test_bulk_delete_abort_discards_events(self): """ - Check that a bulk delete aborted by an exception (rather than by a per-object error) also - queues no background tasks. A protection rule raises AbortRequest from a signal receiver, - which propagates out of the per-object loop. + Check that a bulk delete blocked by a signal receiver raising AbortRequest (rather than by a + database constraint) also queues no background tasks for the objects that were provisionally + deleted before the failure. """ sites = ( Site(name='Site 1', slug='site-1', description='Has a description'), @@ -617,9 +617,12 @@ class EventRuleTestCase(RQQueueTestMixin, APITestCase): protection_rules = {'dcim.site': [{'description': {'required': True}}]} with override_settings(PROTECTION_RULES=protection_rules): response = self.client.delete(url, data, format='json', **self.header) - self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + self.assertHttpStatus(response, status.HTTP_409_CONFLICT) self.assertEqual(Site.objects.count(), 2) + # The failure is correlated to the blocked object only + self.assertEqual([e['id'] for e in response.data['errors']], [sites[1].pk]) + # No task may be queued for a deletion that was rolled back self.assertEqual(self.queue.count, 0) diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index 8cc45fb0a..0f4933094 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -16,7 +16,7 @@ from extras.models import ExportTemplate from netbox.api.serializers import BulkOperationSerializer from netbox.api.serializers.bulk import get_bulk_update_serializer_class from netbox.jobs import AsyncAPIJob -from utilities.exceptions import RQWorkerNotRunningException +from utilities.exceptions import AbortRequest, RQWorkerNotRunningException from utilities.request import copy_safe_request from utilities.rqworker import any_workers_for_queue @@ -260,14 +260,18 @@ class SequentialBulkCreatesMixin: total = len(request.data) for i, data in enumerate(request.data): serializer = self.get_serializer(data=data) - if serializer.is_valid(): + 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) - return_data.append(serializer.data) + except AbortRequest as e: + errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}}) else: - errors.append({'index': i, 'errors': serializer.errors}) + return_data.append(serializer.data) if errors: transaction.set_rollback(True) @@ -370,11 +374,20 @@ class BulkUpdateModelMixin: if hasattr(obj, 'snapshot'): obj.snapshot() serializer = self.get_serializer(obj, data=data, partial=partial) - if serializer.is_valid(): - self.perform_update(serializer) - updated_pks.append(obj.pk) - else: + if not serializer.is_valid(): errors.append({'id': obj.pk, 'errors': serializer.errors}) + continue + try: + self.perform_update(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_update() 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({'id': obj.pk, 'errors': {'__all__': [str(e.message)]}}) + else: + updated_pks.append(obj.pk) if errors: transaction.set_rollback(True) return updated_pks, errors @@ -479,11 +492,20 @@ class BulkDestroyModelMixin: errors.append({ 'id': pk, 'errors': { - '__all__': _( - 'Unable to delete: {n} dependent object(s) prevent deletion.' - ).format(n=len(protected)), + '__all__': [ + _('Unable to delete: {n} dependent object(s) prevent deletion.').format( + n=len(protected) + ), + ], }, }) + except AbortRequest as e: + # Raised by a signal receiver rather than by a database constraint (e.g. a + # PROTECTION_RULES violation caught in core.signals.handle_deleted_object). + # perform_destroy() wraps its delete 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. + errors.append({'id': pk, 'errors': {'__all__': [str(e.message)]}}) if errors: transaction.set_rollback(True) return errors, total