refactor: move globalInterceptor to beanFactory#2510
Conversation
📝 WalkthroughWalkthroughAdds bean-based lookup and registration APIs to the BeanRegistry, updates implementations and tests, and switches Router/Transport to obtain GlobalInterceptor via the registry instead of a direct Router field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Transport
participant Router
participant Registry
participant GlobalInterceptor as GI
rect rgb(230, 245, 255)
note over Transport,Router: Transport.init() (new flow)
end
Transport->>Router: getRegistry()
Router->>Registry: return BeanRegistry
Transport->>Registry: getBean(GlobalInterceptor.class)
alt GlobalInterceptor present
Registry-->>Transport: Optional.of(GI)
Transport->>Transport: add GI to interceptors list
else not present
Registry-->>Transport: Optional.empty()
Transport->>Transport: proceed without global interceptor
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
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: 4
🧹 Nitpick comments (3)
core/src/test/java/com/predic8/membrane/core/transport/http/HttpTransportTest.java (1)
50-51: Test setup correctly adapted to new registry pattern.The comment "Do not inline!" is appropriate since Mockito needs a real object reference to set up the stub. However, passing
nullfor first and third constructor parameters may be fragile ifBeanRegistryImplementationevolves to use those parameters.Consider adding a test-specific factory method or builder for creating test
BeanRegistryImplementationinstances to reduce coupling to constructor signature.annot/src/test/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementationTest.java (1)
35-53: Add test coverage for edge cases.The existing tests cover happy paths, but consider adding tests for:
getBean()with zero matching beans: Should returnOptional.empty().getBean()with multiple matching beans: Should throwRuntimeExceptionper the implementation (lines 179-183 in BeanRegistryImplementation.java).🔎 Proposed additional tests
@Test void getBeanReturnsEmptyWhenNoBeans() { Optional<A> result = registry.getBean(A.class); assertTrue(result.isEmpty()); } @Test void getBeanThrowsWhenMultipleBeans() { registry.register("bean1", new A("a1")); registry.register("bean2", new A("a2")); assertThrows(RuntimeException.class, () -> registry.getBean(A.class)); }annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
177-185: Improve error handling and logging level for multiple beans.
- Line 181: Using
log.debug()before throwing an exception is unusual. Considerlog.error()since this represents a configuration error.- Lines 179-183: The error message could include which beans were found to aid debugging.
🔎 Proposed improvements
public <T> Optional<T> getBean(Class<T> clazz) { var beans = getBeans(clazz); if (beans.size() > 1) { - var msg = "One bean was asked. But found %d beans of %s".formatted(beans.size(),clazz); - log.debug(msg); + var msg = "Expected single bean of type %s but found %d: %s".formatted( + clazz.getName(), beans.size(), beans); + log.error(msg); throw new RuntimeException(msg); } return beans.size() == 1 ? Optional.of(beans.getFirst()) : Optional.empty(); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
annot/pom.xmlannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javaannot/src/test/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementationTest.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/transport/Transport.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.javacore/src/test/java/com/predic8/membrane/core/transport/http/HttpTransportTest.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-19T09:01:40.311Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2441
File: annot/pom.xml:0-0
Timestamp: 2025-12-19T09:01:40.311Z
Learning: In annot/pom.xml, ensure log4j dependencies log4j-slf4j2-impl and log4j-core have <scope>test</scope>. In the distribution module (separate pom.xml), these dependencies should use the normal compile-scope. This guidance applies to all pom.xml files under the annot and distribution module structure where these dependencies appear; verify both modules are configured accordingly to reflect testing vs. runtime usage.
Applied to files:
annot/pom.xml
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
annot/pom.xml
⏰ 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 (2)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
370-376: LGTM! Test registry stubs implement new BeanRegistry API.The stub implementations returning
Optional.empty()and no-op forregisterare appropriate for this test context, where bean resolution is handled via therefsmap.annot/pom.xml (1)
92-96: Mockito version is properly managed in the parent POM.The parent POM (./pom.xml) defines
mockito-coreversion4.11.0in dependencyManagement, and theannot/pom.xmlmodule correctly omits the version to rely on this parent configuration. Mockito 4.11.0 is compatible with the JUnit 5.10.0 version specified in the parent POM.
|
This pull request needs "/ok-to-test" from an authorized committer. |
… Javadoc comments across classes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)
30-46: JavaDoc successfully added as requested.The documentation now clearly describes both methods' behavior, including exception conditions and nullability constraints. This addresses the previous review feedback.
Optional enhancements you could consider:
- Explicitly document thread-safety guarantees (or lack thereof) for both methods
- Clarify behavior in
registerwhenbeanNamealready exists (current implementation generates unique UUIDs, allowing duplicate names)annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
187-193: Critical bug from previous review has been fixed.Line 191 now correctly stores the bean object (
bean) rather than the BeanContainer (bc), ensuringgetBeans(Class<T>)filtering operates on actual bean instances as intended.Optional enhancement: Consider adding null validation for the
beanparameter to enforce the "must not be null" contract documented in the interface JavaDoc.🔎 Optional null validation
public void register(String beanName, Object bean) { + Objects.requireNonNull(bean, "bean must not be null"); var uuid = UUID.randomUUID().toString(); BeanContainer bc = new BeanContainer(new BeanDefinition("component", beanName,null, uuid, null)); bc.setSingleton(bean); singletonBeans.put(uuid,bean); bcs.put(uuid, bc); }
📜 Review details
Configuration used: defaults
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/GenericYamlParser.javacore/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/GlobalInterceptor.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (1)
108-108: LGTM! Clean refactoring improves readability.The change from string concatenation to
String.formatted()makes the error message construction more concise and maintainable, while preserving identical functionality. This is consistent with the similar pattern used at lines 73-76.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
177-185: LGTM!The implementation correctly delegates to
getBeans(clazz), enforces single-bean semantics with a clear error message, and returns the appropriate Optional value. The logic aligns with the interface contract.core/src/main/java/com/predic8/membrane/core/interceptor/GlobalInterceptor.java (1)
25-25: Annotation correctly configured for registry-based bean lookup.The addition of
component = falseandtopLevel = trueappropriately supports the refactoring to move GlobalInterceptor from direct field storage in Router to registry-based bean lookup. Transport retrieves the interceptor viarouter.getRegistry().getBean(GlobalInterceptor.class)while Router'ssetGlobalInterceptor()method registers it in the registry, eliminating direct field dependencies.core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
176-176: No changes needed. The inconsistency between Router and HttpRouter is intentional by design.The change from
HttpRoutertoRouterat line 176 in the YAML initialization path is correct. Router has all required methods:setBaseLocation(),getResolverMap(),applyConfiguration(), andsetAsynchronousInitialization().The difference in initialization paths is deliberate:
HttpRouterprovides pre-configured HTTP transport and interceptors for OpenAPI spec handling, while the YAML path uses the genericRouterclass to allow YAML configuration to define all transport and interceptor behavior. This is a valid design pattern, not an incomplete refactor.Likely an incorrect or invalid review comment.
3 participants
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.