tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.4k stars 266 forks source link

{format} #110

Closed jayvdb closed 4 years ago

jayvdb commented 4 years ago

{format} currently creates a duplicate end-point with {format} literally in the OpenAPI endpoint path, add the duplicate includes a path parameter of type 'string'.

e.g.

/oscarapi/baskets/{basket_pk}/lines/{id}/
/oscarapi/baskets/{basket_pk}/lines/{id}{format}

They should not be considered distinct endpoints - effectively / is a added by Django's APPEND_SLASH = True setting, and the duplication in the paths is caused by that and the fact that foo{format} must be able to have non-/ values because it supports foo.json, not foo/.json. The trailing / is just "no url format" for the same end-point, deferring to content-type header.

It should be an non-mandatory enum, typically it will contain only literals ['/', '.json'], or just ['.json'] if APPEND_SLASH = False. The allowable values are dynamic, depending on the view, or DEFAULT_RENDERER_CLASSES.

{format} also provides a URL query argument, as described at https://www.django-rest-framework.org/api-guide/format-suffixes/#query-parameter-formats . This should be included in the OpenAPI as an in:query parameter. https://swagger.io/docs/specification/describing-parameters/#query-parameters . I have not tried url .../blah.json?format=yaml

Worth noting, DRF setting 'URL_FORMAT_OVERRIDE': None, does not appear to prevent this {format} behaviour (@master drf). Maybe it only inhibits the addition of the format parameter when not specified by the view. i.e. django-oscar-api explicitly has a format arg in all the view action methods. I'll dig into this aspect in the drf codebase to find out the specifics of why I dont seem to be able to turn it off. May also be some package doing monkey patching, or altering of live settings.

IMO even if the settings are set up to allow .json, I can forsee interest in preventing it in their API docs, so that {format} becomes a mandatory / in the path, and the format path arg becomes redundant. The reason is that .json is handy/quick for devs to pick a content type for get requests explicitly in the urlbar etc, but that isnt worth the generated OpenAPI being more complicated. Some sites might consider it it is a hidden feature that isnt supported.

jayvdb commented 4 years ago

The DRF HTML views also do not respect 'URL_FORMAT_OVERRIDE': None, so it seems to not be well implemented.

tfranzel commented 4 years ago

i understand the context of what you are saying but what would be the concrete actions points for us? i understand that the DRF handling of those parameters is weird. what can we achieve here?

if you suggest unifying the format endpoint with the regular one, i'm afraid that is not possible atm. it would require having the id/{format} and making the format an optional parameter. unfortunately, parameters of type path are by definition required=True by the OpenAPI3 specification. that would be the showstopper here.

jayvdb commented 4 years ago

tl;dr

tfranzel commented 4 years ago

actually found a bug when i looked up the code: 6b29d1b

jayvdb commented 4 years ago

Update:

Setting DRF setting 'URL_FORMAT_OVERRIDE': None does not prevent {format} because oscar-api uses DRF rest_framework.urlpatterns.format_suffix_patterns to force it.

The DRF HTML views also do not respect 'URL_FORMAT_OVERRIDE': None, so it seems to not be well implemented.

This can be seen on the APIBrowser views, there is a format UI widget, and it shows all of the formats, even if URL_FORMAT_OVERRIDE has been disabled. I'll raise that upstream to get some clarity on it.

One upstream PR done, removing some unnecessary/no-longer-needed complexity: https://github.com/encode/django-rest-framework/pull/7405

https://github.com/tfranzel/drf-spectacular/pull/123 for some of the easy improvements.

Still lots to do.

jayvdb commented 4 years ago

What about an setting to ignore any duplicated endpoint that only adds path {format}?

And for APIs which actually want .json, I am sure they would prefer that the endpoint path includes the .json instead of {format}, and they would probably usually prefer that even if they have multiple render format classes, as they would have a preferred format that is used in their prose documentation. It is very rare to see written documentation which covers multiple media types for input or output, as json is so dominant in that space.

That can be done in the end-point generator, very early in the processing, eliminating what is effectively duplicate work. De-duplicating {format} endpoints it in this phase will be more efficient if done after the endpoint de-duplication is already done for identical endpoint paths coming from different includes (e.g. the oscarapi & oscarapicheck scenario), and the endpoint sorting to move pi{format} immediately after pi.

Alternatively, it can be done as post-processing helper, but that doesn't perform as well.

tfranzel commented 4 years ago

closed by #123

jayvdb commented 4 years ago

This is not fixed yet; so many aspects not done yet.

tfranzel commented 4 years ago

fair enough, but we cleared most points i think. can you summarize the open points? i can see at least .../?format=yaml and URL_FORMAT_OVERRIDE. depending on the scope we could open a new issue but i leave that up to you.

jayvdb commented 4 years ago

Please re-open the issue. I can not. I know how to use Closing .. vs other commit messages, and use them appropriately.

Open PR https://github.com/tfranzel/drf-spectacular/pull/124 directly relates to this.

And ?format=.. is another significant chunk needed to accurately provide format support, but it is only one of two unimplemented mechanisms of defining formats.

And there are at least three other ugly bugs after that PR, described above, at least one of which requires another fix for the endpoint enumerator provided by DRF core.

tfranzel commented 4 years ago

regarding the 2 unimplemented:

  1. format query param added. depending on URL_FORMAT_OVERRIDE. only shown when there is more than one choice to reduce noise.
  2. accept header is not supposed to be explicitly modeled in the schema. found out after i had it implemented.

    If in is "header" and the name field is "Accept", "Content-Type" or "Authorization", the parameter definition SHALL be ignored.

tfranzel commented 4 years ago

closing this issue because most points got resolved and it is unclear what is still unresolved. happy to tackle remaining issues in a new thread.

several commits have addressed points that were made here: