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

{format} #110

Closed
jayvdb opened this issue Jul 1, 2020 · 12 comments
Closed

{format} #110

jayvdb opened this issue Jul 1, 2020 · 12 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jul 1, 2020

{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
Copy link
Contributor Author

jayvdb commented Jul 1, 2020

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

@tfranzel
Copy link
Owner

tfranzel commented Jul 1, 2020

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
Copy link
Contributor Author

jayvdb commented Jul 1, 2020

tl;dr

  • better handling of the path parameter.

    • While they are required, couldnt it be an empty string, or enum including empty string?
    • replace "string" type with explicit list of values that DRF will accept - others generate an error.
  • add query parameter so .../?format=yaml is part of the generated spec.

@tfranzel
Copy link
Owner

tfranzel commented Jul 1, 2020

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

jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 11, 2020
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 11, 2020
Sort foo{arg} after foo/, but before foo/bar.

Related to tfranzel#110
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 11, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 12, 2020

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: encode/django-rest-framework#7405

#123 for some of the easy improvements.

Still lots to do.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 13, 2020

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.

jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
Also document custom DEFAULT_GENERATOR_CLASS, and
api_settings.DEFAULT_SCHEMA_CLASS .

Related to tfranzel#110
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
Sort foo{arg} after foo/, but before foo/bar.

Related to tfranzel#110
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
Use DRF renderer based list of possible formats, and filtered
by the URLConf-based `format_suffix_patterns` formats if used.

Related to tfranzel#110
jayvdb added a commit to jayvdb/drf-spectacular that referenced this issue Jul 14, 2020
Sort foo{arg} after foo/, but before foo/bar.

Related to tfranzel#110
@tfranzel
Copy link
Owner

closed by #123

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 16, 2020

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

@tfranzel
Copy link
Owner

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
Copy link
Contributor Author

jayvdb commented Jul 16, 2020

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

Open PR #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 tfranzel reopened this Jul 16, 2020
tfranzel added a commit that referenced this issue Jul 17, 2020
@tfranzel
Copy link
Owner

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
Copy link
Owner

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:

  • the overlooked issue with literal {format} is fixed by 8876f39
  • handling of URL_FORMAT_OVERRIDE
  • DRF format improvements #123
  • PREPROCESSING_HOOKS for filtering out format endpoints

Dark-217 added a commit to Dark-217/drf-spectacular that referenced this issue Oct 7, 2024
Use DRF renderer based list of possible formats, and filtered
by the URLConf-based `format_suffix_patterns` formats if used.

Related to tfranzel/drf-spectacular#110
Dark-217 added a commit to Dark-217/drf-spectacular that referenced this issue Oct 7, 2024
Sort foo{arg} after foo/, but before foo/bar.

Related to tfranzel/drf-spectacular#110
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

No branches or pull requests

2 participants