2
0
Эх сурвалжийг харах

Document bulk errors format in OpenAPI schema

Jeremy Stretch 2 долоо хоног өмнө
parent
commit
66480beeca

+ 79 - 2
netbox/core/api/schema.py

@@ -2,6 +2,7 @@ import re
 import typing
 from collections import OrderedDict
 
+from django.utils.translation import gettext_lazy as _
 from drf_spectacular.contrib.django_filters import DjangoFilterExtension
 from drf_spectacular.extensions import OpenApiSerializerExtension, OpenApiSerializerFieldExtension, _SchemaType
 from drf_spectacular.openapi import AutoSchema
@@ -14,10 +15,10 @@ from drf_spectacular.plumbing import (
     get_doc,
 )
 from drf_spectacular.types import OpenApiTypes
-from drf_spectacular.utils import Direction, OpenApiParameter
+from drf_spectacular.utils import Direction, OpenApiParameter, OpenApiResponse
 
 from netbox.api.fields import ChoiceField
-from netbox.api.serializers import WritableNestedSerializer
+from netbox.api.serializers import BulkOperationErrorSerializer, WritableNestedSerializer
 from netbox.api.viewsets import NetBoxModelViewSet
 
 # see netbox.api.routers.NetBoxRouter
@@ -182,6 +183,82 @@ class NetBoxAutoSchema(AutoSchema):
 
         return response_serializers
 
+    def _get_bulk_error_responses(self, direction) -> typing.Any:
+        """
+        Return the error responses of the current bulk write action, keyed by status code, or an
+        empty dict if this action is not a bulk write.
+
+        A failed bulk write returns a structured body correlating each failure with the object (or,
+        where no object could be identified, the request position) responsible for it. This is a
+        documented part of the API contract, but drf-spectacular cannot infer it: responses are
+        derived from the request/response serializer alone, which describes only the success case.
+        """
+        action = getattr(self.view, 'action', None)
+
+        if action in ('bulk_update', 'bulk_partial_update'):
+            return {
+                '400': OpenApiResponse(
+                    response=BulkOperationErrorSerializer,
+                    description=_(
+                        "One or more of the objects specified could not be updated. No objects were "
+                        "modified: a bulk update is an all-or-none operation."
+                    ),
+                ),
+            }
+
+        if action == 'bulk_destroy':
+            return {
+                '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."
+                    ),
+                ),
+                '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."
+                    ),
+                ),
+            }
+
+        if action == 'create' and viewset_handles_bulk_create(self.view):
+            # A POST to a list endpoint accepts either a single object or a list of them (see
+            # _get_request_for_media_type()), so its error body takes one of two shapes
+            # accordingly: field-keyed errors for a single object, or the bulk envelope for a list.
+            component = self.resolve_serializer(BulkOperationErrorSerializer, direction)
+            return {
+                '400': OpenApiResponse(
+                    response={
+                        'oneOf': [
+                            build_basic_type(OpenApiTypes.OBJECT),
+                            component.ref if component else build_basic_type(OpenApiTypes.OBJECT),
+                        ],
+                    },
+                    description=_(
+                        "The object could not be created. Where a list was submitted, no objects "
+                        "were created: a bulk creation is an all-or-none operation."
+                    ),
+                ),
+            }
+
+        return {}
+
+    def _get_response_bodies(self, direction='response') -> typing.Any:
+        responses = super()._get_response_bodies(direction=direction)
+
+        # Document the error responses of the bulk write actions, which cannot be inferred (see
+        # _get_bulk_error_responses). A status code already present -- for instance one declared
+        # via @extend_schema on a custom action -- is left as it is.
+        for code, response in self._get_bulk_error_responses(direction).items():
+            if code not in responses:
+                responses[code] = self._get_response_for_code(response, code, direction=direction)
+
+        return responses
+
     def _get_request_for_media_type(self, serializer, direction='request'):
         """
         Override to generate oneOf schema for serializers that support both

+ 95 - 0
netbox/core/tests/test_openapi_schema.py

@@ -107,3 +107,98 @@ class OpenAPISchemaTestCase(TestCase):
         self.assertNotIn('oneOf', request_schema, "DELETE should NOT have oneOf")
         self.assertEqual(request_schema['type'], 'array', "DELETE should require array")
         self.assertIn('items', request_schema, "DELETE array should have items")
+
+    def _get_response_schema(self, path, method, code):
+        """Return the JSON response schema documented for the given operation and status code."""
+        responses = self.schema['paths'][path][method]['responses']
+        self.assertIn(code, responses, f"{method.upper()} {path} should document a {code} response")
+        return responses[code]['content']['application/json']['schema']
+
+    def test_bulk_error_component_is_defined(self):
+        """
+        The structured error body returned by a failed bulk operation should be a named component,
+        so that generated clients have a type for it.
+
+        Refs: #20054
+        """
+        components = self.schema['components']['schemas']
+
+        self.assertIn('BulkOperationError', components)
+        envelope = components['BulkOperationError']
+        self.assertEqual(sorted(envelope['properties']), ['detail', 'errors'])
+        # `errors` is absent where the request could not be attributed to individual entries
+        self.assertEqual(envelope['required'], ['detail'])
+        self.assertEqual(
+            envelope['properties']['errors']['items']['$ref'],
+            '#/components/schemas/BulkOperationEntryError',
+        )
+
+        self.assertIn('BulkOperationEntryError', components)
+        entry = components['BulkOperationEntryError']
+        # An entry is correlated by `id` or by `index`, so neither is required; `errors` always is
+        self.assertEqual(sorted(entry['properties']), ['errors', 'id', 'index'])
+        self.assertEqual(entry['required'], ['errors'])
+
+    def test_bulk_update_documents_error_response(self):
+        """
+        Bulk update operations should document the structured 400 response.
+
+        Refs: #20054
+        """
+        ref = {'$ref': '#/components/schemas/BulkOperationError'}
+
+        for path in ('/api/dcim/sites/', '/api/ipam/prefixes/', '/api/users/users/'):
+            for method in ('put', 'patch'):
+                with self.subTest(path=path, method=method):
+                    self.assertEqual(self._get_response_schema(path, method, '400'), ref)
+
+    def test_bulk_delete_documents_error_responses(self):
+        """
+        Bulk delete operations should document both the 400 (unresolvable request) and the 409
+        (dependency or protection rule) responses.
+
+        Refs: #20054
+        """
+        ref = {'$ref': '#/components/schemas/BulkOperationError'}
+
+        for path in ('/api/dcim/sites/', '/api/ipam/prefixes/', '/api/users/users/'):
+            with self.subTest(path=path):
+                self.assertEqual(self._get_response_schema(path, 'delete', '400'), ref)
+                self.assertEqual(self._get_response_schema(path, 'delete', '409'), ref)
+
+    def test_create_documents_error_response_for_either_shape(self):
+        """
+        A POST to a list endpoint accepts either a single object or a list, so its 400 response
+        should document both the field-keyed and the bulk error shapes.
+
+        Refs: #20054
+        """
+        for path in ('/api/dcim/sites/', '/api/ipam/prefixes/', '/api/users/users/'):
+            with self.subTest(path=path):
+                schema = self._get_response_schema(path, 'post', '400')
+                self.assertEqual(
+                    schema['oneOf'],
+                    [
+                        {'type': 'object', 'additionalProperties': {}},
+                        {'$ref': '#/components/schemas/BulkOperationError'},
+                    ],
+                )
+
+    def test_detail_operations_omit_bulk_error_response(self):
+        """
+        The bulk error body applies only to list endpoints; detail endpoints must not advertise it.
+
+        Refs: #20054
+        """
+        path = '/api/dcim/sites/{id}/'
+
+        for method in ('get', 'put', 'patch', 'delete'):
+            with self.subTest(method=method):
+                responses = self.schema['paths'][path][method]['responses']
+                self.assertNotIn('409', responses)
+                for code, response in responses.items():
+                    schema = response.get('content', {}).get('application/json', {}).get('schema', {})
+                    self.assertNotEqual(
+                        schema.get('$ref'), '#/components/schemas/BulkOperationError',
+                        f"{method.upper()} {path} ({code}) should not reference the bulk error body"
+                    )

+ 57 - 0
netbox/netbox/api/serializers/bulk.py

@@ -1,11 +1,14 @@
 import copy
 import functools
 
+from django.utils.translation import gettext_lazy as _
 from rest_framework import serializers
 
 from .features import ChangeLogMessageSerializer
 
 __all__ = (
+    'BulkOperationEntryErrorSerializer',
+    'BulkOperationErrorSerializer',
     'BulkOperationSerializer',
     'BulkPartialUpdateSchemaMixin',
     'BulkUpdateSchemaMixin',
@@ -17,6 +20,60 @@ class BulkOperationSerializer(ChangeLogMessageSerializer):
     id = serializers.IntegerField()
 
 
+# The two serializers below are schema-only: they are never used to validate or render data. The
+# bulk actions in netbox.api.viewsets.mixins assemble these payloads directly; these exist so that
+# their error responses are a documented part of the OpenAPI schema rather than an untyped body.
+# Note that a class docstring becomes the component's description in the published schema, so keep
+# it user-facing.
+class BulkOperationEntryErrorSerializer(serializers.Serializer):
+    """
+    The failure of a single object within a bulk operation.
+    """
+    id = serializers.IntegerField(
+        required=False,
+        help_text=_(
+            "The ID of the object which failed. Present once the entry has been matched to an "
+            "object; mutually exclusive with `index`."
+        )
+    )
+    index = serializers.IntegerField(
+        required=False,
+        help_text=_(
+            "The zero-based position of the entry within the submitted list. Used where no object "
+            "has been identified for the entry: always for creations, and for updates and deletions "
+            "where the entry itself could not be interpreted (e.g. a missing or non-numeric `id`). "
+            "Mutually exclusive with `id`."
+        )
+    )
+    errors = serializers.DictField(
+        help_text=_(
+            "The errors for this entry, keyed by field name. Values are ordinarily arrays of "
+            "messages. Errors which do not pertain to a specific field appear under `__all__` "
+            "(model validation, protection rules, restricted tags) or `non_field_errors` (errors "
+            "concerning the shape of the entry itself)."
+        )
+    )
+
+
+class BulkOperationErrorSerializer(serializers.Serializer):
+    """
+    The body returned when a bulk operation fails, correlating each failure with the object
+    responsible for it.
+    """
+    detail = serializers.CharField(
+        help_text=_('A summary of the failure, e.g. "1 of 3 objects failed validation."')
+    )
+    errors = BulkOperationEntryErrorSerializer(
+        many=True,
+        required=False,
+        help_text=_(
+            "One entry per object which failed; objects which would have succeeded are omitted, as "
+            "a bulk operation is all-or-none. Absent where the request could not be attributed to "
+            "individual entries at all (e.g. a request body which is not a list)."
+        )
+    )
+
+
 class BulkUpdateSchemaMixin:
     def get_fields(self):
         fields = super().get_fields()