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

WIP: Content type specification #171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: Content type specification #171

wants to merge 3 commits into from

Conversation

mbierma
Copy link
Contributor

@mbierma mbierma commented Feb 9, 2020

This is a quick and dirty proof of concept to allow for the specification of content type other than application/json for OpenAPI 3 generation. Currently, it is possible to create an endpoint without specifying the response_body_schema, and then returning a Response object with the desired content type:

@registry.handles(
    rule='/todos',
    method='GET',
    query_string_schema=GetTodosQueryStringSchema(),
)
def get_todos():
    return send_file(pdf, as_attachment=True, mimetype='application/pdf')

However, in existing implementation, the generated swagger specification does not reflect the correct content type. This PR is intended to demonstrate a way to allow for the specification of content type using a __content_type__ magic attribute. Additionally, a primitive response_body_schema type is used to allow for responses that are not json objects.

class PDF(m.fields.String):
    __content_type__ = "application/pdf"

@registry.handles(
    rule='/todos',
    method='GET',
    query_string_schema=GetTodosQueryStringSchema(),
    response_body_schema=PDF
)
def get_todos():
    return send_file(pdf, as_attachment=True, mimetype='application/pdf')

Possibly related to #91

@mbierma mbierma requested a review from a team as a code owner February 9, 2020 08:58
@ghost ghost assigned badam5 Feb 9, 2020
Copy link

@badam5 badam5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mbierma! Thanks for the PR. Is this still WIP or would you like preliminary feedback?

@mbierma
Copy link
Contributor Author

mbierma commented Feb 11, 2020

Mostly just looking for preliminary feedback. The main intent of this PR is to:

  • Generate the correct OpenAPI 3 specification when the content type is not application/json
    • Account for the case where a single endpoint may return different content types based on the return code. For example, there may be consistent application/json error responses across all endpoints, even for ones that return a csv/file/etc.

Since this deals with response content types, and #91 is dealing with content negotiation, it would be great if there was a consistent interface between the code here and what you're planning for the future

@badam5
Copy link

badam5 commented Feb 12, 2020

@RookieRick @airstandley Tagging you folks here, since you may have more context than I do regarding content types! Any thoughts?

@airstandley
Copy link
Contributor

airstandley commented Feb 13, 2020

@mbierma The example you've given uses a Field rather than a Schema which presents all kinds of issues. I'm assuming that's a small mistake and you intended for PDF to be a marshmallow.Schema or some class that implements the dump interface. I'd be very interested if you could provide an example that actually handled validation (some assertion that pdf data is being returned).

The approach you are taking has some merit, but I think it would be helpful if you included a working example of a simple handler which returned a valid pdf(or something easier like plain text) when hooked up to a test app.

I also think we want to keep in mind that at least in OpenApi v3 you can specify multiple content-types. I'd image this behaviour would be much more useful it we could introduce a way to support multiple content-types (eg. a handler that can return video/mp4 or video/avi)

@mbierma
Copy link
Contributor Author

mbierma commented Feb 14, 2020

@airstandley thanks for the feedback. I'm going to be working on a better example

I am definitely in agreement that it would be much more useful to introduce a way to support multiple content-types. Do you have any recommendations on how to specify something like that? The first things that come to mind are

@registry.handles(rule="/foo/media", method="GET",
                  response_body_schema={200: [MP4(), OGG(), Quicktime()]})

or

@registry.handles(rule="/foo/media", method="GET",
                  response_body_schema={200: {'video/mp4': MP4(),
                                              'video/ogg': OGG(),
                                              'video/quicktime': Quicktime()}})

For the first case, there'd likely be some validation that each response schema in the list contains a unique __content_type__ magic attribute. For the second example, the magic attribute could be done away with, since the content-type is specified in the keys.

@airstandley
Copy link
Contributor

airstandley commented Feb 14, 2020

I haven't checked the 2.0 spec, but 3.0 specs content as a map so I'd say that

@registry.handles(rule="/foo/media", method="GET",
                  response_body_schema={200: {'video/mp4': MP4(),
                                              'video/ogg': OGG(),
                                              'video/quicktime': Quicktime()}})

seems like a great approach and then we can fallback on the old behaviour and assume 'application/json' if just a schema is given.
@RookieRick What are your thoughts on this?

@gtmanfred
Copy link
Contributor

Those defaults sound good to me. Is there any chance that you still plan on doing this work @mbierma ?

Thanks,
Daniel

@badam5 badam5 removed their assignment Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants