Yaml bean registry #2339
Yaml bean registry #2339
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/BeanCache.java (1)
116-147: DELETED events can’t be processed correctly (bds entry removed before activationRun uses it)In
handle(BeanDefinition bd)you remove DELETED definitions frombds, butactivationRun()later looks them up viabds.get(uid)and then unconditionally callsdefine(bd). For DELETED actions this makesbdnull, causing an NPE and/or silently dropping delete events.To keep full BeanDefinition context (action, map, scope, etc.) available to
activationRun()and still clean up after processing, you can keep the BD inbdsuntil after the activation run:- void handle(BeanDefinition bd) { - if (bd.getAction() == WatchAction.DELETED) - bds.remove(bd.getUid()); - else - bds.put(bd.getUid(), bd); - - if (router.isActivatable(bd)) - uidsToActivate.add(bd.getUid()); - if (changeEvents.isEmpty()) - activationRun(); - } + void handle(BeanDefinition bd) { + // Keep the latest BeanDefinition for all actions so activationRun + // can see both metadata and the action (including DELETED). + bds.put(bd.getUid(), bd); + + if (router.isActivatable(bd)) + uidsToActivate.add(bd.getUid()); + if (changeEvents.isEmpty()) + activationRun(); + } @@ - BeanDefinition bd = bds.get(uid); + BeanDefinition bd = bds.get(uid); + if (bd == null) { + // Definition was removed concurrently or never stored; skip safely. + uidsToRemove.add(uid); + continue; + } try { Object o = define(bd); bd.setBean(o); @@ if (bd.getAction() == WatchAction.DELETED) uuidMap.remove(bd.getUid()); + if (bd.getAction() == WatchAction.DELETED) + bds.remove(bd.getUid()); uidsToRemove.add(bd.getUid());This avoids the NPE and ensures delete events actually reach
router.handleBeanEvent(...). If you don’t need a freshly constructed bean for deletes, you could further optimize by skippingdefine(bd)whenbd.getAction() == WatchAction.DELETEDand using onlyoldBeanfromuuidMap.
🧹 Nitpick comments (15)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
157-166: Close configuration InputStream when parsing YAML to avoid leaks
router.getResolverMap().resolve(location)returns anInputStreamthat is passed intoparseMembraneResourcesbut never closed. SinceparseMembraneResourcesreads from the stream without closing it, this can leak file descriptors for file‑based configs.You can eliminate this by wrapping the resolution in try‑with‑resources:
private static Router initRouterByYAML(String location) throws Exception { var router = new HttpRouter(); router.setBaseLocation(location); router.setHotDeploy(false); router.setAsynchronousInitialization(true); router.start(); try (InputStream in = router.getResolverMap().resolve(location)) { parseMembraneResources(in, new K8sHelperGeneratorAutoGenerated(), router); } return router; }annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
86-102: Clarify whether non‑topLevelelements should be allowed as top‑level kindsIn
addTopLevelProperties, only elements withe.getAnnotation().topLevel()contribute a rootschema.property(...), but all elements (top‑level or not) are added tokindsand thus become valid root shapes viaschema.oneOf(kinds):main.getElements().values().forEach(e -> { if (e.getAnnotation().topLevel()) schema.property(...); kinds.add(object() .additionalProperties(false) .property(ref(e.getAnnotation().name()) .ref("#/$defs/" + e.getXSDTypeName(m)) .required(true))); }); schema.oneOf(new ArrayList<>(kinds));If
main.getElements()can contain elements that are not meant to be top‑level in user configs, they are now still accepted as whole‑document roots. If that’s not desired, consider restrictingkindstotopLevel()elements as well.annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
11-16: Be explicit about bean ordering in tests; consider improving assertions
assertStructure(BeanRegistry registry, Asserter... asserter)indexes intoregistry.getBeans():assertEquals(registry.getBeans().size(), asserter.length); for (int i = 0; i < asserter.length; i++) { asserter[i].assertStructure(registry.getBeans().get(i)); }Given that the default
BeanRegistryimplementation backed by aConcurrentHashMapdoes not guarantee iteration order, these assertions implicitly rely on whatever ordergetBeans()happens to return. That can make tests brittle or flaky if the underlying map or concurrency behavior changes.Two potential improvements:
- Either document and enforce a deterministic ordering in
BeanRegistry.getBeans()(e.g., backing it with an order‑preserving collection or sorting before returning), or adaptassertStructureto work in an order‑insensitive way (e.g., by matching on class or properties rather than index).- For clearer failures, in
clazz(...)you could replaceassertTrue(bean.getClass().getSimpleName().equals(clazzName));withassertEquals(clazzName, bean.getClass().getSimpleName());, which yields more informative diff output when assertions fail.Also applies to: 26-45, 47-60
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (2)
61-72: SchemaRegistry wiring looks correct; consider updating naming and docs for clarityThe migration to
SpecificationVersion+SchemaRegistry+Schemaand loading viaSchemaLocation.of(jsonSchema)is consistent and thread-safe for reuse. The remaining references to “JsonSchemaFactory” (field namejsonSchemaFactoryand the Javadoc above it) are now misleading, though, as the type is aSchemaRegistry. Renaming the field and updating the comment would make the code easier to follow for future maintainers.Also applies to: 100-106
146-155: YAML multi-document loop works but can be simplified and made saferThe manual
while (!parser.isClosed())loop withreadTree(parser)plusparser.nextToken()works but is a bit fragile and harder to reason about at EOF or in the presence of trailing whitespace/comments.You could simplify and tighten this by looping on
readTreedirectly, which avoids extranextToken()calls and makes the stop condition explicit:- private @NotNull List<Error> handleMultipleYAMLDocuments(Exchange exc, Flow flow) throws IOException { - List<Error> assertions; - assertions = new ArrayList<>(); - YAMLParser parser = factory.createParser(exc.getMessage(flow).getBodyAsStreamDecoded()); - while (!parser.isClosed()) { - assertions.addAll(schema.validate(objectMapper.readTree(parser))); - parser.nextToken(); - } - return assertions; - } + private @NotNull List<Error> handleMultipleYAMLDocuments(Exchange exc, Flow flow) throws IOException { + List<Error> assertions = new ArrayList<>(); + YAMLParser parser = factory.createParser(exc.getMessage(flow).getBodyAsStreamDecoded()); + JsonNode node; + while ((node = objectMapper.readTree(parser)) != null) { + assertions.addAll(schema.validate(node)); + } + return assertions; + }This keeps behavior the same for well-formed multi-doc YAML while reducing the chance of odd interactions at the end of the stream.
annot/src/main/java/com/predic8/membrane/annot/K8sHelperGenerator.java (1)
19-28: Document the expected format/semantics ofgetSchemaLocation()Adding
getSchemaLocation()to the interface is reasonable, but the contract doesn’t specify whether callers should expect aclasspath:..., file path, or full URI. A short Javadoc on the method (and/or on the primary implementation) clarifying the expected scheme and whether the path is absolute or relative would help avoid misuse.annot/pom.xml (1)
70-74: Dependency addition is appropriate; consider centralizing the versionBringing in
com.networknt:json-schema-validator:2.0.0here matches the version in core and keeps modules aligned. Both modules already declare version 2.0.0 explicitly, confirming consistency. To follow the pattern already established in the parent POM (which centralizes jackson.version, spring.version, etc.), you may want to add a${json-schema-validator.version}property in the parent's<properties>section and reference it from both core and annot pom files. This prevents future drift and simplifies version updates.core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
16-17: BeanCache wiring with Router and generated helper is consistentUsing
new BeanCache(router, new K8sHelperGeneratorAutoGenerated())aligns with the new BeanCache constructor and the relocatedWatchActiontype; lifecycle is correctly tied intostart()/stop().If you want to avoid repeated instantiation, consider holding a single
K8sHelperGeneratorAutoGeneratedfield and reusing it both forbeanCacheand forgetCrdSingularNames().Also applies to: 47-50
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)
104-108: Expanded root delegation set is reasonable for shared APIsDelegating
org.xml.sax.*,org.w3c.dom*, andcom.predic8.membrane.annot.yaml.BeanRegistryto the root class loader should help avoid duplicate definitions of common or shared types when using this in‑memory loader.If you prefer to avoid a hard‑coded FQN string for
BeanRegistry, you could optionally useBeanRegistry.class.getName()instead, at the cost of an extra direct dependency on that type in this test utility.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
132-136: Generated getSchemaLocation implementation matches schema path conventionThe new
getSchemaLocation()override builds a classpath URL from theoutputPackage, swapping"spring"for"json"and converting dots to slashes, which should resolve to<outputPackage with spring→json>/membrane.schema.jsonas intended.Optionally, you could use
String#replace/replace('.', '/')instead ofreplaceAll(...)to avoid regex semantics here, but that’s purely a readability micro‑optimization in the generator.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)
5-10: Clarify boolean semantics inhandleAsynchronousInitializationResultThe parameter name
emptysuggests “registry is empty”, whileRouter.handleAsynchronousInitializationResult(boolean)currently treats the argument as a success flag. To avoid confusion between “success” and “empty”, consider aligning the name (e.g.success) and/or adding brief Javadoc describing whattruemeans here.core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
331-344: MinimalBeanRegistrystub looks appropriate for this test
TestRegistry.getBeans()andgetBeansOfType(...)returningList.of()cleanly satisfy the expandedBeanRegistrycontract without affecting these tests, which only rely on reference resolution viaresolveReference. If future tests need bean enumeration here, you can extend this stub to exposerefsaccordingly.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java (1)
157-181: Reference resolution ignores namespace/kind and may be ambiguous
resolveReference(String url)currently picks the firstBeanDefinitionwhosenameequalsurl, regardless of namespace or kind:Optional<BeanDefinition> obd = bds.values().stream() .filter(bd -> bd.getName().equals(url)) .findFirst();If you ever support beans with the same name in different namespaces (or of different kinds), this will resolve arbitrarily to one of them. If cross-namespace collisions are possible in your YAML / CRD model, consider including namespace and/or kind in the lookup key (or enforcing uniqueness at configuration time).
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
102-133: Optional: assert that the YAML root key matches the providedkindIn
readMembraneObject(...)you consume the first scalar (root key) but ignore its value and instead rely solely on thekindparameter:event = events.next(); ensureMappingStart(event); event = events.next(); if (!(event instanceof ScalarEvent se)) throw ... Class clazz = generator.getElement(kind); ... Object result = GenericYamlParser.parse(kind, clazz, events, registry, generator);If
bd.getKind()ever drifts from the YAML structure (e.g. misconstructedBeanDefinition), this will quietly parse the wrong structure. As a defensive check you could assert thatse.getValue().equals(kind)(or at least log a warning) to fail fast on mismatches.Not required for correctness today, but would make debugging configuration issues easier.
92-99: Reuse SchemaRegistry and Schema instances to avoid repeated parsing/compilation overheadAccording to json-schema-validator 2.0.x documentation,
SchemaRegistryandSchemainstances are designed to be thread-safe and should be cached and reused across multiple validations rather than recreated for each document. The current implementation recreates both for every call tovalidate():var jsonSchemaFactory = SchemaRegistry.withDefaultDialect(DRAFT_2020_12, builder -> {}); var schema = jsonSchemaFactory.getSchema(SchemaLocation.of(generator.getSchemaLocation())); schema.initializeValidators();Consider moving the
SchemaRegistrycreation outside this method (e.g., as a static or per-generator field) and reusing it. This will eliminate repeated parsing and validator compilation, improving performance when validating multiple documents.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
annot/pom.xml(1 hunks)annot/src/main/java/com/predic8/membrane/annot/K8sHelperGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java(7 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.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(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/YamlUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java(2 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(3 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.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)annot/src/test/java/com/predic8/membrane/annot/yaml/YamlUtilTest.java(1 hunks)annot/src/test/resources/log4j2.xml(1 hunks)core/pom.xml(1 hunks)core/src/main/java/com/predic8/membrane/core/Router.java(3 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(3 hunks)core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(6 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java(2 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(2 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/client/KubernetesClient.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/client/Watcher.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorYAMLTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java
🧰 Additional context used
🧠 Learnings (3)
📚 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.xmlcore/pom.xml
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst Repo: membrane/api-gateway PR: 1906 File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0 Timestamp: 2025-06-18T12:03:09.931Z Learning: The mediaType method in the Request class throws jakarta.mail.internet.ParseException, not java.text.ParseException, making the jakarta.mail.internet.ParseException import necessary and correct in JsonSchemaTestSuiteTests.java. Applied to files:
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.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:
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.javaannot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java
🧬 Code graph analysis (8)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java (1)
BeanCache(34-192)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(48-329)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java (1)
BeanCache(34-192)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (2)
annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
StructureAssertionUtil(10-62)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(36-221)
core/src/main/java/com/predic8/membrane/core/Router.java (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
BeanDefinition(18-93)core/src/main/java/com/predic8/membrane/core/RuleManager.java (1)
RuleManager(35-344)core/src/main/java/com/predic8/membrane/core/exceptions/SpringConfigurationErrorHandler.java (1)
SpringConfigurationErrorHandler(30-197)
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(48-329)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java (3)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
K8sHelperGenerator(30-198)annot/src/main/java/com/predic8/membrane/annot/yaml/YamlUtil.java (1)
YamlUtil(17-46)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
BeanDefinition(18-93)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
core/src/main/java/com/predic8/membrane/core/transport/http2/frame/Error.java (1)
Error(17-53)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
K8sHelperGenerator(30-198)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-128)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/transport/http2/frame/Error.java (1)
Error(17-53)
⏰ 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 (21)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java (1)
33-37: Classpath-based schema location is appropriate withClasspathSchemaResolver.Using
"classpath:/validation/json-schema/simple-schema.json"matches the resolver type and makes the test independent of the working directory or filesystem layout. This looks correct and consistent with a classpath-based loading model.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java (2)
29-32: LGTM!The implementation correctly delegates to the existing
resolver.resolve()infrastructure, maintaining the same behavior while adapting to the new interface.
21-21: Based on my web search verification, the review comment contains an incorrect assumption about the library API. In networknt json-schema-validator 2.0.0, theSchemaLoaderinterface is the documented API for loading schema content, notResourceLoader. The review comment's claim about changing fromSchemaLoadertoResourceLoaderdoes not align with the library's actual 2.0.0 API.To finalize the verification, I need to confirm whether
ResourceLoaderis a custom interface defined in the codebase or if this indicates a code issue:The review comment requires manual verification because:
- The claimed
ResourceLoaderinterface is not part of networknt json-schema-validator 2.0.0 public API—onlySchemaLoaderis documented as the schema loading mechanism- Without access to the actual code file, I cannot determine if
ResourceLoaderis a custom interface in the Membrane codebase or if this represents a bug in the implementationPlease verify:
- Is
ResourceLoadera custom interface defined in your codebase?- If so, does it correctly adapt the networknt library's
SchemaLoaderAPI?- If not, the class should likely implement
SchemaLoaderinstead, aligning with the library's 2.0.0 APIcore/pom.xml (1)
173-176: JSON Schema validator version bump – rely on test suite to catch API regressionsThe version increase here is straightforward; the real risk is subtle behavior changes in the validator. As long as all JSON‑schema–related tests (core + annot) pass in CI, this change looks fine to me.
Please ensure the full JSON schema validation test suite is run (e.g.
mvn -pl core,annot test) to confirm there are no regressions.annot/src/test/resources/log4j2.xml (1)
1-16: Test Log4j2 configuration looks saneConsole appender + INFO log level for core and root is a reasonable default for tests; configuration is minimal and clear.
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
37-47: Alias-to-version test coverage is appropriateThe assertions cover all supported aliases (including 2019‑09 and 2020‑12) plus null/invalid cases, matching the current parser behavior. This gives good confidence in the mapping logic.
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
25-38: JSON Schema version parsing logic is clear and consistentThe switch-based alias mapping is straightforward and matches the test expectations (including null/unknown handling). Returning the enum directly keeps callers aligned with the underlying validator API.
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
156-165: Dynamic output package for schema resource is a nice generalizationUsing
main.getAnnotation().outputPackage().replaceAll("spring", "json")to derive the target package formembrane.schema.jsonkeeps the generator aligned with annotation metadata and avoids hard‑coded package names. No issues here.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (2)
109-120: Validation outcome logic is soundThe consolidated
validateMessageflow (single entry point,List<Error>for both JSON and YAML, counters updated once per exchange, and early return on success) is straightforward and matches the tests’ expectations. No issues from a correctness perspective.
157-189: Problem-details mapping and pointer generation look consistent with the new error modelMapping
Errorinstances into problem-details maps (message, message key, optional details, keyword, escaped pointer, and instance node) preserves rich diagnostics and usesNodePathcorrectly with RFC 6901 escaping. This should give callers more structured and precise locations than the previous message-based approach.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorYAMLTest.java (1)
37-41: Good coverage for YAML and multi-document behaviorSwitching to
ClasspathSchemaResolverwith aclasspath:/...schema location matches the new registry-based loading, and the added tests around invalid values, additional properties, and mixed-validity multi-doc streams exercise the critical YAML paths inJSONYAMLSchemaValidator. This is a solid regression net for the refactor.Also applies to: 77-98
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlUtil.java (1)
15-44: Package relocation is consistent; utility behavior remains correctMoving
YamlUtilintocom.predic8.membrane.annot.yamlaligns it with the rest of the annotation/YAML tooling, and theremoveFirstYamlDocStartMarkerimplementation still matches its documented contract without regressions.core/src/main/java/com/predic8/membrane/core/kubernetes/client/KubernetesClient.java (1)
16-18: WatchAction import relocation looks correctImport now points to
com.predic8.membrane.annot.yaml.WatchAction, matching the enum’s new location without changing behavior.annot/src/test/java/com/predic8/membrane/annot/yaml/YamlUtilTest.java (1)
15-70: Test package relocation matches YAML utility moduleMoving
YamlUtilTestintocom.predic8.membrane.annot.yamlis consistent with the YamlUtil location; the existing tests still cover the key behaviors ofremoveFirstYamlDocStartMarker.annot/src/main/java/com/predic8/membrane/annot/yaml/WatchAction.java (1)
14-18: Enum relocation to annot.yaml is consistent with YAML-centric APIsMoving
WatchActiontocom.predic8.membrane.annot.yamlkeeps the enum values intact while aligning it with the YAML/bean infrastructure that now consumes it.core/src/main/java/com/predic8/membrane/core/kubernetes/client/Watcher.java (1)
16-24: Typing onEvent with WatchAction clarifies watcher APIUsing
WatchActionas the first parameter ofonEvent(and importing it fromannot.yaml) makes the watcher contract explicit and consistent with the rest of the YAML/K8s wiring, without changing overall semantics.annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
8-128: Good coverage for new YAML→bean behaviorsThe new tests (
attribute,singleChild,twoObjects) nicely exercise attribute mapping, nested child elements, and multiple top-level instances usingassertStructure/clazz/property. They align with the newBeanRegistry-basedparseYAMLhelper and should give good regression protection around the bean-centric YAML parsing.annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
16-16:parseYAML’s BeanRegistry-centric return type is consistent with the new designSwitching
parseYAMLto returnBeanRegistryand delegating toYamlParser.getBeanRegistry()integrates cleanly with the new tests (e.g.,YAMLParsingTest) that assert onregistry.getBeans(). The reflective call is already wrapped in the existing error handling and classloader setup, so this change is cohesive and low-risk as long asYamlParserexposesgetBeanRegistry()as expected.If you haven’t already, please double-check that the generated
com.predic8.membrane.annot.util.YamlParserindeed has agetBeanRegistry()method returning the concreteBeanRegistryimplementation used at runtime.Also applies to: 71-81
core/src/main/java/com/predic8/membrane/core/Router.java (1)
18-24: Fix NPE inisActivatableand tightenhandleBeanEventcontract complianceVerification confirms the concerns are partially justified:
isActivatableNPE is real. TheK8sHelperGenerator.getElement()method can return null—this is evidenced by defensive null checks throughoutGenericYamlParser(lines 118, 172–173, 319 all guard against null). Router's call at line 707 does not check beforeisAssignableFrom(), so a null return causes an NPE.
handleBeanEventparameter contract issue is real. The method receives anactionparameter but ignores it, instead usingbd.getAction()at lines 690, 692, 694. WhileBeanCachecurrently always passes a non-nullbean(it callsdefine(bd)first), this coupling is fragile. Additionally, forDELETEDevents,newProxyis still initialized (lines 683–694) even though onlyoldBeanis used.Suggested fixes:
Add null check in
isActivatable:public boolean isActivatable(BeanDefinition bd) { - return Proxy.class.isAssignableFrom(new K8sHelperGeneratorAutoGenerated().getElement(bd.getKind())); + Class<?> elementClass = new K8sHelperGeneratorAutoGenerated().getElement(bd.getKind()); + return elementClass != null && Proxy.class.isAssignableFrom(elementClass); }Respect the
actionparameter inhandleBeanEventand skip proxy initialization forDELETED:public void handleBeanEvent(BeanDefinition bd, Object bean, Object oldBean, WatchAction action) throws IOException { - Proxy newProxy = (Proxy) bean; - try { - if (newProxy.getName() == null) - newProxy.setName(bd.getName()); - newProxy.init(this); - } catch (ConfigurationException e) { ... - } - - if (bd.getAction() == WatchAction.ADDED) + if (action == WatchAction.DELETED) { + getRuleManager().removeRule((Proxy) oldBean); + } else { + Proxy newProxy = (Proxy) bean; + try { + if (newProxy.getName() == null) + newProxy.setName(bd.getName()); + newProxy.init(this); + } catch (ConfigurationException e) { ... + } catch (Exception e) { + throw new RuntimeException("Could not init rule.", e); + } + + if (action == WatchAction.ADDED)Likely an incorrect or invalid review comment.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java (1)
183-191: getBeans / getBeansOfType look good for post-activation accessThe new
getBeans()andgetBeansOfType(...)implementations are straightforward and rely onBeanDefinition.beanbeing populated duringactivationRun(). Given the activation callback contract (e.g.,handleAsynchronousInitializationResult), this is a clean way to expose the active bean list.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
25-82: kind/bean additions align well with the new bean-centric flowThe introduction of
kindandbeanplus the additional constructor cleanly support the newBeanCache/GenericYamlParserflow (static config vs. watch-based) without breaking the existing metadata-based constructor. The default"api"kind preserves previous behavior whenkindis missing.Looks good to me.
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCache.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java Outdated Show resolved Hide resolved
annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java Outdated Show resolved Hide resolved
| This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
31-56: Guard YAML resource loading and close the stream (and consider BeanCache lifecycle)
getClass().getResourceAsStream(resourceName)can returnnull, and the resultingInputStreamis never closed. Since you already importrequireNonNull, you can both validate and manage the resource more explicitly, for example:- CountDownLatch cdl = new CountDownLatch(1); - beanRegistry = GenericYamlParser.parseMembraneResources(getClass().getResourceAsStream(resourceName), generator, + CountDownLatch cdl = new CountDownLatch(1); + try (var in = requireNonNull( + getClass().getResourceAsStream(resourceName), + "YAML resource '%s' not found".formatted(resourceName) + )) { + beanRegistry = GenericYamlParser.parseMembraneResources(in, generator, new BeanCacheObserver() { @Override public void handleAsynchronousInitializationResult(boolean empty) { cdl.countDown(); } @@ public boolean isActivatable(BeanDefinition bd) { return true; } }); - cdl.await(); + } + cdl.await();This keeps failures local and avoids leaking the underlying stream. If feasible in your test setup, you might also want a small hook to stop the underlying
BeanCacheonce tests are done to avoid accumulating non-daemon worker threads.
🧹 Nitpick comments (1)
annot/src/test/java/com/predic8/membrane/annot/YamlSetterConflictTest.java (1)
13-40: Finalize TODOs or clarify expectations in list-based conflict testsThe first three tests (
sameConcreteChildOnTwoSetters,sameChildNameFromDifferentAbstractHierarchies,sameChildNameViaBaseAndConcreteSetter) currently only assertcompilationSuccess()and keep theassertCompilerResult(...); // TODOcommented out. As written, they don't verify any specific conflict diagnostics and may not reflect the intended semantics (especially given the commentederror("...")expectations). Once the behavior is settled, either wire these throughassertCompilerResult(...)with the exact messages or, if success is the expected outcome for list-based setters, drop/update the TODOs and rename or document the tests accordingly.Also applies to: 42-88, 90-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.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(2 hunks)annot/src/test/java/com/predic8/membrane/annot/YamlSetterConflictTest.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)
✅ Files skipped from review due to trivial changes (1)
- annot/src/main/java/com/predic8/membrane/annot/yaml/YamlLoader.java
🚧 Files skipped from review as they are similar to previous changes (1)
- annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java
🧰 Additional context used
🧬 Code graph analysis (3)
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-198)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(48-329)
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (3)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(36-221)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(24-76)
annot/src/test/java/com/predic8/membrane/annot/YamlSetterConflictTest.java (2)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(36-221)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
SpringConfigurationXSDGeneratingAnnotationProcessorTest(23-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 (3)
annot/src/test/java/com/predic8/membrane/annot/YamlSetterConflictTest.java (1)
124-160: Diagnostic-based no-list conflict tests look consistentThe
_noListvariants andchildNameNotUniqueAcrossPackagesuseassertCompilerResult(false, of(error(...)), result)to pin exact diagnostics for the various name clash scenarios. This aligns withCompilerHelper.error(...)'s exact-message matching and gives good regression coverage of the annotation processor’s error reporting.Also applies to: 162-303
annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
46-141: New YAML parsing tests provide good coverage of core scenarios
attribute,singleChild, andtwoObjectsnicely exercise attribute mapping, single child element wiring, and multi-document YAML handling via the BeanRegistry +StructureAssertionUtilDSL. The setups are minimal and readable, and the assertions match the expected object graph shape.annot/src/test/java/com/predic8/membrane/annot/util/StructureAssertionUtil.java (1)
25-74: Assertion DSL over BeanRegistry is clear and fit for purposeThe
Asserter/Propertyhelpers create a small, readable DSL overBeanRegistryfor structural assertions, and the reflection-basedpropertyimplementation is adequate for a test-only utility. No issues spotted with the control flow or assertion logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
133-145: Consider making schema path derivation more explicit/robustThe
outputPackage().replaceAll("spring", "json").replaceAll("\\.", "/")heuristic works for the current naming scheme but might behave unexpectedly if"spring"appears in other parts of the package name (e.g., as part of another word) or if the naming convention changes.If you expect more variation in package names later, you could consider a slightly more explicit mapping, e.g.:
- mainInfo.getAnnotation().outputPackage() - .replaceAll("spring", "json") - .replaceAll("\\.", "/") + mainInfo.getAnnotation().outputPackage() + // Replace only a dedicated `.spring.` segment if present + .replace(".spring.", ".json.") + .replace('.', '/')This keeps behavior the same for the common case (
...spring...) but avoids transforming arbitrary"spring"substrings and usesreplaceinstead of regex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java(1 hunks)
⏰ 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/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java (1)
85-145: Text block structure and placeholders are consistentThe generated class template (imports, fields, methods, and static block) is coherent, and the
.formatted(...)call correctly matches the four%splaceholders. The newgetSchemaLocation()override slots in cleanly with the existing API and keeps the generator logic readable.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
74-74: Typo in Apache License URL.The URL path should be
licenses(plural), notlicense.- " http://www.apache.org/license/LICENSE-2.0", + " http://www.apache.org/licenses/LICENSE-2.0",
♻️ Duplicate comments (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
166-172: The regex anchor fix has been applied correctly.The
replaceAll("\\.spring$", ".json")pattern now properly anchors the replacement to only match a trailing ".spring" segment, preventing unintended substitutions.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
35-53: Still no lifecycle hook to stop BeanCache/worker threads started byparseMembraneResourcesThis helper starts the YAML/bean pipeline via
GenericYamlParser.parseMembraneResources(...)and waits for async initialization via theBeanCacheObserver, but it still has no way to shut down any underlying worker threads (e.g., those started byBeanCache.start()if they are non-daemon). In longer-running or repeatedly invoked tests, that can leave stray threads around and, in the worst case, keep the JVM alive after tests complete.If feasible, consider:
- Extending the API so tests can explicitly stop the associated
BeanCache(e.g., by returning a handle or makingYamlParserAutoCloseableand delegating to astop()method), and- Using it from callers in a try-with-resources pattern.
This mirrors the earlier review note about BeanCache shutdown, which still appears unaddressed here.
🧹 Nitpick comments (11)
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)
31-38: Consider enriching the error message with lookup details.The error message could be more helpful by indicating which lookups were attempted (local vs. global scope).
Example:
public Class<?> resolveClass(String key) { Class<?> clazz = grammar.getLocal(context, key); if (clazz == null) clazz = grammar.getElement(key); if (clazz == null) - throw new RuntimeException("Did not find java class for key '%s'.".formatted(key)); + throw new RuntimeException("Did not find java class for key '%s' in context '%s' or global scope.".formatted(key, context)); return clazz; }annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (4)
46-49: Add comment explaining the nullification logic.The setter nullification for non-List MCChildElements is subtle and would benefit from an inline comment explaining why this case requires the bean to be resolved as an element rather than via its setter.
Example:
if (setter != null && setter.getAnnotation(MCChildElement.class) != null) { + // Non-List child elements represent single bean instances, so we need to resolve + // the element name to a bean class rather than using the setter directly if (!List.class.isAssignableFrom(setter.getParameterTypes()[0])) setter = null; }
58-60: Narrow the exception catch to avoid masking unexpected errors.Catching
Exceptionis too broad and may hide unexpected failures. Consider catching only the specific exceptions that grammar lookups andgetChildSettercan throw (e.g.,RuntimeException), or let them propagate naturally since the outer exception wrapping provides sufficient context.- } catch (Exception e) { + } catch (RuntimeException e) { throw new RuntimeException("Can't find method or bean for key '%s' in %s".formatted(key, clazz.getName()), e); }
73-83: Remove redundant fully qualified class names.The delegation methods use fully qualified class names (
com.predic8.membrane.annot.yaml.McYamlIntrospector) even though the methods are already statically imported at lines 24-25. Simply reference the methods directly.public boolean isStructured() { - return com.predic8.membrane.annot.yaml.McYamlIntrospector.isStructured(setter); + return isStructured(setter); } public boolean isReferenceAttribute() { - return com.predic8.membrane.annot.yaml.McYamlIntrospector.isReferenceAttribute(setter); + return isReferenceAttribute(setter); } public boolean hasOtherAttributes() { - return com.predic8.membrane.annot.yaml.McYamlIntrospector.hasOtherAttributes(setter); + return hasOtherAttributes(setter); }
85-87: RenamesetSetterto clarify its purpose.The method name
setSetteris misleading—it doesn't set thesetterfield but instead invokes a setter method reflectively. Consider renaming toinvokeSetterorapplySetterto avoid confusion with the instance methodsetSetter(Method setter)at lines 93-95.-public static <T> void setSetter(T instance, Method method, Object value) throws InvocationTargetException, IllegalAccessException { +public static <T> void invokeSetter(T instance, Method method, Object value) throws InvocationTargetException, IllegalAccessException { method.invoke(instance, value); }annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (2)
55-57: Consider using proper logging instead ofprintStackTrace().In annotation processors,
processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR, ...)is the standard approach for error reporting. This provides better integration with build tools.
187-206: Consider using a Java record for thePairclass.If Java 16+ is targeted, this can be simplified to a record:
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/yaml/NodeValidationUtils.java (3)
21-23: Consider renaming toensureObjectfor consistency.The name
ensureMappingStartrefers to YAML event parsing terminology, but this utility operates on JsonNode. Renaming toensureObjectwould be more consistent with the other methods (ensureArray,ensureTextual) and align with the shift from event-driven to JsonNode-based parsing.Apply this diff:
- public static void ensureMappingStart(JsonNode node) throws ParsingException { + public static void ensureObject(JsonNode node) throws ParsingException { if (!(node.isObject())) throw new ParsingException("Expected object", node); } public static void ensureSingleKey(JsonNode node) throws ParsingException { - ensureMappingStart(node); + ensureObject(node); if (node.size() != 1) throw new ParsingException("Expected exactly one key.", node); }
22-22: Standardize error message punctuation.Error messages have inconsistent punctuation. Consider standardizing to either include or exclude periods consistently across all messages.
Apply this diff to add periods consistently:
- if (!(node.isObject())) throw new ParsingException("Expected object", node); + if (!(node.isObject())) throw new ParsingException("Expected object.", node);Also applies to: 27-27, 39-39
19-42: Add JavaDoc documentation.As a public utility class, adding JavaDoc would improve maintainability by documenting the purpose of the class and the validation methods, their parameters, thrown exceptions, and usage examples.
Example documentation structure:
/** * Utility class providing validation helpers for Jackson JsonNode objects. * Used by YAML/JSON parsing to ensure nodes meet expected structure requirements. */ public final class NodeValidationUtils { /** * Validates that the given node is a JSON object. * * @param node the JsonNode to validate * @throws ParsingException if the node is not an object */ public static void ensureMappingStart(JsonNode node) throws ParsingException { // ... } // ... similar documentation for other methods }annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
29-37: Tighten constructor robustness around resource loading and async initThe overall wiring looks correct, but a couple of test-robustness tweaks would help:
- Consider adding an explicit error message to
requireNonNull(getClass().getResourceAsStream(resourceName))so a missing YAML resource fails with a clear message tied toresourceName.- If
parseMembraneResourcesuses asynchronous initialization,cdl.await()can hang indefinitely if the callback is never invoked (due to a bug or unexpected code path). Using a bounded wait and failing fast (e.g., viaIllegalStateExceptionor an assertion) would make test failures more diagnosable.- If
parseMembraneResourcesdoes not itself close theInputStream, wrapping the call in a try-with-resources block would keep the helper leak-free even under repeated use.These are mainly test-hardening refactors rather than functional issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/AbstractGrammar.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sYamlGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/KubernetesBootstrapper.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingException.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingException.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8 Repo: membrane/api-gateway PR: 2240 File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215 Timestamp: 2025-10-23T13:04:11.388Z Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision. Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
🧬 Code graph analysis (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-137)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
Grammar(30-207)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(45-267)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
annot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.java (1)
19-25: LGTM! Clean encapsulation of parsing state.This record effectively addresses the "long parameter list" code smell mentioned in the PR objectives by grouping related parsing parameters into a cohesive, immutable context object.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/AbstractGrammar.java (1)
35-56: LGTM! Class rename aligns with the Grammar abstraction refactor.The rename from
AbstractK8sGeneratortoAbstractGrammaris consistent with the broader refactoring. Consider whether this class should eventually move out of thekubernetespackage given its now-generic name, but that can be deferred.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/KubernetesBootstrapper.java (1)
32-36: LGTM! Bootstrap step correctly updated to use the new Grammar generator.The replacement of
K8sHelperGeneratorwithGrammarcompletes the migration to the Grammar-based generation approach.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java (1)
31-35: LGTM! Inheritance updated to extend AbstractGrammar.The change is consistent with the broader refactoring effort.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sYamlGenerator.java (1)
32-38: LGTM! Inheritance updated to extend AbstractGrammar.The change is consistent with the other generator classes in this refactoring.
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (4)
40-46: LGTM! Inheritance updated to extend AbstractGrammar.The change aligns with the broader refactoring across generator classes.
86-108: LGTM! The addTopLevelProperties logic is now correct.The previous bug where all elements were added to the
kindslist has been properly addressed. Only top-level elements now contribute to both the schema properties and theoneOfconstraint.
144-146: LGTM! Dynamic additionalProperties based on @MCOtherAttributes annotation.Setting
additionalPropertiesbased on whether the element has anOtherAttributesInfoannotation allows schemas to permit extra attributes only where explicitly allowed.
174-182: Verify the intent of hidingidonly for top-level elements.The logic now hides the
idattribute specifically for top-level elements while exposing it for nested elements. This differs fromK8sJsonSchemaGeneratorwhich hidesidunconditionally (line 76-77). Confirm this asymmetry is intentional.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (2)
1-13: License header is fine; nothing to change hereStandard Apache 2.0 header; no further review needed on this block.
27-27: BeanRegistry field and accessor look goodEncapsulating the parsed configuration behind a final
BeanRegistryand exposing it viagetBeanRegistry()is straightforward and appropriate for the test utility.Also applies to: 57-59
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java Show resolved Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
107-113: Potential NullPointerException and improper logging (previously flagged).This issue was already identified in a previous review:
System.err.printlnshould be replaced with proper logging viaLOG.- JsonNode traversal lacks null-safety checks—
node.get("metadata"),get("namespace"),get("name"), andget("spec")can all returnnullif fields are missing, causing NPEs.Please refer to the previous review comment for the suggested fix.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
196-199: Remove the commented-out dead code.This TODO with dead code was flagged in a previous review as "Addressed in commit 525fe99" but remains present. Please remove it to avoid confusion.
return envelope; - // TODO -// if (spec instanceof Bean) -// return ((Bean) spec).getBean(); }annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
31-41: Align constructorthrowsclause with actual behavior and improve missing-resource diagnosticsThe constructor declares
YamlSchemaValidationExceptionbut all schema validation errors are currently wrapped asIOExceptioninsideGenericYamlParser.parseMembraneResources. As written, this checked exception is never thrown, which can mislead callers and makes the API harder to reason about.At the same time, when the YAML resource is missing you get a bare
NullPointerExceptionfromrequireNonNull(getResourceAsStream(resourceName))with no context.Consider:
- Dropping
YamlSchemaValidationExceptionfrom the constructor signature (or re-exposing it deliberately if you want callers to distinguish schema vs IO).- Supplying an explicit message to
requireNonNull(...)so test failures clearly indicate which resource was not found, e.g.:- beanRegistry.registerBeanDefinitions(GenericYamlParser.parseMembraneResources( - requireNonNull(getClass().getResourceAsStream(resourceName)), generator)); + beanRegistry.registerBeanDefinitions(GenericYamlParser.parseMembraneResources( + requireNonNull( + getClass().getResourceAsStream(resourceName), + "YAML resource '%s' not found".formatted(resourceName) + ), + generator));annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
68-73: Guard against missing location entries when mapping schema validation errors
jsonLocationMap.getLocationMap().get(e.getErrors().getFirst().getInstanceNode())may returnnullif no location has been recorded for the reported instance node. The current code unconditionally dereferenceslocationand will throw aNullPointerExceptionwhen building the error message.To keep error reporting robust, consider:
- JsonLocation location = jsonLocationMap.getLocationMap().get( - e.getErrors().getFirst().getInstanceNode()); - throw new IOException("Invalid YAML: %s at line %d, column %d.".formatted( - e.getErrors().getFirst().getMessage(), - location.getLineNr(), - location.getColumnNr()), e); + var error = e.getErrors().getFirst(); + JsonLocation location = jsonLocationMap.getLocationMap().get(error.getInstanceNode()); + if (location == null) { + throw new IOException("Invalid YAML: %s".formatted(error.getMessage()), e); + } + throw new IOException("Invalid YAML: %s at line %d, column %d.".formatted( + error.getMessage(), + location.getLineNr(), + location.getColumnNr()), e);This preserves detailed location info when available while avoiding an additional NPE when it's not.
141-147: Validate that the single map key actually matcheskindbefore accessingnode.get(kind)
readMembraneObjectensures the node has exactly one key viaensureSingleKey(node), but it never checks that this sole key equals the expectedkind. If the caller passes a node like{ someOtherKey: {...} }withkind="api",node.get(kind)will benull, leading to aNullPointerExceptioninsidecreateAndPopulateNodeand an unhelpful error.To make this API safer and yield clearer diagnostics, consider:
- ensureSingleKey(node); - Class<?> clazz = grammar.getElement(kind); + ensureSingleKey(node); + String actualKey = node.fieldNames().next(); + if (!kind.equals(actualKey)) { + throw new ParsingException( + "Expected top-level key '%s' but found '%s'.".formatted(kind, actualKey), + node); + } + Class<?> clazz = grammar.getElement(kind); if (clazz == null) throw new ParsingException("Did not find java class for kind '%s'.".formatted(kind), node); - return createAndPopulateNode(new ParsingContext(kind, registry, grammar), clazz, node.get(kind)); + return createAndPopulateNode(new ParsingContext(kind, registry, grammar), clazz, node.get(kind));This keeps existing happy-path behavior while turning misuse into a clear
ParsingExceptioninstead of a hidden NPE.
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
63-63: Avoid creating duplicate GrammarAutoGenerated instances.A new
GrammarAutoGeneratedinstance is created here, but one already exists withinbeanCache(created at line 50). Consider extracting it to a field for reuse.Apply this diff to extract and reuse the Grammar instance:
private final Router router; private final BeanRegistryImplementation beanCache; + private final GrammarAutoGenerated grammar; private KubernetesClient client; public KubernetesWatcher(Router router) { this.router = router; - this.beanCache = new BeanRegistryImplementation(router, new GrammarAutoGenerated()); + this.grammar = new GrammarAutoGenerated(); + this.beanCache = new BeanRegistryImplementation(router, grammar); } public void start() { ... client = getClient(); - List<String> crds = new GrammarAutoGenerated().getCrdSingularNames(); + List<String> crds = grammar.getCrdSingularNames();annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java (1)
34-46: Consider exposing the event type explicitly.The TODO on line 43 raises a valid concern. Currently, consumers must infer the event type from
bean/oldBeannullity patterns:
- ADD:
bean != null,oldBean == null- MODIFY:
bean != null,oldBean != null- DELETE:
bean == null,oldBean != nullThis is error-prone. Consider adding an enum parameter to make the event type explicit.
+ enum EventType { ADDED, MODIFIED, DELETED } + /** * Called for an add/modify/delete event of a bean. * + * @param event the type of event * @param bd the bean definition * @param bean the current instance (on ADD/MODIFY) or {@code null} (on DELETE) * @param oldBean the previous instance (on MODIFY) or {@code null} * @throws IOException if handling the event performs I/O and it fails - * - * - * TODO: Make event visible: enum and add to signature? - * */ - void handleBeanEvent(BeanDefinition bd, Object bean, Object oldBean) throws IOException; + void handleBeanEvent(EventType event, BeanDefinition bd, Object bean, Object oldBean) throws IOException;Would you like me to open an issue to track this refactor?
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
351-357: Consider caching the ObjectMapper instance.
ObjectMapperis thread-safe and expensive to create. For tests this is acceptable, but if this helper is used frequently, consider extracting to a static constant.+ private static final ObjectMapper YAML_MAPPER = new ObjectMapper(new YAMLFactory()); + public static JsonNode parse(String yaml) { try { - return new ObjectMapper(new YAMLFactory()).readTree(yaml); + return YAML_MAPPER.readTree(yaml); } catch (JsonProcessingException e) { throw new RuntimeException(e); } }annot/src/main/java/com/predic8/membrane/annot/yaml/ChangeEvent.java (1)
23-27: Clarify or remove the TODO comment.The TODO asks about the semantics of
StaticConfigurationLoaded. Based on usage inBeanRegistryImplementation.fireConfigurationLoaded(), this event signals that all static configuration (e.g., from files) has been passed to the handler. Consider documenting this or removing the TODO if the behavior is clear./** - * TODO What does this exactly mean? Is the complete configuration loaded? + * Signals that all static configuration (e.g., from YAML files) has been + * passed to the registry and initial activation can proceed. */ record StaticConfigurationLoaded() implements ChangeEvent { }annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
83-99: BroadenresolveSetterValuetype support (boxed primitives,Collectionsubtypes) to avoid runtime "Not implemented setter type"The current resolution logic is quite strict:
- Lists are only recognized when the parameter type is exactly
List.classorCollection.class.- Only primitive
int/long/booleanare handled; boxed types likeInteger,Long,Boolean(and other common numeric types) will fall through to the finalRuntimeException("Not implemented setter type " + wanted).This is brittle and can turn otherwise valid annotated setters into runtime failures.
Consider:
- Using assignability rather than equality for collections:
- if (wanted.equals(List.class) || wanted.equals(Collection.class)) return parseListIncludingStartEvent(ctx, node); + if (Collection.class.isAssignableFrom(wanted)) { + return parseListIncludingStartEvent(ctx, node); + }
- Adding boxed primitive cases alongside the existing ones, e.g.:
- if (wanted.equals(Integer.TYPE)) return parseInt(node.asText()); - if (wanted.equals(Long.TYPE)) return parseLong(node.asText()); - if (wanted.equals(Boolean.TYPE)) return parseBoolean(node.asText()); + if (wanted == Integer.TYPE || wanted == Integer.class) return parseInt(node.asText()); + if (wanted == Long.TYPE || wanted == Long.class) return parseLong(node.asText()); + if (wanted == Boolean.TYPE || wanted == Boolean.class) return parseBoolean(node.asText());This keeps existing behavior but makes the resolver resilient to more natural method signatures (e.g.,
setFoo(Integer foo),setItems(ArrayList<Bar> items)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanCacheObserver.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/ChangeEvent.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(6 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(4 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java
- core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
🧰 Additional context used
🧬 Code graph analysis (4)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
BeanRegistryImplementation(28-209)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
BeanDefinition(20-106)
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(35-224)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-137)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-137)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
MethodSetter(33-127)annot/src/main/java/com/predic8/membrane/annot/yaml/NodeValidationUtils.java (1)
NodeValidationUtils(19-42)
⏰ 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: Analyze (java)
- GitHub Check: Automated tests
🔇 Additional comments (7)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (3)
16-20: Imports are appropriate for the refactoring.The new imports support the migration from Map-based to JsonNode-based event processing and the introduction of BeanRegistryImplementation with GrammarAutoGenerated.
37-37: Documentation updated correctly.The Javadoc reference has been updated to reflect the renamed class.
43-51: Field and constructor updated correctly.The migration from BeanCache to BeanRegistryImplementation is implemented properly. The GrammarAutoGenerated instance is passed to the registry as required.
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
336-344: Empty implementations in TestRegistry are appropriate for test mocking.The empty
registerBeanDefinitionsand theList.of()return fromgetBeans()are suitable for these unit tests since the tests focus on parsing behavior rather than bean registration lifecycle.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
72-88: Potential infinite block or race condition instart()loop.The loop condition
!changeEvents.isEmpty()paired with blockingchangeEvents.take()creates a subtle issue:
- If the queue is non-empty, we enter the loop
take()blocks until an element is available- If another thread drains the queue between iterations, the next
isEmpty()check exits immediatelyThe commented-out virtual thread suggests this was intended as a background processor. The current synchronous design works only if all events are queued before
start()is called, whichregisterBeanDefinitions()ensures. However, this is fragile for dynamic event handling.Is synchronous processing the intended behavior for CLI mode? If background processing is needed (e.g., for Kubernetes watch), the loop should use a sentinel/poison-pill pattern or an infinite loop with interrupt handling:
- while (!changeEvents.isEmpty()) { + while (true) { try { ChangeEvent changeEvent = changeEvents.take(); + if (changeEvent instanceof ShutdownEvent) break;
135-137: InlineactivationRun()may cause unexpected reentrancy.When
changeEvents.isEmpty()is true,activationRun()is called synchronously. IfactivationRun()triggers code that calls back intohandle(), this could cause unexpected behavior. Consider documenting this invariant or deferring activation.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
93-101: Potential NPE ingetScope()when annotation key is missing.Line 100 calls
annotations.get("membrane-soa.org/scope").asText()without null-checking. If the annotation key doesn't exist,get()returnsnullandasText()throws NPE.public String getScope() { JsonNode meta = node.get("metadata"); if (meta == null) return null; JsonNode annotations = meta.get("annotations"); if (annotations == null) return null; - return annotations.get("membrane-soa.org/scope").asText(); // TODO migrate to membrane-api.io + JsonNode scopeNode = annotations.get("membrane-soa.org/scope"); + return scopeNode != null ? scopeNode.asText() : null; // TODO migrate to membrane-api.io }Likely an incorrect or invalid review comment.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java Outdated Show resolved Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java Show resolved Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
197-199: Remove or implement the commented-out dead code.The TODO with commented-out code checking
if (spec instanceof Bean)should either be implemented or removed to avoid confusion.
92-95: stop() method is currently a no-op.As noted in the past review, the
threadfield (line 49) is never assigned because virtual thread creation is commented out (line 73). This meansstop()does nothing. Either remove the unusedthreadfield andstop()method, or implement the background thread properly.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java (1)
24-28: API-breaking change for external implementorsAdding
getBeans(),registerBeanDefinitions(...), andgetGrammar()to this interface breaks backward compatibility for any external implementations. The past review comment on lines 18-30 already identified this issue and suggested mitigation strategies (default methods or a new extended interface).core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
109-113: Replace System.err with proper logging and add null-safety checks.This code has two critical issues already flagged in the past review:
System.err.printlnshould use the classLOGfor proper observability.node.get("metadata"),.get("namespace"),.get("name"), and.get("spec")can all returnnullif fields are missing, causing NPE.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
35-48: Potential NullPointerExceptions in constructor.The past review comment identified multiple NPE risks here:
- Line 38:
node.get("metadata")can returnnull- Line 39:
node.get("kind")can returnnull, then.asText()throws NPE- Lines 43, 46, 47:
metadata.get(...)can returnnullConsider using
path()instead ofget(), which returns aMissingNodethat safely handlesasText()with defaults.
🧹 Nitpick comments (4)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
103-103: Address TODO: Migrate annotation key.The TODO suggests migrating from
"membrane-soa.org/scope"to"membrane-api.io". Consider whether this migration should happen in this PR or be tracked separately.Do you want me to open an issue to track this migration task?
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
40-42: Consider addressing TODO comments.Multiple TODO comments indicate incomplete design decisions:
- Line 40: Rename
uuidMapto something more meaningful- Line 47: Remove unused
threadfield (relates to stop() issue)- Lines 140-145: Thread-safety and event-processing concerns
These indicate areas where the implementation may need refinement before production use.
Do you want me to help generate refactored code for any of these TODOs?
Also applies to: 47-49, 140-145
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
83-102: Consider refactoring the type-resolution chain.The
resolveSetterValuemethod uses a long if/else chain to handle type conversions. While functional, this could be refactored using a strategy pattern with a map ofTypeResolverimplementations for better maintainability and extensibility.This aligns with the refactoring suggestions mentioned in the PR objectives (nice-to-have category).
Do you want me to generate a refactored version using the strategy pattern?
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
355-355: Extract hardcoded "api" string to a constant.The hardcoded
"api"string passed toParsingContextis a magic string that represents the resource kind. If this value changes in production code or needs to be reused, it could lead to maintenance issues.Apply this diff to extract the magic string:
+ private static final String API_KIND = "api"; + private static APIProxy parse(String yaml, BeanRegistry reg) { - return GenericYamlParser.createAndPopulateNode(new ParsingContext("api", reg, K8S_HELPER), APIProxy.class, parse(yaml)); + return GenericYamlParser.createAndPopulateNode(new ParsingContext(API_KIND, reg, K8S_HELPER), APIProxy.class, parse(yaml)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistry.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/ChangeEvent.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(5 hunks)core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
BeanRegistryImplementation(29-215)
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(35-224)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-137)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
BeanDefinition(20-109)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (2)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
Grammar(30-207)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)
⏰ 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 (4)
annot/src/main/java/com/predic8/membrane/annot/yaml/ChangeEvent.java (1)
17-28: LGTM! Clean event hierarchy design.The sealed interface with record implementations is a modern, type-safe approach to modeling change events. The package-private scope appropriately limits visibility to internal components.
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
33-73: LGTM! Setter resolution logic is well-structured.The
getMethodSettermethod appropriately handles multiple resolution strategies: direct setter lookup, bean class resolution via grammar, child setters, and fallback to any-setters. Error messages are clear and helpful.core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (2)
51-53: LGTM! Well-structured Jackson integration.The static
ObjectMapperwithYAMLFactoryis thread-safe and follows Jackson best practices. TheGrammarAutoGeneratedinstance aligns with the PR's migration to a Grammar-based approach.
344-351: No action needed—the empty implementations are safe.The
TestRegistry'sregisterBeanDefinitions()andgetGrammar()implementations do not break parsing.GenericYamlParsernever calls either method; it uses theGrammarpassed directly to its constructor and accesses the registry only viaresolveReference()for"$ref"handling. The test registry properly supports the parsing flow.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
168-170: Address or remove the TODO comment with dead code.The commented-out code at lines 168-170 should either be implemented or removed to avoid confusion.
This issue was marked as addressed in commit 525fe99, but the code is still present.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java (1)
143-144: Regex pattern does not escape the dot, which matches any character.The pattern
.spring$uses an unescaped dot that matches any character before "spring". This should be\\.spring$to match only a literal dot.This issue was previously flagged but remains unaddressed.
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
100-106: Potential NullPointerException and use of System.err.The JsonNode navigation at lines 103-104 and 106 lacks null-safety checks. If
metadata,namespace,name, orspecfields are missing, this will throw a NullPointerException. Additionally,System.err.printlnshould be replaced with proper logging viaLOG.This issue was previously flagged but remains unaddressed. Please add null-checks for JsonNode traversal and replace System.err with the LOG instance.
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
79-102: LGTM!The type resolution logic correctly handles collections, primitives, enums, structured beans, and references. The error handling at line 101 provides a clear message for unsupported types.
Optional refactor: The if/else chain in
resolveSetterValue(lines 85-101) could be refactored to use a strategy pattern with a map of type resolvers for improved maintainability, but this is not critical for now.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
37-39: Address the TODO: renameuuidMapto a more meaningful name.The TODO suggests this field needs a better name. Consider renaming it to something like
activeBeansByUidorbeanInstanceCacheto better reflect its purpose of storing active bean instances by their unique ID.Would you like me to suggest a comprehensive renaming diff?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/Grammar.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(7 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java
- core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
🧰 Additional context used
🧬 Code graph analysis (2)
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(35-224)annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (1)
McYamlIntrospector(25-137)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanDefinition.java (1)
BeanDefinition(20-109)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)
⏰ 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/MethodSetter.java (3)
35-41: LGTM!The constructor and field declarations are clean and straightforward.
48-73: LGTM!The setter resolution logic correctly handles MCChildElement, grammar-based bean resolution, and fallback to any-setter. Error messages are clear and informative.
112-121: LGTM!The enum parsing logic with uppercase normalization and clear error handling is well-implemented.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (5)
52-77: LGTM!The bean registration and event processing logic is well-structured. The
start()method correctly handles interruption and processes configuration loaded events and bean definition changes.
79-85: LGTM!The
definemethod correctly delegates toGenericYamlParser.readMembraneObjectwith appropriate parameters.
90-119: LGTM!The event handling methods correctly queue bean definition changes and trigger activation when appropriate. The logic for determining when to activate (via
observer.isActivatable) is well-structured.
121-149: LGTM!The activation logic correctly handles bean definition, observer notification, and lifecycle management for ADDED, MODIFIED, and DELETED actions. The exception handling is appropriate.
152-185: LGTM!The reference resolution logic correctly implements lazy initialization with caching, respecting prototype scope. The accessor methods are straightforward and correct.
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/AbstractParser.java (2)
118-129: Both methods appear to be dead code and should be removedVerification shows no usages of
parseElementToProperty()orisMembraneNamespace()anywhere in the codebase—only their definitions in this file. The TODOs are legitimate: these methods can be safely deleted.Also applies to: 178-181
145-154: Guard against nulls and unsupported types when resolving child bean classesThe new child-handling logic has robustness issues:
delegate.parsePropertySubElement(...)can returnnull(it's @nullable in Spring Framework); passing nullointogetBeanClassNameFromObjectwill causeo.getClass()in the default switch arm to throw an unintendedNullPointerExceptioninstead of a clean parse error.getBeanClassNameFromObjectis annotated@Nullablebut the result is passed directly intoloadClass(...)without a null check. IfgetBeanDefinition().getBeanClassName()isnull(e.g., factory-method definitions),loadClasswill NPE.- The default switch arm throws a
RuntimeExceptionafter callingparserContext.getReaderContext().error(...). Sinceerror(...)is Spring's canonical way to report parse problems, the extra throw is redundant and masks the framework's standard error handling.A safer version would guard against both
o == nullandbeanClassName == nullbefore class loading, and rely onReaderContext.error()for error reporting:protected void handleChildElement(Element ele, ParserContext parserContext, BeanDefinitionBuilder builder) { BeanDefinitionParserDelegate delegate = parserContext.getDelegate(); - try { - Object o = delegate.parsePropertySubElement(ele, builder.getBeanDefinition()); - handleChildObject(ele, parserContext, builder, Thread.currentThread().getContextClassLoader().loadClass(getBeanClassNameFromObject(ele, parserContext, o)), o); - } catch (ClassNotFoundException e) { - throw new RuntimeException(e); - } + Object o = delegate.parsePropertySubElement(ele, builder.getBeanDefinition()); + if (o == null) { + String msg = "Parsed child element '" + ele.getLocalName() + "' to null value."; + log.warn(msg); + parserContext.getReaderContext().error(msg, ele); + return; + } + + String beanClassName = getBeanClassNameFromObject(ele, parserContext, o); + if (beanClassName == null) { + return; // error already reported in helper + } + + try { + Class<?> clazz = Thread.currentThread() + .getContextClassLoader() + .loadClass(beanClassName); + handleChildObject(ele, parserContext, builder, clazz, o); + } catch (ClassNotFoundException e) { + parserContext.getReaderContext().error("Cannot load bean class '" + beanClassName + "'", ele, e); + } } -private static @Nullable String getBeanClassNameFromObject(Element ele, ParserContext parserContext, Object o) { - return switch (o) { +private static @Nullable String getBeanClassNameFromObject(Element ele, ParserContext parserContext, Object o) { + if (o == null) { + String msg = "Cannot resolve bean class from null child value."; + log.warn(msg); + parserContext.getReaderContext().error(msg, ele); + return null; + } + return switch (o) { case BeanDefinitionHolder beanDefinitionHolder -> beanDefinitionHolder.getBeanDefinition().getBeanClassName(); case RuntimeBeanReference runtimeBeanReference -> parserContext.getRegistry().getBeanDefinition(runtimeBeanReference.getBeanName()).getBeanClassName(); case RuntimeBeanNameReference runtimeBeanNameReference -> parserContext.getRegistry().getBeanDefinition(runtimeBeanNameReference.getBeanName()).getBeanClassName(); default -> { var msg = "Don't know how to get bean class from " + o.getClass(); log.warn(msg); parserContext.getReaderContext().error(msg, ele); - throw new RuntimeException(msg); + return null; } }; }The pattern-matching switch is compatible with the project's JDK 21 target.
♻️ Duplicate comments (2)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
39-53: YAML resource loading is now guarded; consider a clearer error messageUsing
requireNonNull(cl.getResourceAsStream(normalized))is a good improvement over the previous implicit NPE risk. For easier debugging when a test mis‑spells the resource name, you might want to supply a message:InputStream in = requireNonNull( cl.getResourceAsStream(normalized), "YAML resource '%s' not found on test classpath".formatted(normalized) ); beanRegistry.registerBeanDefinitions( GenericYamlParser.parseMembraneResources(in, generator) );This keeps behavior the same but produces a much more actionable failure when the resource is missing.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
152-172: Clean up the leftover TODO/commented‑out code inresolveReference
resolveReferenceends with a commented‑out TODO block:return envelope; // TODO // if (spec instanceof Bean) // return ((Bean) spec).getBean();This is dead/commented code and doesn’t affect behavior, but it’s confusing for readers and suggests an unfinished alternative contract. Either remove it, or, if you still plan to support an envelope/inner‑bean distinction, implement that explicitly with a clear type instead of a commented cast.
🧹 Nitpick comments (7)
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java (1)
55-62: Consider conditional/failed-only RestAssured logging to keep output leanUsing
log().all()here will dump full request/response on every successful run, which can get noisy as the suite grows. Since this test already asserts on captured log output, you might consider limiting HTTP logging to failures (e.g., via a failed-only logging mode or a debug flag) so CI logs stay compact while still giving full detail when something breaks.annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)
111-115: Explicit root delegation forBeanRegistryavoids test‑time shadowingAdding
com.predic8.membrane.annot.yaml.BeanRegistrytodelegateToRootClassLoaderis reasonable to guarantee the registry interface comes from the real classpath, not an in‑memory overlay. If more “core” annotation/yaml types need the same treatment later, consider centralizing these FQCNs in one place (e.g., a small list or predicate) so class‑loading rules stay discoverable.annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.java (1)
1-18: ReuseYAML_PARSER_CLASS_NAMEconstant instead of hard‑coding the FQCNThe reflection check is good, but you now have the same class name string both here and in
CompilerHelper.YAML_PARSER_CLASS_NAME. To avoid them drifting apart, consider:Class.forName(CompilerHelper.YAML_PARSER_CLASS_NAME);This keeps the contract in a single place.
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (3)
37-105: SimplifyparseYAMLby reusing a singleCompositeClassLoaderThe new reflection‑based YAML path is fine, but
parseYAMLcurrently recomputes the composite class loader three times:Thread.currentThread().setContextClassLoader(getCompositeClassLoader(cr, yamlConfig)); return (BeanRegistry) getParserClass(cr, yamlConfig) .getMethod("getBeanRegistry") .invoke(getYAMLParser(getCompositeClassLoader(cr, yamlConfig)));Given that
getCompositeClassLoaderalso mutates theInMemoryClassLoaderviadefineOverlay, it would be clearer and marginally safer to compute it once and pass it through:public static BeanRegistry parseYAML(CompilerResult cr, String yamlConfig) { ClassLoader original = Thread.currentThread().getContextClassLoader(); CompositeClassLoader cl = getCompositeClassLoader(cr, yamlConfig); try { Thread.currentThread().setContextClassLoader(cl); Class<?> parserClass = cl.loadClass(YAML_PARSER_CLASS_NAME); Object parser = getParser(parserClass); return (BeanRegistry) parserClass .getMethod("getBeanRegistry") .invoke(parser); } catch (Exception e) { throw new RuntimeException(e); } finally { Thread.currentThread().setContextClassLoader(original); } }This makes the class‑loader story easier to follow and avoids repeated overlay setup.
91-105: Double‑checkCompositeClassLoaderargument order vs. XMLparseFor XML you build
new CompositeClassLoader(loaderA, CompilerHelper.class.getClassLoader()), whereas for YAML you now usenew CompositeClassLoader(CompilerHelper.class.getClassLoader(), loaderA).If
CompositeClassLoaderconsults its delegates in order (e.g., child‑first vs. parent‑first), this inversion can change which loader wins on conflicts (e.g., GrammarAutoGenerated vs. classes from the main test classpath). If the order is significant, it’s worth confirming that YAML should indeed prefer the “outer” loader first.
138-150: Use try‑with‑resources forcopyResourcesToOutput
copyResourcesToOutputmanually managesPrintWriterand throws aRuntimeExceptiononIOException. Using try‑with‑resources would ensure the writer is closed even ifwritethrows:private static void copyResourcesToOutput(List<? extends OverlayInMemoryFile> sources, JavaFileManager fileManager) { sources.forEach(i -> { try (PrintWriter pw = new PrintWriter( fileManager.getFileForOutput(CLASS_OUTPUT, "", i.getName(), null) .openWriter())) { pw.write(i.getCharContent(true).toString()); } catch (IOException e) { throw new RuntimeException(e); } }); }This keeps the behavior but tightens resource handling.
annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
51-76:start()drains the current batch only; verify this matches watcher usage
start()currently loops withwhile (!changeEvents.isEmpty()) { ... take() ... }. This works well for the static config path (registerBeanDefinitionsenqueues a finite batch then callsstart()), but for a long‑lived source like a Kubernetes watcher:
- If
start()is called once in a background thread and the queue ever becomes empty, the loop exits and the method returns.- Any
handle(...)calls that enqueue newChangeEvents after that point won’t be processed unless someone callsstart()again.If the intended contract is “process the initial static batch and then stop”, this is fine. If instead you expect
start()to be a long‑running event loop in a watcher thread, consider either:
- Removing the
isEmpty()condition and using a sentinel event or interrupt to terminate, or- Having the watcher explicitly re‑invoke
start()when new events are expected.Worth double‑checking the current call pattern to avoid silently dropped change events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
annot/src/main/java/com/predic8/membrane/annot/AbstractParser.java(4 hunks)annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(6 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java(3 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.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/Grammar.java (1)
Grammar(30-207)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)
⏰ 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/AbstractParser.java (3)
21-22: Logging setup is appropriateSLF4J logger initialization is idiomatic and scoped correctly for this abstract parser; no issues from my side.
Also applies to: 41-41
45-69: Use ofinlinedflag and accessor remains coherentThe
inlinedstate andisInlined()accessor are consistent with howparsesets the flag; nothing problematic here.
170-176:incrementCounterhelper remains simple and correctThe counter helper still does what it advertises (null-safe initialization and increment of an attribute); no change needed here.
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (1)
38-55: Child‑first test loader is now restricted to the demo packageLimiting custom loading to
DEMO_YAML_PARSING_PACKAGEand delegating everything else straight to the parent is a good way to avoid accidental interference with framework/runtime classes. This looks correct for thecom.predic8.membrane.demotest setup; just ensure no other dynamically compiled packages are expected to be loaded via this in‑memory loader.annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java (1)
25-36: Text‑block +.formattedrefactor is clean and equivalentSwitching
wrapSpringto a text block with%s.formatted(...) keeps the XML structure intact and improves readability with no behavioral change.annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
159-195: Regex‑basedextractName/extractPackageare fine for test sourcesUsing
PACKAGE_PATTERNandCLASS_PATTERNto drivetoInMemoryJavaFileis reasonable given these are controlled test snippets. The error messages on missing package/class also look clear. No issues from a correctness perspective here.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
56-70: Grammar lookup via TCCL matches the reflective bootstrap design
getGrammar()resolvesGrammarAutoGeneratedviaClass.forName(AUTOGENERATED_GRAMMER_CLASSNAME, true, cl)whereclis the thread context loader. GivenCompilerHelper.parseYAMLexplicitly sets the TCCL to aCompositeClassLoaderthat includes the in‑memory compiler output, this is consistent with the rest of the test infrastructure. No issues here; just be aware that any future move of the generated class package will need this FQCN updated.annot/src/main/java/com/predic8/membrane/annot/yaml/BeanRegistryImplementation.java (1)
152-181: Reference resolution & bean caching behavior look consistent withBeanDefinition
resolveReference:
- Locates the first
BeanDefinitionby name,- Lazily defines the bean if
getBean()is null,- Caches it back into the definition for non‑prototype scopes.
This matches
BeanDefinition.isPrototype()and the intended envelope vs. singleton semantics. From a correctness point of view this is fine; just keep in mind that prototype references will be re‑parsed on each call, which is expected but can be costly for large specs.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
117-133: Address the TODO: Consolidate parseYAML and parse methods.Both methods share the same pattern: create a composite classloader, overlay a config file, set the thread context classloader, and use reflection to load/instantiate a class. This duplication could be reduced by extracting the common flow into a parameterized helper method.
Do you want me to generate a refactored implementation that consolidates this common logic, or would you prefer to open a new issue to track this task?
♻️ Duplicate comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
40-55: Improve error reporting and guard against hangs in the async initializationThe constructor looks good overall, but a couple of tweaks would make failures easier to diagnose and tests more robust:
- Add a descriptive message to
requireNonNullso a missing resource fails with a clear error instead of a bare NPE.- Use a timed
awaiton the latch so tests can fail fast instead of hanging indefinitely if the async initialization never signals completion (e.g., exception before calling the observer).Example adjustment:
@@ - beanRegistry = new BeanRegistryImplementation(getLatchObserver(cdl),generator); - beanRegistry.registerBeanDefinitions(GenericYamlParser.parseMembraneResources( - requireNonNull(cl.getResourceAsStream(normalized)), generator)); - - cdl.await(); + beanRegistry = new BeanRegistryImplementation(getLatchObserver(cdl), generator); + beanRegistry.registerBeanDefinitions(GenericYamlParser.parseMembraneResources( + requireNonNull( + cl.getResourceAsStream(normalized), + String.format("YAML resource '%s' not found on test classpath", normalized) + ), + generator + )); + + if (!cdl.await(30, java.util.concurrent.TimeUnit.SECONDS)) { + throw new IllegalStateException("BeanRegistry initialization timed out for resource " + resourceName); + }Additionally, if
BeanRegistryImplementation/ its underlying cache exposes a lifecycle method (close(),stop(), etc.), consider wiringYamlParserasAutoCloseablein tests and invoking that in a try‑with‑resources to avoid leaking any worker threads spawned during async initialization (as noted in the earlier review).
🧹 Nitpick comments (4)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (3)
33-33: Typo in constant name (GRAMMER→GRAMMAR)Purely cosmetic, but since this constant is only used locally, you can still fix the spelling without affecting external APIs.
- public static final String AUTOGENERATED_GRAMMER_CLASSNAME = "com.predic8.membrane.demo.config.spring.GrammarAutoGenerated"; + public static final String AUTOGENERATED_GRAMMAR_CLASSNAME = "com.predic8.membrane.demo.config.spring.GrammarAutoGenerated";And update the usage below:
- Class<?> grammarClass = Class.forName( - AUTOGENERATED_GRAMMER_CLASSNAME, + Class<?> grammarClass = Class.forName( + AUTOGENERATED_GRAMMAR_CLASSNAME, true, cl );
73-92: Latch-basedBeanCacheObserveris simple and sufficient for testsThe latch observer correctly signals completion via
handleAsynchronousInitializationResultand unconditionally activates beans inisActivatable, which is reasonable for test wiring. No functional issues here.You might optionally mark the anonymous class as
@SuppressWarnings("unused")if static analysis complains about the unusedempty/beanparameters, but that’s not strictly necessary.
95-101: ExposeBeanRegistryas non-null API
getBeanRegistry()matches the reflective usage described in the Javadoc and is straightforward. SincebeanRegistryisfinaland always initialized in the constructor, consider annotating the getter as@NotNullfor clarity and static analysis alignment:- public BeanRegistry getBeanRegistry() { + public @NotNull BeanRegistry getBeanRegistry() { return beanRegistry; }annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
173-186: Clarify the resource parsing logic.The
while(true)loop with abreakstatement (lines 175-182) could be made clearer. Additionally, the TODO comments at lines 173 and 184 indicate this logic should be extracted and the magic number9(the length of "resource ") should be replaced with a named constant.Consider this refactoring:
- private static FileObject toFile(String content) { + private static FileObject toFile(String content) { if (!content.trim().startsWith("resource")) return toInMemoryJavaFile(content); + return parseResourceFile(content); + } + + private static final String RESOURCE_PREFIX = "resource "; + + private static OverlayInMemoryFile parseResourceFile(String content) { + // Skip leading empty lines + content = content.replaceFirst("^\\s*\\n", ""); - // TODO extract method - String[] parts; - while (true) { - parts = content.split("\n", 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]; - } - - String name = parts[0].substring(9).trim(); // TODO Refactor and give meaningful name - return new OverlayInMemoryFile(name, parts[1]); + String[] parts = content.split("\n", 2); + if (parts.length != 2 || parts[0].trim().isEmpty()) + throw new RuntimeException("Invalid resource file: %s. The resource is expected to have the format 'resource <path>\\n<content>'.".formatted(content)); + + String name = parts[0].substring(RESOURCE_PREFIX.length()).trim(); + return new OverlayInMemoryFile(name, parts[1]); }Would you like me to open an issue to track this refactoring?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(7 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.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). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java (1)
60-60: Good addition for test observability.Adding
.log().ifValidationFails()improves debugging by logging response details only when the test fails, without adding noise to successful test runs.annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
57-71: Grammar loading via TCCL looks appropriateUsing the thread context class loader and a dedicated
getGrammar()helper is a clean way to loadGrammarAutoGeneratedfrom the in‑memory compiler loader. The implementation is straightforward and matches the reflective usage pattern described in the comments.annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (5)
37-41: LGTM: Constants extraction improves maintainability.Extracting class names and patterns into constants is a good refactoring that eliminates magic strings and enables pattern reuse.
77-89: LGTM: Improved type safety and proper resource cleanup.The updated method signature with
BeanRegistryreturn type provides better type safety. The finally block ensures the thread context classloader is properly restored.
149-160: LGTM: try-with-resources improves resource management.Replacing manual resource cleanup with try-with-resources is a good modernization that ensures proper resource disposal.
188-205: LGTM: Pattern constants improve efficiency.Using precompiled pattern constants instead of inline
Pattern.compile()calls avoids repeated regex compilation overhead.
207-244: LGTM: Code modernization with diamond operators and formatted strings.These updates improve code conciseness without changing behavior.
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java Outdated Show resolved Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
33-55: Tighten error reporting for missing resources and consider BeanRegistry/BeanCache lifecycle in testsThe constructor wiring (grammar via TCCL,
BeanRegistryImplementation,parseMembraneResources, and latch-based await) looks sound for test usage. Two small points:
requireNonNull(cl.getResourceAsStream(normalized))will throw an NPE with a null message if the resource is missing. For test debugging it’s often worth using the overload with a message that includesresourceName/normalized, so failures clearly point at the missing YAML.- If
BeanRegistryImplementation/BeanCachespin up background threads as part of async initialization, this helper never tears them down. RepeatedparseYAMLcalls in a larger test suite could accumulate threads or keep the JVM alive. Consider exposing a close/shutdown hook on the registry (or onYamlParseritself) and having tests invoke it once they’re done with theBeanRegistry.Also applies to: 76-92
🧹 Nitpick comments (2)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (2)
39-44: Deduplicate YAML/XML overlay + classloader handling betweenparseYAMLandparseBoth
parseYAMLandparsefollow the same pattern: define an in‑memory overlay (/demo.yamlor/demo.xml), build aCompositeClassLoader, swap the thread context classloader, instantiate a parser by class name, and then restore the original TCCL.Given the similarity (and the TODO on Line 111), consider extracting a small helper like:
withOverlay(CompilerResult cr, String resourcePath, String contents, ClassLoaderConsumer action)orwithCompositeClassLoader(CompilerResult cr, String resourcePath, String contents, Function<ClassLoader, R> action)and using it from both methods. That would centralize the TCCL swap/restore and overlay wiring, and make it harder for the two flows to drift apart over time.
Also applies to: 79-107, 113-123
210-238: Diagnostic matcher uses exact message equality; may be brittle across JDKs/locales
compilerResultcurrently matches diagnostics by requiringd.getKind() == kindandd.getMessage(null).equals(text). Exact string equality on the full compiler message can be sensitive to:
- JDK version differences (slightly different wording), and
- Locale changes, if tests ever run under a non‑default locale.
If you intend the tests to be robust across JDKs/locales, consider relaxing this to a substring or prefix match using existing Hamcrest matchers (e.g.
containsString(text)orstartsWith(text)), or by normalizing the diagnostic message before comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(6 hunks)annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- annot/src/test/java/com/predic8/membrane/annot/util/ArchitectureTest.java
🧰 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/Grammar.java (1)
Grammar(30-207)annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
GenericYamlParser(35-224)
⏰ 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/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
143-153: Good use of try‑with‑resources incopyResourcesToOutputSwitching to a try‑with‑resources
PrintWriteraroundfileManager.getFileForOutput(...).openWriter()ensures writers are always closed, even on exceptions, and removes the risk of leaked handles in the test harness.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.