[codegen] Improve java code comments and argument documentation; Fix issue with ComposedSchema that has undeclared properties - #5316
Conversation
|
This PR is also a fix for #5317 |
sebastien-rosset
changed the title
There was a problem hiding this comment.
@wing328 IMHO the title should never be used for code generation, but I'm not sure how to replace with another (better) logic. I am tracking this issue in #5248. One potential issue is that some OAS documents may have been written specifically for OpenAPITools, hence removing the title logic would end up breaking these specs. This could be addressed with a --use-title-attribute-to-flatten-inline-models-but-really-don-t-do-this command-line argument.
On the other hand, I argue it's just wrong to use the "title" and we should move away from it.
| // The 'title' attribute is supposed to be for human consumption, not for code generation. | ||
| // OAS authors should not be expected to set a 'title' value that will control the | ||
| // code generation logic. | ||
| String innerModelName = resolveModelName(op.getTitle(), key); |
There was a problem hiding this comment.
IMO, the "title" attribute in a OAS document should not have any impact whatsover on the code generation (codegen currently uses title to flatten inline schemas). Using 'title' can lead to completely unexpected outcomes for the generated code.
The OAS spec clearly states the 'title' attribute is for human consumption, so codegen should not use it.
I was working with a draft OAS document that has a composed allOf schema. The "title" of the inner schema happens to be the name of the outer (composed) object, but it could also have been any value, including names of totally unrelated schema.
That had the weird side effect to nullify the inheritance hierarchy. Then you have to debug the code to finally understand why the "parent" attribute ends up being nullified as a side effect of the "title" attribute
There was a problem hiding this comment.
I agree with this, and always thought it was odd.
Maybe we should consider it a bug and replace it's support with a vendor extension in 4.3.0. We don't have any metrics about who is actually using the feature, but I doubt anyone is because:
- it's not really documented
- it's confusing
- it may block users with well-defined specs from using the tooling
There was a problem hiding this comment.
Based on the discussion in #5248 I'll file a PR to revise the wording here.
jimschubert
left a comment
jimschubert
left a comment
There was a problem hiding this comment.
Thank you for increasing the standards in our code for quality comments!
|
Thank you @jimschubert ! |
…issue with ComposedSchema that has undeclared properties (OpenAPITools#5316) * improve documentation * Add use case of composed schema with additional properties
3 participants
Add code comments in ModelUtils and documentation for the --generate-alias-as-model argument.
PR checklist
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.