Move Yaml Parsing to annot and add yaml parsing test infra#2332
Conversation
…ce API Extract YamlLoader, GenericYamlParser, BeanRegistry and related exceptions into com.predic8.membrane.annot.yaml. Change generated K8sHelperGeneratorAutoGenerated to implement K8sHelperGenerator (instance methods) and update callers (Envelope, BeanDefinition, KubernetesWatcher, tests) to use new instance-based API. Add YAML testing utilities (YamlParser, YAMLParsingTest, StructureAssertionUtil, CompilerHelper.parseYAML) and necessary dependencies (jackson-dataformat-yaml, org.jetbrains annotations). Update imports and tests accordingly.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughAdds YAML parsing support and JetBrains annotations to the annot module, relocates several YAML-related classes into com.predic8.membrane.annot.yaml, introduces an event-driven YamlLoader and parser flow, converts generated K8s helper data from public static fields to an interface-backed instance API, and adds tests and test utilities for YAML parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as TestRunner
participant Parser as GenericYamlParser
participant Loader as YamlLoader
participant Gen as K8sHelperGenerator
participant Registry as BeanRegistry
Test->>Parser: parseMembraneObject(events, generator, registry)
Parser->>Loader: iterate events / readObj
Loader-->>Parser: scalar or structured value
Parser->>Gen: getElement(kind)
Gen-->>Parser: concrete Java class
Parser->>Parser: parse(events, concreteClass, registry)
Parser->>Loader: readMap / readSequence / readString
Loader-->>Parser: parsed primitives/structures
Parser-->>Test: constructed membrane object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/WrongEnumConstantException.java (1)
24-31: Critical: Missing null-check for getEnumConstants() causes NPE.
getEnumConstants()returnsnullifclazzis not an enum class. Streaming over it on line 27 without a null-check will throw aNullPointerException.Apply this diff to add validation:
public WrongEnumConstantException(Class<?> clazz, String value) { Object[] enumConstants = clazz.getEnumConstants(); + if (enumConstants == null) { + throw new IllegalArgumentException("Class " + clazz.getName() + " is not an enum"); + } this.constants = Arrays.stream(enumConstants) .map(e -> ((Enum<?>) e).name().toLowerCase()) .toList(); this.value = value; }
🧹 Nitpick comments (13)
annot/src/main/java/com/predic8/membrane/annot/yaml/PublicMarkedYAMLException.java (1)
19-25: Consider updating the Javadoc reference (optional).The Javadoc mentions "Kubernetes integration," which is accurate in terms of usage but could be updated to reflect that this is now part of the general YAML annotation utilities package.
Consider this minor update:
/** * Public wrapper for SnakeYAML's MarkedYAMLException that exposes the protected constructor - * for use by YAML parsing components in the Kubernetes integration. + * for use by YAML parsing components. * <p> * This exception provides detailed error context including source marks for both * the context and problem locations in YAML files. */annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (3)
24-24: Inconsistent annotation lookup usage.The code mixes
AnnotationUtils.findAnnotation(lines 24, 39, 90) with the statically importedfindAnnotation(lines 34, 43, 57, 62, 106, 108, 112). This inconsistency reduces readability.Apply this diff to use the static import consistently:
- return AnnotationUtils.findAnnotation(method, MCChildElement.class) != null; + return findAnnotation(method, MCChildElement.class) != null;
39-39: Inconsistent annotation lookup usage.Use the statically imported
findAnnotationfor consistency.Apply this diff:
- return AnnotationUtils.findAnnotation(method, MCTextContent.class) != null && method.getName().substring(3).equalsIgnoreCase(key); + return findAnnotation(method, MCTextContent.class) != null && method.getName().substring(3).equalsIgnoreCase(key);
90-90: Inconsistent annotation lookup usage.Use the statically imported
findAnnotationfor consistency.Apply this diff:
- .filter(method -> AnnotationUtils.findAnnotation(method, MCOtherAttributes.class) != null) + .filter(method -> findAnnotation(method, MCOtherAttributes.class) != null)core/src/main/java/com/predic8/membrane/core/kubernetes/BeanDefinition.java (1)
40-40: Consider caching the K8sHelperGeneratorAutoGenerated instance.Creating a new
K8sHelperGeneratorAutoGeneratedinstance for eachBeanDefinitionconstruction may be inefficient if this constructor is called frequently.Consider using a static cached instance:
+ private static final K8sHelperGenerator K8S_HELPER = new K8sHelperGeneratorAutoGenerated(); + public BeanDefinition(WatchAction action, Map<String,Object> m) { this.action = action; this.m = m; Map<String,Object> metadata = (Map<String,Object>) m.get("metadata"); var kind = (String) m.get("kind"); if (kind == null) kind = "api"; - isRule = Proxy.class.isAssignableFrom(new K8sHelperGeneratorAutoGenerated().getElement(kind)); + isRule = Proxy.class.isAssignableFrom(K8S_HELPER.getElement(kind)); name = (String) metadata.get("name");annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
70-85: Reflection + TCCL handling looks soundClassloader swapping and reflective invocation are wrapped correctly with a finally-block to restore the original TCCL; the in-memory overlay for
/demo.yamlis also wired coherently. The behavior of this helper now largely depends onYamlParser.getResult(), which is currently not returning the parsed object (see comment onYamlParser).core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
60-60: Instance-based CRD lookup aligns with newK8sHelperGeneratorAPISwitching from a static field to
new K8sHelperGeneratorAutoGenerated().getCrdSingularNames()matches the new instance‑based helper contract and keepsKubernetesWatcherlogic unchanged.If you ever find this called frequently, you could cache the helper in a static or injected field instead of creating a new instance, but for a typical startup path this is fine.
core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java (1)
16-20:Envelopenow cleanly delegates spec parsing to shared annot YAML infraSwitching to:
BeanRegistry/GenericYamlParserfromcom.predic8.membrane.annot.yaml,- a static
K8sHelperGeneratorAutoGeneratedinstance (K8S_HELPER), and- resolving
clazzviaK8S_HELPER.getElement(kind)inreadSpecmakes
Envelopereuse the same mapping logic as the generic YAML parser while preserving existing control flow and error handling. The rawClassusage in theGenericYamlParser.parse(...)call is acceptable here given the dynamically resolved types.Also applies to: 27-29, 37-37, 77-84
annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
5-21: Consider relaxingAssertervisibility and guarding against null inputsUsing a private nested
Assertertype in the public API ofassertStructure/clazzis a bit unusual and can confuse tooling and future extensions, even though the current usage viaclazz(...)works.You might want to:
- Make
Asserterpublicor package‑private instead ofprivate, and- Optionally add a null check (e.g.
assertNotNull(o1)) beforeo1.getClass()to produce a clearer assertion failure if parsing ever returns null.The current implementation is fine for internal tests but could be made more robust with these tweaks.
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
30-58: NewparseMembraneObjectentrypoint andYamlLoaderusage look consistent
parseMembraneObject(...)correctly walks the event stream until the first mapping and then delegates based on the first scalar key, which matches the intendeddemo: {}style input.readMembraneObject(...)reusesK8sHelperGeneratorfor kind→class resolution and feeds straight into the existingparse(...)logic with clear failure when no class is found.- Switching object creation to
clazz.getConstructor().newInstance()and standardizing scalar reads viaYamlLoader.readString(...)(including enums, primitives, map other‑attributes, and reference attributes) improves robustness and centralizes error reporting.You might consider throwing explicitly when no mapping is encountered instead of returning
nullfromparseMembraneObject(...), but that’s a design choice rather than a correctness issue.Also applies to: 67-67, 132-171
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java (1)
8-65:YamlLoaderprovides a clear, reusable event-level parsing utilityThe helper cleanly encapsulates common SnakeYAML event patterns:
readStringfor scalar values with location-aware errors,readObjto branch into scalars/maps/sequences,readSequenceandreadMapwith proper termination onSequenceEndEvent/MappingEndEventand detailed error messages.This is a good foundation for both
EnvelopeandGenericYamlParser; adding type parameters to theList/Mapreturn types would be a minor polish if you want stronger typing, but is not required.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (2)
85-103: Generated helper now cleanly implements the shared interface and hides internalsThe generated class header and imports look good: importing
com.predic8.membrane.annot.K8sHelperGeneratorand declaringimplements K8sHelperGeneratoraligns the auto-generated helper with the new interface-based access pattern. MakingelementMapping,localElementMappingandcrdSingularNamesprivate staticinstead of public improves encapsulation while still allowing access through the new instance methods below.Only minor nit: having both this annotation processor and the runtime interface named
K8sHelperGeneratorcan be slightly confusing when reading the code; if this ever gets revisited, renaming the processor to something likeK8sHelperClassGeneratorwould clarify roles, but it’s not functionally required.
115-130: Instance methods correctly wrap the static registries and preserve behaviorThe new
getLocal,getElement, andgetCrdSingularNamesimplementations correctly delegate to the existing static structures and are consistent with how those maps/lists are populated:
getLocal(context, key)returnsnullwhen no context map exists, mirroring prior behavior while honoring both original and lower-cased keys inserted vialocalElementMappingPut.getElement(key)simply forwards toelementMapping.get(key), again preserving the prior key behavior where both original and lower-case variants are present.getCrdSingularNames()exposes the same backing list that was previously public and static, so mutability and semantics remain unchanged.If you ever want to enforce immutability at the API boundary, consider returning
Collections.unmodifiableList(crdSingularNames)fromgetCrdSingularNames, but that would be a behavioral change and isn’t necessary for this refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
annot/pom.xml(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java(1 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(4 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/PublicMarkedYAMLException.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/WrongEnumConstantException.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java(3 hunks)core/src/main/java/com/predic8/membrane/core/config/spring/k8s/YamlLoader.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/BeanCache.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/BeanDefinition.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(1 hunks)core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
annot/pom.xml
🧬 Code graph analysis (6)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(28-254)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (3)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(35-220)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
SpringConfigurationXSDGeneratingAnnotationProcessorTest(23-66)annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
StructureAssertionUtil(5-23)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(28-254)
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java (1)
core/src/main/java/com/predic8/membrane/core/config/spring/k8s/YamlLoader.java (1)
YamlLoader(24-32)
core/src/main/java/com/predic8/membrane/core/config/spring/k8s/Envelope.java (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(28-254)annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java (1)
YamlLoader(8-66)core/src/main/java/com/predic8/membrane/core/config/spring/k8s/YamlLoader.java (1)
YamlLoader(24-32)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(12-115)annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java (1)
YamlLoader(8-66)
⏰ 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/main/java/com/predic8/membrane/annot/yaml/PublicMarkedYAMLException.java (1)
15-15: Package refactoring is correct and complete—no breaking changes.Verification confirms that
PublicMarkedYAMLExceptionis only used within the same package (com.predic8.membrane.annot.yaml) byGenericYamlParser.java, which can access it without an explicit import. No other files depend on this class, so the package move introduces no breaking changes.annot/src/main/java/com/predic8/membrane/annot/yaml/WrongEnumConstantException.java (1)
15-15: Package relocation verified as complete.The class has been successfully relocated to
com.predic8.membrane.annot.yaml. All references in the codebase (found inGenericYamlParser.java) are in the same package and properly updated. No orphaned references to the old package location remain.However, note a potential null pointer issue in the constructor:
clazz.getEnumConstants()can return null if the argument is not an enum type, which would cause an NPE on the subsequentArrays.stream()call. Consider adding a null check or validation before dereferencing the array.core/src/main/java/com/predic8/membrane/core/kubernetes/BeanCache.java (1)
18-18: LGTM! Import updated for relocated BeanRegistry.The import correctly reflects the BeanRegistry relocation to the annot.yaml package.
core/src/test/java/com/predic8/membrane/core/config/spring/k8s/EnvelopeTest.java (1)
25-25: LGTM! Import updated for relocated BeanRegistry.The import correctly reflects the BeanRegistry relocation to the annot.yaml package.
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
49-51: LGTM! Test updated to use auto-generated K8sHelper.The change from an anonymous K8sHelperGenerator implementation to a direct instantiation of
K8sHelperGeneratorAutoGeneratedaligns with the architectural shift from static field-based to instance method-based access patterns.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (1)
14-18: LGTM! BeanRegistry successfully relocated to annot.yaml package.The interface relocation is part of the broader architectural refactoring to consolidate YAML parsing functionality in the annot module. All dependent files have been updated with the new import path.
core/src/main/java/com/predic8/membrane/core/config/spring/k8s/YamlLoader.java (1)
16-31: LGTM! YamlLoader correctly refactored and helper methods properly migrated.Verification confirms all four helper methods (readString, readObj, readSequence, readMap) have been successfully migrated to the annot module's YamlLoader at
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java. The core module's YamlLoader now correctly delegates parsing to Envelope, aligning with the architectural consolidation of YAML parsing logic in the annot module.annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
11-31: Test logic is correct; will pass onceYamlParserreturns the parsed objectThe test compiles a minimal
DemoElement, verifies annotation processing, then parses:demo: {}and asserts that the returned object’s runtime class is
DemoElement. This is exactly what the new YAML parsing path should guarantee.Right now the assertion cannot succeed because
parseYAML(...)ultimately returns the constanttruefromYamlParser.getResult(). OnceYamlParseris fixed to return the parsed instance, this test should provide good coverage for the new flow.
….java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
21-23: Optional: provide a clearer error when the YAML resource is missing
requireNonNull(getClass().getResourceAsStream(resourceName))will throw a bareNullPointerExceptionif the resource is not on the classpath, which can be harder to diagnose. You could pass a custom message torequireNonNull(...)to make test failures more self‑explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
K8sHelperGenerator(30-193)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(28-254)
⏰ 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 (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
12-33: Parsed result is now stored and exposed correctly
YamlParsernow wiresGenericYamlParserwith the generatedK8sHelperGeneratorAutoGenerated, uses UTF‑8 with try‑with‑resources, and persists the parsed object in a finalresultfield thatgetResult()returns. This resolves the earlier issue where the parsed value was discarded.
….java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/ok-to-test |
# Conflicts: # annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
2 participants
Summary by CodeRabbit
New Features
Tests
Chores