-
Notifications
You must be signed in to change notification settings - Fork 189
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
bug: API docs generator incorrectly chooses descriptions for allOf
inherited schemas
#4610
Comments
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 descriptionIn the following schema, the 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 descriptionThe following schema does not work — the 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 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 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. |
Interesting find, @pepopowitz 👍 Edit: I just saw your comment here, that question is resolved for me 👍 |
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. |
## Description 1. Adds a vacuum rule that requires a `description` on every property in a schema. 2. Adds a `description` to all properties that fail this new rule. 3. Modifies a few search query schemas to use `allOf` inheritance on their `filter` property, to accommodate [this issue from the docs](camunda/camunda-docs#4610 (comment)). ## What's not included I considered also adding a rule for operations that define request/response properties inline instead of through a schema. I decided against this, because there are not very many operations that define properties this way -- and [only one inline property right now does not have a `description`](https://github.com/camunda/camunda/blob/f0a7064d6a1370c83618b2749377e2970d99c4b6/zeebe/gateway-protocol/src/main/proto/rest-api.yaml#L2303). Let me know if you think I made the wrong choice, and should include a rule for this. ## I learned some things about schema inheritance, and I'm not sure this PR ended up in the right place. I think we are forced to choose the least bad option. **Edit:** we concluded that we would prefer `allOf` inheritance over direct `$ref`, especially because of camunda/camunda-docs#4610 (comment). See [this thread](#24294 (comment)) for more details. Basically, I ended up converting a handful of `allOf` inherited schemas to direct `$ref` inheritance. During some investigation about specific properties I came to better understand the difference between these two approaches, and I felt this better portrayed the schema. However, the conversion _does_ propagate a pattern that _could_ result in a non-obvious crash for developers, if they introduce direct `$ref` inheritance of a schema that lacks a `description`. On the other hand, with `allOf` inheritance, if a property is lacking a `description` in the parent schema, there is no crash, but _vacuum still fails with a red state but no description why_. In addition, the docs render the inherited description of a property that uses direct `$ref` inheritance. They do not render an explicit description on an `allOf` inherited property. So it's tradeoffs. Here's a table defining the approaches, consolidating the pros/cons that I just described. | Inheritance approach | Pros | Cons | | -- | -- | -- | | direct `$ref` | a more accurate portrayal of the schema | crashes vacuum if we miss a description on an inherited schema | | | displays the inherited schema's description in the docs | | | `allOf` | does not crash vacuum if a property is missing a description | fails validation with no error if a property is missing a description | | | | does not display the property description in the docs | One other thought is that turning the `component-description` to `error` severity might result in fewer crashes. This doesn't prevent the crash _directly_; if an inherited schema lacks a property, the crash happens rather than the `component-description` violation being emitted. It _would_ require us to fill in more schema-level descriptions, though, which would give developers better examples to copy-paste when they add a schema. I don't know....what do you think???
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 aresourceType
property defined. This property includes adescription
, and inherits theResourceTypeEnum
schema via theallOf
operator:camunda-docs/api/camunda/camunda-openapi.yaml
Lines 3852 to 3856 in 13b8540
The inherited
ResourceTypeEnum
schema itself also includes adescription
, which is different than the one above:camunda-docs/api/camunda/camunda-openapi.yaml
Lines 3763 to 3764 in 13b8540
Expected behavior
The rendered docs should use the
description
from theresourceType
property, as that is expected to be more precise.Actual behavior
The rendered docs use the
description
from theResourceTypeEnum
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
Other thoughts
allOf
inheritance with adescription
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.The text was updated successfully, but these errors were encountered: