ghazi-git / drf-standardized-errors

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

Use Pattern type rather than Enum type for error component attr property #76

Open GiancarloFusiello opened 3 months ago

GiancarloFusiello commented 3 months ago

As described here in issue #74 , for DRF serializer ListField or DictField drf-spectacular combined with drf-standardized-errors will generate the following:

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 you're like me and use Schemathesis to generate and run tests against your documented API, this will result in errors due to list_field_name.INDEX being a string literal as see here:

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

Suggestion To use the Pattern data type rather than Enum for the attr error component property. Examples can be found here.

ghazi-git commented 2 months ago

Sorry it took me so long to get to this and unfortunately, I don't have good news. The gist of it is: using a pattern for attr isn't enough to solve the schema conformance issues raised by schemathesis. For now, I'll keep things as they are until we have a solution for all issues raised by schemathesis.

in #79, I went ahead and added schemathesis to the test suite along with the exact cases reproducing the issues described here. After that, I updated the implementation so that attr is a string with a pattern (for a list field, the pattern is some_list_field\.\d+). Still, schemathesis kept showing similar conformance issues (can be seen in the PR checks). Looking at the docs, I thought this FAQ entry will help since drf-spectacular doesn't add additionalProperties: false by default. So, I tried adding that manually to see if it solves the issue. Again, schemathesis kept raising a conformance issue.

Then I created a minimal API spec to experiment with it (notice how attr is set as a string with a pattern)

API Spec

```yaml openapi: 3.0.3 info: title: API version: 1.0.0 description: Amazing API paths: /fuzzing/list_field/: post: operationId: fuzzing_list_field_create tags: - fuzzing requestBody: content: application/json: schema: $ref: '#/components/schemas/ListFieldFuzzing' required: true responses: '200': content: application/json: schema: $ref: '#/components/schemas/ListFieldFuzzing' description: '' '400': content: application/json: schema: $ref: '#/components/schemas/FuzzingListFieldCreateErrorResponse400' description: '' components: schemas: FuzzingListFieldCreateError: oneOf: - $ref: '#/components/schemas/FuzzingListFieldCreateNonFieldErrorsErrorComponent' - $ref: '#/components/schemas/FuzzingListFieldCreateField1ErrorComponent' - $ref: '#/components/schemas/FuzzingListFieldCreateField1INDEXErrorComponent' discriminator: propertyName: attr mapping: non_field_errors: '#/components/schemas/FuzzingListFieldCreateNonFieldErrorsErrorComponent' field1: '#/components/schemas/FuzzingListFieldCreateField1ErrorComponent' field1.INDEX: '#/components/schemas/FuzzingListFieldCreateField1INDEXErrorComponent' FuzzingListFieldCreateErrorResponse400: oneOf: - $ref: '#/components/schemas/FuzzingListFieldCreateValidationError' discriminator: propertyName: type mapping: validation_error: '#/components/schemas/FuzzingListFieldCreateValidationError' FuzzingListFieldCreateField1ErrorComponent: type: object properties: attr: type: string pattern: field1 code: enum: - not_a_list - 'null' - required type: string detail: type: string required: - attr - code - detail additionalProperties: false FuzzingListFieldCreateField1INDEXErrorComponent: type: object properties: attr: type: string pattern: field1\.\d+ code: enum: - invalid - max_string_length - 'null' - required type: string detail: type: string required: - attr - code - detail additionalProperties: false FuzzingListFieldCreateNonFieldErrorsErrorComponent: type: object properties: attr: type: string pattern: non_field_errors code: enum: - invalid - 'null' type: string detail: type: string required: - attr - code - detail additionalProperties: false FuzzingListFieldCreateValidationError: type: object properties: type: $ref: '#/components/schemas/ValidationErrorEnum' errors: type: array items: $ref: '#/components/schemas/FuzzingListFieldCreateError' required: - errors - type additionalProperties: false ListFieldFuzzing: type: object properties: field1: type: array items: type: integer required: - field1 additionalProperties: false ValidationErrorEnum: enum: - validation_error type: string ```

The above API spec generates the following conformance issue

Conformance issue details

``` E 1. Response violates schema E E {'code': 'null', 'detail': 'This field may not be null.', 'attr': 'field1.0'} is valid under each of {'$ref': '#/components/schemas/FuzzingListFieldCreateField1INDEXErrorComponent'}, {'$ref': '#/components/schemas/FuzzingListFieldCreateField1ErrorComponent'} E E Schema at /oneOf/0/properties/errors/items: E E { E "oneOf": [ E { E "$ref": "#/components/schemas/FuzzingListFieldCreateNonFieldError... E }, E { E "$ref": "#/components/schemas/FuzzingListFieldCreateField1ErrorCo... E }, E { E "$ref": "#/components/schemas/FuzzingListFieldCreateField1INDEXEr... E } E ], E "discriminator": { E "propertyName": "attr", E "mapping": { E "non_field_errors": "#/components/schemas/FuzzingListFieldCreateN... E "field1": "#/components/schemas/FuzzingListFieldCreateField1Error... E "field1.INDEX": "#/components/schemas/FuzzingListFieldCreateField... E } E // Output truncated... E } E E Value: E E { E "code": "null", E "detail": "This field may not be null.", E "attr": "field1.0" E } E E [400] Bad Request: E E `{"type":"validation_error","errors":[{"code":"null","detail":"This field may not be null.","attr":"field1.0"}]}` E E Reproduce with: E E curl -X POST -H 'Content-Type: application/json' -d '{"field1": [null]}' http://127.0.0.1:8000/fuzzing/list_field/ E E Falsifying explicit example: test_compliance_to_api_schema_for_list_field( E case=Case(body={'field1': [None]}), E ) ```

That's when I understood the real issue is in the discriminator mapping which still shows field1.INDEX:

    FuzzingListFieldCreateError:
      oneOf:
        - $ref: '#/components/schemas/FuzzingListFieldCreateNonFieldErrorsErrorComponent'
        - $ref: '#/components/schemas/FuzzingListFieldCreateField1ErrorComponent'
        - $ref: '#/components/schemas/FuzzingListFieldCreateField1INDEXErrorComponent'
      discriminator:
        propertyName: attr
        mapping:
          non_field_errors: '#/components/schemas/FuzzingListFieldCreateNonFieldErrorsErrorComponent'
          field1: '#/components/schemas/FuzzingListFieldCreateField1ErrorComponent'
 ======>  field1.INDEX: '#/components/schemas/FuzzingListFieldCreateField1INDEXErrorComponent'

Afterwards, I tried to see if it's possible to express the pattern at the discriminator level in openapi but couldn't find a way to do that. That led me to think: even though #79 is a step in the right direction, the API spec generated doesn't properly describe the API response. So for now, I'll keep things as they are without merging #79 until openapi adds some sort of conditional schemas that fully solves the issue or someone finds a different solution.

GiancarloFusiello commented 2 months ago

I see. That's a shame. Thanks for trying @ghazi-git.