[core] Provide User feedback on confusing use of oneOf #4695
Conversation
OAS 3.x specification isn't entirely clear on oneOf support. JSON Schema defines oneOf in such a way that the Schema is only valid if it validates against exactly one of the referenced Schemas. This suggests that a Schema defined with oneOf can't include additional "dynamic" properties. OpenAPI extends on this by adding the necessary discriminator object, which allows tooling to decide the intended Schema. As tooling, openapi-generator may support loose or confusing definitions in the Specification to better support our user's use cases. In this case, we may warn that while this usage is technically valid the two target specifications are unclear about the actual constraints regarding oneOf.
|
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
| // parent model | ||
| final String parentName = ModelUtils.getParentName(composed, allDefinitions); | ||
| final List<String> allParents = ModelUtils.getAllParentsName(composed, allDefinitions); | ||
| final Schema parent = StringUtils.isBlank(parentName) || allDefinitions == null ? null : allDefinitions.get(parentName); | ||
| final boolean hasParent = StringUtils.isNotBlank(parentName); |
There was a problem hiding this comment.
Are we deleting this because it is unused?
There was a problem hiding this comment.
Yeah. This would fail compilation of it was still expected.
|
@jimschubert this looks good. Should we add tests? |
|
@spacether I'm not sure how we'd add tests. The changes in non-cli code is logging and a special case to test a minimal scenario which resulted in an NPE. That NPE will need to be investigated later. For the CLI, we don't have tests that verify generated files. The change in this PR only results in stdout text, for which we don't have tests on other commands. |
Good to know, sounds like adding them would be more hassle than the benefit that we would get. |
OAS 3.x specification isn't entirely clear on oneOf support. JSON Schema
defines oneOf in such a way that the Schema is only valid if it
validates against exactly one of the referenced Schemas. This suggests
that a Schema defined with oneOf can't include additional "dynamic"
properties. OpenAPI extends on this by adding the necessary
discriminator object, which allows tooling to decide the intended
Schema.
As tooling, openapi-generator may support loose or confusing definitions
in the Specification to better support our user's use cases. In this
case, we may warn that while this usage is technically valid the two
target specifications are unclear about the actual constraints regarding
oneOf.
This adds the warning and recommendation I proposed in review of #4482
cc @spacether @OpenAPITools/generator-core-team
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.