fix(java): map OpenAPI 3.1 null type to Object to prevent ModelNull generation - #23961
fix(java): map OpenAPI 3.1 null type to Object to prevent ModelNull generation#23961anupamchaubey wants to merge 2 commits into
Conversation
|
thanks for the PR i'll take a look and try to get it merged before the next stable release. |
|
Thank you! Sounds great. |
|
Thanks for the earlier feedback! Just noting that this PR also addresses the root cause behind #24519 and #24520 (OAS 3.1 |
| typeMapping.put("date", "Date"); | ||
| typeMapping.put("file", "File"); | ||
| typeMapping.put("AnyType", "Object"); | ||
| typeMapping.put("null", "Object"); |
There was a problem hiding this comment.
Does this serve any purpose? Isn't the idea that null should always be handled before any typeMapping?
|
|
||
| @Override | ||
| public String getSchemaType(Schema p) { | ||
| if (p == null) { |
There was a problem hiding this comment.
Is this fallback necessary for the fix? To me it seems like a rather large change in behavior, and I fail to see why we would want to have a fallback with an error rather than a complete ceasing of processing? Surely something is very wrong if we get here (and this isn't the proper way to handle that)?
|
|
||
| // don't apply renaming on types from the typeMapping | ||
| if (null == openAPIType) { | ||
| LOGGER.error("No Type defined for Schema {}", p); |
There was a problem hiding this comment.
Should we really log an error if this is an expected processing branch now?
| } | ||
|
|
||
| // 2. Intercept the "null" type string immediately before it routes to typeMapping or toModelName | ||
| if ("null".equalsIgnoreCase(openAPIType) || "null".equalsIgnoreCase(p.getType())) { |
There was a problem hiding this comment.
Are we sure that we should never check OAS 3.1 types here? Or is it expected that the openAPIType or normalization has handled that (but if so, why even check getType)?
|
I would strongly suggest that some sample test is modified to contain something that mirrors what the solved issue is, so that it is clear that this change in the Codegen is sent through correctly and handled as expected. Especially since it is the classical "spec is OAS 3.1, generally normalized and converted to 3.0 somewhere and then processed further", which comes with its unclarities. Would it also be possible to highlight the actual regression so that one can deduce why this happened? With the intent to reason if the fix is placed in the right location or if it would be better to place it even further down (earlier) in the chain? |
|
Hi @Mattias-Sehlstedt, thank you for the detailed review! You raise some excellent points regarding where this should be handled in the pipeline and whether some of these fallbacks are masking larger issues. I agree with your feedback and will prepare a follow-up commit. Here are my thoughts: On adding On the On the On intercepting OAS 3.1 types here: On adding a sample test and highlighting the regression: |
|
Hi @Mattias-Sehlstedt, thanks again for the review guidance! I've pushed a refactored update addressing your points: Removed the redundant typeMapping entry. Preserved strict fail-fast behavior for p == null and openAPIType == null. Cleaned up the null-type interception logic. All tests pass successfully locally. Ready for your re-review whenever you have a moment! |
|
I couldn't repeat the issue and just merged a test to ensure it's covered moving forward: (I also did a test with are you able to repeat the issue with the latest master or stable version? if yes, can you share the details please? |
|
Thanks for taking the time to review this PR and for adding the regression coverage in #24701. After looking into the issue further, I’m not currently able to reliably reproduce the original I’m going to close this PR for now. I appreciate the review and feedback, especially around handling the issue at the appropriate stage of the code generation pipeline. Thank you! |
Problem / Context
When processing OpenAPI 3.1 specifications containing schema properties with an explicit
type: 'null', the core code generator was failing to capture it as a primitive mapping. Instead, it was treating"null"as a custom model class name. This caused the engine to flag it as a language reserved word, fallback to appending "Null", and ultimately generate broken java compilation variables such asprivate ModelNull records = null;(#23908).Solution Implemented
AbstractJavaCodegen.javawithin the core engine translation layers (getSchemaTypeandgetTypeDeclaration).type: 'null'is resolved, it maps directly back to the standard native Java primitive wrapper type (Object).Tests Added / Verification
JavaClientCodegenTest.java(testNullTypeMapsToObject) to assert that structural schemas defined explicitly with anulltype correctly declare variable instances as anObject.webclientconfiguration library—confirming the invalidModelNullimport and variable syntax are successfully cleared out.mvn test -pl modules/openapi-generator -Dtest=*CodegenTest) to ensure all concurrent Java-dependent framework engines build perfectly.Fixes #23908
Summary by cubic
Maps OpenAPI 3.1
type: "null"to JavaObjectin the Java code generator to prevent phantomModelNullclasses and invalid fields. Fixes #23908."null"toObjectingetSchemaTypeandgetTypeDeclaration, intercepting bothSchema.typeand resolvedopenAPIType, while preserving fail-fast behavior when type is missing.testNullTypeMapsToObjectto ensure schemas withtype: "null"generateObject.Written for commit ffac7aa. Summary will update on new commits.