Просмотр исходного кода

Correct error message for bodyless DELETE requests

Jeremy Stretch 2 недель назад
Родитель
Сommit
f6e1bacfa1

+ 34 - 0
netbox/dcim/tests/test_api.py

@@ -490,6 +490,40 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase):
         site.refresh_from_db()
         self.assertEqual(site.description, '')
 
+        # A non-list body is described by its type, so that the client can see what was sent
+        self.assertEqual(response.data['detail'], 'Expected a list of objects, but got dict.')
+
+    def test_bulk_write_objects_empty_body(self):
+        """
+        Address a list endpoint with no body at all. An absent body reaches the bulk actions as an
+        empty dict, so it must not be reported as having "got dict" -- there is no object to describe.
+        """
+        obj_perm = ObjectPermission(name='Test permission', actions=['change', 'delete'])
+        obj_perm.save()
+        obj_perm.users.add(self.user)
+        obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model))
+
+        initial_count = Site.objects.count()
+
+        for method in ('patch', 'put', 'delete'):
+            with self.subTest(method=method):
+                response = getattr(self.client, method)(self._get_list_url(), **self.header)
+
+                self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+                self.assertEqual(
+                    response.data['detail'], 'Expected a list of objects, but no data was submitted.'
+                )
+                self.assertNotIn('errors', response.data)
+
+        # An explicitly submitted empty object is indistinguishable, and reads the same way
+        response = self.client.patch(self._get_list_url(), {}, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(
+            response.data['detail'], 'Expected a list of objects, but no data was submitted.'
+        )
+
+        self.assertEqual(Site.objects.count(), initial_count, 'No objects should have been deleted')
+
     def test_bulk_update_objects_non_numeric_id(self):
         """
         PATCH a set of objects where one entry carries a non-numeric ID. The failure must be

+ 12 - 8
netbox/netbox/api/viewsets/mixins.py

@@ -93,14 +93,18 @@ def get_non_list_response(data):
     if isinstance(data, list):
         return None
 
-    return Response(
-        {
-            'detail': _('Expected a list of objects, but got {datatype}.').format(
-                datatype=type(data).__name__
-            ),
-        },
-        status=status.HTTP_400_BAD_REQUEST,
-    )
+    # A request with no body at all arrives here as an empty dict, so reporting its type would tell
+    # the client only that it "got dict" -- unhelpful for what is the likeliest way to reach this
+    # point: a DELETE addressed to a list endpoint with nothing in the body. An explicitly submitted
+    # empty object is indistinguishable at this stage, and wants the same message anyway.
+    if data is None or data == {} or data == '':
+        detail = _('Expected a list of objects, but no data was submitted.')
+    else:
+        detail = _('Expected a list of objects, but got {datatype}.').format(
+            datatype=type(data).__name__
+        )
+
+    return Response({'detail': detail}, status=status.HTTP_400_BAD_REQUEST)
 
 
 def get_invalid_entries_response(entry_errors):

+ 27 - 0
netbox/utilities/testing/api.py

@@ -968,6 +968,33 @@ class APIViewTestCases:
             # No object named in the request may have been deleted
             self.assertEqual(self._get_queryset().count(), initial_count)
 
+        def test_bulk_delete_objects_no_body(self):
+            """
+            DELETE a list endpoint with no body at all. Nothing may be deleted -- the request names no
+            objects, so it cannot mean "all of them" -- and the response must say so intelligibly.
+            """
+            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))
+
+            initial_count = self._get_queryset().count()
+            self.assertNotEqual(initial_count, 0, 'No objects exist against which to test bulk deletion')
+
+            response = self.client.delete(self._get_list_url(), **self.header)
+
+            self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
+            self.assertIn('detail', response.data)
+            # There are no entries to report against, so no per-object errors are returned
+            self.assertNotIn('errors', response.data)
+            self.assertEqual(
+                self._get_queryset().count(), initial_count,
+                'A bulk delete naming no objects must not delete anything'
+            )
+
     class GraphQLTestCase(APITestCase):
         graphql_auto_filter_tests = True
         graphql_auto_filter_exclude = ()