Add Yaml bean support#2436
Conversation
WalkthroughReplaces topLevel with component semantics, adds per-child Changes
Sequence Diagram(s)sequenceDiagram
participant YAML as "YAML docs"
participant Parser as "GenericYamlParser"
participant Registry as "BeanRegistryImplementation"
participant Factory as "BeanFactory"
participant Parent as "Parent object"
YAML->>Parser: feed documents (may include top-level "components")
Parser->>Registry: register extracted component BeanDefinitions
Parser->>Parser: encounter object node (may contain "$ref")
alt object-level $ref present
Parser->>Registry: resolveReference($ref)
Registry->>Factory: create(JsonNode componentBody)
Factory->>Factory: load class, choose ctor, resolve ctor args (may call Registry)
Factory-->>Registry: return constructed bean
Registry-->>Parser: resolved object
Parser->>Parent: set child property with resolved object
else inline object
Parser->>Factory: create(inline object node)
Factory-->>Parser: populated object
Parser->>Parent: set child property with populated object
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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). (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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
…ID exclusion in JSON schema generation
When generating JSON schema for top-level @mcelement classes, enforce that any setId method is exactly setId(String), that at most one exists, and that it is annotated with @MCAttribute using the name "id" (or default). Throw ProcessingException on violations. If no @MCAttribute("id") is present, add an implicit optional "id" string property to the top-level schema.
…hema; extract bean id from YAML and fix bean index increment - Comment out early return for cei.excludedFromJsonSchema() so child elements are still added to schema (avoids validation failures) - Add extractIdOrDefault to GenericYamlParser to use an explicit "id" from the bean node when present - Use kind variable and correct idx++ placement when creating BeanDefinition entries
|
/ok-to-test |
Add a boolean rootDef() to MCElement (with javadoc) to mark elements allowed at the config root. Mark APIProxy as a root-def element. Add new com.predic8.membrane.core.kubernetes.Components class annotated as a root-def element to represent top-level components with a child elements list and getter/setter.
…move legacy bean; mark Components noEnvelope; relax noEnvelope/topLevel check - Rename shouldGenerateParserType to shouldGenerateFlowParserType and update all call sites in JsonSchemaGenerator - Filter root definitions using rootDef() when generating JSON schema top-level defs - Adjust parser creation logic for flow parser refs - Remove legacy kubernetes bean field and its getter/setter from Bean.java - Mark Components as noEnvelope = true on MCElement - Comment out strict check in SpringConfigurationXSDGeneratingAnnotationProcessor for noEnvelope/topLevel and add TODO notes
… functionality and comments for clarity.
…ons in Components - Introduce `allOf` field and related functionality in SchemaObject for JSON schema generation. - Update Components class to adjust MCElement annotations by commenting out `noEnvelope` attribute.
…ng for list items in K8sJsonSchemaGenerator
…n-support # Conflicts: # annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java
…ents lists Add createComponentParser and call ensureValidIdSetter for component-only elements. Make components without a real id gain an optional "id" property; alias schemas for no-envelope or elements with a real id. Update addChildsAsProperties to use component-specific $defs names when in a components list context. Add helpers: isComponentsList, componentDefName, hasRealIdAttribute.
current use:
components:
- basicAuthentication:
id: asd
---
api:
port: 2000
flow:
- basicAuthentication:
$ref: asd
------------------------------------------------------
Generate component-or-ref schema definitions and use them for components
- Add createComponentOrRefParser(Model, ElementInfo) to produce a oneOf schema that accepts either the inline component object or a {$ref: string}; handle noEnvelope case by returning a simple ref for now.
- Add componentOrRefDefName helper to name "OrRef" defs.
- Register component-or-ref parser for each element definition.
- Update component name resolution: when an element is a component, use componentDefName in components context, otherwise use componentOrRefDefName so references point to the new oneOf definition.
- Adjust list child processing to reference the OrRef definition for component children.
Add support for referencing a component instance directly at the list-item level (allow "- $ref: ...") when the list can contain component types. Make component schemas simpler: only the components list gets the id-augmented schema name, remove the separate createComponentOrRefParser and related helpers, and simplify createComponentParser behavior. Update addChildsAsProperties signature and adjust schema property requirements accordingly.
…tes value types - Change Components to store components as Map<String,Object> and use @MCOtherAttributes setter. - Extend OtherAttributesInfo to detect Map value type (String or Object) and expose ValueType enum. - In JsonSchemaGenerator, treat @MCOtherAttributes with non-string values as an object map and create a special components parser that uses patternProperties with anyOf variants referencing $defs for component types. Only allow additionalProperties when otherAttributes map values are String. - Add SchemaObject.patternProperties support and include it in generated JSON output.
When a type has a child element whose setter expects a component, add an optional "$ref" string property to the generated object schema so the object can reference a component via JSON Pointer. Avoid adding the property if a "$ref" already exists on the parser. Add hasComponentChild(ElementInfo, MainInfo) to detect component children and SchemaObject.hasProperty(String) helper to check existing properties.
…element annotations - Eliminated outdated and unused TODO comments. - Reintroduced validation for `noEnvelope=true` with conflicting attributes (e.g., `component=true` or `mixed=true`).
…ackage - Removed redundant getters and setters in `Grammar` and `ElementInfo`. - Marked `ais` and `ceis` lists in `ElementInfo` as `final` to ensure immutability. - Cleaned up unused return documentation in `McYamlIntrospector`.
![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 (1)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
181-192: Typo in method name:stripFistLine→stripFirstLine.The method name appears to have a typo.
- static String @NotNull [] stripFistLine(String content) { + static String @NotNull [] stripFirstLine(String content) {
♻️ Duplicate comments (6)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)
21-21: Remove unused import.The
Typesimport is not used anywhere in this file. While line 149 referencesTypesin a JavaDoc link, the import itself is unnecessary.-import javax.lang.model.util.Types;
148-155: Missing primitive types inisBeanReferencecheck.The check excludes
int,long,float,double, andbooleanprimitives but omitschar,short, andbyte. If a setter takes one of these primitive types,isBeanReferencereturnstrue, potentially causing incorrect bean reference resolution attempts.private boolean isBeanReference(Class<?> wanted) { - if (wanted == Integer.TYPE || wanted == Long.TYPE || wanted == Float.TYPE || wanted == Double.TYPE || wanted == Boolean.TYPE || wanted == String.class) + if (wanted.isPrimitive() || wanted == String.class) return false; return !wanted.isEnum(); }Using
wanted.isPrimitive()covers all eight primitive types more concisely.annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (3)
57-57: Remove the outdated TODO comment.The
loadBeanClassmethod is used in production code (line 47), not just for testing. The TODO comment is misleading.
144-146: Optimize constructor collection.While the
LinkedHashSetprevents duplicates,getConstructors()returns all public constructors, andgetDeclaredConstructors()also returns all public ones. This causes redundant iterations. Using onlygetDeclaredConstructors()withsetAccessible(true)(already done at line 165) is sufficient.
240-251: Verify primitive/wrapper compatibility check.The logic at line 244 checks
targetType.isPrimitive() && isWrapperOfPrimitive(targetType, o.getClass()). A previous review flagged this as potentially incorrect. Ensure thatReflectionUtil.isWrapperOfPrimitiveaccepts(primitiveType, wrapperClass)in that order, and that whentargetTypeis a primitive,o.getClass()returns the corresponding wrapper class.Run the following script to verify the signature and usage:
#!/bin/bash # Find the isWrapperOfPrimitive method signature rg -nP 'isWrapperOfPrimitive\s*\(' --type=java -C3annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
368-393: Add guard for empty component variants.If no
ElementInfohascomponent=true(or all are filtered bytopLevel), thevariantslist remains empty at line 392, andanyOf(variants)on line 372 will create ananyOfwith no schemas. This may produce invalid JSON Schema.Apply this diff to add a guard:
private SchemaObject createComponentsMapParser(Model m, MainInfo main, ElementInfo elementInfo, String parserName) { SchemaObject parser = object(parserName) .additionalProperties(false) .description(getDescriptionContent(elementInfo)); + var variants = getComponents(m, main); + if (variants.isEmpty()) { + // No component types declared; allow any object as fallback + parser.patternProperty(COMPONENT_ID_PATTERN, object().additionalProperties(true)); + return parser; + } - parser.patternProperty(COMPONENT_ID_PATTERN, anyOf(getComponents(m, main))); + parser.patternProperty(COMPONENT_ID_PATTERN, anyOf(variants)); return parser; }
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
184-196: Consider using a Java record for the Pair class.The TODO notes merging with a
Pairrecord from core. Since this is a simple immutable data carrier with direct field access, a record would be cleaner:private record Pair<X, Y>(X x, Y y) { public static <X, Y> Pair<X, Y> of(X x, Y y) { return new Pair<>(x, y); } }annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java (1)
171-177: Consider using streams forhasProperty.The linear search is acceptable for typical schema sizes, but could be slightly more idiomatic:
public boolean hasProperty(String name) { - for (AbstractSchema<?> p : properties) { - if (name.equals(p.getName())) - return true; - } - return false; + return properties.stream().anyMatch(p -> name.equals(p.getName())); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(10 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/model/ElementInfo.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(5 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java(5 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java(7 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java(4 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/bean/BeanFactory.javaannot/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/MethodSetter.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java
🧬 Code graph analysis (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(37-298)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(28-185)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/ClassGenerator.java (1)
ClassGenerator(12-72)
annot/src/main/java/com/predic8/membrane/annot/model/ElementInfo.java (2)
annot/src/main/java/com/predic8/membrane/annot/model/OtherAttributesInfo.java (1)
OtherAttributesInfo(24-67)annot/src/main/java/com/predic8/membrane/annot/AnnotUtils.java (1)
AnnotUtils(22-99)
⏰ 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: Automated tests
🔇 Additional comments (38)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (4)
17-27: LGTM on import updates.The imports are well-organized: JetBrains annotations for nullability,
Functionfor the new mapping helpers, and the static import for the sharedCOPYRIGHTconstant fromClassGeneratorpromotes DRY principles.
50-63: LGTM on extracted helper method.Good extraction of
getSourceFilefor better readability. The helper correctly passes originating elements tocreateSourceFilefor proper incremental compilation tracking.
145-175: LGTM on the refactored element mapping generation.The switch from
topLevel()tocomponent()aligns with the PR's goal of introducing component-based semantics. The extractedgenerateElementMappingString()andgenerateLocalElementMapping()helpers improve readability and reduce inline complexity.
177-182: No action required. The methodgetComponentStreamis properly defined as a protected method inAbstractGrammarand is correctly available to theGrammarclass through inheritance. The implementation filters component elements as expected and the code change is valid.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java (3)
31-32: LGTM on new schema composition fields.Using
LinkedHashMapforpatternPropertiesensures deterministic iteration order in the generated JSON Schema output.
39-59: LGTM on the centralized JSON serialization method.The
json(ObjectNode)method provides a clean orchestration point that:
- Calls
super.json(node)first to handle base properties- Sets
additionalProperties: falseonly for objects without explicit additional properties- Delegates to focused helper methods for each schema construct
This design supports JSON Schema's
allOf,oneOf, andpatternPropertieskeywords correctly.
79-101: LGTM on allOf/oneOf serialization helpers.Both methods follow the same pattern: null/empty guard, array construction, and proper delegation to each schema's
json()method. The separation keeps the code maintainable.annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (3)
111-128: LGTM on collection handling with element-type validation.The
getObjectListmethod correctly:
- Checks if the target type is a
Collection- Parses the list using the shared helper
- Validates each element against the inferred generic type
- Provides descriptive error messages on type mismatch
The null-check on
elemTypegracefully handles raw types.
130-146: LGTM on bean reference resolution.The
resolveReferencehelper provides robust error handling:
- Wraps registry lookup failures in
ParsingExceptionwith node context- Validates the resolved bean's type against the expected parameter type
- Produces clear error messages including the reference path, actual type, and expected type
174-185: LGTM on generic type extraction.The
getCollectionElementTypemethod correctly handles the common generic patterns:
- Direct
Class<?>type argumentsWildcardTypewith upper bounds (e.g.,? extends Foo)- Nested
ParameterizedType(e.g.,List<Optional<Foo>>)Returning
nullfor unhandled cases is a safe fallback that skips validation.annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (2)
81-96: LGTM on refactored YAML/XML parsing with context isolation.The
withContextClassLoaderpattern properly isolates the thread context class loader during parsing, which is essential when loading classes from the in-memory compiler output. BothparseYAMLandparseXMLnow follow the same pattern, improving consistency.
98-113: LGTM on the context class loader helper.The
withContextClassLoadermethod:
- Saves the original context class loader
- Sets the new class loader in a try block
- Restores the original in a finally block (ensuring cleanup even on exceptions)
- Uses a
ThrowingSupplierto allow checked exceptions from the actionThis is a well-structured utility pattern.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (3)
34-38: LGTM on the new bean field.The Javadoc clearly explains the field's purpose. The TODO on line 99 appropriately notes the mutability concern for future consideration.
114-136: LGTM on the new predicate methods.These predicates provide a clean public API:
isComponent()correctly identifies component definitions by the#/components/JSON pointer prefixisBean()checks for the "bean" kindisDeleted(),isModified(),isAdded()encapsulate action state checks without exposing the internal enumThis design follows the "Tell, Don't Ask" principle by letting callers query state rather than accessing the enum directly.
114-116: Consider null-safety forisComponent().The method could throw a
NullPointerExceptionifnameis null (though the constructor validates against null). Adding a null check would make it more defensive:public boolean isComponent() { - return name != null && name.startsWith("#/components/"); + return name.startsWith("#/components/"); }Actually, looking again, the current code already has
name != null &&which is correct defensive programming given that the K8S constructor could theoretically receive null frommetadata.get("name").asText(). The current implementation is fine.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (5)
77-90: LGTM: Exception handling refactored to use RuntimeException.The signature no longer declares checked exceptions, and the body wraps all operations in a try-catch that rethrows as
RuntimeException. The new path usingBeanFactoryfor "bean" kind aligns with the broader component model introduced in this PR.
120-122: Condition correctly refactored to avoid NPE.The change from
isNotComponent(bd)to!bd.isComponent()resolves the NPE risk flagged in previous reviews. TheBeanDefinition.isComponent()method (line 96 in BeanDefinition.java snippet) safely handles null names:return name != null && name.startsWith("#/components/").
163-177: LGTM: Prototype scope handling implemented correctly.The refactored
resolveReferencenow distinguishes between prototype and singleton beans. Singleton beans are cached in theBeanDefinition, while prototype beans are instantiated on each resolution. The logic is sound and aligns with standard bean lifecycle semantics.
196-203: LGTM: Scope detection logic is correct.The
isPrototypeScopemethod correctly handles both bean and non-bean types. For beans, it reads thescopefield from the JSON node with a default of "SINGLETON", performing a case-insensitive comparison. For non-beans, it delegates tobd.isPrototype().
185-189: LGTM: Component filtering logic is correct.The filter correctly excludes components (whose names start with
#/components/) from the returned bean list, ensuring only non-component beans are included.annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (1)
42-55: LGTM: Bean creation flow is well-structured.The
createmethod follows a clear three-step process: load the class, instantiate it with constructor args, and apply properties. The exception handling wraps failures with helpful context about the class being instantiated.annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (4)
72-74: LGTM: Property name matching logic is correct.The
matchesPropertyNamemethod extracts the property name from method names like "setFoo" or "getFoo" and performs a case-insensitive comparison. Since callers ensure the method is a setter (starts with "set"), the substring operation is safe.
85-98: LGTM: Single child setter validation is thorough.The refactored method adds validation to ensure:
- No
@MCAttributesetters exist whennoEnvelope=true- Exactly one
@MCChildElementsetter exists- That setter accepts a
CollectiontypeThe error messages are clear and helpful.
100-112: LGTM: Child setter collection logic is clear.The method correctly identifies
@MCChildElementsetters and validates that exactly one exists, throwing clear exceptions for empty or multiple results.
178-183: LGTM: Element name resolution is well-designed.The
getElementNamemethod provides a sensible fallback hierarchy: first checking the@MCElementannotation's name, then falling back to the class's simple name if the annotation is missing or blank.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (4)
45-48: LGTM: Component ID pattern is reasonable.The regex pattern
^[A-Za-z_][A-Za-z0-9_-]*$enforces valid identifier-like names for component IDs, starting with a letter or underscore, followed by alphanumeric characters, underscores, or hyphens.
119-122: LGTM: Components map detection is straightforward.The check correctly identifies a components map by matching the annotation name and verifying the element is an object type.
144-149: LGTM: Object-level component reference support added correctly.The code adds a
$refproperty when the parent element has component children, enabling object-level references to components. The condition ensures$refis not duplicated if already present.
395-404: LGTM: Component child detection logic is correct.The method iterates through child element specifications and checks if any child element declaration contains a component type, returning
trueon the first match.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (5)
77-79: LGTM: Component extraction integration is clean.The code correctly detects top-level "components" sections and delegates to
extractComponentBeanDefinitionsto process them, aligning with the new component model.
174-177: LGTM: Object-level reference handling is well-integrated.The code detects a
$refkey early in object processing and delegates to the specializedapplyObjectLevelRefmethod, maintaining clean separation of concerns.
202-233: LGTM: Component extraction is thorough and well-validated.The method:
- Handles null/empty components gracefully
- Validates the structure is an object
- Ensures each component definition has exactly one key (the component type)
- Wraps components in a standard top-level node structure
- Uses synthetic names with
#/components/prefix for identificationThe validation and error handling are appropriate.
240-260: LGTM: Object-level reference resolution is robust.The method correctly:
- Validates the
$refvalue is a string- Resolves the referenced component
- Detects and prevents conflicts between inline definitions and
$ref- Injects the referenced component via the appropriate child setter
- Provides clear error messages for incompatible types
262-268: LGTM: Reference resolution wrapper provides consistent error handling.The method wraps the registry's
resolveReferencecall and converts anyRuntimeExceptioninto aParsingExceptionwith node context, ensuring consistent error reporting across the parsing flow.annot/src/main/java/com/predic8/membrane/annot/model/ElementInfo.java (4)
78-84: LGTM with note on fallback behavior.The method searches for a
MainInfoby matching output package with config package. If no match is found, it falls back to the first main. Ensure this fallback is appropriate for your use case, as it may return an unexpectedMainInfoif packages don't align.
90-93: LGTM: XSD type name transformation is appropriate.The method replaces dots with underscores to create valid JSON Schema type names, as explained in the comment. This ensures compatibility with JSON Schema parsers.
125-131: LGTM: Type predicates are clear and safe.The
isStringandisObjectmethods safely check the value type by first ensuringoaiis non-null, then comparing against the expectedValueTypeenum values.
134-143: LGTM: Equals and hashCode implementation is correct.The methods use modern pattern matching and maintain a consistent contract: equality is based on the
elementreference, andhashCodeis derived from the element's qualified name.
|
/ok-to-test |
- Simplified property-checking logic using streams in `SchemaObject`. - Corrected method name typo from `stripFistLine` to `stripFirstLine` in `CompilerHelper`. - Replaced inner `Pair` class with `record` for better conciseness. - Improved formatting and consistency in generated content for `Grammar`.
…tility methods - Introduced `ReflectionUtil` for converting string values to specified Java types. - Added `isWrapperOfPrimitive` method for validating primitive-wrapper type relationships. - Included comprehensive JUnit tests for all utility methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (3)
103-113: Consider preserving exception types in the wrapper.Wrapping all exceptions in
RuntimeExceptionloses the original exception type, which may complicate debugging in test scenarios. Consider using a more specific exception wrapper or rethrowing unchecked exceptions as-is.private static <T> T withContextClassLoader(ClassLoader cl, ThrowingSupplier<T> action) { ClassLoader original = Thread.currentThread().getContextClassLoader(); try { Thread.currentThread().setContextClassLoader(cl); return action.get(); - } catch (Exception e) { + } catch (RuntimeException | Error e) { + throw e; + } catch (Exception e) { throw new RuntimeException(e); } finally { Thread.currentThread().setContextClassLoader(original); } }
115-119: Consider extracting common logic withgetCompositeClassLoader.Both
xmlClassLoaderandgetCompositeClassLoader(lines 127-131) follow nearly identical patterns: cast toInMemoryClassLoader, define an overlay with different file extensions, and return aCompositeClassLoader. This duplication could be reduced with a shared helper.private static CompositeClassLoader overlayClassLoader(CompilerResult cr, String content, String resourcePath) { InMemoryClassLoader inMemory = (InMemoryClassLoader) cr.classLoader(); inMemory.defineOverlay(new OverlayInMemoryFile(resourcePath, content)); return new CompositeClassLoader(CompilerHelper.class.getClassLoader(), inMemory); }Then simplify both methods:
private static CompositeClassLoader xmlClassLoader(CompilerResult cr, String xmlSpringConfig) { return overlayClassLoader(cr, xmlSpringConfig, "/demo.xml"); } private static CompositeClassLoader getCompositeClassLoader(CompilerResult cr, String yamlConfig) { return overlayClassLoader(cr, yamlConfig, "/demo.yaml"); }
175-192: Consider clarifying the empty-line-skipping logic.The
while(true)loop with abreakstatement (lines 183-190) correctly skips leading empty lines, but the flow could be more explicit for readability.static String @NotNull [] stripFirstLine(String content) { - String[] parts; - while (true) { - parts = content.split("\n", 2); + String[] parts = content.split("\n", 2); + while (parts.length == 2 && parts[0].isEmpty()) { + content = parts[1]; + parts = content.split("\n", 2); + } + if (parts.length != 2) { - if (parts.length != 2) - throw new RuntimeException("Invalid resource file: %s. The resource is expected to have the format 'resource <path>\n<content>'.".formatted(content)); - if (!parts[0].isEmpty()) - break; - content = parts[1]; + throw new RuntimeException("Invalid resource file: %s. The resource is expected to have the format 'resource <path>\n<content>'.".formatted(content)); } return parts; }annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
184-189: Address the TODO: Merge with core Pair record.The TODO at line 184 notes this
Pairrecord duplicates one in core. Consider consolidating to avoid maintenance overhead.Would you like me to search for the core
Pairrecord and open an issue to track the consolidation?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java(4 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/kubernetes/model/SchemaObject.java
🧬 Code graph analysis (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/ClassGenerator.java (1)
ClassGenerator(12-72)
⏰ 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: Automated tests
🔇 Additional comments (14)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (3)
16-39: LGTM!The import additions appropriately support the new YAML/XML parsing infrastructure and testing utilities.
81-87: LGTM!The refactoring to use
withContextClassLoadereliminates duplication and improves readability while preserving the class loader switching behavior.
98-101: LGTM!The
ThrowingSupplierinterface provides a clean pattern for lambda expressions with checked exceptions.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (4)
59-63: LGTM! Clean extraction of source file creation.The helper method encapsulates file creation logic nicely and passes originating elements for proper incremental compilation tracking.
70-72: Good use of shared COPYRIGHT constant.Consolidating the copyright notice via static import from
ClassGeneratorreduces duplication across generators.
74-143: Well-structured text block template.The template is readable and the
formatted()parameters are correctly ordered. The schema location path transformation logic at lines 131-133 correctly maps the Spring package to the JSON resource path.
145-159: The filtering logic correctly separates global and local elements: global elements havecomponent() == true(line 149), while local elements havecomponent() == false(line 156). Verification of the codebase shows consistent usage of the@MCElementannotation with only 2 global components (RedisConnector, XmlConfig) and numerous properly annotated local elements throughout the codebase. The implementation is correct and requires no changes.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java (7)
31-32: Good choice ofLinkedHashMapforpatternProperties.Using
LinkedHashMappreserves insertion order, ensuring deterministic JSON output which aids in testing and debugging.
74-77: LGTM! Fluent API for pattern properties.Consistent with the existing builder pattern used for other properties.
79-101: Consistent implementation ofaddOneOfandaddAllOf.Both methods follow the same pattern with proper null/empty checks before adding to the node. The logic is correct and symmetric.
103-112: LGTM! Pattern properties serialization.Correctly iterates over the
LinkedHashMapentries and builds thepatternPropertiesJSON object.
114-140: LGTM! Well-documentedaddPropertiesmethod.The JavaDoc clearly explains the purpose. The logic correctly:
- Collects required property names
- Builds the properties object
- Only emits
requiredarray when non-empty
166-173: LGTM! NewallOfsetter andhasPropertyquery.Both methods follow existing conventions. The
hasPropertycheck via stream is clear and concise.
47-59: VerifyadditionalProperties: falsecompatibility withallOf/oneOfcomposition.Setting
additionalProperties: false(line 51) when composing schemas viaallOforoneOfcan cause unintended validation failures depending on the JSON Schema validator—properties from referenced sub-schemas may not be recognized as valid properties. Verify this does not conflict with your validator's handling of schema composition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java (1)
37-40: Use a more specific exception type in assertion.The assertion catches
RuntimeException, which is too broad. The actual exception thrown byInteger.parseInt("abc")isNumberFormatException. Using a more specific exception type improves test precision and makes failures easier to diagnose.Apply this diff to use the specific exception type:
- assertThrows(RuntimeException.class, () -> ReflectionUtil.convert("abc", Integer.class)); + assertThrows(NumberFormatException.class, () -> ReflectionUtil.convert("abc", Integer.class));Consider expanding test coverage.
The test suite covers basic scenarios but could be enhanced with additional cases:
- Null-to-primitive conversion (should throw
IllegalArgumentException)- Character validation with invalid length
- Enum conversion
- Other numeric types (long, double, float, short, byte)
- The
isWrapperOfPrimitivemethod (currently untested)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (1)
annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java (1)
annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java (1)
ReflectionUtil(3-54)
⏰ 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 (6)
annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java (2)
8-34: LGTM!The conversion logic correctly handles String, primitives, wrappers, char (with length validation), and enum types. The method appropriately throws exceptions for null-to-primitive assignments and unsupported conversions.
44-53: LGTM!The method correctly covers all 8 primitive/wrapper type pairs and returns an appropriate boolean result.
annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java (4)
10-15: LGTM!The test correctly validates String conversion behavior.
17-22: LGTM!The test correctly validates Integer conversion behavior.
24-29: LGTM!The test correctly validates Boolean conversion behavior.
31-35: LGTM!The test correctly validates null conversion behavior for non-primitive types.
- Consolidated repetitive class loader overlay logic into a reusable `overlayClassLoader` method. - Simplified `withContextClassLoader` error handling for better clarity. - Resolved minor issues in object creation with `APPLICATION_CONTEXT_CLASSNAME` instance lifecycle management. - Improved imports organization and reduced redundancy in both structure and functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
85-97: Resource leak fixed; ApplicationContext now properly closed.The previous resource leak concern has been addressed—the ApplicationContext is now correctly closed in the finally block (line 93). The try-finally structure ensures proper cleanup even if an exception occurs.
Optional: Consider API consistency.
parseYAMLreturns aBeanRegistrywhileparseXMLreturnsvoid. IfparseXMLis intended purely for validation (as the comment suggests), the current design is acceptable. Otherwise, consider returning theApplicationContextor documenting the asymmetry.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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 (3)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (3)
99-111: LGTM! Clean context class loader management.The implementation correctly:
- Saves and restores the thread context class loader in a finally block
- Preserves RuntimeException and Error types by rethrowing them directly
- Wraps checked exceptions in RuntimeException for easier lambda usage
This is a well-established pattern for temporary class loader context switching.
119-131: LGTM! Good refactoring of class loader creation.Extracting
overlayClassLoaderas a common helper eliminates duplication and provides a clear abstraction for creating class loaders with in-memory resource overlays. The specific methods (getCompositeClassLoader,xmlClassLoader) are appropriately focused.
251-254: LGTM! Standard pattern for checked exceptions in lambdas.
ThrowingSupplieris a well-established pattern that enables clean lambda syntax when working with APIs that throw checked exceptions. The interface integrates nicely withwithContextClassLoader.
…annotation in `BeanDefinition`, and clarify `BeanFactory` class loading logic
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (2)
121-127: Add null check to prevent NullPointerException.Unlike
parseConstructorArgList(line 114), this method doesn't check for null before callingisArray(), which will throw a NullPointerException ifarris null.🔎 Apply this diff to add the null check:
private List<Property> parsePropertyList(JsonNode arr) { - if (!arr.isArray()) return List.of(); + if (arr == null || !arr.isArray()) return List.of(); return StreamSupport.stream(arr.spliterator(), false) .map(Property::new) .toList(); }
144-146: Remove duplicate constructor collection.
getConstructors()returns all public constructors, whilegetDeclaredConstructors()returns all constructors (including public ones). Public constructors will appear twice in theLinkedHashSet, though the Set deduplicates them. SincesetAccessible(true)is used at line 165, onlygetDeclaredConstructors()is needed.🔎 Apply this diff to use only getDeclaredConstructors:
- Set<Constructor<?>> constructors = new LinkedHashSet<>(); - constructors.addAll(Arrays.asList(type.getConstructors())); - constructors.addAll(Arrays.asList(type.getDeclaredConstructors())); + Constructor<?>[] constructors = type.getDeclaredConstructors();Also update line 151 to iterate over the array:
- for (Constructor<?> c : constructors) { + for (Constructor<?> c : constructors) {
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
17-18: Consider using specific imports instead of wildcards.Wildcard imports can obscure which classes are actually used and may lead to naming conflicts or make refactoring harder.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java(4 hunks)annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/bean/BeanFactory.java
🧬 Code graph analysis (1)
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (1)
annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java (1)
ReflectionUtil(3-54)
⏰ 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 (8)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (5)
22-23: LGTM!The static import of WatchAction constants improves readability.
35-38: LGTM!Good practice to document the purpose and lifecycle of the bean field.
68-68: LGTM!Consistent use of the static import for WatchAction constants.
114-136: LGTM!The new predicate methods provide clear, convenient ways to query the bean definition state:
isComponent()safely checks for component references with null protectionisBean()identifies bean-kind definitionsisDeleted(),isModified(),isAdded()provide clean access to action statesThese methods improve code readability and encapsulate the logic for state checks.
111-111: No migration concerns needed — the old annotation key does not exist in the codebase.The search confirms that "membrane-soa.org/scope" has no references anywhere in the repository, including YAML/configuration files. The code at line 111 appears to be using the new "membrane-api.io/scope" annotation key as a clean, intentional implementation rather than replacing legacy code. This suggests the namespace change (from "membrane-soa.org" to "membrane-api.io") represents a deliberate rebranding of the Membrane project and has been fully transitioned without remnants of the old key.
annot/src/main/java/com/predic8/membrane/annot/bean/BeanFactory.java (2)
220-226: Property name validation has been added.Good defensive programming - the method now validates the property name is non-null and non-empty before attempting to access
charAt(0), addressing the previous review concern.
240-251: Verify primitive/wrapper compatibility logic is correct.The logic at line 244 checks
isWrapperOfPrimitive(targetType, o.getClass())whentargetType.isPrimitive(). Let me trace through a scenario:
- If
targetTypeisint.classandois anIntegerinstance:
targetType.isInstance(o)returns false (primitives aren't instance-checkable)targetType.isPrimitive()is trueisWrapperOfPrimitive(int.class, Integer.class)should return true- The condition
!(true && true)evaluates to false, so no exception is thrownThis appears correct, but the logic is somewhat convoluted. A previous review comment suggested this was incorrect, but based on my analysis and the signature of
isWrapperOfPrimitive(Class<?> primitive, Class<?> wrapper)from ReflectionUtil, the current implementation seems accurate.Can you confirm this logic handles all primitive/wrapper combinations correctly? Consider adding a unit test to verify scenarios like:
- int.class target with Integer reference
- Integer.class target with Integer reference
- int.class target with String reference (should throw)
annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (1)
188-188: No action required. The use of@MCElement(name="holder", topLevel=true)at line 188 is correct and valid.The
topLevelattribute and thecomponentattribute serve different purposes in the@MCElementannotation and are not a replacement for each other.topLevelcontrols whether an element can be defined at the top-level of config, whilecomponentcontrols whether it can be a separate bean in XML schema or a separate document in YAML/JSON. Both attributes are independently supported and coexist throughout the codebase.
… import `Scope` in `BeanClassGenerator`. - Introduced `Scope` enum with `SINGLETON` and `PROTOTYPE` values. - Replaced `IllegalArgumentException` with `ProcessingException` for better exception handling in `OtherAttributesInfo`. - Added `Scope` import to `BeanClassGenerator`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/Scope.java (1)
3-6: Consider adding JavaDoc to clarify the enum's purpose.The enum lacks documentation explaining its role in the bean lifecycle and the meaning of each scope constant. Adding JavaDoc would improve maintainability.
🔎 View suggested improvement:
+/** + * Defines the lifecycle scope for beans in the configuration. + */ public enum Scope { + /** Single shared instance per context */ SINGLETON, + /** New instance created for each request */ PROTOTYPE }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/generator/BeanClassGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/Scope.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/model/OtherAttributesInfo.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- annot/src/main/java/com/predic8/membrane/annot/generator/BeanClassGenerator.java
- annot/src/main/java/com/predic8/membrane/annot/model/OtherAttributesInfo.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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)
3 participants
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.