Rename singleAttribute to collapsed and make it work for both Attributes and TextContent#2585
Conversation
📝 WalkthroughWalkthroughRenames MCElement.singleAttribute() → MCElement.collapsed() and updates YAML parsing, JSON schema generation, introspection, and tests to implement collapsed inline-element handling and related validations and parser paths. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Parser as GenericYamlParser
participant Introspector as McYamlIntrospector
participant SchemaGen as JsonSchemaGenerator
participant Model as Model/ElementInfo
User->>Parser: provide YAML node for element
Parser->>Introspector: isCollapsed(clazz)?
Introspector-->>Parser: true/false
alt collapsed == true
Parser->>Model: find single setter (MCAttribute/MCTextContent)
Parser->>SchemaGen: request collapsed schema (parserName)
else
Parser->>Model: resolve object properties/children
Parser->>SchemaGen: request standard element schema
end
SchemaGen->>Model: inspect attributes/text/children, validate noEnvelope/collapsed rules
SchemaGen-->>Parser: return AbstractSchema (scalar or object)
Parser-->>User: constructed object or scalar value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
🔇 Additional comments (1)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
164-175: Update error messages to referencecollapsedinstead ofsingleAttribute.The error messages on lines 166 and 172 still reference the old property name
singleAttribute=true, but the annotation property has been renamed tocollapsed.📝 Proposed fix for error messages
if (attrs.size() != 1) { throw new ProcessingException( - "@MCElement(singleAttribute=true) requires exactly one @MCAttribute.", + "@MCElement(collapsed=true) requires exactly one @MCAttribute.", ei.getElement() ); } if (ei.getTci() != null || !ei.getChildElementSpecs().isEmpty()) { throw new ProcessingException( - "@MCElement(singleAttribute=true) must not declare text content or child elements.", + "@MCElement(collapsed=true) must not declare text content or child elements.", ei.getElement() ); }annot/src/main/java/com/predic8/membrane/annot/MCElement.java (1)
68-74: Update Javadoc to align with the new property name and PR objectives.The Javadoc still describes this as "Whether the element has only one attribute" but the property is now named
collapsed. Consider updating the documentation to:
- Explain what "collapsed" means (inline/flattened representation)
- Clarify the constraints (currently only supports single attribute case)
- If the PR title's mention of "make it work for both Attributes and ChildElements" reflects future intent, document that or update the title to match current scope
📚 Suggested Javadoc improvement
/** - * Whether the element has only one attribute. - * Enables inline yaml object configuration + * Whether the element should use a collapsed (inline) representation. + * Currently requires exactly one @MCAttribute and no child elements. + * Enables inline YAML/JSON object configuration where the value is + * provided directly instead of as a nested object. + * * e.g. - * allow: foo + * collapsed: allow: foo + * standard: allow: { value: foo } */ boolean collapsed() default false;
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
197-200: LGTM! Logic correctly updated to use the renamed property.The method now checks
el.collapsed()instead ofel.singleAttribute().Optional: Consider renaming
isSingleAttribute()toisCollapsed()orhasCollapsedForm()for consistency with the new annotation property name, though this is not critical for this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/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
⏰ 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 (3)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (2)
683-683: LGTM! Annotation property correctly updated.The rename from
singleAttribute=truetocollapsed=trueis applied correctly and the test validates the inline YAML configuration behavior.
737-737: LGTM! Annotation property correctly updated.The rename from
singleAttribute=truetocollapsed=trueis applied correctly for the list scenario test.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
148-148: LGTM! Condition correctly updated to use the renamed property.The check now uses
collapsed()instead ofsingleAttribute(), maintaining the same behavior.
|
This pull request needs "/ok-to-test" from an authorized committer. |
christiangoerdes
changed the title
singleAttribute to collapsed and make it work for both Attributes and ChildElementssingleAttribute to collapsed and make it work for both Attributes and TextContent
There was a problem hiding this comment.
Actionable comments posted: 2
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/yaml/GenericYamlParser.java (1)
369-400: Preserve YAML location info in collapsed errors (avoid throwing withnullNode()) and include class context.When multiple setters exist,
findSingleSetterOrNullForAnnotation(...)currently throws withJsonNodeFactory.instance.nullNode(), which drops the helpful line/column mapping.Proposed adjustment (thread the current node through)
- private static <T> void applyCollapsedScalar(Class<T> clazz, JsonNode node, T target) { + private static <T> void applyCollapsedScalar(Class<T> clazz, JsonNode node, T target) { if (node == null || node.isNull()) { - throw new ParsingException("Collapsed element must not be null.", node); + throw new ParsingException("Collapsed element must not be null. (" + clazz.getName() + ")", node); } - Method attributeSetter = findSingleSetterOrNullForAnnotation(clazz, MCAttribute.class); - Method textSetter = findSingleSetterOrNullForAnnotation(clazz, MCTextContent.class); + Method attributeSetter = findSingleSetterOrNullForAnnotation(clazz, MCAttribute.class, node); + Method textSetter = findSingleSetterOrNullForAnnotation(clazz, MCTextContent.class, node); if ((attributeSetter == null) == (textSetter == null)) { // both null or both non-null -> invalid - throw new ParsingException("@MCElement(collapsed=true) requires exactly one @MCAttribute setter OR exactly one @MCTextContent setter.", node); + throw new ParsingException( + "@MCElement(collapsed=true) on %s requires exactly one @MCAttribute setter OR exactly one @MCTextContent setter." + .formatted(clazz.getName()), + node + ); } Method setter = (attributeSetter != null) ? attributeSetter : textSetter; Class<?> paramType = setter.getParameterTypes()[0]; Object value; try { value = SCALAR_MAPPER.convertValue(node, paramType); } catch (IllegalArgumentException e) { throw new ParsingException("Cannot convert inline value to " + paramType.getSimpleName() + ".", node); } @@ - private static Method findSingleSetterOrNullForAnnotation(Class<?> clazz, Class<? extends java.lang.annotation.Annotation> annotation) { + private static Method findSingleSetterOrNullForAnnotation( + Class<?> clazz, + Class<? extends java.lang.annotation.Annotation> annotation, + JsonNode node + ) { List<Method> setters = new ArrayList<>(); doWithMethods(clazz, m -> { if (m.isAnnotationPresent(annotation) && m.getParameterCount() == 1) { setters.add(m); } }); if (setters.isEmpty()) return null; if (setters.size() != 1) { throw new ParsingException( - "Multiple @%s setters found for collapsed element.".formatted(annotation.getSimpleName()), - JsonNodeFactory.instance.nullNode() + "Multiple @%s setters found for collapsed element on %s." + .formatted(annotation.getSimpleName(), clazz.getName()), + node ); } return setters.getFirst(); }Also applies to: 402-418
🤖 Fix all issues with AI agents
In
@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:
- Around line 158-199: The createCollapsedInlineParser method currently filters
attributes via ei.getAis().stream().filter(...).toList() before validating
collapsed shapes; this causes a mismatch with runtime parsing which considers
all annotated setters. Remove the filter and use the full attribute list from
ei.getAis() when computing attrs and when selecting the single-attribute
collapsed schema (i.e., gather the unfiltered list, keep the subsequent checks
for hasText/hasChildren, the single-attribute branch using AttributeInfo ai =
attrs.getFirst(), and the final error), so schema generation aligns with
GenericYamlParser.findSingleSetterOrNullForAnnotation behavior.
In @annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java:
- Around line 195-203: The code currently treats any non-object node (including
arrays) as valid for collapsed inline parsing; change the check in
GenericYamlParser (around isCollapsed(clazz) handling) to require a scalar/value
node by using node.isValueNode() (or equivalent scalar check) before calling
applyCollapsedScalar(clazz, node, configObj), and throw the ParsingException for
non-scalar nodes (arrays/objects) to enforce "inline value" semantics; update
the exception message to state "expected a scalar/inline value" and reference
the isCollapsed and applyCollapsedScalar locations when making this change.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
350-365: Consider using Spring'sUSER_DECLARED_METHODSfilter to exclude synthetic and bridge methods.Both
doWithMethodscalls (lines 350 and 404) process all methods without filtering. Spring'sReflectionUtils.USER_DECLARED_METHODSfilter automatically excludes synthetic/bridge methods, Object class methods, and reduces the risk of unintended behavior if tooling generates annotated bridge methods.Example:
doWithMethods(bean.getClass(), method -> { ... }, ReflectionUtils.USER_DECLARED_METHODS);This is optional but recommended for robustness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.javaannot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-07T15:43:16.568Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2559
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:76-93
Timestamp: 2026-01-07T15:43:16.568Z
Learning: In BeanRegistryImplementation (annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java), only one FallbackBeanDefiner should be registered per type, so concurrent registration of multiple fallbacks for the same type is not expected to occur.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 Learning: 2026-01-07T12:43:52.805Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java:50-53
Timestamp: 2026-01-07T12:43:52.805Z
Learning: SpringContextAdapter in annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java is only used during tests, not in production code, so stub implementations throwing UnsupportedOperationException are acceptable.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 Learning: 2026-01-03T19:24:51.595Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:51.595Z
Learning: In BeanRegistryImplementation, the `getOrCreate(BeanRegistry, Grammar)` method on BeanContainer is guaranteed to always return a non-null object or throw an exception, so null filtering after calling getOrCreate is not necessary.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
28-30: Imports updated consistently for collapsed + reflection traversal.Also applies to: 43-43, 57-57
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
113-143: Collapsed inline schema routing looks good; the earlynoEnvelopeguard is helpful.annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
197-200: Method rename fromisSingleAttributetoisCollapsedis complete; verify if deprecated alias is needed for external consumers.The method has been successfully renamed—no remaining call sites of
isSingleAttributeexist in the repository, andisCollapsedis now the active implementation. However, if theannotmodule is consumed externally, add a deprecated alias to maintain backward compatibility:Proposed compatibility bridge (if needed for external consumers)
public final class McYamlIntrospector { + /** + * @deprecated use {@link #isCollapsed(Class)}. Kept for compatibility during the rename. + */ + @Deprecated(forRemoval = true) + public static boolean isSingleAttribute(Class<?> clazz) { + return isCollapsed(clazz); + } + public static boolean isCollapsed(Class<?> clazz) { MCElement el = clazz.getAnnotation(MCElement.class); return el != null && el.collapsed(); } }
…ter organization.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java:
- Around line 166-198: The test collapsedRejectsObjectShape_schemaValidation
uses an unsafe cast of rootCause(ex) to YamlSchemaValidationException; replace
that cast with assertInstanceOf(YamlSchemaValidationException.class,
rootCause(ex)) to both assert the type and obtain the typed instance (e.g., var
cause = assertInstanceOf(YamlSchemaValidationException.class, rootCause(ex)));
then call cause.getErrors().isEmpty() for the assertion instead of the cast.
Ensure you import or statically use assertInstanceOf and retain the existing
parseYAML/assertThrows usage.
🧹 Nitpick comments (1)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java (1)
200-210: Consider simplifying the loop structure.The
while(true)loop works correctly but could be more readable. Consider restructuring to make the exit conditions more explicit.♻️ Optional simplification
private static Throwable rootCause(Throwable e) { - Throwable cur = e; - while (true) { - if (cur instanceof java.lang.reflect.InvocationTargetException ite) { - cur = ite.getTargetException(); - continue; - } - if (cur.getCause() == null) return cur; - cur = cur.getCause(); - } + Throwable cur = e; + while (cur != null) { + if (cur instanceof java.lang.reflect.InvocationTargetException ite) { + cur = ite.getTargetException(); + } else if (cur.getCause() != null) { + cur = cur.getCause(); + } else { + return cur; + } + } + return e; // fallback if cur becomes null unexpectedly }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.javaannot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java
💤 Files with no reviewable changes (1)
- annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java
⏰ 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 (4)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java (4)
1-12: LGTM!The test class setup and imports are appropriate. The use of JUnit 5 and internal testing utilities is well-organized.
14-48: LGTM!The test correctly validates that a collapsed element with a single attribute accepts inline scalar notation (
child: 12).
50-88: LGTM!The test properly validates collapsed elements within a list, demonstrating that
noEnvelope=truecombined withcollapsed=truecorrectly parses list notation.
90-164: LGTM!These tests correctly validate the core PR objective: demonstrating that
collapsed=trueworks for both@MCAttributeand@MCTextContentannotations. The single and list scenarios are well-covered.
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java:
- Around line 166-210: The test collapsedRejectsObjectShape_schemaValidation
currently casts rootCause(ex) directly to YamlSchemaValidationException which
can throw ClassCastException if a different exception (e.g., ParsingException)
is the root cause; change the test to first capture Throwable rc =
rootCause(ex), assert its type (e.g., assertTrue(rc instanceof
YamlSchemaValidationException) or assertThat(rc).isInstanceOf(...)), then safely
cast to YamlSchemaValidationException to inspect getErrors(); use the existing
rootCause(...) helper and refer to YamlSchemaValidationException and
ParsingException in the assertions.
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
404-420: Preserve error location when multiple setters exist (don’t throw withnullNode).
ThrowingParsingException(..., JsonNodeFactory.instance.nullNode())will drop line/column context, making this harder to debug.Proposed change
- private static Method findSingleSetterOrNullForAnnotation(Class<?> clazz, Class<? extends java.lang.annotation.Annotation> annotation) { + private static Method findSingleSetterOrNullForAnnotation( + Class<?> clazz, + Class<? extends java.lang.annotation.Annotation> annotation, + JsonNode nodeForError + ) { List<Method> setters = new ArrayList<>(); doWithMethods(clazz, m -> { if (m.isAnnotationPresent(annotation) && m.getParameterCount() == 1) { setters.add(m); } }); if (setters.isEmpty()) return null; if (setters.size() != 1) { throw new ParsingException( "Multiple @%s setters found for collapsed element.".formatted(annotation.getSimpleName()), - JsonNodeFactory.instance.nullNode() + nodeForError ); } return setters.getFirst(); }And update the call sites:
- Method attributeSetter = findSingleSetterOrNullForAnnotation(clazz, MCAttribute.class); - Method textSetter = findSingleSetterOrNullForAnnotation(clazz, MCTextContent.class); + Method attributeSetter = findSingleSetterOrNullForAnnotation(clazz, MCAttribute.class, node); + Method textSetter = findSingleSetterOrNullForAnnotation(clazz, MCTextContent.class, node);annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
158-199: Collapsed-inline schema generator is well constrained and matches runtime expectations.
Good enforcement of “no children” and “exactly one of attribute/text”, and nice reuse of enum emission logic.Optional cleanup (redundant type setting)
- return SchemaFactory.from("string") - .name(parserName) - .type("string") + return SchemaFactory.from("string") + .name(parserName) .description(getDescriptionContent(ei));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.javaannot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java
🧰 Additional context used
🧠 Learnings (4)
📚 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
📚 Learning: 2026-01-07T15:43:16.568Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2559
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:76-93
Timestamp: 2026-01-07T15:43:16.568Z
Learning: In BeanRegistryImplementation (annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java), only one FallbackBeanDefiner should be registered per type, so concurrent registration of multiple fallbacks for the same type is not expected to occur.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 Learning: 2026-01-07T12:43:52.805Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java:50-53
Timestamp: 2026-01-07T12:43:52.805Z
Learning: SpringContextAdapter in annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java is only used during tests, not in production code, so stub implementations throwing UnsupportedOperationException are acceptable.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 Learning: 2026-01-03T19:24:51.595Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:51.595Z
Learning: In BeanRegistryImplementation, the `getOrCreate(BeanRegistry, Grammar)` method on BeanContainer is guaranteed to always return a non-null object or throw an exception, so null filtering after calling getOrCreate is not necessary.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
🧬 Code graph analysis (2)
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)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.java (1)
MCElement(52-229)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java (1)
MCElement(51-386)
⏰ 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 (8)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingCollapsedTest.java (4)
14-49: Collapsed inline attribute scenario looks solid.
Good end-to-end coverage (schema generation + parsing + structure assertion) for@MCElement(collapsed=true)+ single@MCAttribute.
50-89: Nice coverage fornoEnvelope=truelist items with collapsed inline attribute.
This is a great regression test for the “array at element-level, scalar at item-level” shape.
90-125: Collapsed inline text-content scenario looks good.
Covers the new “collapsed applies to@MCTextContent” requirement directly.
126-165: GoodnoEnvelope=truelist coverage for collapsed text content.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
185-206: Collapsed scalar-inline parsing path is clear and consistent with the new semantics.
Null + non-scalar rejections read well and should produce better errors than the old “single attribute” flow.
348-369: Lifecycle reflection change todoWithMethodslooks fine.
371-402: Good: collapsed now supports@MCAttributeor@MCTextContent(exactly one).
The “both null or both present” guard is a nice, explicit constraint.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
113-156: Collapsed routing +noEnvelopeincompatibility enforcement looks correct.
This should prevent invalid shapes from ever reaching runtime parsing.
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/MCElement.java (1)
68-80: Update documentation to reflect both Attributes and TextContent support.The documentation states "Whether the element has only one attribute" but the PR objectives indicate that
collapsedshould work for both Attributes and TextContent. The Javadoc should be updated to accurately reflect this expanded functionality.Consider updating line 69 to something like:
* Whether the element has only one attribute or only text content.Additionally, consider adding an example demonstrating the TextContent case to complement the existing attribute example.
📜 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/MCElement.javadocs/ROADMAP.md
✅ Files skipped from review due to trivial changes (1)
- docs/ROADMAP.md
⏰ 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 (1)
annot/src/main/java/com/predic8/membrane/annot/MCElement.java (1)
81-81: All references tosingleAttributehave been successfully updated tocollapsed.The breaking API change is complete and consistently applied throughout the codebase. No stale references to the old attribute name remain.
2 participants
Summary by CodeRabbit
Refactor
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.