Add scalar child list handling in JSON schema generation#2821
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks JsonSchemaGenerator internals: replaces per-child parser tracking with a no-envelope defs map and constants, centralizes defs/$ref helpers, adds scalar-list detection and scalar item schema mapping, and refactors parser/list/property creation and children processing; no public API changes. Changes
Sequence Diagram(s)(omitted — changes are internal refactor within a single component and do not introduce new multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
357-393: Duplicated type-mapping logic betweenisScalarChildListandscalarItemsSchema; silent fallback hides bugs.Two concerns:
DRY violation: The qualified-name checks in
isScalarChildList(lines 363-370) andscalarItemsSchema(lines 376-390) must be kept perfectly in sync. If a type is added to one but not the other, you get either a missed scalar list or a silent default to"string".Silent fallback (line 392): This return is unreachable today (the method is only called after
isScalarChildListreturnstrue), but it silently defaults to"string"instead of failing fast. If the two methods drift apart, this will mask the bug.Consider extracting the mapping into a single
Map<String, String>(or an enum) and deriving both detection and schema from it:♻️ Suggested refactor
+ private static final Map<String, String> SCALAR_WRAPPER_TYPES = Map.of( + "java.lang.String", "string", + "java.lang.Boolean", "boolean", + "java.lang.Integer", "integer", + "java.lang.Long", "integer", + "java.lang.Short", "integer", + "java.lang.Byte", "integer", + "java.lang.Double", "number", + "java.lang.Float", "number" + ); + private boolean isScalarChildList(ChildElementInfo cei) { if (!cei.isList()) return false; if (cei.getTypeDeclaration() == null) return false; - - String qn = cei.getTypeDeclaration().getQualifiedName().toString(); - - return "java.lang.String".equals(qn) - || "java.lang.Boolean".equals(qn) - || "java.lang.Integer".equals(qn) - || "java.lang.Long".equals(qn) - || "java.lang.Double".equals(qn) - || "java.lang.Float".equals(qn) - || "java.lang.Short".equals(qn) - || "java.lang.Byte".equals(qn); + return SCALAR_WRAPPER_TYPES.containsKey( + cei.getTypeDeclaration().getQualifiedName().toString()); } private AbstractSchema<?> scalarItemsSchema(ChildElementInfo cei) { String qn = cei.getTypeDeclaration().getQualifiedName().toString(); - - if ("java.lang.String".equals(qn)) { - return SchemaFactory.from("string").type("string"); - } - if ("java.lang.Boolean".equals(qn)) { - return SchemaFactory.from("boolean").type("boolean"); - } - if ("java.lang.Integer".equals(qn) - || "java.lang.Long".equals(qn) - || "java.lang.Short".equals(qn) - || "java.lang.Byte".equals(qn)) { - return SchemaFactory.from("integer").type("integer"); - } - if ("java.lang.Double".equals(qn) || "java.lang.Float".equals(qn)) { - return SchemaFactory.from("number").type("number"); - } - - return SchemaFactory.from("string").type("string"); + String schemaType = SCALAR_WRAPPER_TYPES.get(qn); + if (schemaType == null) { + throw new IllegalStateException("Not a scalar wrapper type: " + qn); + } + return SchemaFactory.from(schemaType).type(schemaType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 357 - 393, Extract a single centralized mapping (e.g., Map<String,String> PRIMITIVE_QN_TO_JSON_TYPE or an enum) that maps Java qualified names to JSON Schema types, then update isScalarChildList(ChildElementInfo cei) to consult that map (returning true if cei.isList() && map contains the type qn) and update scalarItemsSchema(ChildElementInfo cei) to look up the JSON type from the same map and build the SchemaFactory accordingly; remove the duplicated string checks and replace the silent default return with a fail-fast behavior (throw IllegalStateException or return Optional and let the caller handle) so a drift between detection and schema construction is immediately visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 357-393: Extract a single centralized mapping (e.g.,
Map<String,String> PRIMITIVE_QN_TO_JSON_TYPE or an enum) that maps Java
qualified names to JSON Schema types, then update
isScalarChildList(ChildElementInfo cei) to consult that map (returning true if
cei.isList() && map contains the type qn) and update
scalarItemsSchema(ChildElementInfo cei) to look up the JSON type from the same
map and build the SchemaFactory accordingly; remove the duplicated string checks
and replace the silent default return with a fail-fast behavior (throw
IllegalStateException or return Optional and let the caller handle) so a drift
between detection and schema construction is immediately visible.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
|
This pull request needs "/ok-to-test" from an authorized committer. |
…ability and clarity
…roved readability and maintainability
…proved type handling and code clarity
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Line 55: The current componentParsersAdded Set in JsonSchemaGenerator silently
ignores collisions when two noEnvelope child parsers share the same name but
different types; replace the Set<String> componentParsersAdded with a
Map<String,String> (or Map<defName, typeName>) and, wherever entries are added
(the code paths handling noEnvelope child parser registration and the logic
around the previous Set usage), check if an existing mapping for that def name
has a different type and throw an exception (or fail fast) describing the
mismatch (include def name, existing type and new type); update any
lookups/contains checks to use map.get/put and ensure downstream code that
relied on componentParsersAdded is adjusted to use the map’s keys or values as
needed.
- Around line 312-334: The componentRef inline-list variant currently marks the
$ref property as optional which allows an empty object to validate; in
processInlineList update the componentRef schema so the $ref property on the
object produced by object().title("componentRef").additionalProperties(false) is
required (make string($_REF).required(true) or call .required(...) with the $ref
property) when building the variants (refer to itemElementInfo,
isComponentsList, and attachArrayItems) so componentRef cannot validate as {}.
- Around line 151-155: The non-list component reference is currently added as
"$ref" which can collide with JSON Schema keywords; change the code in
JsonSchemaGenerator so when hasComponentChild(elementInfo, main) (non-list
context) and !parser.hasProperty("ref"), you add a string property named "ref"
(not "$ref") via parser.property(...) using the same string(...) builder (ensure
description indicates a Spring bean/component reference and required(false));
leave "$ref" usage untouched for list-item shapes. Reference
functions/identifiers: hasComponentChild, parser.hasProperty, parser.property,
and the string($_REF) builder.
- Around line 454-471: The code in addChildsAsProperties dereferences
getChildElementDeclarationInfo(main, childSpec) without a null check which can
cause NPEs; modify addChildsAsProperties to call
getChildElementDeclarationInfo(...) into a local variable, check for null, and
either return early (no children to add) or throw a clear IllegalStateException
mentioning main and childSpec; then proceed to compute childElementInfos and use
parentObjectSchema as before (references: addChildsAsProperties,
getChildElementDeclarationInfo, childElementInfos, parentObjectSchema).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
…rray items and object properties into helper methods for improved readability and maintainability
… method for improved code reuse and readability
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (4)
438-455:⚠️ Potential issue | 🟠 MajorNull-guard child declaration lookup to avoid NPEs.
getChildElementDeclarationInfo(...)can be null; this path dereferences it unconditionally.🛡️ Suggested guard
- var childElementInfos = getChildElementDeclarationInfo(main, childSpec).getElementInfo().stream() + var childDeclaration = getChildElementDeclarationInfo(main, childSpec); + if (childDeclaration == null) { + return; // or throw a ProcessingException with context + } + var childElementInfos = childDeclaration.getElementInfo().stream()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 438 - 455, The call to getChildElementDeclarationInfo(...) inside JsonSchemaGenerator.addChildsAsProperties can return null and is dereferenced immediately; add a null-guard: call var decl = getChildElementDeclarationInfo(main, childSpec); if decl is null (or decl.getElementInfo() is null/empty) return/continue so the subsequent .getElementInfo().stream() never dereferences null. Ensure you update references in addChildsAsProperties to use the local decl variable and bail out early when there's nothing to process.
338-345:⚠️ Potential issue | 🟠 MajorMake componentRef
$refrequired in inline list variant.
Optional$refallows{}to validate as a list item.✅ Suggested fix
- .property(string($_REF).required(false))); + .property(string($_REF).required(true)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 338 - 345, The inline list variant builds an anyOf between itemsSchema and a componentRef object but marks the $ref property as optional, which lets an empty object pass; in JsonSchemaGenerator change the component branch where you construct the second variant (the object() titled "componentRef" with .property(string($_REF).required(false))) so that the $ref property is required (e.g., use .property(string($_REF).required(true)) or remove the false) before creating itemsSchema = anyOf(variants); keep references to isComponentsList, itemElementInfo.getAnnotation().component(), itemsSchema, and anyOf(variants) when locating the code to modify.
161-174:⚠️ Potential issue | 🟠 MajorUse
ref(not$ref) for non-list component references.
This avoids JSON Schema keyword collision and preserves IDE completion hints for component references.Based on learnings "In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision."🔧 Suggested change
- if (hasComponentChild(elementInfo, main) && !parserSchema.hasProperty($_REF)) { - parserSchema.property(string($_REF) + if (hasComponentChild(elementInfo, main) && !parserSchema.hasProperty("ref")) { + parserSchema.property(string("ref") .description("JSON Pointer to a component.") .required(false)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 161 - 174, In createRegularParser(Model model, MainInfo main, ElementInfo elementInfo, String parserName) the code adds a property named $_REF for non-list component references which collides with JSON Schema keywords and breaks IDE completion; change the conditional that currently checks parserSchema.hasProperty($_REF) and the subsequent property creation to use the literal property name "ref" (string type) instead of $_REF so when hasComponentChild(elementInfo, main) is true you add parserSchema.property(string("ref").description("JSON Pointer to a component.").required(false)) only if parserSchema.hasProperty("ref") is false; update any related checks that look for $_REF to look for "ref" for non-list children in this class.
55-55:⚠️ Potential issue | 🟠 MajorPrevent silent collisions for noEnvelope child parser defs.
Using a Set keyed by child name can hide mismatched parser definitions when different types share the same child name. Consider tracking def name → type and failing fast on conflicts.💡 Suggested safeguard
- private final Set<String> componentParsersAdded = new HashSet<>(); + private final Map<String, String> componentParsersAdded = new HashMap<>(); @@ - if (!componentParsersAdded.contains(childName) && !shouldGenerateFlowParserType(childSpec)) { - SchemaArray childParserArray = array(childName + "Parser"); - processMCChilds(model, main, childSpec.getEi(), childParserArray); - schema.definition(childParserArray); - componentParsersAdded.add(childName); - } + String parserDefName = childName + "Parser"; + if (!shouldGenerateFlowParserType(childSpec)) { + String existing = componentParsersAdded.get(parserDefName); + if (existing != null && !existing.equals(childSpec.getEi().getXSDTypeName(model))) { + throw new ProcessingException("Conflicting parser definitions for " + parserDefName, elementInfo.getElement()); + } + if (existing == null) { + SchemaArray childParserArray = array(parserDefName); + processMCChilds(model, main, childSpec.getEi(), childParserArray); + schema.definition(childParserArray); + componentParsersAdded.put(parserDefName, childSpec.getEi().getXSDTypeName(model)); + } + } @@ - return ref(parserName).ref(defsRefPath(childName + "Parser")); + return ref(parserName).ref(defsRefPath(parserDefName));Also applies to: 132-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` at line 55, The Set field componentParsersAdded can silently mask collisions when different parser types reuse the same child name; replace it with a Map<String,String> mapping child-def-name → parser-type (e.g., componentParserNameToType) and, where componentParsersAdded is checked/updated (the code that registers "noEnvelope" child parsers and the logic around the block handling child parser defs), change the logic to lookup the existing type and throw an IllegalStateException (or other fast-fail) if an existing type differs from the new type instead of ignoring the duplicate; update all places that add/check componentParsersAdded to use the new map so conflicts are detected and reported immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 438-455: The call to getChildElementDeclarationInfo(...) inside
JsonSchemaGenerator.addChildsAsProperties can return null and is dereferenced
immediately; add a null-guard: call var decl =
getChildElementDeclarationInfo(main, childSpec); if decl is null (or
decl.getElementInfo() is null/empty) return/continue so the subsequent
.getElementInfo().stream() never dereferences null. Ensure you update references
in addChildsAsProperties to use the local decl variable and bail out early when
there's nothing to process.
- Around line 338-345: The inline list variant builds an anyOf between
itemsSchema and a componentRef object but marks the $ref property as optional,
which lets an empty object pass; in JsonSchemaGenerator change the component
branch where you construct the second variant (the object() titled
"componentRef" with .property(string($_REF).required(false))) so that the $ref
property is required (e.g., use .property(string($_REF).required(true)) or
remove the false) before creating itemsSchema = anyOf(variants); keep references
to isComponentsList, itemElementInfo.getAnnotation().component(), itemsSchema,
and anyOf(variants) when locating the code to modify.
- Around line 161-174: In createRegularParser(Model model, MainInfo main,
ElementInfo elementInfo, String parserName) the code adds a property named $_REF
for non-list component references which collides with JSON Schema keywords and
breaks IDE completion; change the conditional that currently checks
parserSchema.hasProperty($_REF) and the subsequent property creation to use the
literal property name "ref" (string type) instead of $_REF so when
hasComponentChild(elementInfo, main) is true you add
parserSchema.property(string("ref").description("JSON Pointer to a
component.").required(false)) only if parserSchema.hasProperty("ref") is false;
update any related checks that look for $_REF to look for "ref" for non-list
children in this class.
- Line 55: The Set field componentParsersAdded can silently mask collisions when
different parser types reuse the same child name; replace it with a
Map<String,String> mapping child-def-name → parser-type (e.g.,
componentParserNameToType) and, where componentParsersAdded is checked/updated
(the code that registers "noEnvelope" child parsers and the logic around the
block handling child parser defs), change the logic to lookup the existing type
and throw an IllegalStateException (or other fast-fail) if an existing type
differs from the new type instead of ignoring the duplicate; update all places
that add/check componentParsersAdded to use the new map so conflicts are
detected and reported immediately.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
432-434: Null dereference ongetChildElementDeclarationInfo(main, childSpec)still unguarded.
getChildElementDeclarations(main).get(childSpec.getTypeDeclaration())returnsnullfor unknown type declarations, and line 433 chains.getElementInfo()directly. Other callers (hasComponentChild,shouldInlineListItems) already null-check; this path does not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 432 - 434, In addChildsAsProperties, guard against a null return from getChildElementDeclarationInfo(main, childSpec) before calling .getElementInfo(): retrieve the declaration into a local (e.g., childDecl), if childDecl is null then return or skip processing (same pattern used in hasComponentChild/shouldInlineListItems), otherwise continue by calling childDecl.getElementInfo().stream() so you avoid the null dereference on unknown type declarations.
338-345:componentRefVariantSchema(false)still marks$refas optional, allowing{}to validate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 338 - 345, The anyOf variant that allows a component reference currently uses componentRefVariantSchema(false), which leaves the $ref property optional and lets {} validate; change the variant to require $ref (e.g., call componentRefVariantSchema(true) or otherwise set the returned schema to require the "$ref" property) so the component-ref variant is not satisfied by an empty object; update the code around the anyOf creation (where itemsSchema is combined with componentRefVariantSchema(...) before attachArrayItems) to use the required-$ref variant.
55-55:componentParsersAddedstill tracks def names without collision detection.A
Set<String>silently ignores the case where two noEnvelope child parsers share the same child property name but belong to different types. If a second (different-typed) child maps to the samechildName + "Parser"key, the existing def is reused incorrectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` at line 55, componentParsersAdded currently uses a Set<String> of parser def names and therefore silently reuses the same parser def when two different parent types have child parsers with identical childName + "Parser" keys; change the tracking to include the owning type (e.g. use a Map<String,String> or Set of composite keys) so keys are unique per owner and detect collisions: replace componentParsersAdded with a structure that records the owner type (class/type name) with the parser key, update the code paths in JsonSchemaGenerator that add/check componentParsersAdded (references: componentParsersAdded, the logic that builds childName + "Parser") to use the composite key (ownerType + ":" + childName) or generate a unique name (append owner or a counter) when a name collision is found, and ensure existing usages that reference the parser name are updated to the new unique name.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
372-378:NONEinscalarItemsSchemasilently returns aSTRINGschema instead of failing fast.The
NONEcase is unreachable today becauseisScalarChildListguards every call site, but ifscalarItemsSchemais ever called directly or the guard is loosened, the fallback to STRING will silently produce an incorrect schema rather than a clear error.♻️ Proposed change — throw on NONE
private AbstractSchema<?> scalarItemsSchema(ChildElementInfo childSpec) { return switch (getScalarSchemaKind(childSpec)) { - case STRING, NONE -> SchemaFactory.from("string").type("string"); + case STRING -> SchemaFactory.from("string").type("string"); + case NONE -> throw new IllegalStateException("scalarItemsSchema called for non-scalar child: " + childSpec.getPropertyName()); case BOOLEAN -> SchemaFactory.from("boolean").type("boolean"); case INTEGER -> SchemaFactory.from("integer").type("integer"); case NUMBER -> SchemaFactory.from("number").type("number"); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 372 - 378, scalarItemsSchema currently maps getScalarSchemaKind(childSpec) NONE to a STRING schema which masks bugs; change scalarItemsSchema to throw an IllegalStateException (or similar runtime exception) when getScalarSchemaKind(childSpec) returns NONE so the method fails fast instead of silently producing a wrong schema. Locate the method scalarItemsSchema(ChildElementInfo) and replace the NONE branch with a throw including context (childSpec or its name/type) so callers get a clear error; keep the existing STRING/BOOLEAN/INTEGER/NUMBER branches unchanged. Ensure any callers guarded by isScalarChildList continue to work, but the method will now surface misuse immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 133-135: The call to elementInfo.getChildElementSpecs().getFirst()
in JsonSchemaGenerator is unguarded and will throw NoSuchElementException when
there are no child specs; update the code that accesses
elementInfo.getChildElementSpecs() (before calling getFirst()) to check whether
the collection is empty and if so throw a ProcessingException with a clear
message referencing the element (e.g., include elementInfo.getName() or similar)
and the annotation contract (noEnvelope=true requires exactly one child);
otherwise proceed to call getFirst() and continue using the resulting
ChildElementInfo and its propertyName.
- Around line 300-305: The non-list branch in processMCChilds currently does an
unsafe (SchemaObject) parentSchema cast; change it to type-check parentSchema
first: if parentSchema instanceof SchemaObject call addChildsAsProperties(model,
main, childSpec, (SchemaObject) parentSchema,
isComponentsList(parentElementInfo, childSpec), false); else if parentSchema
instanceof SchemaArray obtain the array's item schema (use
setItemsIfArray/getItems or the existing helper that ensures items are an
object) and call addChildsAsProperties with that SchemaObject; this mirrors the
guarded list-path behavior (setItemsIfArray/addPropertyIfObject) and prevents
ClassCastException when parentSchema is a SchemaArray.
---
Duplicate comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 432-434: In addChildsAsProperties, guard against a null return
from getChildElementDeclarationInfo(main, childSpec) before calling
.getElementInfo(): retrieve the declaration into a local (e.g., childDecl), if
childDecl is null then return or skip processing (same pattern used in
hasComponentChild/shouldInlineListItems), otherwise continue by calling
childDecl.getElementInfo().stream() so you avoid the null dereference on unknown
type declarations.
- Around line 338-345: The anyOf variant that allows a component reference
currently uses componentRefVariantSchema(false), which leaves the $ref property
optional and lets {} validate; change the variant to require $ref (e.g., call
componentRefVariantSchema(true) or otherwise set the returned schema to require
the "$ref" property) so the component-ref variant is not satisfied by an empty
object; update the code around the anyOf creation (where itemsSchema is combined
with componentRefVariantSchema(...) before attachArrayItems) to use the
required-$ref variant.
- Line 55: componentParsersAdded currently uses a Set<String> of parser def
names and therefore silently reuses the same parser def when two different
parent types have child parsers with identical childName + "Parser" keys; change
the tracking to include the owning type (e.g. use a Map<String,String> or Set of
composite keys) so keys are unique per owner and detect collisions: replace
componentParsersAdded with a structure that records the owner type (class/type
name) with the parser key, update the code paths in JsonSchemaGenerator that
add/check componentParsersAdded (references: componentParsersAdded, the logic
that builds childName + "Parser") to use the composite key (ownerType + ":" +
childName) or generate a unique name (append owner or a counter) when a name
collision is found, and ensure existing usages that reference the parser name
are updated to the new unique name.
---
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 372-378: scalarItemsSchema currently maps
getScalarSchemaKind(childSpec) NONE to a STRING schema which masks bugs; change
scalarItemsSchema to throw an IllegalStateException (or similar runtime
exception) when getScalarSchemaKind(childSpec) returns NONE so the method fails
fast instead of silently producing a wrong schema. Locate the method
scalarItemsSchema(ChildElementInfo) and replace the NONE branch with a throw
including context (childSpec or its name/type) so callers get a clear error;
keep the existing STRING/BOOLEAN/INTEGER/NUMBER branches unchanged. Ensure any
callers guarded by isScalarChildList continue to work, but the method will now
surface misuse immediately.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
…ng parser sources and improve declaration handling logic
…to json-schema-enhancement-list
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
178-181:⚠️ Potential issue | 🟠 Major
"$ref"is still used for the non-list object-level component reference — likely a regression.A previous review flagged this and it was marked addressed, but the current code still adds
string($_REF)(i.e."$ref") as the property name in the non-list context. For non-list elements, the property should be named"ref"to avoid collision with the JSON Schema$refkeyword and to preserve IDE completion for Spring bean references.🔧 Proposed fix
- if (hasComponentChild(elementInfo, main) && !parserSchema.hasProperty($_REF)) { - parserSchema.property(string($_REF) + if (hasComponentChild(elementInfo, main) && !parserSchema.hasProperty("ref")) { + parserSchema.property(string("ref") .description("JSON Pointer to a component.") .required(false)); }Based on learnings: in
JsonSchemaGenerator.java, whenallowForeignis true for non-list child elements, use a property named"ref"(not"$ref") to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 178 - 181, The code still adds a property named "$ref" via string($_REF) for non-list component children; change the logic in JsonSchemaGenerator (around hasComponentChild and parserSchema.property calls) so that when the element is a non-list component child (e.g., check elementInfo.isList() or allowForeign for non-list), you add/guard a property named "ref" (string("ref")) instead of "$ref" while preserving the description and required(false); keep using "$ref" only for list contexts where appropriate and update the parserSchema.hasProperty checks to match the chosen property name.
313-314:⚠️ Potential issue | 🔴 CriticalUnsafe
(SchemaObject) parentSchemacast is still present —ClassCastExceptionat annotation-processing time.
processMCChildsis called fromcreateNoEnvelopeParser(line 155) withchildParserArray— aSchemaArray. If the noEnvelope child element declares any non-list@MCChildElement, the!childSpec.isList()branch at line 313 executes and the unchecked cast on line 314 throwsClassCastException, producing an opaque build failure. This was flagged in a previous review with no resolution marker.🔒 Proposed fix — pattern-match instead of unchecked cast
if (!childSpec.isList()) { - addChildsAsProperties(model, main, childSpec, (SchemaObject) parentSchema, isComponentsList(parentElementInfo, childSpec), false); + if (parentSchema instanceof SchemaObject parentObjectSchema) { + addChildsAsProperties(model, main, childSpec, parentObjectSchema, isComponentsList(parentElementInfo, childSpec), false); + } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 313 - 314, processMCChilds currently does an unchecked cast `(SchemaObject) parentSchema` which throws ClassCastException when parentSchema is a SchemaArray (e.g., called from createNoEnvelopeParser with childParserArray). Fix by pattern-matching the runtime type of parentSchema before casting: if parentSchema instanceof SchemaObject, cast and call addChildsAsProperties(model, main, childSpec, (SchemaObject) parentSchema, isComponentsList(parentElementInfo, childSpec), false); else if parentSchema instanceof SchemaArray, obtain the array's item schema (or create/unwrap a SchemaObject representation) and call addChildsAsProperties with that SchemaObject variant; update processMCChilds to handle both branches (or delegate to separate helpers) to avoid the unchecked cast and ClassCastException, using identifiers processMCChilds, createNoEnvelopeParser, addChildsAsProperties, parentSchema, parentElementInfo, and childSpec to locate the changes.
133-135:⚠️ Potential issue | 🔴 Critical
getFirst()is still unguarded — throwsNoSuchElementExceptionwith a cryptic stack trace when@MCElement(noEnvelope=true)declares no@MCChildElement.This was flagged in a previous review without a resolution marker; the fix was not applied.
🛡️ Proposed guard
+ var childSpecs = elementInfo.getChildElementSpecs(); + if (childSpecs.isEmpty()) { + throw new ProcessingException( + "@MCElement(noEnvelope=true) requires exactly one `@MCChildElement`.", + elementInfo.getElement() + ); + } - ChildElementInfo childSpec = elementInfo.getChildElementSpecs().getFirst(); + ChildElementInfo childSpec = childSpecs.getFirst();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 133 - 135, The code calls elementInfo.getChildElementSpecs().getFirst() without checking for an empty list, which leads to NoSuchElementException when `@MCElement`(noEnvelope=true) declares no `@MCChildElement`; update JsonSchemaGenerator to first check whether elementInfo.getChildElementSpecs() is empty (or has size() == 0) before calling getFirst() and, if empty, throw a clear IllegalStateException (or similar) that mentions the element name/annotation (e.g., include elementInfo.getName() and the fact that noEnvelope=true requires exactly one child) so the error is descriptive; modify the block around ChildElementInfo childSpec = elementInfo.getChildElementSpecs().getFirst() and subsequent use of childSpec/childName accordingly.
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
396-403: TheNONEarm inscalarItemsSchemais dead code.
scalarItemsSchemais called exclusively from theisScalarChildList(childSpec)guard, which returnstrueonly whengetScalarSchemaKind != NONE. TheNONEcase in the switch is therefore unreachable. Removing it (or replacing it with athrow) makes the invariant explicit and eliminates confusion about the fallback-to-string behavior.♻️ Proposed change
private AbstractSchema<?> scalarItemsSchema(ChildElementInfo childSpec) { return switch (getScalarSchemaKind(childSpec)) { - case STRING, NONE -> SchemaFactory.from("string").type("string"); + case STRING -> SchemaFactory.from("string").type("string"); case BOOLEAN -> SchemaFactory.from("boolean").type("boolean"); case INTEGER -> SchemaFactory.from("integer").type("integer"); case NUMBER -> SchemaFactory.from("number").type("number"); + case NONE -> throw new IllegalStateException("scalarItemsSchema called with NONE kind"); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 396 - 403, The NONE branch in scalarItemsSchema is unreachable because isScalarChildList(childSpec) guarantees getScalarSchemaKind(childSpec) != NONE; update scalarItemsSchema to remove the NONE->string fallback and make the invariant explicit by replacing the NONE case with an exception (e.g., throw new IllegalStateException) or by eliminating the NONE arm entirely so that any unexpected value fails fast; reference scalarItemsSchema, getScalarSchemaKind, and isScalarChildList to locate and adjust the switch accordingly.
610-620: Use primitivebooleaninstead of boxedBoolean— the null branch is dead code.No call site passes
nulltocomponentRefVariantSchema(Boolean). The null-guard on line 612 is never triggered. UsingBoolean(boxed) overboolean(primitive) signals intentional nullability, which is misleading here.♻️ Proposed simplification
- private static SchemaObject componentRefVariantSchema(Boolean required) { + private static SchemaObject componentRefVariantSchema(boolean required) { var refProperty = string($_REF); - if (required != null) { - refProperty.required(required); - } + refProperty.required(required); return object() .title("componentRef") .additionalProperties(false) .property(refProperty); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 610 - 620, The componentRefVariantSchema(Boolean) method uses a boxed Boolean though callers never pass null; change the parameter to primitive boolean in the method signature (componentRefVariantSchema(boolean required)) and remove the dead null-check branch that conditionally calls refProperty.required(required), instead always call refProperty.required(required); update any call sites if necessary to pass a boolean (no behavior change).
162-170: ThenoEnvelopeguard insidecreateCollapsedParseris unreachable dead code.
createParseralready routesnoEnvelopeelements tocreateNoEnvelopeParserbefore checkingcollapsed.createCollapsedParseris therefore only ever called withelementInfo.getAnnotation().noEnvelope() == false, making the inner check andProcessingExceptionpermanently unreachable.♻️ Remove dead guard
private AbstractSchema<?> createCollapsedParser(ElementInfo elementInfo, String parserName) { - if (elementInfo.getAnnotation().noEnvelope()) { - throw new ProcessingException( - "@MCElement(collapsed=true) is not compatible with noEnvelope=true.", - elementInfo.getElement() - ); - } return createCollapsedInlineParser(elementInfo, parserName); }If the compatibility constraint should be documented, add a comment in
createParsernear the ordering of the two guards instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` around lines 162 - 170, The check for elementInfo.getAnnotation().noEnvelope() inside createCollapsedParser is dead code because createParser already routes noEnvelope cases to createNoEnvelopeParser before handling collapsed; remove the unreachable guard and its ProcessingException from createCollapsedParser, and instead add a brief comment in createParser near the ordering of the noEnvelope vs collapsed guards documenting that noEnvelope elements are handled earlier and are incompatible with collapsed behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 138-139: The createNoEnvelopeParser method returns a ref to
`#/`$defs/flowParser (via ref(parserName).ref(defsRefPath(FLOW_PARSER_DEF_NAME)))
but never ensures the flowParser definition is created, leading to a dangling
$ref when addFlowParserRef is never invoked; modify JsonSchemaGenerator so that
whenever createNoEnvelopeParser registers a flow-parser reference it eagerly
ensures the flowParser definition exists (call or inline the logic in
addFlowParserRef or create a helper like ensureFlowParserDef) so
FLOW_PARSER_DEF_NAME is added to the defs map when the first no-envelope flow
reference is created; update createNoEnvelopeParser to invoke that helper (or
directly create the definition) before returning the ref.
---
Duplicate comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 178-181: The code still adds a property named "$ref" via
string($_REF) for non-list component children; change the logic in
JsonSchemaGenerator (around hasComponentChild and parserSchema.property calls)
so that when the element is a non-list component child (e.g., check
elementInfo.isList() or allowForeign for non-list), you add/guard a property
named "ref" (string("ref")) instead of "$ref" while preserving the description
and required(false); keep using "$ref" only for list contexts where appropriate
and update the parserSchema.hasProperty checks to match the chosen property
name.
- Around line 313-314: processMCChilds currently does an unchecked cast
`(SchemaObject) parentSchema` which throws ClassCastException when parentSchema
is a SchemaArray (e.g., called from createNoEnvelopeParser with
childParserArray). Fix by pattern-matching the runtime type of parentSchema
before casting: if parentSchema instanceof SchemaObject, cast and call
addChildsAsProperties(model, main, childSpec, (SchemaObject) parentSchema,
isComponentsList(parentElementInfo, childSpec), false); else if parentSchema
instanceof SchemaArray, obtain the array's item schema (or create/unwrap a
SchemaObject representation) and call addChildsAsProperties with that
SchemaObject variant; update processMCChilds to handle both branches (or
delegate to separate helpers) to avoid the unchecked cast and
ClassCastException, using identifiers processMCChilds, createNoEnvelopeParser,
addChildsAsProperties, parentSchema, parentElementInfo, and childSpec to locate
the changes.
- Around line 133-135: The code calls
elementInfo.getChildElementSpecs().getFirst() without checking for an empty
list, which leads to NoSuchElementException when `@MCElement`(noEnvelope=true)
declares no `@MCChildElement`; update JsonSchemaGenerator to first check whether
elementInfo.getChildElementSpecs() is empty (or has size() == 0) before calling
getFirst() and, if empty, throw a clear IllegalStateException (or similar) that
mentions the element name/annotation (e.g., include elementInfo.getName() and
the fact that noEnvelope=true requires exactly one child) so the error is
descriptive; modify the block around ChildElementInfo childSpec =
elementInfo.getChildElementSpecs().getFirst() and subsequent use of
childSpec/childName accordingly.
---
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 396-403: The NONE branch in scalarItemsSchema is unreachable
because isScalarChildList(childSpec) guarantees getScalarSchemaKind(childSpec)
!= NONE; update scalarItemsSchema to remove the NONE->string fallback and make
the invariant explicit by replacing the NONE case with an exception (e.g., throw
new IllegalStateException) or by eliminating the NONE arm entirely so that any
unexpected value fails fast; reference scalarItemsSchema, getScalarSchemaKind,
and isScalarChildList to locate and adjust the switch accordingly.
- Around line 610-620: The componentRefVariantSchema(Boolean) method uses a
boxed Boolean though callers never pass null; change the parameter to primitive
boolean in the method signature (componentRefVariantSchema(boolean required))
and remove the dead null-check branch that conditionally calls
refProperty.required(required), instead always call
refProperty.required(required); update any call sites if necessary to pass a
boolean (no behavior change).
- Around line 162-170: The check for elementInfo.getAnnotation().noEnvelope()
inside createCollapsedParser is dead code because createParser already routes
noEnvelope cases to createNoEnvelopeParser before handling collapsed; remove the
unreachable guard and its ProcessingException from createCollapsedParser, and
instead add a brief comment in createParser near the ordering of the noEnvelope
vs collapsed guards documenting that noEnvelope elements are handled earlier and
are incompatible with collapsed behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
…` and improve flow parser handling logic
predic8
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor