Correct behavior of returning a 400 vs. 409

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."
                     ),
                 ),
             }

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

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

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: