Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow globally excluding certain content types #598

Closed
johnthagen opened this issue Nov 9, 2021 · 10 comments
Closed

Allow globally excluding certain content types #598

johnthagen opened this issue Nov 9, 2021 · 10 comments
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@johnthagen
Copy link
Contributor

By default, (I believe because DRF supports the Browsable Web API) drf-spectacular will generate a schema with both form-data and json content types:

      requestBody:
        content:
          application/json:
            schema:
              type: array
              items:
                $ref: '#/components/schemas/REDACTED'
          application/x-www-form-urlencoded:
            schema:
              type: array
              items:
                $ref: '#/components/schemas/REDACTED'
          multipart/form-data:
            schema:
              type: array
              items:
                $ref: '#/components/schemas/REDACTED'
        required: true

While the form-data type is used for the browseable web API, users might want to exclude it from the schema so that generated clients are simpler. Without an exclusion, multiple client code paths are generated which complicates the generated code and sometimes conflicts with one another:

Adding a global setting to somehow disable certain content types could be useful to solve this issue:

SPECTACULAR_SETTINGS = {
    "EXCLUDE_CONTENT_TYPES": ["multipart/form-data"],
 }

This would allow the browsable API to continue to work (by not actually changing any views) but allow the exported DRF schema to be much simpler.

@ngnpope
Copy link
Contributor

ngnpope commented Nov 9, 2021

I ran into this same problem. I believe that this seems to be a problem many popular client generators:

There are a number of workarounds:

  • Override DEFAULT_PARSER_CLASSES to ["rest_framework.parsers.JSONParser"].
    (I didn't like this as it was likely to break some endpoints that need the others.)
  • Override by using parser_classes attribute for APIView class-based views.
    Override by using @parser_classes decorator for @api_view-decorated function-based views.
    (I didn't like this as it still prevents use of other content types which should be perfectly valid.)
    See https://www.django-rest-framework.org/api-guide/parsers/#setting-the-parsers
  • Hide the other content types from drf-spectacular only so that they don't appear in the schema.
    (This isn't perfect as the other content types are perfectly valid, but I only really cared about JSON in my schema.)

The last option can be achieved by overriding the AutoSchema class:

from drf_spectacular.openapi import AutoSchema as _AutoSchema
from rest_framework.request import is_form_media_type


class AutoSchema(_AutoSchema):
    def map_parsers(self) -> list[str]:
        """
        Return a unique list of media types.

        Overrides the default behaviour to remove form media types as done in ``drf_yasg.utils.get_consumes()``.
        We could have changed DRF's ``DEFAULT_PARSER_CLASSES`` setting but we don't want to inadvertently break things.

        The reason for implementing this hack is to work around broken client generation from ``swagger-codegen`` which
        is used by editor.swagger.io and seems to be incapable of handling multiple media types in request bodies. As
        this may be used by clients, we'll tweak this even though our specification is perfectly valid when unaltered.

        It may be better to steer clients to use ``openapi-generator`` instead. This doesn't support multiple media
        types in the generated client code, but it doesn't generate invalid output by using the first available media
        type (which is ``application/json`` based on the default value of ``DEFAULT_PARSER_CLASSES``.

        See the following links for additional information:

        - https://github.com/swagger-api/swagger-codegen/issues/8191
        - https://github.com/OpenAPITools/openapi-generator/issues/2668
        - https://openapi-generator.tech/
        """
        media_types = super().map_parsers()
        non_form_media_types = [x for x in media_types if not is_form_media_type(x)]
        return non_form_media_types if non_form_media_types else media_types

This can then be configured:

REST_FRAMEWORK = {
    ...
    "DEFAULT_SCHEMA_CLASS": "<path>.<to>.AutoSchema",
    ...
}

Hope that helps someone.

@johnthagen
Copy link
Contributor Author

@ngnpope Thank you for the discussion. I can confirm your final work around works well at addressing this issue. ❤️

@tfranzel
Copy link
Owner

Interesting! Generally i would say that it has helped me seeing the feature that are enabled on the API. At the end of the day these are the things your API actually supports.

Regarding openapi-generator, I believe they simply pick the first content type and ignore the rest. Has worked for me without issues.

I usually immediately remove the BrowsableAPIRenderer, as it is surprisingly resource-heavy and has slowed down servers for us. So much that you could DoS the servers with it.

these are the DRF defaults:

    'DEFAULT_RENDERER_CLASSES': [
        'rest_framework.renderers.JSONRenderer',
        'rest_framework.renderers.BrowsableAPIRenderer',
    ],
    'DEFAULT_PARSER_CLASSES': [
        'rest_framework.parsers.JSONParser',
        'rest_framework.parsers.FormParser',
        'rest_framework.parsers.MultiPartParser'
    ],

We already do explicitly ignore BrowsableAPIRenderer. For auth hiding we introduced AUTHENTICATION_WHITELIST. I think we could add RENDER_WHITELIST and PARSER_WHITELIST accordingly, to control this on a global level. I don't think targeting the content type directly is a good idea.

What do you guys think of this approach?

@johnthagen
Copy link
Contributor Author

A PARSER_WHITELIST sounds perfect. Would it look something like:

SPECTACULAR_SETTINGS = {
    "PARSER_WHITELIST": ["rest_framework.parsers.JSONParser"],
 }

@tfranzel
Copy link
Owner

yes it would be exactly like that

@johnthagen
Copy link
Contributor Author

That sounds perfect to me, and likely very useful for many/most DRF users.

Like you said, most people don't realize this is actually a problem because openapi-generator doesn't (yet) look at these fields. Once they "fix" this, this will become very apparent quickly to everyone and they'll likely want to disable this or else the generated code gets messy quickly.

It would be a breaking change (for non-openapi-generator users) but I'm almost tempted to say this should be the default. I feel like once openapi-generator is updated, a vast majority of users will want to enabled this.

But for "purity" / "correctness", this should probably remain opt-in.

@ngnpope
Copy link
Contributor

ngnpope commented Nov 10, 2021

Sounds like the settings would be useful. The only reason for this is that nearly all the client generators get it wrong and unfortunately users depend on them heavily.

@tfranzel
Copy link
Owner

But for "purity" / "correctness", this should probably remain opt-in.

I think so too. Generally I look at the Swagger UI and adapt the API according to my expectations, which is mostly reducing the potential surface area of the API.

I think this feature definitely has it's purpose, but i would generally recommend adapting the API and not the schema where possible. I added the two new settings.

@tfranzel tfranzel added enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Nov 10, 2021
@johnthagen
Copy link
Contributor Author

@tfranzel Confirmed that 0.21.0 fixes this issue. Thanks!

Feel free to close this issue.

@ngnpope
Copy link
Contributor

ngnpope commented Nov 10, 2021

This works well on a global level, but I think it's worth pointing out that my suggestion for overriding AutoSchema.map_parsers() still has the benefit that it is a bit more dynamic and can detect when to use multipart/form-data over application/json if, for example, the endpoint supports file uploads.

But as we've already discussed, it is the code generators that are broken, and these hacks are just one more of the ways that we can work around the fact that they cannot handle correct specifications. So no further changes needed. Just highlighting something that others may need to be aware of from my experience.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

3 participants