Add Yaml support for inlining list items (Breaking Change)#2742
Conversation
…specific conditions are met.
📝 WalkthroughWalkthroughSchema generator and YAML parser were updated to support inlining of single-type list items; parser gained element-type-aware list parsing and inline-item construction; MethodSetter exposes collection element types; many example YAMLs were flattened; tests and headers updated. Changes
Sequence Diagram(s)sequenceDiagram
participant YAML as "YAML Input"
participant Parser as "GenericYamlParser"
participant Setter as "MethodSetter"
participant Generator as "JsonSchemaGenerator"
YAML->>Parser: receive list node
Parser->>Setter: getCollectionElementType(setter)
Setter-->>Parser: elemType (may be null)
alt elemType allows inlining
Parser->>Parser: parseListItem / parseInlineListItem using elemType
Parser-->>Generator: indicate inline-able list & item type
Generator->>Generator: processInlineList -> build items schema / component $ref
Generator-->>Parser: attach inlined items schema to parent schema
else fallback
Parser->>Parser: parse list items generically (allow $ref)
Parser-->>Generator: provide generic item schemas
Generator->>Generator: attachArrayItems -> array property on parent
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 486-509: The guard in shouldInlineListItems (which returns
countRelevantAttributes(eis.getFirst()) > 1) is unclear and flagged by TODOs
because `@collapsed` elements with ≤1 relevant attributes become scalar $defs via
createCollapsedInlineParser; either document the rationale or change behavior:
update shouldInlineListItems/ countRelevantAttributes to remove the >1 threshold
and add a unit/integration test that demonstrates collapsed items inline
correctly (exercise ChildElementInfo/ElementInfo cases, including `@collapsed`
elements) OR keep the threshold but add a concise comment next to the >1 check
explaining exactly why a minimum of 2 relevant attributes is required (reference
createCollapsedInlineParser and UX rendering concerns) so future readers
understand the intent.
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (2)
511-524: Silent no-op ifparentSchemais neitherSchemaArraynorSchemaObject.If a future refactoring passes an unexpected schema type, list items would silently be dropped from the generated schema. Consider adding a fallback
elsebranch that logs a warning or throws, to surface such issues early.
296-297:nullpassed forsosis safe but non-obvious.
processList(i, so, cei, null)works becauseshouldGenerateFlowParserTypewas already ruled out on line 286, so thesosparameter is never dereferenced insideprocessList. A brief inline comment (e.g.,/* sos unused — flow branch excluded above */) would aid readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 280-298: The code unconditionally casts `so` to `SchemaObject`
when calling `addChildsAsProperties`, which can throw ClassCastException if `so`
is a `SchemaArray`; update the branch that handles non-list children to guard
the cast: check `if (so instanceof SchemaObject)` and call
`addChildsAsProperties(m, main, cei, (SchemaObject) so, ...)` as before, else if
`so instanceof SchemaArray` obtain the array items schema (e.g. `((SchemaArray)
so).getItems()`), ensure that items is a `SchemaObject` and pass that to
`addChildsAsProperties` (or handle the non-`SchemaObject` case accordingly), so
the calls in the non-list path and the final `addChildsAsProperties` use a safe,
checked `SchemaObject` instead of an unchecked cast.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
505-518: Silent no-op whenparentSchemais neitherSchemaArraynorSchemaObject.If a future schema type is passed, this method silently drops the items — hard to debug. Consider throwing
IllegalStateExceptionin anelsebranch.Also, the noEnvelope-vs-object branching duplicates the same pattern in
processList(lines 373-379). A shared helper could reduce divergence risk.Optional: add a defensive else branch
if (parentSchema instanceof SchemaObject so) { so.property(array(cei.getPropertyName()) .items(itemsSchema) .required(cei.isRequired()) .description(getDescriptionContent(cei))); + } else { + throw new IllegalStateException("Unexpected parent schema type: " + parentSchema.getClass()); }
…y` method to improve JSON schema generation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 341-345: The property definition for the componentRef schema in
JsonSchemaGenerator (the sos.add(object().title("componentRef")...
.property(string("$ref"))) is missing an explicit .required(false); update that
call to .property(string("$ref").required(false)) for consistency with the
analogous properties at lines 318 and 407 so intent is explicit.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
296-297: Passingnullforsosworks but is fragile.
processListonly accessessosinside theshouldGenerateFlowParserTypebranch (line 366-368), sonullis currently safe. However, this implicit contract is easy to break during future refactoring. Consider splittingprocessListinto two methods — one for the flow-parser path and one for the generic path — to make the null-safety explicit.
christiangoerdes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distribution/examples/loadbalancing/1-static/apis.yaml (1)
1-1:⚠️ Potential issue | 🟠 MajorUpdate schema version to v7.0.7 in loadbalancing examples.
The loadbalancing examples (1-static through 5-multiple) were updated with the new flattened
clusters/nodesstructure but still referencev7.0.6.json. The project version is currently7.0.7-SNAPSHOTin pom.xml, and only 8 other examples have been updated tov7.0.7. Update these schema references fromhttps://www.membrane-api.io/v7.0.6.jsontov7.0.7.jsonto match the code version and ensure consistent schema validation.
🧹 Nitpick comments (1)
distribution/tutorials/getting-started/90-OpenAPI-Validation.yaml (1)
13-16: Flattened spec structure is consistent with the broader pattern.The inlining looks good.
Minor nit: this file uses
true/falsefor booleans whiledistribution/examples/openapi/validation/apis.yamlusesyes/no. Both are valid YAML 1.1 booleans, but using a consistent style across examples would reduce potential confusion for users.,
|
/ok-to-test |
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation
Examples