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

AWS2OPENAPI-45 fix: remove generic 'allOf' composites #46

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ivy-rew
Copy link

@ivy-rew ivy-rew commented Aug 19, 2021

  • only keep them where we actually have multiple types that are accepted

- only keep them where we actually have multiple types that are accepted
@ivy-rew
Copy link
Author

ivy-rew commented Aug 23, 2021

Hi @MikeRalphson
Thanks for looking at this PR. Is there any work left to do until it can be merged?
Can I help somehow?

@MikeRalphson
Copy link
Contributor

Can you expand a little on why an allOf in a schema object would be "unexpected"?

https://github.com/APIs-guru/openapi-directory/blob/1772399f6fedfd650b6eff7c9a6b35c4c04b732d/APIs/amazonaws.com/runtime.lex.v2/2020-08-07/openapi.yaml#L393-L395

Your PR appears to replace these with

    ProgressUpdateStream:
          $ref: '#/components/schemas/ProgressUpdateStream'
          description: 'The name of the ProgressUpdateStream. '

which is less valid according to the spec, because in OAS 3.0 sibling properties of a $ref MUST be ignored.

There are 5 pages of open issues on swagger-codegen about $ref and allOf and I don't think working around them by tweaking the output of this project is really practical.

Could you test to see if something like

  description: foo
  allOf:
    - $ref: '#/components/schemas/bar'

solves the problem you're having with code-gen?

@ivy-rew
Copy link
Author

ivy-rew commented Aug 24, 2021

Thanks for the update @MikeRalphson
I wasn't aware of the restriction, that $ref schema should not have sibling properties. Therefore indeed your current schema looks more compliant.
And I see that it doesn't make too much sense to bend the world too much around swagger-codegen and rather suggest a fix in the swagger repos.

I've tested manually your proposal, to place the description on the same level as the 'allOf' property. openapi-lex2.yaml
And yes, this solves the most severe problem of the swagger-codegen stack: I no longer get invalid java sources that can't be compiled.
So I'd vote to change the AWSOPENAPI generator towards that end. Should I fire another PR for this?

However, due to the generic 'allOf' reference, I still loose all the 'type' support in the swagger generated java classes. I'm left with only 'object' references for the available properties. Which makes the generated output less attractive, especially in the complex type hierarchy of this amazon.lex service.

@MikeRalphson
Copy link
Contributor

I've tested manually your proposal, to place the description on the same level as the 'allOf' property. And yes, this solves the most severe problem of the swagger-codegen stack: I no longer get invalid java sources that can't be compiled.
So I'd vote to change the AWSOPENAPI generator towards that end. Should I fire another PR for this?

Yes, a new or updated PR to use this pattern for $ref + description would be welcome.

However, due to the generic 'allOf' reference, I still loose all the 'type' support in the swagger generated java classes. I'm left with only 'object' references for the available properties. Which makes the generated output less attractive, especially in the complex type hierarchy of this amazon.lex service.

That definitely sounds like a bug in the codegen tool.

makes the spec more compliant with swagger-codegen
@ivy-rew
Copy link
Author

ivy-rew commented Aug 24, 2021

Update the PR as discussed: There are still some structures that actually have properties other than $ref within the 'allOf' composites. See the 'xml' properties in ...
amazonaws.com/apigateway/2015-07-09/openapi.yaml

        items:
          description: The current page of elements from this collection.
          allOf:
            - $ref: '#/components/schemas/ListOfApiKey'
            - xml:
                name: item

I guess this is ok and still valid.

@ivy-rew
Copy link
Author

ivy-rew commented Oct 12, 2021

Anything left to do here @MikeRalphson ?

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.

unexpected multi-type 'allOff' references in runtime.lex.v2 spec
2 participants