[Java] [Spring] OpenAPI normalizer USE_UNWRAPPED_FOR_COMPOSITE_ONEOF for mixed OneOf support and jackson JsonUnwrapped#23761
Conversation
…e/inlineOneOfWithProperties
merge master
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache:19">
P1: Unsynchronized lazy initialization can overwrite a concurrently supplied custom mapper and make mapper configuration nondeterministic.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java:2872">
P1: Unconditionally wrapping `importMapping.get("JsonNode")` in `Map.of(...)` can NPE when that mapping is absent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,44 @@ | |||
| package {{invokerPackage}}; | |||
There was a problem hiding this comment.
P1: Unsynchronized lazy initialization can overwrite a concurrently supplied custom mapper and make mapper configuration nondeterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache, line 19:
<comment>Unsynchronized lazy initialization can overwrite a concurrently supplied custom mapper and make mapper configuration nondeterministic.</comment>
<file context>
@@ -0,0 +1,44 @@
+ Get the {{vendorExtensions.x-jackson-mixins-mapper}} used by the @JsonCreator in @JsonUnWrapped interfaces.
+ */
+ public static {{vendorExtensions.x-jackson-mixins-mapper}} getMapper() {
+ if (INSTANCE == null) {
+ setBuilder({{^useJackson3}}JsonMapper.builder().findAndAddModules(){{/useJackson3}}{{#useJackson3}}JsonMapper.shared().rebuild(){{/useJackson3}});
+ }
</file context>
There was a problem hiding this comment.
4 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java:2751">
P2: Reads `x-one-of-name` from the generator-level `vendorExtensions` map instead of the current schema, so the schema-scoped oneOf name added during preprocessing is ignored.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java:2830">
P1: Wrapper schema names are deterministic and inserted without any collision check, so this can overwrite an existing component schema.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java:2836">
P2: Hardcoded synthetic property name "oneOf" can silently overwrite an existing schema property with the same name</violation>
<violation number="3" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java:2872">
P2: Unconditional `Map.of` call can throw if `JsonNode` is not mapped for this generator.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (property.dataType != null && property.dataType.equals(property.name) && property.dataType.toUpperCase(Locale.ROOT).equals(property.name)) { | ||
| property.name = property.name.toLowerCase(Locale.ROOT); | ||
| } | ||
| if (property.getVendorExtensions().containsKey("x-unwrappedOneOf")) { |
There was a problem hiding this comment.
Could we add this to the vendor extensions that are defined in CodegenConstants to encourage reuse of vendor extensions between language implementations.
| } | ||
|
|
||
| @Override | ||
| protected void preprocessMixedOneOf(Schema s, String schemaName) { |
There was a problem hiding this comment.
Should this discriminator logic be moved out into a separate component to highlight that it might be of "general interest" between languages rather than strictly tied to Java?
I find that the library is now starting to support a lot of scenarios, and especially that Java spearheads those changes with your contributions. I believe that it would be preferable if the logic for handling these scenarios was generalized directly if possible.
There was a problem hiding this comment.
@Mattias-Sehlstedt initially I put the logic as an OpenapiNormalizer. It was in fact simpler than finding the right place in the DefaultCodeGen.
Proposal:
- The normalizer implements the logic of
AbstractJavaCodeGen.preprocessMixedOneOfwith a new ruleUSE_WRAPPER_FOR_MIXED_ONE_OF - Remove the option from the java generators. The generators detect the constant
x-unwrappedOneOfand configure the imports and the correct vendor extensions (also using constants) - moving a few common functionalities from JavaClientCodeGen and SpringCodeGen to AbstractJavaCodeGen (isJackson3(), getConfigOrInvokerPackage()....)
What do you think?
There was a problem hiding this comment.
@Mattias-Sehlstedt now an OpenapiNormalizer. The downside is the naming convention (OneOf sufix)
There was a problem hiding this comment.
It being handled as a normalized I believe is generally preferred since then the codegen logic can be centralized towards "best practices" regarding how to express concepts with OAS.
For the suffix, is that technically something that would be possible to attached to the naming mappers? (e.g., the inline naming one). I have personally not used them myself since I have always avoided inline models.
# Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java
There was a problem hiding this comment.
1 issue found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java">
<violation number="1">
P1: NullPointerException risk when components.schemas is null in preprocessOpenAPI</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@jpfinne I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache:44">
P1: Class closing brace is outside the conditional Mustache section, producing invalid Java if the section is omitted</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java:1108">
P1: NullPointerException in processUnwrapCompositeOneOf when oneOf list contains null elements. normalizeOneOf explicitly tolerates null entries (`if (item == null) continue;`) but processUnwrapCompositeOneOf's stream allMatch dereferences elements without null checking (`oneOf.get$ref()`), causing an NPE.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java:1125">
P2: Synthetic `oneOf` property can silently overwrite an existing user-defined `oneOf` property</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .addMixIn({{.}}.class, {{.}}.{{.}}Mixin.class){{/mixins}} | ||
| .build(); | ||
| } | ||
| {{/vendorExtensions.x-jacksonMixinConfig}} |
There was a problem hiding this comment.
P1: Class closing brace is outside the conditional Mustache section, producing invalid Java if the section is omitted
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/Java/jacksonMixinConfig.mustache, line 44:
<comment>Class closing brace is outside the conditional Mustache section, producing invalid Java if the section is omitted</comment>
<file context>
@@ -0,0 +1,45 @@
+ .addMixIn({{.}}.class, {{.}}.{{.}}Mixin.class){{/mixins}}
+ .build();
+ }
+ {{/vendorExtensions.x-jacksonMixinConfig}}
+}
\ No newline at end of file
</file context>
| boolean hasDiscriminator = schema.getDiscriminator() != null; | ||
| if (hasDiscriminator) { | ||
| List<Schema> oneOfs = schema.getOneOf(); | ||
| if (oneOfs.stream().allMatch(oneOf -> oneOf.get$ref() != null)) { |
There was a problem hiding this comment.
P1: NullPointerException in processUnwrapCompositeOneOf when oneOf list contains null elements. normalizeOneOf explicitly tolerates null entries (if (item == null) continue;) but processUnwrapCompositeOneOf's stream allMatch dereferences elements without null checking (oneOf.get$ref()), causing an NPE.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java, line 1108:
<comment>NullPointerException in processUnwrapCompositeOneOf when oneOf list contains null elements. normalizeOneOf explicitly tolerates null entries (`if (item == null) continue;`) but processUnwrapCompositeOneOf's stream allMatch dereferences elements without null checking (`oneOf.get$ref()`), causing an NPE.</comment>
<file context>
@@ -1087,6 +1091,49 @@ protected Schema normalizeOneOf(Schema schema, Set<Schema> visitedSchemas) {
+ boolean hasDiscriminator = schema.getDiscriminator() != null;
+ if (hasDiscriminator) {
+ List<Schema> oneOfs = schema.getOneOf();
+ if (oneOfs.stream().allMatch(oneOf -> oneOf.get$ref() != null)) {
+ // skip normalization if discriminator but not maping
+ if (discriminator.getMapping() == null && discriminator.getPropertyName() != null) {
</file context>
| if (oneOfs.stream().allMatch(oneOf -> oneOf.get$ref() != null)) { | |
| if (oneOfs.stream().allMatch(oneOf -> oneOf != null && oneOf.get$ref() != null)) { |
| newOneOfSchema.addExtension(X_ONE_OF_UNWRAPPED, true); | ||
| schema.oneOf(null); | ||
| // TODO: configuration of the property name | ||
| String propertyName = "oneOf"; |
There was a problem hiding this comment.
P2: Synthetic oneOf property can silently overwrite an existing user-defined oneOf property
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java, line 1125:
<comment>Synthetic `oneOf` property can silently overwrite an existing user-defined `oneOf` property</comment>
<file context>
@@ -1087,6 +1091,49 @@ protected Schema normalizeOneOf(Schema schema, Set<Schema> visitedSchemas) {
+ newOneOfSchema.addExtension(X_ONE_OF_UNWRAPPED, true);
+ schema.oneOf(null);
+ // TODO: configuration of the property name
+ String propertyName = "oneOf";
+ if (ModelUtils.hasProperties(schema)) {
+ schema.getProperties().put(propertyName, newOneOfSchema);
</file context>
Fix #23759
Support models with oneOf without discriminator combined with properties (or allOf)
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)java | @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @martin-mfg (2023/08)
Java Spring | @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08)
Please review
Summary by cubic
Adds optional support for inline oneOf mixed with properties/allOf (no discriminator) in Java and Spring by nesting the union under a new oneOf property annotated with @JsonUnwrapped. When oneOf interfaces are enabled, a static @JsonCreator with Jackson mixins handles (de)serialization. Fixes #23759.
New Features
USE_UNWRAPPED_FOR_COMPOSITE_ONEOF(setsx-oneOfunwrapped); the original inline oneOf is moved under a newoneOfproperty which generators annotate with @JsonUnwrapped.useOneOfInterfaces=true: generate avalueOf(JsonNode)@JsonCreator, a nested mixin using@JsonTypeInfo(Id.DEDUCTION)/@JsonSubTypes, and aJacksonMixinConfigthat registers mixins and provides a mapper.useOneOfInterfaces=false: generate a normal class (no mixins or @JsonCreator).JsonNodeandJsonMapper; generates mixin config for both Java client and Spring Boot 3/4.CodegenConstants(JACKSON2_PACKAGE,JACKSON3_PACKAGE,jacksonPackage,x-oneOfunwrapped,x-oneof-jsonCreator) to align generators.Bug Fixes
Written for commit 5ebc7e2. Summary will update on new commits.