tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.35k stars 260 forks source link

Returning different types of 400 exceptions #101

Open andrewgy8 opened 4 years ago

andrewgy8 commented 4 years ago

Hey there!

We have a view which may return many different 40X responses due to validation of the data. They all have the same general structure as they are derived from DRF's API exception class.

Is there a way to include these in the extend_schema decorator?

I was expecting something like this to work:

class InvalidCode(APIException):
    status_code = 400
    default_detail = 'Code is not available.'
    default_code = 'unavailable_code_selected'

class IncorrectDatetime(APIException):
    status_code = 400
    default_detail = 'Incorrect datetime'
    default_code = 'Incorrect_datetime'

@extend_schema(responses={200: UpdateSerializer, 400: [InvalidCode, IncorrectDatetime]})
def update(self, request, *args, **kwargs):
    instance = self.get_object()
    ...

Does something like this exist?

tfranzel commented 4 years ago

Hey!

absolutely! depending on what you want to achieve there are multiple ways.

  1. @extend_schema(responses={200: UpdateSerializer, '40X': ErrorSerializerBaseClass})

  2. PolymorphicProxySerializer

  3. may look nicer but 2. will prob work better with client generation. i have not used '40X' with the generator myself yet

beware that i just merged the "pro-mode" PolymorphicProxySerializer #91, thus the "dict" part is not in the pypi release yet), but it is recommended to use the "list" version anyway. :smile:

andrewgy8 commented 4 years ago

As far as I understand though, DRF's APIExcpetion power lies in the fact that a Serializer is not necessary after it is raised in the stack. This is the reason that I was thinking an APIException (or list of them as a number of them can occur) would be beneficial to pass into the responses argument of extend_schema.

Just to give you an idea of what we have right now in our manually generated openapi yaml, is the following:

ServiceUnavailable:
  description: Service temporarily unavailable, a client can try again later
  content:
    application/json:
      schema:
        type: object
        properties:
          errors:
            type: array
            items:
              $ref: '#/schemas/ServiceUnavailable'

BadRequest:
  description: Bad Request
  content:
    application/json:
      schema:
        type: object
        properties:
          errors:
            type: array
            items:
              $ref: '#/schemas/Error'

NotFound:
  description: Not found
  content:
    application/json:
      schema:
        type: object
        properties:
          errors:
            type: array
            items:
              $ref: '#/schemas/Error'

All three of these (40X, 500) responses plus a 200 response would be displayed on one endpoint.

I hope Im making my question clearer 😉

tfranzel commented 4 years ago

@andrewgy8 ahh now i understand what you are getting at here. that is a good point. we more or less ignored that that part of DRF until now. indeed, my proposed options do not really fit well here.

in theory, every endpoint could raise those errors. however listing all of them for every endpoint would be a bit spammy, so listing them explicitly with @extend_schema makes sense imho.

this needs some investigating on how to do error handling in a flexible and generic way.

  1. this might be best located in the responses section of the schema
  2. do wildcards like 40X work with client generator? mayne this needs to be demultiplexed
  3. is there a a good schema representation for the errors with a payload? i believe ValidationError can take multiple forms.
  4. is this always application/json or are the view's renderer classes honored?
  5. how to handle custom error exceptions in addition to the DRF's native ones?
  6. do we need another argument to @extend_schema or can this be folded into responses?
andrewgy8 commented 4 years ago

this might be best located in the responses section of the schema

Exactly. Id suggest as a first iteration placing it in the responses argument. I think from the users POV, that is where it would naturally be.

is there a a good schema representation for the errors with a payload?

AFAIK, out of the box DRF exception responses contain a standard structure.

However there will be complexity involved when the application has a custom exception handler. This can indeed manipulate the standard response structure. I was thinking in that case we could use the custom exception handler somehow to "inspect" how the final format will look like.

is this always application/json or are the view's renderer classes honored?

I assume always application/json but I may be wrong about that...

jayvdb commented 4 years ago

Notes from #119 , there are many different responses from DRF core

https://github.com/ivlevdenis/drf_pretty_exception_handler#features has a nice summary of the four main quite varied response payloads that default DRF emits as 400 responses.

The default exception subclasses also emit 500, 403, 401, 404, 405, 406, 415, and 429

In addition, Http404 is caught and emits a 404 response.

This would result in a huge increase in the schema if those codes are each described for each endpoint, so I proposed using the default which is sort of built for this.

      responses:
        ...
        default:
          insert_ref_here_to_a_component_broadly_describing_of_all_drf_possible_responses_merged_with_AnyOf

And IMO the extension framework should be used, so that the setting REST_FRAMEWORK.EXCEPTION_HANDLER is used for determining what populates that default response, and a fake unused serializer is created in drf-spectacular to describe it. Probably best to have a SPECTACULAR_SETTINGS value DEFAULT_RESPONSE_CLASS, which if blank falls back to REST_FRAMEWORK.EXCEPTION_HANDLER, or better yet meld two classes together because unless REST_FRAMEWORK.EXCEPTION_HANDLER is modified, it will cause all those responses, and the schema should describe them. It should take very explicit action from the implementer to avoid those responses being in the schema.

tiholic commented 2 years ago

This is how I implemented it:

  1. 400 is added on all non-GET endpoints
  2. 401 and 403 on all endpoints which have at-least one authentication class
  3. 404 on all endpoints which have path variables

If atleast one 4xx response is defined on an endpoint (using extend_schema or otherwise), the default behaviour will not be applied

Only catch here is it overrides private method _get_response_bodies.

from rest_framework import serializers
from drf_spectacular.openapi import AutoSchema

class DummySerializer(serializers.Serializer):
    def to_internal_value(self, data):
        return data

    def to_representation(self, instance):
        return instance

    def update(self, instance, validated_data):
        pass

    def create(self, validated_data):
        pass

class ValidationErrorSerializer(DummySerializer):
    errors = serializers.DictField(child=serializers.ListSerializer(child=serializers.CharField()))
    non_field_errors = serializers.ListSerializer(child=serializers.CharField())

class GenericErrorSerializer(DummySerializer):
    detail = serializers.CharField()

class UnauthenticatedErrorSerializer(GenericErrorSerializer):
    pass

class ForbiddenErrorSerializer(GenericErrorSerializer):
    pass

class NotFoundErrorSerializer(GenericErrorSerializer):
    pass

class MyAutoSchema(AutoSchema):
    def _get_response_bodies(self):
        response_bodies = super()._get_response_bodies()
        if len(list(filter(lambda _:_.startswith('4'), response_bodies.keys()))):
            return response_bodies

        add_error_codes = []
        if not self.method == 'GET':
            add_error_codes.append('400')

        if self.get_auth():
            add_error_codes.append('401')
            add_error_codes.append('403')

        if not (self.method == 'GET' and self._is_list_view()):
            if len(list(filter(lambda _: _['in'] == 'path', self._get_parameters()))):
                add_error_codes.append('404')

        self.error_response_bodies = {
            '400': self._get_response_for_code(ValidationErrorSerializer, '400'),
            '401': self._get_response_for_code(UnauthenticatedErrorSerializer, '401'),
            '403': self._get_response_for_code(ForbiddenErrorSerializer, '403'),
            '404': self._get_response_for_code(NotFoundErrorSerializer, '404')
            }
        for code in add_error_codes:
            response_bodies[code] = self.error_response_bodies[code]
        return response_bodies

@tfranzel any better way to handle this (without overriding private method)

tfranzel commented 2 years ago

@tiholic pretty nice! don't get discouraged from overriding private methods. the distinction is a bit fuzzy. public methods are "mainly" the methods that get overridden by @extend_schema. You can override all methods according for your actual needs. so all is well.

I have not implemented this because by default there is only the generic and very dynamic error handler and at the end of the day everybody has slightly different requirements. there is likely no "one size fits all". I may look at this again when I have spare time but don't get your hopes up. this particular issue is pretty low prio at the moment.

ghazi-git commented 2 years ago

@tfranzel I created drf-standardized-errors which is a custom exception handler that returns the same error response format for 4XX and 5XX status codes. The package integrates with drf-spectatcular to automatically generate the error responses schema. By creating a custom exception handler, I avoided the issues linked to "the generic and very dynamic error handler".

Now, I'm interested in implementing the automatic error response generation directly in drf-spectacular. That would be useful to anyone happy with drf error format and is not interested in standardizing their error responses. So, I wanted to check with you first if it is still a good idea to do that as part of drf-spectacular, and if so, any advice about the implementation and potential problems to keep in mind.

michaelbaisch commented 1 year ago

I've been looking into documenting my responses properly and came across this problem. In general, how should one document the same HTTP status code with different response schemas and descriptions? Without any custom responses, only within DRF we have ValidationError (400) and Parse Error (400) with different schemas and descriptions. Every endpoint that takes data and is working with a serializer should probably define those two responses, but it's not possible to define different responses for the same status code, eg:

@extend_schema(
    responses={
        400: OpenApiResponse(response=ValidationErrorSerializer, description='Validation error'),
        400: OpenApiResponse(response=GenericErrorSerializer, description='Parse error'),
    },
)
tfranzel commented 1 year ago

@michaelbaisch, that is how responses are structured in OpenAPI. However, you can apply a trick and multiplex responses for one code:

    @extend_schema(
        responses={
            400: OpenApiResponse(
                description="Parse error or Validation error",
                response=PolymorphicProxySerializer(
                    component_name='Errors400',
                    serializers=[ValidationErrorSerializer, GenericErrorSerializer],
                    resource_type_field_name=None
                )
            )
        }
    )
michaelbaisch commented 1 year ago

I guess for this to work, there should be serializers for all the DRF error responses, pretty straightforward for most of them (only detail property) but the default ValidationError response doesn't have a consistent structure. I'm already not sure how to reflect a very simple response:

{
    "bar": [
        "This field is required."
    ]
}

since bar can be any property name, in a model for example. The suggestion from @tiholic doesn't seem to fit the default ValidationError behavior, I think.

tfranzel commented 1 year ago

Congratulations @michaelbaisch, you discovered the reason this was never implemented. 😄

The ValidationError serializer is super generic and not consistently structured. Shoehorning this into a generic OpenAPI formulation will have very little functional benefit, while never being a satisfactorily expressive solution. I just think it is not worth the trouble.