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

Move the "description" out of "allOf"-declaration #204

Closed
maxl2287 opened this issue Aug 17, 2023 · 8 comments · Fixed by #205
Closed

Move the "description" out of "allOf"-declaration #204

maxl2287 opened this issue Aug 17, 2023 · 8 comments · Fixed by #205

Comments

@maxl2287
Copy link
Collaborator

Problem description
"allOf" including a $ref and a 'description' is not compatible with openapi - generators for later implemantation usage.

    CreateSession:
      description: Attributes required to create a session
      type: object
      properties:
        <...>
        devicePorts:
          allOf:
            - description: The ports used locally by the device for flows to which the requested QoS profile should apply. If omitted, then the qosProfile will apply to all flows between the device and the specified application server address and ports
            - $ref: "#/components/schemas/PortsSpec" 
        applicationServerPorts:
          allOf:
            - description: A list of single ports or port ranges on the application server
            - $ref: "#/components/schemas/PortsSpec"
        <...>

Expected behavior
The description should be out of the "allOf".

    CreateSession:
      <...>
        devicePorts:
          description: The ports used locally by the device for flows to which the requested QoS profile should apply. If omitted, then the qosProfile will apply to all flows between the device and the specified application server address and ports
          allOf:
            - $ref: "#/components/schemas/PortsSpec"
        applicationServerPorts:
          description: A list of single ports or port ranges on the application server
          allOf:
            - $ref: "#/components/schemas/PortsSpec"
         <...>
@RandyLevensalor
Copy link
Collaborator

@maxl2287 Thanks for sharing your issues with open api generators.

The allOf with the description was added so that when the OpenAPI documentation is rendered it displays the top level description and not the description of the $ref. We use this in both redoc and the swagger editor

To my knowledge, this is consistent with the Open API 3.0 specification. Please share if you find a reference in the spec where this isn't compliant.

Which openapi generators are you having issues with? Have you confirmed that this isn't a bug with the generator?

@jlurien I believe that you helped with this formating. Can you comment as well?

@eric-murray
Copy link
Collaborator

Both the Swagger editor and ReDoc appear happy with this alternative construct, so I would support this for v0.10.0 onwards.

For v0.9.0 however, I think that should remain as it is, as this is a tooling rather than specification issue, and a release candidate version was available for some time to give developers an opportunity to pick up on such issues with their tooling. It is not so difficult to make the changes manually if it is desired to implement v0.9.0 and your tooling objects.

Alternatively, if we update v0.10.0-wip now, then that updated version (which will remain available through github, even if subsequent changes are made) will still effectively be v0.9.0, and can be used for implementation with tooling that experiences this issue.

It was proposed in Commonalities that we adopt OAS v3.1 so that we can use reference objects that support descriptions without the need for the allOf workaround. If you have an opinion on this, please comment that issue.

@maxl2287
Copy link
Collaborator Author

@RandyLevensalor

@eric-murray I guess when we will have v3.1 then maybe these "allOf"-behaviours may also be gone, yes.

@patrice-conil
Copy link
Collaborator

Hi @maxl2287,
I had the same problem and offered to migrate to oas 3.1.0 in commonalities/issue#11.
Please take a look and add your comments.

@jlurien
Copy link
Collaborator

jlurien commented Aug 25, 2023

The alternate proposal is OK as it preserves the principle of $ref not having sibling elements. OAS 3.1 would also solve this but migrating to it has other implications. It allows to use all the potential JSON-schema to define more complex schemas for objects, which is nice, but many tools don't support it properly, so we'd probably have to limit in the guidelines how to use it or face many issues complaining about certain tools not supporting this or that.

@hdamker
Copy link
Collaborator

hdamker commented Aug 25, 2023

I'm here with @eric-murray and @jlurien

  • we should not change v0.9.0
  • we shouldn't rush to OAS 3.1, at least not without additional guidelines
  • if it works fine for all we can accept the PR from @maxl2287 for v0.10.0-wip

@jlurien
Copy link
Collaborator

jlurien commented Aug 25, 2023

The new feature in OAS 3.1 is not supported by the viewers (Redoc, Swagger editor), while the alternate proposal is, so I would stick to the new alternate version. About fixing 0.9.x, it depends on the severity of the issue with the tooling. It is not a functional change so release scope is not altered.

@patrice-conil
Copy link
Collaborator

Hi all,
Openapi-generator 7.0.0-beta supports allOf with description inside but generates unwanted extra models...as explained in [commons issue 11](https://github.com/camaraproject/ Commonalities/issues/11) I had to rewrite all the definitions before generating the code. I provided a rewritten version of the spec in PI2 that one could use as a workaround.

And as I said in commonalities, editor-next.swagger.io supports version 3.1 as a redocly viewer in IntelliJ.
I vote to accept PR for 0.10.0-wip ... because we can live with this workaround in 0.9.0.

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 a pull request may close this issue.

6 participants