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

Component schema returned from OpenApiSerializerExtension is discarded if not object #351

Closed
spookylukey opened this issue Mar 29, 2021 · 4 comments
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@spookylukey
Copy link
Contributor

If an OpenApiSerializerExtension returns an array or simple type as the schema for a serializer, as opposed to object, then the component is discarded, and any fields that refer to it are also discarded, silently.

Here is a test case (could be added to tests/test_extensions.py, tested against current master):

from drf_spectacular.extensions import OpenApiSerializerExtension


class CustomListSerializer(serializers.Serializer):
    pass  # pragma: no cover


def test_serializer_extension_array():
    class ListSerializerExtension(OpenApiSerializerExtension):
        target_class = 'tests.test_extensions.CustomListSerializer'

        def map_serializer(self, auto_schema, direction):
            # For reference of good output:
            # return build_object_type(properties={'name': build_basic_type(OpenApiTypes.INT)})

            # Bad output
            # return build_basic_type(OpenApiTypes.INT)
            return build_array_type(build_basic_type(OpenApiTypes.INT))

    class XViewset(mixins.RetrieveModelMixin, viewsets.GenericViewSet):
        serializer_class = CustomListSerializer

    schema = generate_schema('x', XViewset)
    assert schema['components']['schemas']['CustomList']['type'] == 'array'
    assert schema['components']['schemas']['CustomList']['items']['type'] == 'integer'

If I change the line above to build_object_type(...), I get this schema as expected (trimmed to the relevant bits):

{
    "openapi": "3.0.3",
    "paths": {
        "/x/{id}/": {
            "get": {
                "operationId": "x_retrieve",
                "responses": {
                    "200": {
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/CustomList"
                                }
                            }
                        },
                        "description": ""
                    }
                }
            }
        }
    },
    "components": {
        "schemas": {
            "CustomList": {
                "type": "object",
                "properties": {
                    "name": {
                        "type": "integer"
                    }
                }
            }
        }
    }
}

With build_array_type or build_basic_type, however, I get this (again trimmed in the same way):

{
    "openapi": "3.0.3",
    "paths": {
        "/x/{id}/": {
            "get": {
                "operationId": "x_retrieve",
                "responses": {
                    "200": {
                        "description": "No response body"
                    }
                }
            }
        }
    },
    "components": {
    }
}

As you can see, CustomList component has disappeared, and everything that referred to it. In this case the entire response has no content. If you are using the CustomListSerializer as a serializer for a single field within another serializer, then just that field is missing (as well as the components bit).

If you look at the OpenAPI spec for SchemaObject, it says:

The Schema Object allows the definition of input and output data types. These types can be objects, but also primitives and arrays.

So I would have expected this to be accepted (or at the very least give an explicit error that this is not yet supported).

It looks like this bug is caused by this logic which silently discards all but a whitelist of certain types of things:

keep_component = (

This logic may have been appropriate at some stage, and is needed for something - if I just set keep_component = True then I get some failures. But I think this is now too fragile a mechanism, and the logic should be inverted - we should include everything, and if we want to exclude something then an explicit black list of certain objects should be used, where the blacklist isn't matching certain patterns of schema, but a list of specific things that is generated by other parts of the code. It's particularly bad this way as the failure is completely silent, you just get things discarded.

I'm not sure where to start with fixing this, with some guidance I could have a go.

@tfranzel
Copy link
Owner

hi @spookylukey, my sincere apologies for not addressing this earlier. i was kind of afraid of this change breaking all kinds of things. it turns out that it was unfounded as previous unrelated improvements made this less tricky than i thought. i went through all cases i could imagine and only really found one case that needed exclusion. i therefore went with your analysis and inverted the logic.

i would really appreciate your feedback on this. thank you.

@tfranzel tfranzel added the fix confirmation pending issue has been fixed and confirmation from issue reporter is pending label May 20, 2021
@spookylukey
Copy link
Contributor Author

@tfranzel No need to apologise, you don't owe me your time! Thanks so much taking a look, I'll look at in on Monday when I'm next working on my client's code that is affected by this.

@tfranzel
Copy link
Owner

tfranzel commented Jun 1, 2021

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.

@tfranzel tfranzel closed this as completed Jun 1, 2021
@spookylukey
Copy link
Contributor Author

Sorry I wasn't able to check earlier, I've upgrade drf-spectacular for my client and everything looks good now, with this fix and my other PRs merged - thanks! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

2 participants