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

Disable yaml aliases for generateschema #7131

Merged
merged 3 commits into from Feb 3, 2020
Merged

Disable yaml aliases for generateschema #7131

merged 3 commits into from Feb 3, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 9, 2020

@carltongibson as requested in #7096

when objects are reused in a structure, yaml creates aliases for those objects.

  • makes the schema harder to read with only alias tags like &id001 instead of proper names
  • usage of advanced yaml feature possibly not supported everywhere
  • the openapi client generators also chokes on that feature

(will push fix once test fails)

@@ -1053,6 +1053,8 @@ def __init__(self):
assert yaml, 'Using OpenAPIRenderer, but `pyyaml` is not installed.'

def render(self, data, media_type=None, renderer_context=None):
# disable yaml advanced feature 'alias' for clean, portable, and readable output
yaml.Dumper.ignore_aliases = lambda *args: True
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to do this without overriding ignore_aliases on the Dumper class? I'm wondering what the interaction is for anyone else who wishes to use yaml.dump after this call.

@carltongibson
Copy link
Collaborator

the openapi client generators also chokes on that feature

(Just checking) but it is valid yaml being generated yes? So surely this last is a bug in whatever yaml parser these generators are using?

@ghost
Copy link
Author

ghost commented Jan 10, 2020

The generator i use for testing is https://github.com/OpenAPITools/openapi-generator, which i believe is the defacto standard besides the swagger one. it will choke with errors like:

attribute paths.'/api/users/password/'(post).requestBody.content.schema is not of type `object`

because it does not recognize the alias labels:

        content:
          application/json:
           schema: &id017
             $ref: '#/components/schemas/SetPassword'
           application/x-www-form-urlencoded:
             schema: *id017
           multipart/form-data:
             schema: *id017

As the yaml is valid i think (no expert on that), you could say it is an upstream bug. So we can either do a upstream PR to fix this or only use the most common set of yaml features. In my opinion the aliases add complexity, non-descript names without much of a benefit except slightly lower schema file size.

@kevin-brown Absolutely. Unfortunately there was no option for that. The last time i tried with an explicit Dumper it had no effect. Maybe i had an old pyyaml version. It works now.

yaml/pyyaml#103
https://stackoverflow.com/questions/13518819/avoid-references-in-pyyaml

@carltongibson
Copy link
Collaborator

@tfranzel-cashlink Can you open an issue on openapi-generator to at least see what they say?

@ghost
Copy link
Author

ghost commented Feb 3, 2020

Actually there is: OpenAPITools/openapi-generator#1593
there is also this: https://stackoverflow.com/questions/46689801/yaml-jackson-anchor-keys-of-array

apparently the problem lies deeper (openapig-generator -> Jackson lib -> SnakeYaml lib)

Nonetheless, i have not seen any openapi spec yet that uses aliases. Upstream bug or not, it will not go through the de-facto standard generator, which makes the generated schema rather pointless to begin with.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

I´m not sure I´m convinced by the de facto here. There are lots of tools out there. However...

Thanks for the extra references. I need to look into those and OpenAPI Generator but the immediate work-around is to use a custom renderer with your fix in place. That gets you going at least.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

@tfranzel-cashlink It looks like you´ve resolved @kevin-brown´s worry so let´s have this. Thanks for the effort.

@carltongibson carltongibson merged commit 4137ef4 into encode:master Feb 3, 2020
@ghost ghost deleted the oap3-ext-part1.2 branch February 13, 2020 07:04
@adnathanail
Copy link

Would you be able to release a version with this change in?

@carltongibson
Copy link
Collaborator

We're working on 3.12 currently. It'll be in that.

Pip can help you if you're in a rush...

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants