[core] Provide User feedback on confusing use of oneOf by jimschubert · Pull Request #4695 · OpenAPITools/openapi-generator

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

jimschubert · 2019-12-04T05:14:27Z

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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.

auto-labeler · 2019-12-04T05:14:33Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

spacether · 2019-12-04T05:22:34Z

 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);


Are we deleting this because it is unused?

Yeah. This would fail compilation of it was still expected.

spacether · 2019-12-04T05:24:16Z

@jimschubert this looks good. Should we add tests?

jimschubert · 2019-12-05T00:25:05Z

@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.

spacether · 2019-12-05T00:26:09Z

@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.

auto-labelerBot added the Enhancement: General label Dec 4, 2019

spacether reviewed Dec 4, 2019
View reviewed changes

jimschubert added this to the 4.2.3 milestone Dec 7, 2019

jimschubert merged commit 0c00505 into masterDec 7, 2019