bug: API docs generator incorrectly chooses descriptions for `allOf` inherited schemas

pepopowitz commented 1 week ago

The plugin we're using to generate API docs is choosing the incorrect description for schemas that are inherited/extended via the allOf operator.

Example

The AuthorizationResponse, returned by the Authorization search request, has a resourceType property defined. This property includes a description, and inherits the ResourceTypeEnum schema via the allOf operator:

https://github.com/camunda/camunda-docs/blob/13b8540efdc4e2321db0548fc12b0bc3bcdbf286/api/camunda/camunda-openapi.yaml#L3852-L3856

The inherited ResourceTypeEnum schema itself also includes a description, which is different than the one above:

https://github.com/camunda/camunda-docs/blob/13b8540efdc4e2321db0548fc12b0bc3bcdbf286/api/camunda/camunda-openapi.yaml#L3763-L3764

Expected behavior

The rendered docs should use the description from the resourceType property, as that is expected to be more precise.

Actual behavior

The rendered docs use the description from the ResourceTypeEnum schema:

(from https://docs.camunda.io/docs/next/apis-tools/camunda-api-rest/specifications/find-authorizations/)

Impact

For the example described above, this bug is not very significant. The descriptions are very similar, and either one describes the field pretty well.

However, there is a lot of work in progress right now to abstract commonly used field types in the spec (example, example). In these cases, the descriptions describing the inherited schema are extremely generic (as they should be), and losing the parent description will damage our docs significantly.

For example,

Possible fixes

If we can get through a docusaurus 3 upgrade, and upgrade this plugin, the issue may resolve itself per this issue at the source.
In order to override the functionality of this plugin, I think we'd have to use patch-package to apply a patch directly in this repo -- I don't think there is any seam for us to customize the plugin functionality. This would also mean we'd have to figure out how to fix it in the source.

Other thoughts

Using allOf inheritance with a description is absolutely the correct way to author the schema, when a more specific description exists. There's no question in my mind the spec is correct, but the plugin is handling it incorrectly. More evidence -- https://editor.swagger.io/ handles the descriptions as described in "Expected behavior" above.

pepopowitz commented 1 week ago

Wellll actually......

I found a case where the correct descriptions were being emitted, and that enabled me to track down a way to solve this problem in the schema, without introducing too much docs-induced damage.

A schema that displays the correct property description

In the following schema, the processInstanceKey property renders the correct description in the docs. The field is described as "The key of this process instance.", which is accurate and precise, and makes sense.

    ProcessInstanceSearchQueryRequest:
      description: Process instance search request.
      allOf:
        - $ref: "#/components/schemas/SearchQueryRequest"
      type: object
      properties:
        filter:
          allOf:
            - $ref: "#/components/schemas/ProcessInstanceFilterRequest"     <!------ take note of this
    ProcessInstanceFilterRequest:
      description: Process instance search filter.
      type: object
      properties:
        processInstanceKey:
          description: The key of this process instance.
          allOf:
            - $ref: "#/components/schemas/LongFilterProperty"
    LongFilterProperty:
      description: Long property with full advanced search capabilities.
      type: object
      oneOf:
        - type: integer
          format: int64
        - $ref: "#/components/schemas/AdvancedLongFilter"

A schema that does not display the correct property description

The following schema does not work — the scopeKey description is ignored, and the docs render the description for the LongFilterProperty. This makes for really dumb docs, where the scope key was described as “Long property with full advanced search capabilities.“. This doesn't help anyone.

    VariableSearchQueryRequest:
      description: Variable search query request.
      allOf:
        - $ref: "#/components/schemas/SearchQueryRequest"
      type: object
      properties:
        filter:
          $ref: "#/components/schemas/VariableFilterRequest"         <!----- here's where the issue is
    VariableFilterRequest:
      description: Variable filter request.
      type: object
      properties:
...
        scopeKey:
          description: The key of the scope of this variable.
          allOf:
            - $ref: "#/components/schemas/LongFilterProperty"
    LongFilterProperty:
      description: Long property with full advanced search capabilities.
      type: object
      oneOf:
        - type: integer
          format: int64
        - $ref: "#/components/schemas/AdvancedLongFilter"

What's the difference?

The one that works inherits the filter property via allOf; the one that doesn't inherits via a direct $ref. Both of those are appropriate ways to inherit a schema; why the generator handles one properly but not the other, I don't know. And the fact that it affects a child property, instead of the filter property itself, is weird.

How should we fix this?

I had a recent conversation with @tmetzke about when to use each of those approaches, and based on this specific issue, I think I'm changing my mind. Since the generator can handle allOf inheritance properly, but not direct $ref, I think we should avoid direct $ref in the schema when possible. In that conversation I am the one who was nudging toward direct $ref, so I don't think anyone would be upset by me changing my mind.

Long term, it would really be nice if we didn't have to think about this at all. I think an update to docusaurus, so that we can update to the latest version of the plugin, is coming soon. Hopefully when that happens, we can stop worrying about it.

tmetzke commented 4 days ago

Interesting find, @pepopowitz 👍 So, in the end, it simply is a Docusaurus bug, right? What would you suggest for the time being we do in the OpenAPI spec when writing them currently?

Edit: I just saw your comment here, that question is resolved for me 👍

pepopowitz commented 4 days ago

yeah, sorry for not being more direct with my communication on this @tmetzke, it was a real emotional whirlwind for me 😅

To be more clear, direct, and succinct: it seems this is a bug with the generator plugin; however it is not as urgent as I initially feared because there is a reasonable workaround within the spec, one which we're already using in most places anyway.

camunda / camunda-docs