add singleAttribute option for the @MCElement annotation#2583
Conversation
📝 WalkthroughWalkthroughAdds MCElement.singleAttribute(), enables inline scalar YAML mapping for single-attribute elements, updates JSON Schema generation to emit scalar inline variants, adapts YAML parser to accept scalars for such elements, and adds tests covering inline single-attribute parsing. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
713-769: Good coverage for list scenario withnoEnvelope.This test effectively validates the combination of
singleAttribute=trueandnoEnvelope=true. The test uses String type which complements the int type test above.Consider adding negative test cases:
- An element with
singleAttribute=truebut multiple@MCAttributesetters (should fail at parse time)- Invalid type conversion (e.g., non-numeric string for an int attribute)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
390-405: Consider improving error context for validation failures.The
findSingleAttributeSettermethod throws aParsingExceptionwith a syntheticnullNode()when validation fails. This loses location context that could help users debug configuration issues.♻️ Suggested improvement: pass the node for better error context
- private static Method findSingleAttributeSetter(Class<?> clazz) { + private static Method findSingleAttributeSetter(Class<?> clazz, JsonNode node) { List<Method> setters = new ArrayList<>(); ReflectionUtils.doWithMethods(clazz, m -> { if (m.isAnnotationPresent(MCAttribute.class) && m.getParameterCount() == 1) { setters.add(m); } }); if (setters.size() != 1) { throw new ParsingException( "@MCElement(singleAttribute=true) requires exactly one @MCAttribute setter, but found " + setters.size() + ".", - JsonNodeFactory.instance.nullNode() + node ); } return setters.getFirst(); }Then update the call site at line 370:
- Method setter = findSingleAttributeSetter(clazz); + Method setter = findSingleAttributeSetter(clazz, node);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
annot/src/main/java/com/predic8/membrane/annot/MCElement.javaannot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.javaannot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javaannot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: 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.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
🧬 Code graph analysis (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(29-203)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
MethodSetter(33-216)annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java (1)
NodeValidationUtils(19-42)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(19-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
annot/src/main/java/com/predic8/membrane/annot/MCElement.java (1)
67-74: LGTM! Consider enhancing the Javadoc with constraint details.The new annotation property is well-implemented with a sensible default. The Javadoc example is helpful. Consider adding a note about the constraint that the element must have exactly one
@MCAttributesetter for this to work (as enforced inGenericYamlParser.findSingleAttributeSetter).annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
198-201: LGTM!The utility method follows the established pattern of
isNoEnvelopeand provides null-safe annotation checking.annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
664-711: Good test coverage for the basic inline scalar case.The test validates the core functionality with an integer attribute. Consider adding a companion test to verify the object form (
child: { value: 12 }) still works alongside the inline scalar form, confirming backward compatibility.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
61-61: LGTM - ObjectMapper instance is thread-safe for read operations.Using a shared static
ObjectMapperfor scalar conversions is appropriate sinceconvertValueis thread-safe.
190-199: Well-structured short-circuit for scalar inline handling.The logic correctly:
- Detects non-object nodes
- Checks if
singleAttributeis enabled- Applies the scalar value and handles lifecycle callbacks
- Falls through to
ensureMappingStartfor invalid cases whensingleAttributeis false
365-388: Solid implementation with proper error handling.The method correctly handles:
- Null/null-node values
- Type conversion via Jackson
- Reflective invocation with proper exception unwrapping
Minor note:
setAccessible(true)at line 381 is a precaution, but public setters annotated with@MCAttributeshould already be accessible. It's harmless to keep for robustness.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
162-168: Well-designed schema composition for single-attribute elements.Using
anyOfto allow both the object form and the scalar inline form is the correct JSON Schema approach for supporting multiple valid shapes.
173-204: Thorough compile-time validation forsingleAttributeconstraints.The method properly validates:
- Exactly one non-excluded
@MCAttributeexists- No
@MCTextContentor@MCChildElementis declaredThis ensures misconfigured annotations fail fast during annotation processing rather than at runtime.
299-300: Type generalization toAbstractSchema<?>is appropriate.The change from
SchemaObjecttoAbstractSchema<?>is necessary to accommodate the newAnyOfschema type in these collections. This aligns with theSchemaFactory.anyOf(List<AbstractSchema<?>>)signature.Also applies to: 337-337, 358-358, 430-431
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.java (1)
24-29: Type migration to AbstractSchema<?> is consistent and verified.The field, constructor parameter, and iteration variable are correctly updated to
AbstractSchema<?>across all three locations. This aligns with theSchemaFactory.anyOf()signature change and enables the broader schema type support. Thejson(ObjectNode)method exists inAbstractSchema<?>, confirming the call on line 39 is correct.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
64-68: The type signature is appropriately flexible.The
anyOf()method acceptsList<AbstractSchema<?>>, which allows callers to pass any schema type while remaining type-safe. All callers in JsonSchemaGenerator pass lists containing SchemaObject or subclass instances, which are compatible with the signature sinceSchemaObject extends AbstractSchema.
|
This pull request needs "/ok-to-test" from an authorized committer. |
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)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
270-282: Guard the(SchemaObject) parent2cast to avoid possibleClassCastException.With
processMCChilds(..., AbstractSchema<?> so), when called withSchemaArray(e.g., at line 124 in thenoEnvelope=truebranch),parent2can remain aSchemaArrayif a child element hasisList() == false. The unconditional cast at line 280 will then crash at runtime. Even if today's model guarantees "object here", a guard makes failures actionable.Proposed hardening (guard cast + clearer error)
- addChildsAsProperties(m, main, cei, (SchemaObject) parent2, isComponentsList(i, cei), cei.isList()); + if (!(parent2 instanceof SchemaObject parentObj)) { + throw new ProcessingException( + "Expected SchemaObject while processing child elements, but found: " + parent2.getClass().getSimpleName(), + i.getElement() + ); + } + addChildsAsProperties(m, main, cei, parentObj, isComponentsList(i, cei), cei.isList());
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
17-27: Imports refactor looks fine; consider consistency for class-level annotation lookup.You now rely on Spring’s
findAnnotationelsewhere;isNoEnvelope/isSingleAttributeuseClass#getAnnotation. If composed/meta-annotations are a thing here, consider switching these class-level lookups tofindAnnotation(clazz, MCElement.class)for consistency/coverage.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (2)
147-156: Scalar inlineanyOfvariant is a solid approach; consider carrying element-level description to theanyOf.Right now the element description is on
parser, but tooling that reads only the top-level schema node may lose it when wrapped inanyOf. If supported by your schema model, consider also setting the description on theanyOfitself.
158-189: Inline variant validation is strong; minor cleanup: avoid double-settingtype.
SchemaFactory.from(type)already sets the schema type in the factory; calling.type(type)again is redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: 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.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
🧬 Code graph analysis (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(19-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (2)
148-158:anyMatchrefactor is clearer and avoids unnecessary work.
197-200:isSingleAttribute(Class<?>)helper is straightforward; consider validating “presence implies schema-shape”.This method is fine as a predicate, but downstream code should still enforce the structural constraints (exactly one attribute, no children/text) where it matters (parser/schema).
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
284-305:AbstractSchema<?>list refactor looks good and aligns withSchemaFactory.anyOf(List<AbstractSchema<?>>).
343-355: Signature update toList<AbstractSchema<?>>is consistent; flow parser def creation still looks correct.
415-433:getComponentsgeneric refactor is consistent and keeps component variants object-shaped.
3 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.