ghazi-git / drf-standardized-errors

Standardize your DRF API error responses
https://drf-standardized-errors.readthedocs.io/en/latest/
MIT License
249 stars 14 forks source link

Handle 'code': 'null' #74

Closed GiancarloFusiello closed 2 months ago

GiancarloFusiello commented 3 months ago

Hello, I've recently started using Schemathesis to test HTTP APIs based on an OpenAPI 3.0.3 spec schema which is generated by drf-spectacular. I came across your library in a github issue where I was looking for a way to provide schemas for DRF errors. Your library does a great job of this and has been very helpful. Thank you for creating this.

The tests generated by Schemathesis are rigorous and can also be configured to test how well the documented APIs handle unexpected data as well as expected data.

Problem One of the problems I've seen is when null data is sent via a POST request, DRF will run the following:

You can see here that the DRF ValidationError code is set to 'null' in this case. This produces the following error response when using drf-standardized-errors:

{
    'errors': [
        {
            'attr': 'non_field_errors',
            'code': 'null',
            'detail': 'No data provided',
        },
    ],
    'type': 'validation_error',
}

However, this does not match the schema provided by drf-standardized-errors:

{
    'errors': [
        {
            'attr': 'non_field_errors',
            'code': 'invalid',
            'detail': string,
        },
    ],
    'type': 'validation_error',
}

This will cause Schemathesis to fail the test.

Steps to reproduce Using DRF, create a simple view that allows post requests and add the url to your apps urls.py file.

# For example
class MyAPIView(generics.GenericAPIView):
    def post(self, request: Request) -> Response:
        serializer = MyAPISerializer(data=request.data)
        serializer.is_valid(raise_exception=True)
        return Response(data=serializer.data, status=status.HTTP_200_OK)

Then send the string 'null' to your POST API endpoint as the payload. For example:

curl -X POST -H 'Content-Type: application/json' -H 'Authorization: Basic xxxx' -d null http://localhost:8000/my-api/

Question I was able to to get around this by adding a condition in the view to check if the data is empty and then raising a ValidationError. I then extended the validation errors to include this. However, as this does not pertain to any particular field, I've tried setting the field value to None and 'non_field_errors' but to no avail . This adds a null key the generated schema which does not conform to valid JSON which, in turn, causes other problems.

I'm planning on creating a custom handler/formatter to handle this scenario where if the code value is null or None then set it to 'invalid'. What are your thoughts on this approach? And, is this something that could/should be added to the this library as I'm certain there are other scenarios where this can happen and could be helpful for others?

I look forward to hearing from you. Thanks again.

ghazi-git commented 3 months ago

Thank you for the detailed issue report @GiancarloFusiello, it was very helpful in reproducing the issue.

The problem is the missing null error code. At the time, I assumed that drf never raises it since I didn't know how to do that, but your curl example showed me how.

I just pushed a fix, so can you test again with the latest changes? pip install git+https://github.com/ghazi-git/drf-standardized-errors.git. I'd like to make sure that the issue is solved before creating a release.

As for the original question, @extend_validation_errors should add extra error codes to the schema, below is the code I tried even before pushing the fix to add the "null" error code to non_field_errors for a single endpoint

add validation error code to a single endpoint ```python from django.urls import path from drf_standardized_errors.openapi_validation_errors import extend_validation_errors from rest_framework import serializers from rest_framework.generics import GenericAPIView from rest_framework.response import Response class MyAPISerializer(serializers.Serializer): some_field = serializers.CharField() @extend_validation_errors(["null"], field_name="non_field_errors", methods=["post"]) class MyAPIView(GenericAPIView): serializer_class = MyAPISerializer authentication_classes = [] permission_classes = [] def post(self, request): serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) return Response(data=serializer.data) app_name = "api" urlpatterns = [path("my-api/", MyAPIView.as_view())] ```

and if you want to manipulate validation error codes for all endpoints at once, then you'll need to subclass drf_standardized_errors.openapi.AutoSchema and override _get_validation_error_codes_by_field.

GiancarloFusiello commented 3 months ago

Thanks for the quick response @ghazi-git.

I can confirm that using @extend_validation_errors with "null" and non_field_errors with version 0.13.0 resolves the issue and your changes in commit cccda0f also resolve the issue without me having to manually add the decorator to view classes. For what it's worth, I also took the time to take a look at the changes you made in the commit and they look good to me 👍

Thank you again

GiancarloFusiello commented 3 months ago

I did find another issue relating to this but I suspect the issue is with Schemathesis but thought I would mention it in case it's relevant here.

After running more tests using Schemathesis I was presented with this error:

- Response violates schema

    'null' is not one of ['parse_error']

    Schema:

        {
            "enum": [
                "parse_error"
            ],
            "type": "string",
            "description": "* `parse_error` - Parse Error"
        }

    Value:

        "null"

[400] Bad Request:

    `{"type":"validation_error","errors":[{"code":"null","detail":"This field may not be null.","attr":"list_field_name.0"}]}`

Reproduce with:

    curl -X POST -H 'Authorization: [Filtered]' -H 'Content-Type: application/json' -H 'Cookie: [Filtered]' -d '{"list_field_name": [null]}' http://0.0.0.0:8000/api/my-resource/

The field in question is a ListField on the view serializer that expects a list of url strings only.

Looking at the generated schema, everything appears correct but I thought I would check if list_field_name.INDEX is accepted syntax for OpenAPI 3.0.3?

list_field_nameINDEXErrorComponent:
      type: object
      properties:
        attr:
          enum:
          - list_field_name.INDEX
          type: string
          description: '* `list_field_name.INDEX` - list_field_name.INDEX'
        code:
          enum:
          - blank
          - invalid
          - max_length
          - 'null'
          - null_characters_not_allowed
          - required
          - surrogate_characters_not_allowed
          type: string
          description: |-
            * `blank` - blank
            * `invalid` - invalid
            * `max_length` - max_length
            * `null` - null
            * `null_characters_not_allowed` - null_characters_not_allowed
            * `required` - required
            * `surrogate_characters_not_allowed` - surrogate_characters_not_allowed
        detail:
          type: string

If it is then I will raise an issue on Schemathesis side as it is not finding the correct error component. Also, I'm happy to raise a separate issue for this if needed. Do let me know. Thanks again.

Stranger6667 commented 3 months ago

Note that enum values are literal values (per JSON Schema), hence given the schema the only acceptable value for the attr field is list_field_name.INDEX. You may want to use the pattern keyword instead with something like \Alist_field_name\.\d+\Z

ghazi-git commented 3 months ago

Thank you @GiancarloFusiello for confirming the issue resolution and taking the time to review the code.

As for the test failure related to list field, can you open a new issue for it? I will be creating a release soon with the current fixes. Also, list serializers and DictFields should present a similar issue.

And thank you @Stranger6667 for pointing the issue and suggesting a fix. Your work on schemathesis is proving really helpful to catch the issues in the generated schema. Once I get some time, I'll try setting it up to run checks against the auto-generated schema.

GiancarloFusiello commented 3 months ago

@ghazi-git no problem. I'll create a new issue. Thank you again and thank you @Stranger6667.

Stranger6667 commented 3 months ago

@ghazi-git @GiancarloFusiello I am happy that it helps! :) feel free to tag me if there is anything Schemathesis could help to solve :) maybe some validators / checks…

another approach would be to define a custom format (and format checker) instead of a pattern, then configure jsonschema (in case it is used for validation) to check it in tests. However this approach is not really portable across json schema validation implementations, so pattern could be better in this regard.

GiancarloFusiello commented 2 months ago

@ghazi-git I noticed that you added a fix to this in version 0.14.0, therefore, I'll close the issue. Thank again for your help. I'm eagerly anticipating a release including a change for #76 🤞