feat(docservice): Support Jackson polymorphism annotations - #6370
Conversation
|
|
I will investigate it. |
Adds a new `JacksonPolymorphismTypeInfoProvider` to generate correct JSON Schemas with `oneOf` and `discriminator` for polymorphic types annotated with `@JsonTypeInfo` and `@JsonSubTypes`.
4e4ea3c to
ac2b9e3
Compare
Adds a new `JacksonPolymorphismTypeInfoProvider` to generate correct JSON Schemas with `oneOf` and `discriminator` for polymorphic types annotated with `@JsonTypeInfo` and `@JsonSubTypes`.
ac2b9e3 to
e7a0237
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6370 +/- ##
============================================
- Coverage 74.46% 74.25% -0.21%
- Complexity 22234 23952 +1718
============================================
Files 1963 2163 +200
Lines 82437 89592 +7155
Branches 10764 11724 +960
============================================
+ Hits 61385 66525 +5140
- Misses 15918 17470 +1552
- Partials 5134 5597 +463 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| logger.info("JSON Specification: http://127.0.0.1:8080/docs/specification.json"); | ||
| } | ||
|
|
||
| @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "species") |
There was a problem hiding this comment.
It seems like the subtypes are missing species, so deserialization actually doesn't work.
Also, this is not a tutorial, but just an example.
So, what do you think of making this class a test case (PolymorphismDocServiceTest) like we did for AnnotatedDocServiceTest?
We can use TestUtil.isDocServiceDemoMode() to see how it works on a browser.
Could you also use English for comments instead of Korean?
There was a problem hiding this comment.
Thank you for the detailed and helpful feedback! I've addressed all of your suggestions based on our discussion.
-
Converted Example to a Test Case: As you suggested, the
PolymorphismDocServiceExample.javahas been removed entirely. -
Created a Comprehensive Test Suite: I've added a new integration test,
PolymorphismDocServiceTest.java. This new test suite now covers:- Correct documentation generation for polymorphic types (
oneOf,discriminator). - deserialization of various polymorphic objects (
Dog,Cat). - handling of edge cases, including misconfigured
@JsonSubTypes({})annotations and other types likeOptionalandMap. - I also confirmed that the
TestUtil.isDocServiceDemoMode()works correctly for manual UI verification.
- Correct documentation generation for polymorphic types (
-
Used English for Comments: I've reviewed all new and modified files to ensure all comments are in English now.
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
Left a few more suggestions. 😎
Please, keep up the great work. 👍
| if (structInfo != null) { | ||
| visited.put(firstParam.typeSignature(), "#"); | ||
| generateProperties(structInfo.fields(), visited, "#", root); | ||
| } |
There was a problem hiding this comment.
Should we warn if structInfo == null?
There was a problem hiding this comment.
I've updated the code to handle the structInfo == null case as you suggested.
if (structInfo != null) {
visited.put(firstParam.typeSignature(), "#");
generateProperties(structInfo.fields(), visited, "#", root);
} else {
logger.warn("Could not find root struct for signature: {}",
firstParam.typeSignature().signature());
root.put("additionalProperties", true);
}Now, if a StructInfo for a gRPC request(may be) is not found:
- A warning will be logged with the missing
TypeSignature. - The generated schema for that method will fall back to using
"additionalProperties": true, effectively treating it as a generic object.
| fieldNode.set("additionalProperties", additionalPropertiesNode.get("")); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
What happends if the type is other types such as CONTAINER?
There was a problem hiding this comment.
I've analyzed this and improved the generator's logic to handle these cases more robustly by “unwrapping” them to their inner types.
Implemented Solution
I've updated both getSchemaType and generateFieldSchema in JsonSchemaGenerator to recursively process OPTIONAL and CONTAINER types.
Code Snippet from JsonSchemaGenerator.java:
// In getSchemaType()
case OPTIONAL:
case CONTAINER: {
// Unwrap and return the inner type's schema type
final TypeSignature inner =
((ContainerTypeSignature) typeSignature).typeParameters().get(0);
return getSchemaType(inner);
}
// In generateFieldSchema()
if (typeSignature.type() == TypeSignatureType.OPTIONAL ||
typeSignature.type() == TypeSignatureType.CONTAINER) {
final TypeSignature inner =
((ContainerTypeSignature) typeSignature).typeParameters().get(0);
final ObjectNode innerNode = generateFieldSchema(FieldInfo.of("", inner));
fieldNode.setAll(innerNode);
return fieldNode;
}Example and Results
To verify this, I've used the following service methods in the test code PolymorphismDocServiceTest :
// Container example
static final class ApiResponse<T> {
@JsonProperty
private final int status;
@JsonProperty
private final T data;
ApiResponse(int status, T data) {
this.status = status;
this.data = data;
}
}
// OPTIONAL type in parameter
@Post("/animal/optional")
public String processOptionalAnimal(Optional<Animal> animal) { ... }
// CONTAINER type in return value
@Post("/dummy/api_response")
public ApiResponse<Toy> getExampleResponse() { ... }1. Result in specification.json:
The DocService correctly unwraps Optional<Animal> to its inner type Animal and marks it as OPTIONAL. For ApiResponse<Toy>, it correctly identifies the full generic type signature.
// For processOptionalAnimal
{
"id" : "...AnimalService/processOptionalAnimal/POST",
"name" : "processOptionalAnimal",
"returnTypeSignature" : "string",
"parameters" : [ {
"name" : "animal",
"location" : "UNSPECIFIED",
"requirement" : "OPTIONAL",
"typeSignature" : "...$Animal",
"descriptionInfo" : {
"docString" : "",
"markup" : "NONE"
}
}
// For getExampleResponse
{
...
"returnTypeSignature": "ApiResponse<...PolymorphismDocServiceTest$Toy>"
...
}2. Result in schemas.json:
// part of Schema for processOptionalAnimal
{
"$id": ".../processOptionalAnimal/POST",
"properties": {
"animal": {
"$ref": "#/definitions/...PolymorphismDocServiceTest$Animal"
}
}
...
}
//part of Schema for getExampleResponse
{
"$id" : ".../getExampleResponse/POST",
"title" : "getExampleResponse",
"additionalProperties" : false,
"type" : "object",
...
}Adds a new `JacksonPolymorphismTypeInfoProvider` to generate correct JSON Schemas with `oneOf` and `discriminator` for polymorphic types annotated with `@JsonTypeInfo` and `@JsonSubTypes`.
e7a0237 to
4823d7b
Compare
|
I found a couple of issues in the generated JSON schema:
To address this, I propose the following:
Please, let me know your opinion. 🙇 |
Thank you for your Review ! I've changed {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "com.....$AnimalService",
"title": "com....$AnimalService",
"$defs": {
"models": {
"com....$Animal": {
"type": "object",
"title": "com....$Animal",
"oneOf": [
{
"$ref": "#/$defs/models/com...$Dog"
},
{
"$ref": "#/$defs/models/com.l...$Cat"
}
],
"discriminator": {
"propertyName": "species",
"mapping": {
"dog": "#/$defs/models/com....$Dog",
"cat": "#/$defs/models/com.....$Cat"
}
}
},
"com....$Cat": {
"type": "object",
"title": "com....$Cat",
"properties": {
"species": {
"type": "string"
},
"name": {
"type": "string"
},
"likesTuna": {
"type": "boolean"
},
"scratchPost": {
"$ref": "#/$defs/models/com....$Toy"
},
"vetRecord": {
"$ref": "#/$defs/models/com....$VetRecord"
}
},
"required": [ "name", "likesTuna", "scratchPost", "vetRecord", "species" ] // Should "species" be first?
},
"...": "..."
}, //models
"methods": {
"processAnimal": {
"$id": "com....$AnimalService/processAnimal/POST",
"title": "processAnimal",
"additionalProperties": false,
"type": "object",
"properties": {
"animal": {
"$ref": "#/$defs/models/com....$Animal"
}
},
"required": [ "animal" ]
},
"...": "..."
} //methods
}
}
However, this change caused the existing Should I maintain backward compatibility for the gRPC schema, or should I update it to use the new, unified structure as well? Also, as you mentioned, I'm not familiar with the frontend, so I would really appreciate your help with the |
I think we don't have to worry about the compatibility because the browser will fetch the new JSON schema and use it for the autocompletion.
I'm happy to help you. 😉 Will push a commit after the server-side changes are done. |
4823d7b to
e02df82
Compare
e02df82 to
6017c7d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/internal/server/docs/JacksonPolymorphismTypeInfoProvider.java (1)
96-98: Consider centralizing the schema path constant.The hardcoded path
"#/$defs/models/"at line 98 couples this provider to the JSON schema structure defined inJsonSchemaGenerator. If the schema structure changes, this will need to be updated in multiple places.💡 Optional: Define a shared constant for the models path
Consider defining a package-private constant in
JsonSchemaGeneratoror a shared utility:// In JsonSchemaGenerator or a new SchemaConstants class: static final String MODELS_REF_PREFIX = "#/$defs/models/";Then reference it here:
final String schemaName = TypeSignature.ofStruct(subClass).name(); -mapping.put(key, "#/$defs/models/" + schemaName); +mapping.put(key, JsonSchemaGenerator.MODELS_REF_PREFIX + schemaName);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/linecorp/armeria/internal/server/docs/JacksonPolymorphismTypeInfoProvider.javacore/src/main/java/com/linecorp/armeria/server/docs/DiscriminatorInfo.javacore/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.javacore/src/main/resources/META-INF/services/com.linecorp.armeria.server.docs.DescriptiveTypeInfoProvider
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/resources/META-INF/services/com.linecorp.armeria.server.docs.DescriptiveTypeInfoProvider
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.javacore/src/main/java/com/linecorp/armeria/server/docs/DiscriminatorInfo.javacore/src/main/java/com/linecorp/armeria/internal/server/docs/JacksonPolymorphismTypeInfoProvider.java
🧬 Code graph analysis (3)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
kotlin/src/test/kotlin/com/linecorp/armeria/internal/server/annotation/DataClassDefaultNameTypeInfoProviderTest.kt (5)
value(32-116)value(33-74)value(76-88)value(90-106)value(108-115)
core/src/main/java/com/linecorp/armeria/server/docs/DiscriminatorInfo.java (1)
core/src/main/java/com/linecorp/armeria/server/docs/StructInfo.java (1)
UnstableApi(40-234)
core/src/main/java/com/linecorp/armeria/internal/server/docs/JacksonPolymorphismTypeInfoProvider.java (2)
core/src/main/java/com/linecorp/armeria/internal/server/docs/DocServiceTypeUtil.java (1)
DocServiceTypeUtil(47-195)core/src/main/java/com/linecorp/armeria/internal/common/JacksonUtil.java (1)
JacksonUtil(33-88)
🔇 Additional comments (8)
core/src/main/java/com/linecorp/armeria/internal/server/docs/JacksonPolymorphismTypeInfoProvider.java (2)
84-91: Multiple validation checks with early returns are clear and correct.The progressive validation pattern (checking propertyName emptiness, then subtypes length) is appropriate for this SPI provider implementation where null return indicates "cannot handle this type."
52-124: Well-structured provider implementation for Jackson polymorphism.The implementation correctly:
- Validates required annotations and properties
- Builds discriminator mappings with appropriate fallback to simple names
- Introspects properties using Jackson's ObjectMapper
- Returns a complete StructInfo with oneOf and discriminator metadata
core/src/main/java/com/linecorp/armeria/server/docs/DiscriminatorInfo.java (1)
40-106: LGTM! Clean implementation of discriminator metadata.The implementation correctly:
- Uses the static factory pattern as requested in past reviews
- Ensures immutability via
ImmutableMap.copyOf- Includes appropriate
@UnstableApiannotation for this new public API- Provides standard
equals,hashCode, andtoStringimplementations- Applies
@JsonPropertyfor serialization supportThe Javadoc typo ("int the payload" → "in the payload") has been addressed as noted in past reviews.
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (5)
48-73: Well-designed constructor with pre-computed mappings.Pre-computing the
structs,enums, andpolymorphismToBasemappings in the constructor improves performance by avoiding repeated lookups during schema generation. The handling of struct aliases (lines 55-57) and the discriminator-to-subtype mapping (lines 64-72) are correctly implemented.
80-132: Good handling of OPTIONAL and CONTAINER types.The recursive unwrapping of
OPTIONALandCONTAINERtypes (lines 90-93) correctly delegates to the inner type's schema type, addressing the concern raised in past reviews about handling these type signatures.
206-236: Correct discriminator property injection for polymorphic subtypes.Lines 209-213 correctly add the discriminator property to subtype schemas (e.g.,
CatandDogget aspeciesproperty), and line 227 correctly adds it to the required fields. This aligns with the OpenAPI specification for polymorphism and addresses the concern raised by minwoox in the PR comments about subtypes missing the discriminator property.
282-321: Field schema generation handles nested types correctly.The recursive handling at lines 298-299 for
OPTIONALandCONTAINERtypes, combined with the explicit handling ofITERABLE(lines 306-310) andMAP(lines 311-316), correctly generates schemas for complex nested structures without risk of infinite recursion since each recursion processes a simpler type signature.
76-78: Verify frontend compatibility with the new ObjectNode structure.The return type changed from
ArrayNodetoObjectNodewith a$defs-based structure. According to the PR comments, minwoox offered to help with frontend changes (RequestBody.tsx). Ensure the frontend has been updated to consume the new unified schema format with$defs.modelsand$defs.methods.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
165-175: Method name collisions remain unresolved.The comment at lines 169-170 acknowledges the collision risk but the code still uses
m.name()as the key. If multiple services have methods with the same name, only the last one will be retained. This was flagged in a previous review.
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
102-105: Consider mappingshorttointegerfor consistency with other integer types.
shortis an integer type in Java, similar tointandlong. Mapping it to"number"(which includes floating-point values) while other integer types map to"integer"may cause inconsistent schema behavior.🔎 Proposed fix
case "short": + return "integer"; case "float": case "double": return "number"; - case "i8": - case "i16": + case "i8": // Thrift byte + case "i16": // Thrift i16
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🔇 Additional comments (6)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (6)
39-46: LGTM! Clean class structure with proper field declarations.The package-private class with immutable maps for structs/enums provides good encapsulation. The
polymorphismToBaseusingHashMapis acceptable since it's only populated in the constructor.
134-152: LGTM! Proper validation and root schema construction.The empty services check (lines 136-138) addresses the previous review concern. The root schema structure with
$defscontainingmodelsandmethodsaligns with the PR objective for unified schema generation.
177-238: LGTM! Well-structured polymorphism support with oneOf and discriminator.The implementation correctly:
- Handles polymorphic base types with
oneOfanddiscriminator(lines 186-206)- Adds discriminator property to subtypes (lines 211-215)
- Uses a single
requiredFieldslist, addressing the previous review about redundant logicThe
$refpaths correctly point to#/$defs/models/matching the unified schema structure.
240-247: LGTM!Clean enum definition generation following JSON Schema conventions.
249-282: LGTM! Method schema generation with proper parameter filtering.The method correctly filters parameters to include only
BODYandUNSPECIFIEDlocations in the JSON Schema, and usesmethodInfo.id()for the unique$idfield.
307-322: LGTM! Correct handling of ITERABLE and MAP types.The implementation follows JSON Schema conventions:
- Arrays use
itemsfor element type- Maps use
additionalPropertiesfor value type (keys are implicitly strings in JSON)
…fely handle aliases
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
177-187: Method name collisions across services remain unaddressed.The comment at lines 181-183 acknowledges the potential for name collisions when multiple services define methods with the same name, but the code still uses
m.name()as the key. Consider usingm.id()or a qualified name likesvc.name() + "." + m.name()to ensure uniqueness.
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
252-259: Consider adding description for enum definitions.Unlike
generateStructDefinitionwhich includes thedocStringas a description (lines 193-196), this method omits the enum's documentation. For consistency, consider adding the description if available.🔎 Proposed fix
private static ObjectNode generateEnumDefinition(EnumInfo enumInfo) { final ObjectNode schemaNode = mapper.createObjectNode(); schemaNode.put("type", "string"); + final String docString = enumInfo.descriptionInfo().docString(); + if (!docString.isEmpty()) { + schemaNode.put("description", docString); + } final ArrayNode enumValues = mapper.createArrayNode(); enumInfo.values().forEach(value -> enumValues.add(value.name())); schemaNode.set("enum", enumValues); return schemaNode; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🔇 Additional comments (6)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (6)
39-46: LGTM!The class structure with instance fields for precomputed mappings is a clean approach. The use of
ImmutableMapfor structs/enums andHashMapfor the mutable polymorphism mapping is appropriate.
48-85: LGTM!The constructor properly initializes all mappings. The polymorphism-to-base precomputation handles both primary names and aliases correctly, using
putIfAbsentto avoid overwriting existing mappings.
146-164: LGTM!The validation for empty services and the structured
$defsorganization with separatemodelsandmethodssections is clean. Using a representative service for the root schema's$idandtitleis a reasonable approach.
189-250: LGTM!The struct definition generation handles both polymorphic types (with
oneOfanddiscriminator) and regular types cleanly. The discriminator property is correctly added to subtypes and marked as required.
261-294: LGTM!The method schema generation correctly filters parameters by location and handles required fields appropriately. The structure with
$id,title, andadditionalProperties: falseprovides a complete method schema.
296-339: LGTM!The field schema generation handles the type hierarchy well:
- STRUCT/ENUM →
$refto models- OPTIONAL/CONTAINER → unwraps and preserves description
- ITERABLE → array with items
- MAP → object with additionalProperties
The description preservation when unwrapping container types (lines 314-316) addresses the earlier review feedback.
…r schema type mapping
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
178-188: Method name collisions are possible across services.The code uses
m.name()as the key, but the comment acknowledges that usingm.id()would be more unique. If multiple services define methods with the same name, only the last one will be retained in the schema.Consider using the method ID as the key to ensure uniqueness:
🔎 Proposed fix
for (final MethodInfo m : svc.methods()) { - // To avoid potential name collision, we can use a more unique key like - // method id. - methodsNode.set(m.name(), generateMethodSchema(m)); + // Use method ID to avoid name collisions across services + methodsNode.set(m.id(), generateMethodSchema(m)); }
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (1)
253-260: Add description support to enum schema generation for consistency.The
generateEnumDefinitionmethod currently omits thedocStringfromEnumInfo.descriptionInfo(), unlikegenerateStructDefinition,generateMethodSchema, and field schema generation, which all include descriptions.🔎 Proposed enhancement
private static ObjectNode generateEnumDefinition(EnumInfo enumInfo) { final ObjectNode schemaNode = mapper.createObjectNode(); schemaNode.put("type", "string"); + final String docString = enumInfo.descriptionInfo().docString(); + if (!docString.isEmpty()) { + schemaNode.put("description", docString); + } final ArrayNode enumValues = mapper.createArrayNode(); enumInfo.values().forEach(value -> enumValues.add(value.name())); schemaNode.set("enum", enumValues); return schemaNode; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java
🔇 Additional comments (9)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (9)
44-47: LGTM!The instance fields are well-structured with immutable maps for structs, enums, and polymorphism mappings, enabling efficient lookups during schema generation.
49-86: LGTM!The constructor properly initializes all mappings with appropriate null checks and uses
putIfAbsentfor the polymorphism mappings to handle potential duplicates gracefully.
88-91: LGTM!Clean static factory pattern that delegates to instance-based generation.
93-145: LGTM!The type mapping is comprehensive, handles container unwrapping correctly, and uses
Locale.ROOTfor locale-independent lowercase conversion as previously suggested.
147-165: LGTM!The method properly validates for empty services and constructs a well-structured unified schema with
$defscontaining both models and methods, avoiding duplicate definitions.
167-176: LGTM!Clean generation of model definitions for both structs and enums.
190-251: LGTM!The struct definition generation handles both polymorphic types (with
oneOfanddiscriminator) and regular structs cleanly. The required fields logic correctly includes the discriminator property when applicable.
262-295: LGTM!The method schema generation correctly uses the unique method ID for
$id, filters parameters by location, and follows JSON Schema conventions.
297-340: LGTM!The field schema generation correctly handles all type signatures including structs, enums, containers, iterables, and maps. The description is properly preserved when unwrapping
OPTIONAL/CONTAINERtypes as previously suggested.
@minwoox I have addressed all the feedback from CodeRabbit, and server-side changes are done. |
|
@YoungHoney No worry. Let me take a look at it. 😉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/src/test/java/com/linecorp/armeria/server/docs/JsonSchemaGeneratorTest.java`:
- Line 368: Remove the leftover debug print in the test: delete the
System.err.println(generated.toPrettyString()); statement in
JsonSchemaGeneratorTest (the test method that prints the generated JSON schema)
so the test no longer outputs to stderr; ensure no other stray println/debug
logging remains in that test.
🧹 Nitpick comments (3)
core/src/main/java/com/linecorp/armeria/server/docs/JsonSchemaGenerator.java (3)
149-167:$idandtitleuse only the first service name, which may be misleading for multi-service specs.When the
ServiceSpecificationcontains multiple services, only the first one's name is used as the$idandtitleof the root schema. Since$idshould be a unique identifier for the schema, consider using a more representative value (e.g., a composite or a fixed identifier).The comment at line 155 acknowledges this is temporary — just flagging for visibility.
95-147:shortmaps to"number"— consider"integer"for consistency.
shortis an integral type but maps to"number"(line 120) alongsidefloatanddouble. JSON Schema's"integer"would be more precise forshort, similar to howint,long,byte(viai8/i16) all map to"integer".Proposed fix
case "short": - case "float": - case "double": - return "number"; + return "integer"; + case "float": + case "double": + return "number";
268-314: Method-leveladditionalProperties: falsemay break polymorphic or evolving schemas.Line 280 sets
additionalPropertiestofalseon every method schema. This is strict and will reject any extra fields not declared in the schema. If the intent is to support schema validation in the DocService UI (e.g., Monaco editor), this can cause false negatives when the server accepts additional fields. Consider whethertrue(or omitting it) would be more appropriate for a documentation schema.
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
@YoungHoney Sorry that I'm late. 😅
I resolved the conflict and did the UI work.
Thanks a lot!
| * type by inspecting Jackson annotations such as {@link JsonTypeInfo} and | ||
| * {@link JsonSubTypes}. | ||
| */ | ||
| public final class JacksonPolymorphismTypeInfoProvider implements DescriptiveTypeInfoProvider { |
There was a problem hiding this comment.
Do you mind reformatting this file according to project conventions?
https://armeria.dev/community/developer-guide#setting-up-your-ide
There was a problem hiding this comment.
Thank you for your review!
I've reformated JacksonPolymorphismTypeInfoProvider.java to follow LY OSS convention.
| * The keys are the values that appear in the {@link #propertyName()} field, and | ||
| * the values are | ||
| * the schema definitions to use for that value (e.g., | ||
| * {@code "#/$defs/models/Cat"}). |
There was a problem hiding this comment.
Question) At the very least, could you indicate that the values come from StructInfo#name (which is a fqcn)?
| * {@code "#/$defs/models/Cat"}). | |
| * {@code "#/$defs/models/com.linecorp.armeria.Cat"}). |
There was a problem hiding this comment.
Sure! I've updated the Javadoc to use the FQCN for clarity. Thanks for the catch!
| final ImmutableMap.Builder<String, StructInfo> structsBuilder = ImmutableMap | ||
| .builderWithExpectedSize(serviceSpecification.structs().size()); | ||
| for (final StructInfo structInfo : serviceSpecification.structs()) { | ||
| structsBuilder.put(structInfo.name(), structInfo); | ||
| if (structInfo.alias() != null) { | ||
| structsBuilder.put(structInfo.alias(), structInfo); | ||
| } | ||
| } | ||
| typeSignatureToStructMapping = typeSignatureToStructMappingBuilder.build(); | ||
| typeNameToEnumMapping = serviceSpecification.enums().stream().collect( | ||
| toImmutableMap(EnumInfo::name, Function.identity())); | ||
| } | ||
| structs = structsBuilder.build(); | ||
|
|
||
| private ArrayNode generate() { | ||
| final ArrayNode definitions = mapper.createArrayNode(); | ||
| enums = serviceSpecification.enums().stream() | ||
| .collect(toImmutableMap(EnumInfo::name, Function.identity())); |
There was a problem hiding this comment.
It seems like structs, enums isn't used anymore as they are set directly to the ObjectNode
There was a problem hiding this comment.
Thank you, I've updated following changes
- remove
structs,enumsas you suggested - erase unnecessary 3
imports - rearange static method
generateEnumDefinition
- Apply LY OSS formatting and improve readability for JacksonPolymorphismTypeInfoProvider. - Update DiscriminatorInfo Javadoc to use FQCN for clarity. - Remove unused fields and imports in JsonSchemaGenerator. - Fix Checkstyle errors regarding operator wrapping in JsonSchemaGenerator.
8026f6b to
c2d7d80
Compare
|
|
||
| generateProperties(methodFields, visited, currentPath, root); | ||
| return root; | ||
| switch (typeSignature.name().toLowerCase(Locale.ROOT)) { |
There was a problem hiding this comment.
Instead of these hard-coded mappings, could we pass JSON Schema type when creating TypeSignature.ofBase()?
For example:
TypeSignature.ofBase("void", "null");
TypeSignature.ofBase("int32", "integer");
TypeSignature.ofBase("double", "number");There was a problem hiding this comment.
I'll try this.
then, should i maintain single parameter ofBase and move hard-coded mapping into DefaultTypeSignature or just remove it and use only double parameter ofBase as you mentioned? (this may causes lots of code changes)
There was a problem hiding this comment.
just remove it and use only double parameter ofBase as you mentioned?
I intended this.
(this may causes lots of code changes)
If so, I think we can handle this in a follow-up PR.
…Provider - Introduce a “Limitations” section in the class‑level Javadoc that documents the only supported JsonTypeInfo.As inclusion modes (PROPERTY and EXISTING_PROPERTY) when used with JacksonPolymorphismTypeInfoProvider. - Refactor surrounding Javadoc wording for clearer, more consistent documentation.
5 participants
Motivation
This pull request implements support for Jackson's polymorphism annotations (
@JsonTypeInfo,@JsonSubTypes) inDocService, as requested in the community (issue #6313). Currently,DocServicedoes not correctly generate documentation for annotated services that use inheritance in their DTOs, leading to incomplete specifications. This change adds a newDescriptiveTypeInfoProviderto resolve these polymorphic types and generate accurate JSON Schemas.However, this feature has uncovered significant and complex build stability issues when running a full parallel build (
./gradlew clean build --parallel). This PR serves as both the implementation of the feature and a concrete test case for discussing the build instability it triggers.Modifications
JacksonPolymorphismTypeInfoProvider: A new provider that uses pure Java reflection to safely inspect@JsonTypeInfoand@JsonSubTypesannotations. It is registered via Java's SPI mechanism to be discoverable byDocService.DiscriminatorInfo: A new data class to hold polymorphism metadata extracted from the annotations.toTypeSignature) was moved from a separateDocServiceTypeUtilintoAnnotatedDocServicePluginfor better cohesion.StructInfo: Modified to includeoneOfanddiscriminatorfields to carry polymorphism information.JsonSchemaGenerator: The generator now recognizes the new fields inStructInfoand correctly produces JSON Schema withoneOfanddiscriminatorproperties.PolymorphismDocServiceExample: A new example service to demonstrate and manually verify the feature.Result
DocServicecan now correctly generate documentation for annotated services that use polymorphic types with Jackson. The resulting JSON Schema will contain the appropriateoneOfanddiscriminatorfields.Known Issue: This change is known to trigger build instability in the project's CI environment. A detailed summary of the investigation is provided here : Request Guidance on Build Issues in my feature branch #6369Example usage
also, you can try this at PolymorphismDocServiceExample