feat: Implement support for ArrayMap data type in OpenApi2JaxRs code generation#410
feat: Implement support for ArrayMap data type in OpenApi2JaxRs code generation#410matheusandre1 wants to merge 1 commit into
Conversation
EricWittmann
left a comment
There was a problem hiding this comment.
Thanks for the contribution, Matheus! The ArrayMap feature looks well-motivated and the test demonstrates it working for the core use case. I have a few concerns I'd like to see addressed before merging.
Code duplication — resolveJavaType implemented three times
Nearly identical type-resolution logic (mapping OpenAPI schema types to Java FQCNs) now appears in three places:
OpenApi2JaxRs.resolveSchemaType()(operates onJsonNode)OpenApi2CodegenVisitor.resolveJavaType()(operates onOpenApi31Schema)OpenApiMapDataTypeProcessor.resolveJavaType()(operates onOpenApi31Schema)
The last two are almost line-for-line identical. This should be extracted into a shared utility method, likely in CodegenUtil. The JsonNode variant could potentially share logic too, or at minimum the type-name mapping ("string" → "java.lang.String", etc.) should be a shared constant or method.
refToSimpleType produces unqualified names
In OpenApiMapDataTypeProcessor.refToSimpleType(), a $ref like #/components/schemas/ApiList becomes just "ApiList" — the bare schema name, not a fully-qualified class name. But elsewhere (in OpenApi2CodegenVisitor.resolveJavaType() and OpenApi2JaxRs.resolveSchemaType()), refs are resolved to FQCNs via CodegenUtil.schemaRefToFQCN(). This inconsistency is fragile — if the existingJavaType value is ever used directly for import generation or in a context expecting FQCNs, it will silently produce incorrect output.
generateJavaInterface → generateJavaInterfaceSource split
The new generateJavaInterfaceSource method is an exact extract of generateJavaInterface(... topLevelPackage), and the original now just delegates to it. Since OpenApi2Quarkus does not override this method, the refactoring adds an indirection with no current consumer. If this is prep for future subclass work, it should be in a separate PR. Otherwise it can be removed.
Set.of(...) allocated on every iteration
In computeRequestBodyTypes, Set.of("get", "put", "post", ...) is allocated inside the inner forEachRemaining loop — once per HTTP method entry per path. This should be a private static final constant.
computeRequestBodyTypes round-trips the Document through JSON
JsonNode json = mapper.readTree(Library.writeDocumentToJSONString(document));This serializes the entire OpenAPI document to a JSON string then re-parses it into a Jackson tree. Consider using the data model API directly (as OpenApi2CodegenVisitor does) to avoid the extra allocation.
Inconsistent spacing in generated type strings
OpenApiMapDataTypeProcessor.buildJavaType() produces "java.util.Map<String,String>" (no space after comma), matching the existing convention. But OpenApi2CodegenVisitor.resolveMapJavaType() and OpenApi2JaxRs.resolveSchemaType() produce "java.util.Map<String, ..." (with space). Pick one convention — string equality comparisons on these types would fail silently otherwise.
Minor
- The Javadoc on the new test method references
io.apicurio.hub.api.codegen.OpenApi2JaxRs(old package name) instead ofio.apitomy— copy-paste from adjacent methods. - The test fixture
pom.xmlusesmicroprofile-openapi-apiversion4.0.2but the project recently upgraded to4.1.1— this will likely cause a test failure after the version bump merges. Please verify.
…generation Signed-off-by: Matheus André <matheusandr2@gmail.com>
EricWittmann
left a comment
There was a problem hiding this comment.
Code Review — Round 2
Thanks for the contribution, @matheusandre1! The ArrayMap feature is a great addition. There are a few issues to address before this can be merged — most importantly, the CI failures and the items from the previous review that are still outstanding.
CI Failures (4 test failures)
Both CI jobs are red. The failing tests are:
OpenApi2JaxRsTest.testGenerateFull_RegistryApiV2—SearchResource.javaOpenApi2JaxRsTest.testGenerateFull_MultipleMediaTypes—WidgetsResource.javaOpenApi2QuarkusTest.testGenerateFullDifferentNamespace—AdminResource.javaOpenApi2QuarkusTest.testGitHubApisFull—OrgsResource.java
All four fail the same way: the expected output has import java.io.InputStream but the generated code no longer produces it. The root cause is that the PR unintentionally changes parameter type resolution for all schemas with additionalProperties, not just ArrayMap-tagged ones. Here's the chain:
-
OpenApi2CodegenVisitor.resolveMapJavaType()fires for ANYobjectschema withadditionalPropertiesthat is a schema — it doesn't check forx-codegen-type. So schemas likeStringMap,StringObjectMap, and even plain object schemas that happen to haveadditionalPropertiesnow getexistingJavaTypeset tojava.util.Map<String, ...>. -
The new
existingJavaTypecheck at the top ofOpenApi2JaxRs.generateTypeName()short-circuits and returns the map type directly, bypassing the normalInputStreamdefault that body parameters get. -
The net result: any body param referencing a schema with
additionalPropertiesnow generates asMap<String, ...>instead ofInputStream, changing the output for the four existing test fixtures.
Suggested fix: Gate resolveMapJavaType() so it only activates for schemas explicitly tagged with x-codegen-type (ArrayMap, StringMap, StringObjectMap), or limit the existingJavaType short-circuit in generateTypeName to only apply when the param is not a body param. The existing test fixtures should not need updating — only the new ArrayMap behavior should change generated output.
Outstanding Items from Previous Review
These were flagged in the first review and are still present in the current code:
1. Dead document field in DocumentPreProcessor
The constructor stores Document document in this.document, but the field is never read — the process() method takes its own document parameter. Please either remove the field or use it.
2. Duplicated type resolution logic
OpenApi2JaxRs.resolveSchemaType(JsonNode, Map) and CodegenUtil.resolveJavaType(JaxRsProjectSettings, Document, OpenApi31Schema, String) implement the same type-mapping logic (string→String, integer→Integer, array→List, etc.) over different data structures. Any future bug fix or new type must be applied to both. Please either unify them into a single implementation or add cross-reference comments so they don't drift.
3. Serialization roundtrip in computeRequestBodyTypes()
This method serializes the entire document to JSON and re-parses it into a Jackson tree just to walk paths/operations:
JsonNode json = mapper.readTree(Library.writeDocumentToJSONString(document));The rest of the codebase uses the visitor pattern for document traversal. If the Jackson approach is necessary because it must run before preProcess() strips the x-codegen-type annotations, please add a comment explaining why. Otherwise, consider using the existing model navigation APIs.
4. resolveBodyType only examines the first media type
JsonNode mediaType = content.elements().next();JSON object iteration order is not guaranteed. If an operation has multiple content types, this may resolve from the wrong one. Please prefer content.get("application/json") with a fallback.
5. Magic string APICURIO_CODEGEN_BYTE_ARRAY_REPRESENTATION
This sentinel value in generateTypeName isn't defined as a constant. Please extract it to a named constant (perhaps in CodegenExtensions) so it's discoverable and maintainable.
6. Redundant existingJavaType reads in OpenApi2CodegenVisitor
setSchemaProperties() reads the existingJavaType extension at the top of the method, and then resolveMapJavaType() reads the same extension again inside the object branch. Please consolidate to a single read with clear precedence.
Once the CI failures are fixed and the items above are addressed, this should be in good shape. Happy to re-review once updated!
Closes: #409
Added existingJavaType field to CodegenJavaSchema for better type handling.
Enhanced OpenApi2JaxRs to compute request body types and handle ArrayMap inline.
Updated OpenApiMapDataTypeProcessor to recognize ArrayMap and generate appropriate Java types.
Introduced new test case for ArrayMap inline generation in OpenApi2JaxRsTest.
Added necessary JSON and Java files for ArrayMap support in generated API.
mention it to see the progress.
quarkiverse/quarkus-openapi-generator#1352 (comment) and
and pr : quarkiverse/quarkus-openapi-generator#1611