[Java] [Spring] mixed OneOf support and jackson JsonUnwrapped#23761
[Java] [Spring] mixed OneOf support and jackson JsonUnwrapped#23761jpfinne wants to merge 13 commits into
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>
| .add(cm.classname); | ||
|
|
||
| cm.getVendorExtensions().put("x-oneof-jsonCreator", true); | ||
| obj.getImports().add(Map.of("import", importMapping.get("JsonNode"))); |
There was a problem hiding this comment.
P1: Unconditionally wrapping importMapping.get("JsonNode") in Map.of(...) can NPE when that mapping is absent.
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/languages/AbstractJavaCodegen.java, line 2872:
<comment>Unconditionally wrapping `importMapping.get("JsonNode")` in `Map.of(...)` can NPE when that mapping is absent.</comment>
<file context>
@@ -2809,4 +2814,61 @@ protected void addNullableImportForOperation(CodegenOperation codegenOperation)
+ .add(cm.classname);
+
+ cm.getVendorExtensions().put("x-oneof-jsonCreator", true);
+ obj.getImports().add(Map.of("import", importMapping.get("JsonNode")));
+ }
}
</file context>
| obj.getImports().add(Map.of("import", importMapping.get("JsonNode"))); | |
| String jsonNodeImport = importMapping.get("JsonNode"); | |
| if (jsonNodeImport != null) { | |
| obj.getImports().add(Map.of("import", jsonNodeImport)); | |
| } |
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.
| newOneOfSchema.addExtension("x-unwrappedOneOf", true); | ||
| String nOneOf = toModelName(schemaName + "OneOf"); | ||
| String newSchemaName = nOneOf+ "_wrapper"; | ||
| openAPI.getComponents().getSchemas().put(newSchemaName, newOneOfSchema); |
There was a problem hiding this comment.
P1: Wrapper schema names are deterministic and inserted without any collision check, so this can overwrite an existing component schema.
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/languages/AbstractJavaCodegen.java, line 2830:
<comment>Wrapper schema names are deterministic and inserted without any collision check, so this can overwrite an existing component schema.</comment>
<file context>
@@ -2809,4 +2814,61 @@ protected void addNullableImportForOperation(CodegenOperation codegenOperation)
+ newOneOfSchema.addExtension("x-unwrappedOneOf", true);
+ String nOneOf = toModelName(schemaName + "OneOf");
+ String newSchemaName = nOneOf+ "_wrapper";
+ openAPI.getComponents().getSchemas().put(newSchemaName, newOneOfSchema);
+ Schema newSchemaRef = new Schema().$ref("#/components/schemas/" + newSchemaName);
+ s.oneOf(null);
</file context>
| oneOf.oneOf(composed.getOneOf()); | ||
| composed.oneOf(null); | ||
|
|
||
| String oneOfName = (String)vendorExtensions.get("x-one-of-name"); |
There was a problem hiding this comment.
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.
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/DefaultCodegen.java, line 2751:
<comment>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.</comment>
<file context>
@@ -2710,13 +2738,23 @@ protected void updateModelForComposedSchema(CodegenModel m, Schema schema, Map<S
+ oneOf.oneOf(composed.getOneOf());
+ composed.oneOf(null);
+
+ String oneOfName = (String)vendorExtensions.get("x-one-of-name");
+ oneOfName = oneOfName != null ? "oneOf"+ oneOfName: "oneOf";
+ addVars(m, Map.of(oneOfName, oneOf), List.of(), null, null);
</file context>
| String oneOfName = (String)vendorExtensions.get("x-one-of-name"); | |
| String oneOfName = composed.getExtensions() != null ? (String) composed.getExtensions().get(X_ONE_OF_NAME) : null; |
| .add(cm.classname); | ||
|
|
||
| cm.getVendorExtensions().put("x-oneof-jsonCreator", true); | ||
| obj.getImports().add(Map.of("import", importMapping.get("JsonNode"))); |
There was a problem hiding this comment.
P2: Unconditional Map.of call can throw if JsonNode is not mapped for this generator.
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/languages/AbstractJavaCodegen.java, line 2872:
<comment>Unconditional `Map.of` call can throw if `JsonNode` is not mapped for this generator.</comment>
<file context>
@@ -2809,4 +2814,61 @@ protected void addNullableImportForOperation(CodegenOperation codegenOperation)
+ .add(cm.classname);
+
+ cm.getVendorExtensions().put("x-oneof-jsonCreator", true);
+ obj.getImports().add(Map.of("import", importMapping.get("JsonNode")));
+ }
}
</file context>
| obj.getImports().add(Map.of("import", importMapping.get("JsonNode"))); | |
| obj.getImports().add(Map.of("import", importMapping.getOrDefault("JsonNode", "com.fasterxml.jackson.databind.JsonNode"))); |
| // TODO: configuration of the property name | ||
| String propertyName = "oneOf"; | ||
| if (ModelUtils.hasProperties(s)) { | ||
| s.getProperties().put(propertyName, newSchemaRef); |
There was a problem hiding this comment.
P2: Hardcoded synthetic property name "oneOf" can silently overwrite an existing schema property with the same name
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/languages/AbstractJavaCodegen.java, line 2836:
<comment>Hardcoded synthetic property name "oneOf" can silently overwrite an existing schema property with the same name</comment>
<file context>
@@ -2809,4 +2814,61 @@ protected void addNullableImportForOperation(CodegenOperation codegenOperation)
+ // TODO: configuration of the property name
+ String propertyName = "oneOf";
+ if (ModelUtils.hasProperties(s)) {
+ s.getProperties().put(propertyName, newSchemaRef);
+ } else if (ModelUtils.hasAllOf(s)) {
+ Schema schema = new Schema();
</file context>
| 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?
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 combined with properties/allOf (no discriminator) in Java and Spring generators by moving oneOf into a
@JsonUnwrappedfield. When oneOf interfaces are enabled, a wrapper interface with a@JsonCreatorand Jackson mixins ensures correct (de)serialization. Fixes #23759.New Features
useWrapperForMixedOneOf(default false) injava,java-microprofile,spring, andjava-camel.oneOfis moved to<SchemaName>OneOfWrapperand exposed as aoneOffield annotated with@JsonUnwrapped.useOneOfInterfaces:@JsonCreator valueOf(JsonNode)and an inner mixin using@JsonTypeInfo(Id.DEDUCTION)/@JsonSubTypes; generatesJacksonMixinConfigto register mixins.@JsonCreator.JsonNode,JsonMapper, andJsonUnwrapped; hooks mixins viaJsonMapper(Jackson 3) orObjectMapper(Jackson 2).Bug Fixes
Written for commit fd72fd3. Summary will update on new commits.