Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Returning different types of 400 exceptions #101

Open
andrewgy8 opened this issue Jun 19, 2020 · 12 comments
Open

Returning different types of 400 exceptions #101

andrewgy8 opened this issue Jun 19, 2020 · 12 comments
Labels
enhancement New feature or request

Comments

@andrewgy8
Copy link

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?

@andrewgy8 andrewgy8 changed the title Returning different types of 400 expceptions Returning different types of 400 exceptions Jun 19, 2020
@tfranzel
Copy link
Owner

Hey!

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

  1. @extend_schema(responses={200: UpdateSerializer, '40X': ErrorSerializerBaseClass})
  2. PolymorphicProxySerializer
  1. 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. 😄

@andrewgy8
Copy link
Author

andrewgy8 commented Jun 22, 2020

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
Copy link
Owner

@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?

@tfranzel tfranzel added the enhancement New feature or request label Jun 22, 2020
@andrewgy8
Copy link
Author

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...

This was referenced Jul 8, 2020
@jayvdb
Copy link
Contributor

jayvdb commented Jul 11, 2020

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.

wmfgerrit pushed a commit to wikimedia/wikimedia-toolhub that referenced this issue Feb 3, 2021
Document the general error message structure in our generated OpenAPI
schema. This uses a technique suggested in
<tfranzel/drf-spectacular#119> to augment the
schema. In the future an implementation for
<tfranzel/drf-spectacular#101> may make this
code obsolete.

Bug: T268774
Change-Id: I77c3afffc90213d44f394e17314b28938136a325
@tiholic
Copy link

tiholic commented Mar 16, 2022

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
Copy link
Owner

@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
Copy link

@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
Copy link

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
Copy link
Owner

tfranzel commented Apr 5, 2023

@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
Copy link

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
Copy link
Owner

tfranzel commented Apr 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants