From a7971ee7d448d1f6730acd2a0ed72678113d98bf Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 11 Aug 2026 09:59:30 -0400 Subject: [PATCH] Correct behavior of returning a 400 vs. 409 --- netbox/core/api/schema.py | 9 +++--- netbox/dcim/tests/test_api.py | 41 +++++++++++++++++++++++-- netbox/extras/tests/test_event_rules.py | 4 ++- netbox/netbox/api/viewsets/mixins.py | 22 +++++++++++-- 4 files changed, 66 insertions(+), 10 deletions(-) diff --git a/netbox/core/api/schema.py b/netbox/core/api/schema.py index eac66b62e..d69f4a92f 100644 --- a/netbox/core/api/schema.py +++ b/netbox/core/api/schema.py @@ -211,16 +211,17 @@ class NetBoxAutoSchema(AutoSchema): '400': OpenApiResponse( response=BulkOperationErrorSerializer, description=_( - "The request was malformed, or one or more of the objects specified could " - "not be found. No objects were deleted." + "The request was malformed, one or more of the objects specified could not " + "be found, or the deletion of one of them was prevented by a protection " + "rule. No objects were deleted." ), ), '409': OpenApiResponse( response=BulkOperationErrorSerializer, description=_( "One or more of the objects specified could not be deleted, because a " - "dependent object or a protection rule prevents it. No objects were " - "deleted: a bulk deletion is an all-or-none operation." + "dependent object prevents it. No objects were deleted: a bulk deletion is " + "an all-or-none operation." ), ), } diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 34be0da6d..6b93068ce 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -710,7 +710,9 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): 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. + must be correlated to its own object and no object may be deleted. A protection rule is a + rejection of the request rather than a conflict with the state of the database, so this + reports 400 -- as the single-object endpoint does for the same rule. """ site1 = Site.objects.get(slug='site-1') site2 = Site.objects.get(slug='site-2') @@ -727,7 +729,7 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): 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.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']: @@ -737,6 +739,41 @@ 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_delete_objects_conflict_and_abort_request(self): + """ + DELETE a set of objects where one is blocked by a dependent object and another by a + protection rule. Both failures must be reported, and the dependency conflict must determine + the status code: it is the failure which would remain were the request itself corrected. + """ + 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)) + + # Site 1 is blocked by a dependent Device (ProtectedError); Site 2 is blocked by the + # protection rule below, as it has no description (AbortRequest). Site 1 is given a + # description so that only one of the two failure modes applies to it. + create_test_device('Protected Device', site=site1) + site1.description = 'Has a description' + site1.save() + + 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.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 diff --git a/netbox/extras/tests/test_event_rules.py b/netbox/extras/tests/test_event_rules.py index b5a25d9ae..7d8819bed 100644 --- a/netbox/extras/tests/test_event_rules.py +++ b/netbox/extras/tests/test_event_rules.py @@ -617,7 +617,9 @@ 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_409_CONFLICT) + # 400 rather than 409: a protection rule rejects the request, it is not a conflict with a + # dependent object (see BulkDestroyModelMixin.bulk_destroy) + self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) self.assertEqual(Site.objects.count(), 2) # The failure is correlated to the blocked object only diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index ff419ec59..aaa33d21f 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -631,9 +631,14 @@ class BulkDestroyModelMixin: o['id']: o.get('changelog_message') for o in serializer.validated_data } - errors, total = self.perform_bulk_destroy(qs, changelog_messages) + errors, total, has_conflict = self.perform_bulk_destroy(qs, changelog_messages) if errors: + # A dependency conflict reports 409, as it is a conflict with the current state of the + # database; every other failure reports 400, as it is a rejection of the request. This + # matches the single-object endpoint, where dispatch() maps the same two exception + # classes to the same two status codes. Where a batch hit both, the conflict takes + # precedence: it is the failure which would remain were the request itself corrected. return Response( { 'detail': _('{failed_count} of {total} objects could not be deleted.').format( @@ -642,15 +647,25 @@ class BulkDestroyModelMixin: ), 'errors': errors, }, - status=status.HTTP_409_CONFLICT, + status=status.HTTP_409_CONFLICT if has_conflict else status.HTTP_400_BAD_REQUEST, ) return Response(status=status.HTTP_204_NO_CONTENT) def perform_bulk_destroy(self, objects, changelog_messages=None): + """ + Attempt to delete each of the given objects, rolling the entire batch back if any one of + them could not be deleted. + + Returns the per-object errors, the number of objects processed, and whether any of the + failures was a conflict with the current state of the database (a dependent object) rather + than a rejection of the request (a protection rule, or any other signal receiver raising + AbortRequest). The caller uses the last of these to select a status code. + """ changelog_messages = changelog_messages or {} errors = [] total = 0 + has_conflict = False using = router.db_for_write(self.queryset.model) with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): for obj in objects: @@ -662,6 +677,7 @@ class BulkDestroyModelMixin: try: self.perform_destroy(obj) except (ProtectedError, RestrictedError) as e: + has_conflict = True protected = list( e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects ) @@ -689,7 +705,7 @@ class BulkDestroyModelMixin: errors.append({'id': pk, 'errors': {'__all__': [str(e.message)]}}) if errors: transaction.set_rollback(True) - return errors, total + return errors, total, has_conflict class ObjectValidationMixin: