aws2openapi icon indicating copy to clipboard operation
aws2openapi copied to clipboard

AWS2OPENAPI-45 fix: remove generic 'allOf' composites

Open ivy-rew opened this issue 4 years ago • 6 comments

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

ivy-rew avatar Aug 19 '21 14:08 ivy-rew

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

ivy-rew avatar Aug 23 '21 13:08 ivy-rew

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?

MikeRalphson avatar Aug 24 '21 08:08 MikeRalphson

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.

ivy-rew avatar Aug 24 '21 08:08 ivy-rew

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.

MikeRalphson avatar Aug 24 '21 08:08 MikeRalphson

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 avatar Aug 24 '21 09:08 ivy-rew

Anything left to do here @MikeRalphson ?

ivy-rew avatar Oct 12 '21 09:10 ivy-rew