Chain: Migrate Example to YAML - Fix for component names #2529
Conversation
… improve `ChainInterceptor` bean retrieval logic, and update formatting in `DefaultRouter`
|
Warning Rate limit exceeded@rrayst has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded name-and-type bean lookup APIs/implementations ( Changes
Sequence Diagram(s)sequenceDiagram
participant CI as ChainInterceptor
participant REG as BeanRegistry
participant BF as SpringBeanFactory
CI->>REG: getBean(name, ChainDef.class)
alt found in registry
REG-->>CI: Optional<ChainDef> (present)
CI->>CI: use ChainDef.getFlow()
else not found
CI->>BF: getBean(name, ChainDef.class)
alt found in factory
BF-->>CI: ChainDef
CI->>CI: use ChainDef.getFlow()
else not found
BF-->>CI: throws NoSuchBeanDefinitionException
CI-->>CI: propagate error / throw
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
♻️ Duplicate comments (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/ChainExampleTest.java (1)
53-65: Same race condition as in request1().This test has the identical timing issue:
assertTrue(pathFound.get())executes immediately after the HTTP assertion completes, without waiting for the console watcher to process output. Apply the same polling fix suggested forrequest1().
🧹 Nitpick comments (3)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/ChainExampleTest.java (1)
68-74: Consider more specific pattern matching for console output.The current implementation checks for any line containing "Path:" which could match unintended output. Consider using a more specific pattern (e.g., regex) if the expected console format is well-defined, to reduce false positives.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)
41-48: Enhance JavaDoc consistency and completeness.The JavaDoc for the new method should follow the same format as the existing
getBean(Class<T>)method (lines 31-39) and include@paramtags for both parameters. Additionally, clarify the behavior when no bean is found or if the cast fails.🔎 Proposed documentation improvement
/** * Retrieves a bean with the specified name. - * @param beanName - * @param clazz - * @return Optional containing the bean * @param <T> the bean type + * @param beanName the name of the bean to retrieve + * @param clazz the class of the bean to retrieve + * @return Optional containing the bean if found, empty otherwise */ <T> Optional<T> getBean(String beanName, Class<T> clazz);core/src/main/java/com/predic8/membrane/core/interceptor/chain/ChainInterceptor.java (1)
17-24: Consider using explicit imports instead of wildcards.Wildcard imports can reduce code clarity by hiding which specific classes are used and may cause namespace conflicts. Explicit imports make dependencies clearer and more maintainable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javaannot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.javacore/src/main/java/com/predic8/membrane/core/interceptor/chain/ChainInterceptor.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.javadistribution/examples/extending-membrane/reusable-plugin-chains/README.mddistribution/examples/extending-membrane/reusable-plugin-chains/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/ChainExampleTest.javadocs/ROADMAP.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-23T15:59:51.742Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2339
File: distribution/examples/web-services-soap/rest2soap-template/apis.yaml:17-32
Timestamp: 2025-11-23T15:59:51.742Z
Learning: In Membrane API Gateway YAML configuration files, response flows execute in reverse order. Steps that appear later in the response flow list actually execute first. For example, if a template references properties and setProperty steps appear after the template in the YAML, those setProperty steps will execute before the template renders.
Applied to files:
distribution/examples/extending-membrane/reusable-plugin-chains/apis.yaml
🪛 LanguageTool
docs/ROADMAP.md
[style] ~35-~35: Using “mind” here sounds more polite.
Context: ...eans. I just want bean foo and I do not care where it is defined. - See: ChainInte...
(CARE_MIND)
⏰ 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 (10)
docs/ROADMAP.md (1)
35-37: LGTM! New bean lookup feature documented.The roadmap entry clearly describes the configuration-independent bean lookup concept and references the relevant implementation.
distribution/examples/extending-membrane/reusable-plugin-chains/apis.yaml (3)
1-19: LGTM! Well-structured reusable chain components.The component definitions demonstrate clean separation of concerns:
logchain handles request-phase loggingcorschain handles response-phase headersThe CORS headers are correctly placed in the response flow.
22-30: LGTM! Clean API definition with chain reference.The
/fooendpoint demonstrates straightforward chain application using JSON pointer references.
33-45: LGTM! Excellent demonstration of chain composition.The
/barendpoint effectively shows how multiple reusable chains can be composed together, with helpful inline comments explaining the benefits.distribution/examples/extending-membrane/reusable-plugin-chains/README.md (1)
3-23: LGTM! Documentation successfully migrated to YAML.The updated instructions clearly guide users through testing the new YAML-based configuration, with appropriate references to
apis.yamland updated endpoint paths.core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (1)
407-409: LGTM! Formatting adjustment with no functional impact.The
getRegistry()method behavior is preserved.core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
369-372: LGTM!The stub implementation correctly returns
Optional.empty()to satisfy the new interface requirement for name-based bean lookup in tests.core/src/main/java/com/predic8/membrane/core/interceptor/chain/ChainInterceptor.java (3)
27-27: LGTM!The updated description clearly explains the purpose of the Chain interceptor.
42-45: LGTM!The refactored bean lookup is cleaner and provides a clear error message when the chain reference is not found.
47-64: Dual lookup strategy is appropriate, but ensure null-safety.The implementation provides a reasonable fallback from registry to bean factory for migration purposes. However, this method depends on the correct behavior of
BeanRegistryImplementation.getBean(), which currently has a null-handling issue (see review comment on BeanRegistryImplementation.java lines 168-172).Ensure that the bug fix is applied before merging, as this code assumes the registry will return
Optional.empty()rather thanOptional.of(null)when a bean is not found.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
Outdated
Show resolved
Hide resolved
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/ChainExampleTest.java
Show resolved
Hide resolved
…ents in `BeanContainer`, `BeanRegistry`, and `MethodSetter` for better null-safety and readability
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java:
- Around line 58-64: The hardcoded string check against
"com.predic8.membrane.core.config.spring.Components" in MethodSetter should be
replaced with a marker annotation-based check: define a marker annotation (e.g.,
@MCPreferAnySetter) in the core module and annotate the Components class with
it, then change the conditional to test
clazz.isAnnotationPresent(MCPreferAnySetter.class) (or use
clazz.getAnnotation(...) != null) and return new
MethodSetter(getAnySetter(clazz), beanClass) when present; remove the string
literal and its comment so annot no longer depends on a concrete core class
name.
In @docs/ROADMAP.md:
- Around line 51-60: Remove the duplicate "# 7.0.4" section (the later
occurrence that repeats lines about "Discuss renaming the
WebSocketInterceptor.flow..." and "do not pass a `Router`...") so roadmap
sections remain in descending version order, and in the remaining 7.0.4 text fix
the grammar by changing "Old an unused" to "Old and unused" (keep references
like ChainInterceptor.getBean(String) and WebSocketInterceptor.flow intact).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javaannot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.javadocs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
- annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
- annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java
🧰 Additional context used
🪛 LanguageTool
docs/ROADMAP.md
[style] ~53-~53: Using “mind” here sounds more polite.
Context: ...eans. I just want bean foo and I do not care where it is defined. - See: ChainInte...
(CARE_MIND)
🪛 markdownlint-cli2 (0.18.1)
docs/ROADMAP.md
57-57: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Automated tests
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java
Outdated
Show resolved
Hide resolved
…nts without global keyword resolution
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
58-62: Improved approach compared to string-based check.This refactoring replaces the fragile string-based class name comparison with a more generic, annotation-based check. The logic is sound: when a class has only an
@MCOtherAttributessetter and no specific@MCAttributeor@MCChildElementsetters, routing all keys directly to the any-setter avoids unnecessary global keyword resolution.This approach:
- ✅ Eliminates coupling to specific class names
- ✅ Keeps the annot module more generic and reusable
- ✅ Provides clearer intent through the helper method name
However, note that the underlying implementation in
hasOtherAttributesButNoAttributesAndNoChildrenhas some efficiency and consistency concerns (see the review comment on that method).📝 Optional: Consider adding a brief comment explaining the early-return rationale
While the helper method name is descriptive, a short comment explaining why this case needs special handling would improve maintainability:
if (setter == null) { - // if the element ONLY has a MCOtherAttributes and no MCAttributes and no MCChildElement setters, we avoid - // global keyword resolution: the keyword will always be a key for MCOtherAttributes + // When a class has only @MCOtherAttributes and no specific @MCAttribute or @MCChildElement setters, + // all keys should route to the any-setter, avoiding name conflicts with global element keywords. if (hasOtherAttributesButNoAttributesAndNoChildren(clazz)) { return new MethodSetter(getAnySetter(clazz), beanClass); }annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java (2)
22-22: Unnecessary import: AtomicInteger not needed for local counters.
AtomicIntegeris designed for thread-safe concurrent operations, but thehasOtherAttributesButNoAttributesAndNoChildrenmethod uses it for simple local counting in a single-threaded context. This adds unnecessary overhead and misleads readers about threading requirements.See the refactoring suggestion in the review comment for lines 149-159.
149-159: Refactor for efficiency, consistency, and clarity.The implementation has several issues:
- Unnecessary AtomicInteger: These are local, single-threaded counters—plain
intvariables would be simpler and more efficient.- Missing setter filter: The method iterates over all methods (including getters, inherited methods, etc.). Since these annotations should only appear on setters, filter with
isSetter()first.- Inconsistent annotation lookup: Other methods in this class use Spring's
findAnnotationutility. This method usesisAnnotationPresent, which may behave differently for meta-annotations or inherited annotations.- No early exit: The loop continues even after counts exceed thresholds. Short-circuit once you know the condition is false.
- Missing JavaDoc: Document the method's purpose and contract.
♻️ Proposed refactoring
+ /** + * Returns {@code true} if the class has exactly one setter with {@code @MCOtherAttributes} + * and no setters with {@code @MCAttribute} or {@code @MCChildElement}. + * <p> + * Used to determine if all YAML keys should route directly to the any-setter, + * avoiding name conflicts with global element keywords. + * </p> + */ public static boolean hasOtherAttributesButNoAttributesAndNoChildren(Class<?> clazz) { - AtomicInteger otherAttributesCount = new AtomicInteger(); - AtomicInteger attributesCount = new AtomicInteger(); - AtomicInteger childrenCount = new AtomicInteger(); - stream(clazz.getMethods()).forEach(m -> { - if (m.isAnnotationPresent(MCOtherAttributes.class)) otherAttributesCount.incrementAndGet(); - if (m.isAnnotationPresent(MCAttribute.class)) attributesCount.incrementAndGet(); - if (m.isAnnotationPresent(MCChildElement.class)) childrenCount.incrementAndGet(); - }); - return otherAttributesCount.get() == 1 && attributesCount.get() == 0 && childrenCount.get() == 0; + int otherAttributesCount = 0; + int attributesCount = 0; + int childrenCount = 0; + + for (Method m : clazz.getMethods()) { + if (!isSetter(m)) continue; + + if (findAnnotation(m, MCOtherAttributes.class) != null) otherAttributesCount++; + if (findAnnotation(m, MCAttribute.class) != null) attributesCount++; + if (findAnnotation(m, MCChildElement.class) != null) childrenCount++; + + // Early exit: if we already have attributes or children, or multiple otherAttributes + if (attributesCount > 0 || childrenCount > 0 || otherAttributesCount > 1) { + return false; + } + } + + return otherAttributesCount == 1; }This version:
- Uses plain
intcounters- Filters for setters first
- Uses
findAnnotationfor consistency- Short-circuits when the condition can't be met
- Includes documentation
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javaannot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.javaannot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Automated tests
🔇 Additional comments (1)
annot/src/test/java/com/predic8/membrane/annot/YAMLComponentsParsingTest.java (1)
342-369: LGTM! Good edge case coverage.This test validates that component IDs can be grammar keywords (like "bearerToken") without causing parsing conflicts. The test is well-structured and follows the established patterns in this test class.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java:
- Around line 50-53: The getBean(String, Class<T>) in SpringContextAdapter
currently throws UnsupportedOperationException which breaks ChainInterceptor;
implement it to delegate to the underlying Spring ApplicationContext by calling
applicationContext.getBean(name, clazz) (or the existing field/method holding
the ApplicationContext) and return Optional.ofNullable(theBean); if the bean is
not present catch the Spring exception (e.g.,
NoSuchBeanDefinitionException/BeansException) and return Optional.empty()
instead of propagating an exception.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.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)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/SpringContextAdapter.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Documentation
Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.