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

Schema generation doesn't support response schemas #4502

Closed
gcbirzan opened this issue Sep 21, 2016 · 57 comments
Closed

Schema generation doesn't support response schemas #4502

gcbirzan opened this issue Sep 21, 2016 · 57 comments

Comments

@gcbirzan
Copy link
Contributor

This is a useful feature, at least for swagger, and the way things are right now it cannot be implemented from outside of DRF, since the Link object doesn't contain the schema for the response.

While the fields can be implied from the input fields, for read only endpoints this is not an option.

@tomchristie
Copy link
Member

Correct. That's a design decision within Core API, because it comes across to me as unnecessary.

  1. Generic clients do not need to have a strictly specified response structure in order to function.
  2. Specifying response structures is an unbounded domain. Better just to describing expected responses in documentation (eg in the link description.)
  3. Responses might in any case actually end up coming from one of many possible intermediaries. No way of knowing for sure what possible range of error responses might actually occur.

Perhaps this'll change someday, but for now I don't see sufficient clear value, given the complexity it adds.

@rygwdn
Copy link

rygwdn commented Oct 10, 2016

We actually find the response schema to be very useful with swagger/openapi. It gives us:

  • Consistent response body documentation in the automatically generated docs
  • Documentation which is testably accurate (e.g. via a generic OpenAPI validating middleware)
  • Field-level documentation by providing access to the serializer help_text in the schema
  • The ability to generate response body objects in statically typed languages (Java, etc.)

We deal with the complexities you mentioned by:

  • Using a generic type when the response can vary (e.g. a generic json "Object" response body with details in the description)
  • Using OpenAPI "formats" to further specify anything that's not a plain "string" or "int", etc. The formats can be anything, so we can introduce new "types" easily
  • Support for overriding the schema at various layers - the ViewSet can specify a different schema than what the tool thinks it would have, and we have hooks that do transformations on the schema to support our own extensions (e.g. to add tags, show/hide apis based on environment, modify descriptions, add pagination data, etc.)

@tomchristie
Copy link
Member

Indeed. I've been warming to introducing some of this. I'll reopen this against 3.5 in order to keep track of what does and what doesn't go it. It's possible that we might end up part-way towards this (eg. example responses, but not full response schemas) or that we might completely defer it until later, but either way I think coreapi does need to be able to express this enough to be useful for docs generators.

@tomchristie
Copy link
Member

Alrighty, there's plenty in 3.5 that improves the schema handling.
We'll be looking at API documentation as part of the 3.6 headline features,
which will likely include having extra information in coreapi in order to support the docs generators.

Most likely we'll initially be looking at response examples, rather than response schemas, but it's possible that we'll end up with both.

@wtfuii
Copy link

wtfuii commented Nov 11, 2016

@tomchristie I'd prefer schemas, as i.e. https://generator.swagger.io/ generates full featured client libraries out of swagger files, which could be generated out of coreAPI data.

If the response schema is missing, the generated library does not specify the model. which makes this whole code generation pretty useless for static typed languages such as TypeScript, Java or C#.

@tomchristie
Copy link
Member

tomchristie commented Nov 11, 2016

@wtfuii: Not necessarily - get the pattern right and you can perfectly well create a dynamic client library within statically typed language. (For a parallel to this see, eg. browser implementations) We may or we may not follow swagger's choice here - remains to be seen.

@thomasf
Copy link

thomasf commented Nov 24, 2016

I might be missing something since I just started looking for a solution to this but It seems like it would be great if there was some kind of "generic" class for non model views with an optional get_response_serializer or something. I guess the main downside is that it wouldn't automatically translate to usage with @detail_route() but other than that it seems to me that it could be a solution which is similar to what already exists.

@paultiplady
Copy link

@tomchristie apologies if I'm missing the obvious -- how would we code against a dynamic client in a statically typed language?

Here's a common Swagger use-case: I'm building versioned APIs that are going to be supported indefinitely (e.g. the Kubernetes API), and I want to ship versioned clients for developers working in many languages. The big win with a static client in this use-case is getting code completion and type hinting for your API client, just by importing the client library.

I don't see what pattern can enable that case with a dynamic client library. Can you flesh out your thinking a bit more please?

@tomchristie
Copy link
Member

@paultiplady - Out of interest, which languages will you be prioritising?

One way around this would be for the response data object to include typed accessors, eg .getString(...).

However I am considering struct stubs that the response can be unpacked into, so that the library itself remains dynamic, and the only generated bit of code you'd include would be some basic type stubs.

@paultiplady
Copy link

Top of the list is Typescript, with Python, Ruby, Go, and Java in the next tier. (I see a big benefit for having full type stubs in Python/Ruby too, since IDE type hinting is very good these days).

@gcbirzan
Copy link
Contributor Author

gcbirzan commented Dec 8, 2016 via email

@tomchristie
Copy link
Member

@paultiplady @gcbirzan I've got some thoughts of my own, but I'd welcome any examples of the sorts of interface you'd like to see.

@paultiplady
Copy link

Here's a good example of a simple usage of the Kubernetes swagger-codegen client: https://github.com/kubernetes-incubator/client-python/blob/master/examples/example1.py

This shows the response types being unpacked (and you get code-completion on those response values).

The actual generated model code is verbose in the extreme, but here's an example of some getter functions that are generated for an API endpoint: https://github.com/kubernetes-incubator/client-python/blob/master/kubernetes/client/apis/extensions_v1beta1_api.py#L3573

The key piece here is that we have :return: V1beta1DeploymentList, which lives here: https://github.com/kubernetes-incubator/client-python/blob/master/kubernetes/client/models/v1beta1_deployment.py. The models here are thin, so it's really just about being able to autogenerate those response classes and their fields.

@tomchristie
Copy link
Member

👍 Been working towards this lately, both for parameter schemas and response schemas.

@tomchristie
Copy link
Member

Okay, making the decision that we'll defer work on this and the parameter schemas to one of the 3.6.x releases.

The API documentation and JavaScript client are otherwise ready to go, so not worth holding those up on account of work toward this (which is, yes, still a high priority).

@gcbirzan
Copy link
Contributor Author

gcbirzan commented Mar 9, 2017 via email

@tomchristie
Copy link
Member

@gcbirzan Got it - Can see that there's a huge amount of folks backed up as a result of this.
This and parameter schemas probably need to be our highest priority feature ATM.

@m-haziq
Copy link

m-haziq commented Oct 12, 2017

Hi all, I have recently integrated Django Rest Swagger 2 into an existing project, I faced alot of issues, and resolved them by finding solutions at different forums. Now I have documented all the required steps here: https://github.com/m-haziq/django-rest-swagger-docs with all possible issues I could face and their solutions. just an effort to put all the stuff at one place.

@silver8ack
Copy link

@limdauto Your project looks promising. However, I can't currently use it because I'm using Namespace Versioning and I can't seem to get it to work without supplying a url version parameter.

@silver8ack
Copy link

It would be great if this issue would get some priority. This first question I'm getting from clients is why doesn't the openapi format work when they generate clients.

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 23, 2017

It would be great if this issue would get some priority.

What makes you think it isn't? 😃

I think it's worth remembering that the CoreAPI and the schema support in Django REST Framework is less that a year old. It's coming on very well but to expect it to match OpenAPI/Swagger (which is how old now?) on every feature, at this point, is not realistic.

Whilst focusing on it's own priorities, CoreAPI will approach feature equivalence as time goes by. Response Schemas are high on the list. But these things take time. The only way to speed that up is to contribute. (We already understand the demand.)

I guess 95% of people on this issue are more concerned with OpenAPI/Swagger integration than they are with Response Schemas per se.

I think it's important to keep in mind that the core framework supports CoreAPI schemas, rather than OpenAPI/Swagger. (That's the short answer to why it doesn't work with Swagger related tools out of the box.)

We are happy to see renderers that convert CoreAPI schemas to OpenAPI. We're also happy to look at whatever extension points are needed to make OpenAPI schemas feasible.

To this goal, Django REST Framework version 3.7 introduced the per-view schema customisation. My expectation here is that someone will:

  1. Implement a version of AutoSchema that skips CoreAPI and generates OpenAPI, including response schemas, directly.
  2. Implement a version of SchemaGenerator that collates OpenAPI schemas rather than CoreAPI Documents
  3. Publish that as a third-party package targeting the OpenAPI+DRF demographic ( i.e. the people here).

With that you'd just create a base view using the OpenAPI AutoSchema inspector and configure a SchemaView with the OpenAPI SchemaGenerator and you'd be away.

@limdauto's https://github.com/limdauto/drf_openapi is a great project. Maybe that'll be the package above, maybe not. I don't know. But it's the best option available for OpenAPI support as of today. (As far as I know.)

If there are specific issues with drf_openapi (such as @silver8ack's about namespace versioning) then they need to be addressed as issues on that project. If @limdauto, or collaborators, need specific changes in Django REST Framework, we can look at making changes here. (Such issues would be warmly received: our focus is on CoreAPI, but we want to enable other approaches where we can.)

So, in summary:

  1. Response Schemas are coming (as a priority) to CoreAPI.
  2. Django REST Framework exposes hooks which other systems can use directly, in preference to a renderer on top on CoreAPI, if they need more power than CoreAPI (currently) exposes.
  3. Whatever system you want to use, CoreAPI, OpenAPI, something-else, the best way to speed things up is to get involved at the code end. (What's your issue? Can you write some code to solve it?)

@Alexx-G
Copy link

Alexx-G commented Oct 24, 2017

Implement a version of AutoSchema that skips CoreAPI and generates OpenAPI, including response schemas, directly.

@carltongibson It would be nice to have something like DEFAULT_SCHEMA_CLASS in the settings.
This should make customization of response schema easier and more reusable.

@carltongibson
Copy link
Collaborator

No. Create a base view class or mixin and use that. (General principle is to avoid setting a proliferation wherever we can.)

@carltongibson
Copy link
Collaborator

No. ...

But, yeah, maybe. It would mirror the other APIView settings. We can review this as and when there's a candidate AutoSchema replacement to put there.

@Alexx-G
Copy link

Alexx-G commented Oct 24, 2017

I've reviewed current settings and it makes sense to have this option (since there'are default classes for throttles and filter backend).
A concrete use case - I'm implementing a custom schema which fixes (when needed) the problem with response format and some other particular problems with permissions. And I need to apply it for 80-90% of endpoints. And this will require changing literally all views (to add the mixin) + when any new view is added, I will have to keep in mind that there should be applied a custom mixin.

From my point of view, it is worth it to have such option (global override).

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 24, 2017

Fine. Put in a PR. Update: Added DEFAULT_SCHEMA_CLASS setting in #5658. Will be part of v3.7.4

@chrisv2
Copy link
Contributor

chrisv2 commented Nov 14, 2017

Please excuse me if the following idea is too stupid to be useful, but can't we just allow a new location response in schema.Field?

Since everything a serializer can return can already be described by fields (and usually is, if standard serializers/viewsets are used), this would give schema renderers all information they need to generate whatever they want, while not being a big change in itself.

I've done a proof-of-concept implementation of this and tested my app against it, and while it breaks the autogenerated docs visually (response fields are shown as inputs), everything else still works as expected, so it does not seem to be a very problematic change.

proof-of-concept code:

from rest_framework.schemas import SchemaGenerator, field_to_schema
from rest_framework import serializers
import coreschema, coreapi

class ResponseMetaSchemaGenerator(SchemaGenerator):
    def get_response_fields(self, path, method, view):
        """
        return field descriptions for the response
        """
        if not hasattr(view, 'get_serializer'):
            return []

        serializer = view.get_serializer()

        if not isinstance(serializer, serializers.Serializer):
            return []

        fields = []
        for field in serializer.fields.values():
            if field.write_only:
                continue

            field = coreapi.Field(
                name=field.field_name,
                location='response',
                required=False,
                schema=field_to_schema(field)
            )
            fields.append(field)

        return fields

    def get_link(self, path, method, view):
        link = super(ResponseMetaSchemaGenerator, self).get_link(path, method, view)
        link._fields = link._fields + tuple(self.get_response_fields(path, method, view))
        return link

@axnsan12
Copy link
Contributor

axnsan12 commented Dec 13, 2017

Hello,

<shameless-plug>
For what it's worth, I made an attempt at rolling my own openapi package that does not go through coreapi. I managed to implement response schemas, nested schemas and named models at a decent level.

I published an early version you can try here: https://github.com/axnsan12/drf-yasg/
</shameless-plug>

I was motivated to do this because both drf-openapi and django-rest-swagger seemed to make no progress in the direction and the mantainers do not respond to issues any more.

As an aside, I was not really able to reuse much of the SchemaGenerator infrastructure apart from architectural guides (still very useful for that though!) and copying some methods, because it is a bit tightly coupled to coreapi. Everything is an almost completely separate implementation.

@Igonato
Copy link

Igonato commented Dec 13, 2017

@axnsan12 nice! Excited to check it out. Did you see limdauto's attempt? What do you think about it and how's yours different?

@Igonato
Copy link

Igonato commented Dec 13, 2017

Sorry, I noticed you referred to it in your post by drf-openapi. Yea, it seems it went a bit stale

@carltongibson
Copy link
Collaborator

@axnsan12 Nice!

I was not really able to reuse much of the SchemaGenerator infrastructure apart from architectural guides

Yes. I expect you needs a custom SchemaGenerator and Inspectors. But my hope was that would be possible. Happy it was! 🙂

If you have specific issues/feedback from what you've learnt please do open new tickets with that!

General Update: We're looking to get better support for Response schemas (and other openapi aspects) in for 3.8 in the new year. (@axnsan12 I will be looking at your code for that 🙇🏽)

@tback
Copy link

tback commented Dec 3, 2018

It's been almost a year... Any news on this issue?

@carltongibson
Copy link
Collaborator

OK, state of play:

3.8 and 3.9 resolved a number of issues in the schema generation and then brought in OpenAPI as the default schema format.

What's left to do is create a straight-to-OpenAPI schema generator, thus dropping the Core API intermediate format, which cannot express response schema. I began such work in #6119 but it was too big a task for v3.9 so it was postponed. We've not yet had capacity to return to it. If anyone wants to contribute, picking that work up would be the fastest way forward.

In the meantime, I believe drf-yasg remains the best option. (Is that still maintained?)

(I will add an observation on drf-yasg for an eventual DRF implementation: drf-yasg has an openapi.py adding a Core API-like object model of the schema; in Core API we've found this layer limiting and maintenance-heavy; we're inclined towards a straight-to-dict approach. Obviously that's just an inclination.)

Also, your periodic reminder: http://www.commitstrip.com/en/2014/05/07/the-truth-behind-open-source-apps/

Hopefully that clears up any questions.

@axnsan12
Copy link
Contributor

axnsan12 commented Dec 4, 2018

(Is that still maintained?)

Maintained, but not actively worked on for now (see comic above 😄)

drf-yasg has an openapi.py adding a Core API-like object model of the schema; in Core API we've found this layer limiting and maintenance-heavy; we're inclined towards a straight-to-dict approach.

I would argue that it is not the case here, because the model objects you refer to are implemented directly as dict subclasses, and can be converted to or used as dicts at any point. In essence they are just helpers for generating valid dicts for OpenAPI.

What could be improved is the fact that the introspection code is not designed to work with plain dicts, but that shouldn't be too hard.

@RadoRado
Copy link

Sorry for bringing this up but is this still on the table for 3.10 (Referring to #6532) and is there any need for help?

Looks like a great thing to support, especially when OpenAPI is becoming the new default.

@carltongibson
Copy link
Collaborator

Yes. If you install the PR branch you can give it a run. We’re generating both request body and responses sections. Input making this as good as it can be is very welcome.

@RadoRado
Copy link

@carltongibson Awesome! Will give it a try and provide feedback 💪

@carltongibson
Copy link
Collaborator

3.12 includes support for OpenAPI components, with hooks to override. I take it that that will finally close this (although we were probably there before)

@AngellusMortis
Copy link

AngellusMortis commented May 3, 2021

Perhaps I am missing how components is suppose to solve having a different serializes for request/response. Can you provide an example of what you had in mind, @carltongibson?

Both get_request_body and get_responses on AudoSchema calls get_serializer(path, method) followed by _get_reference(serializer) (which in turn calls get_component_name(serializer)).

Have separate request/response components/serializers would still require you to completely override get_responses and patch how the serializer and/or reference/component are constructed.

(using 3.12.4)

EDIT: Nevermind, it seems master has changes that work differently. There is a get_response_serializer method. So this will require 3.13 for it all to work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests