Address client generation warnings
Creating this PR as a draft as I'm not fully convinced these changes would add value beyond cleaning up the client generation output.
Overview
Here's the breakdown of warnings (with the latest spec version at the time) before and after these changes:
| Validation | Before | After |
|---|---|---|
| ? | 10 | 10 |
| Modeler/MissingType | 39 | 29 |
| PreCheck/BinarySchema | 1 | 1 |
| PreCheck/CheckDuplicateSchemas | 1 | 1 |
| PreCheck/EmptyParentSchemaWarning | 40 | 40 |
| PreCheck/PropertyRedeclaration | 2 | 2 |
| PreCheck/SchemaMissingType | 193 | 24 |
| UnkownSecurityScheme | 1 | 1 |
I mainly focused on PreCheck/SchemaMissingType which also resolved a handful of Modeler/MissingType.
Those that remain are either expected or interdependent on others.
Observations
I made the following observations during this research. Some might require further testing but most will likely need to be reported upstream (autorest) for discussion.
Warning: ?
Operation 'cdn/purge_cache' really should not have a media type (because there should be no body)
This example is for DELETE /v2/cdn/endpoints/$ENDPOINT_ID/cache.
DELETE methods, by definition, should not send content.
Our API expects a content body with a list of cached file paths to be purged. In this case, our API breaks convention but the behavior is by design and documented.
Since this still results in a valid OpenAPI definition, we should be able to way to mark operations like this such that the generator knows not to report a warning.
Warning: Modeler/MissingType
The schema 'components·1yvwtk5·schemas·invoicesummary·properties·productcharges·allof·0' has no type or format information whatsoever.
This example warns that the first element in the allOf list of the prodcut_charges property doesn't have a type or format property set (the item with the description property)
specification/resources/billing/models/invoice_summary.yml#L43
product_charges:
allOf:
- description: >- # this is the item being warned on
A summary of the product usage charges contributing to the invoice.
This will include an amount, and grouped aggregates by resource type
under the `items` key.
- $ref: 'product_usage_charges.yml'
The problem I see with this warning is that allOf is supposed to be a way to merge multiple schemas.
While that item doesn't have a type, the second item is a reference to another schema that does have a type.
We wrote this with the assumption that the final schema, after being merged, would have the type of the last item.
As long as none of the listed schemas have conflicting types, I don't expect this to fail or warn.
Warning: PreCheck/CheckDuplicateSchemas
Checking for duplicate schemas, this could take a (long) while. Run with --verbose for more detail.
I don't think this should be marked as a warning as it's just informing the check is starting. No duplicates were found though.
Warning: PreCheck/EmptyParentSchemaWarning
The typical message for this warning looks like:
Schema 'invoicesummary-productcharges' has an allOf list with an empty object schema as a parent, removing it.
This example actually warns on the same property as listed above for the Modeler/MissingType warning (prouct_charges in inventory_summary.yml).
However, here it's referencing the parent of allOf - the product_charges property itself.
Similarly, the resulting merge of items defined in the allOf descriptor should result a schema with type: object (from the ref'd second item).
I would expect the generator to attempt the merge before determining that the property is empty.
Warning: PreCheck/PropertyRedeclaration
Schema 'volumeAction' has a property 'type' that is conflicting with a property in the parent schema 'action' differs more than just description : [example => 'attach_volume']
This is actually true. We've redeclared it with the allOf composition keyword.
We do this in several places. In most cases, we just update the description but we've also updated fields like min/max length for strings.
We also add fields, like nullable, required, or example (which is the case above).
The openapi docs have this note:
It is recommended to avoid using conflicting properties (like properties that have the same names, but different data types).
There are only two warnings for this. We might want to evaluate how we compose these models.
Warning: PreCheck/SchemaMissingType
The schema 'dropletcreate-sshkeysItem' with an undefined type and 'allOf'/'anyOf'/'oneOf' is a bit ambiguous. This has been auto-corrected to 'type:object'
This was the most prevalent warning due to how frequently we use the composition/inheritance/polymorphism keywords (allOf/anyOf/oneOf).
In many cases, we have an allOf list with one or more $ref-ed schemas and we add properties inline as another item.
Many of those inline schemas didn't have a type so the generator would warn there.
In many other cases, the entire schema could be defined in multiple $ref-s. While the referenced files have a type defined, the parent schema doesn't.
There are also a handful of cases where a property could be either a string or an integer as defined by a 4ref-ed schema.
However, the parent schema doesn't specify a type (by design) so the warning is reported.
I understand why most of these warnings are reported but it'd be nice if we could annotate objects (maybe with an extension) to silence known warnings so we don't lose new or unexpected issues in the noise.
For APICLI-1379