Router refactor to beanfactory #2514
Conversation
… Javadoc comments across classes
…ption formatting in `Router`
…ce Javadoc for `Router` settings
…into router-refactor-to-beanfactory
…n up redundant code in YAML integration
…n up redundant code in YAML integration
…mprove exception formatting, enhance concurrency handling
…ability and maintainability
|
Important Review skippedToo many files! 122 files out of 272 files are above the max files limit of 150. You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces a registry-backed Router/IRouter abstraction, extensive API signature migrations from Router→IRouter, adds RouterBootstrap and RuleReinitializer, adds BeanRegistry.registerIfAbsent, replaces global singleton handling with per-container synchronization, removes GateKeeperClientInterceptor, and migrates many tests to per-test router lifecycle and router.add()/start() usage. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Bootstrap
participant Router as Router (IRouter)
participant Registry as BeanRegistry
participant RM as RuleManager
participant Reinit as RuleReinitializer
Note over CLI,Router: startup
CLI->>Router: initByXML / initFromXMLString
Router->>Registry: get/provision core beans (ExchangeStore, Transport, DNSCache, RuleManager)
Registry-->>Router: beans (lazy-created via registerIfAbsent)
Router->>RM: register proxies / add(proxy)
Router->>RM: start() -> open ports
Router->>Reinit: getReinitializer()
Reinit->>Reinit: start() (schedule retry timer)
Note over Registry: registerIfAbsent ensures at-most-once bean creation per type
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/Router.java (1)
85-101: Critical: Duplicate field declaration will cause compilation error.The field
openPortsis declared twice (lines 92 and 101) with identical initialization. This will fail to compile.🔎 Proposed fix - remove duplicate declaration
/** * Indicates whether the router should automatically open TCP ports when adding proxies. * This flag determines if the ports associated with the proxies are opened immediately * when they are added to the router. Setting this to {@code false} allows for proxies * to be defined without opening the associated ports, providing more control over when * the ports are made accessible. */ private boolean openPorts = true; - - /** - * Indicates whether the router should automatically open TCP ports when adding proxies. - * This flag determines if the ports associated with the proxies are opened immediately - * when they are added to the router. Setting this to {@code false} allows for proxies - * to be defined without opening the associated ports, providing more control over when - * the ports are made accessible. - */ - private boolean openPorts = true;
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java (1)
371-375: Test stub returns null, violating interface contract.The
registerIfAbsentstub unconditionally returnsnull, which violates the interface contract (should return either an existing or newly created bean). If any test exercises this method, it will receivenullunexpectedly.Consider implementing minimal behavior to match the contract:
🔎 Proposed fix
@Override public <T> T registerIfAbsent(String name, Class<T> type, Supplier<T> supplier) { - return null; + Object existing = refs.get(name); + if (existing != null && type.isInstance(existing)) { + return type.cast(existing); + } + T created = supplier.get(); + refs.put(name, created); + return created; }
📜 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.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
core/src/main/java/com/predic8/membrane/core/config/spring/CheckableBeanFactory.java (1)
InvalidConfigurationException(26-47)
core/src/main/java/com/predic8/membrane/core/Router.java (1)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
KubernetesWatcher(37-132)
⏰ 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 (6)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java (1)
48-60: Well-documented interface extension for lazy registration.The new
registerIfAbsentmethod provides a clean API for thread-safe lazy bean initialization. The Javadoc clearly specifies the contract, including return behavior for both existing and newly created beans.core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
46-46: Import path update aligns with exception relocation.The static import now references
CheckableBeanFactory.InvalidConfigurationException, consistent with the exception's new location as shown in the relevant code snippet.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
16-24: Import organization change looks good.Wildcards consolidate related imports appropriately, and the addition of
java.util.concurrent.*andjava.util.function.*supports the new concurrency-awareregisterIfAbsentimplementation.core/src/main/java/com/predic8/membrane/core/Router.java (3)
199-200: Registry-based KubernetesWatcher start looks good.The migration from direct field access to registry-based resolution with
ifPresenthandles the optional nature of KubernetesWatcher cleanly.
439-441: Registry-based KubernetesWatcher stop is consistent with start.Mirrors the start() pattern appropriately.
495-497: GlobalInterceptor registration via registry.Clean migration to registry-based registration for the global interceptor.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
Outdated
Show resolved
Hide resolved
|
This pull request needs "/ok-to-test" from an authorized committer. |
…stry`, streamline bean registration logic
…treamline bean registration
…into router-refactor-to-beanfactory
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/kubernetes/GenericYamlParserTest.java
- annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistry.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/Router.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/Router.java (2)
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
KubernetesWatcher(37-132)core/src/main/java/com/predic8/membrane/core/util/DNSCache.java (1)
DNSCache(27-78)
⏰ 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 (6)
core/src/main/java/com/predic8/membrane/core/Router.java (6)
92-92: Well-documented feature addition.The
openPortsfield with its comprehensive Javadoc provides clear control over automatic port opening behavior while maintaining backward compatibility with the defaulttruevalue.
190-191: Clean registry-based dependency lookup.The optional lookup pattern via
getRegistry().getBean(KubernetesWatcher.class).ifPresent(...)is a good approach that avoids NPE if the watcher isn't registered while allowing for flexible configuration.
319-319: Registry-based singleton resolved the past concern.The use of
registerIfAbsentaddresses the previous review comment about creating multipleDNSCacheinstances. The method now ensures a singleton is created and reused. However, note thatregisterIfAbsentitself has potential issues withnullname registration (see comment on BeanRegistryImplementation.java line 193-203).
517-517: Registry-based singleton resolved the past concern.The use of
registerIfAbsentaddresses the previous review comment about creating multipleFlowControllerinstances. However, note thatregisterIfAbsentitself has potential issues withnullname registration (see comment on BeanRegistryImplementation.java line 193-203).
610-613: Excellent documentation of timing concerns.The updated Javadoc clearly documents the race condition risk and recommends calling
setOpenPorts()beforestart(), which helps prevent misuse.
579-580: The lazy initialization with nullobserverandgrammaris safe for Router's actual usage.Router only calls
getBean(),registerIfAbsent(), andregister()on the registry—none of which access observer or grammar. Thehandle()method (which uses observer) is not part of theBeanRegistryinterface and is never invoked from Router. Similarly,resolve()(which would use grammar) is never called. All actual usage paths operate safely with null values.
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
Show resolved
Hide resolved
|
/ok-to-test |
…`, improve logging and null checks, and streamline `activationRun` logic
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
Show resolved
Hide resolved
…ad-safety in `BeanContainer`, and streamline `BeanRegistry` handling
…wn` and `setUp` methods, and clean up `BeanContainer` by removing unused `setIfAbsent` method
|
@coderabbitai review |
…method in test file
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
59-68: Inconsistent synchronization:stop()usessynchronized(this)buthdtis@GuardedBy("lock").The
hdtfield is annotated with@GuardedBy("lock")(line 27), andstartInternal()correctly synchronizes onlock. However,stop()synchronizes onthis, breaking the synchronization contract. This can cause data races between concurrent calls tostartInternal()andstop().🔎 Proposed fix
@Override public void stop() { - synchronized (this) { + synchronized (lock) { if (hdt == null) return; router.getReinitializer().stop(); hdt.stopASAP(); hdt = null; } }core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java (1)
204-211: Add router cleanup in @AfterEach to prevent resource leaks.Each test creates and starts a new router instance, but there's no visible cleanup (@AfterEach with
router.stop()). Since routers typically bind to ports, start threads, and hold other resources, failing to stop them will cause resource leaks across test runs, potentially leading to port binding failures or thread exhaustion.🔎 Proposed fix: Add cleanup method
Add an @AfterEach method to properly stop the router:
@AfterEach public void tearDown() throws Exception { if (router != null) { router.stop(); } }
♻️ Duplicate comments (5)
core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
71-77: NPE fix applied; consider logging whenrouteris null.The null check for
routeraddresses the previously raised NPE concern. However, callingsetEnabled(true)beforestart(Router)will silently do nothing, which might be unexpected. Consider logging a warning to aid debugging.🔎 Proposed enhancement
@Override public void setEnabled(boolean enabled) { - if (enabled && router != null) + if (enabled && router != null) { startInternal(); - else + } else if (enabled) { + log.warn("setEnabled(true) called before start(Router). Ignoring."); + } else { stop(); + } }core/src/main/java/com/predic8/membrane/core/Router.java (2)
348-348: Consider usingregisterIfAbsentfor consistency.While
orElseThrow()ensures fail-fast behavior when DNSCache is missing, it requiresinit()to have been called first. For consistency withgetFlowController()(line 491) and to handle edge cases where this getter might be called beforeinit(), consider usingregisterIfAbsent:🔎 Suggested pattern for consistency
public DNSCache getDnsCache() { - return getRegistry().getBean(DNSCache.class).orElseThrow(); + return getRegistry().registerIfAbsent(DNSCache.class, DNSCache::new); }
556-557: Verify null safety in BeanRegistryImplementation.The lazy initialization creates
BeanRegistryImplementationwithnullobserver and grammar parameters. This was previously flagged as potentially causing NPEs during bean lifecycle operations. While fail-fast behavior is preferred (based on learnings), verify thatBeanRegistryImplementationproperly handles these null parameters or ensure they're never null in production scenarios.Run the following to check null handling in BeanRegistryImplementation:
#!/bin/bash # Check how BeanRegistryImplementation handles null observer and grammar ast-grep --pattern $'class BeanRegistryImplementation { $$$ }' | head -200core/src/test/java/com/predic8/membrane/core/config/ReadRulesConfigurationTest.java (1)
96-112: Consider usingassertInstanceOffor cleaner assertions.The current early-return pattern with
fail()correctly addresses the redundantassertTrue(true)issue from the previous review. However, usingassertInstanceOf()as originally suggested would be more idiomatic and produce clearer test failure messages if the proxy is not anSSLableProxy.🔎 Cleaner approach using assertInstanceOf
@Test void testLocalServiceProxyInboundSSL() { - if (proxies.get(2) instanceof SSLableProxy sp) { - assertFalse(sp.isInboundSSL()); - return; - } - fail(); + assertInstanceOf(SSLableProxy.class, proxies.get(2)); + SSLableProxy sp = (SSLableProxy) proxies.get(2); + assertFalse(sp.isInboundSSL()); } @Test void testLocalServiceProxyOutboundSSL() { - if (proxies.get(2) instanceof SSLableProxy sp) { - assertNull(sp.getSslOutboundContext()); - return; - } - fail(); + assertInstanceOf(SSLableProxy.class, proxies.get(2)); + SSLableProxy sp = (SSLableProxy) proxies.get(2); + assertNull(sp.getSslOutboundContext()); }core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (1)
71-72: Inconsistent request builder pattern - already flagged.This test still uses the old
new Request.Builder().get("/foo")pattern while the first test (line 63) was updated to use the static import patternget("/foo"). This inconsistency was already noted in a previous review.🔎 Proposed fix for consistency
- Exchange exc = new Request.Builder().get("/foo").header("Authorization", "bearer " + getSignedJwt(privateKey, getJwtClaimsArrayScopesList())).buildExchange(); + Exchange exc = get("/foo").header("Authorization", "bearer " + getSignedJwt(privateKey, getJwtClaimsArrayScopesList())).buildExchange();
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
30-32: Consider makingrouterfield volatile for thread-safety.The
routerfield is written instart()and read instartInternal(),stop(), andsetEnabled()without synchronization. If these methods are called from different threads, there's a visibility concern where other threads might not see the updated value.🔎 Proposed fix
- private Router router; + private volatile Router router;core/src/main/java/com/predic8/membrane/core/RuleReinitializer.java (1)
53-78: Consider synchronizing retry() or documenting thread-safety assumptions.The
retry()method is not synchronized and can be invoked concurrently (e.g., by the Timer thread and potentially external callers). While it calls the synchronizedstop()method, the rest of the logic—including cloning proxies and replacing rules—runs without synchronization. This could lead to race conditions ifRuleManageris not fully thread-safe or if multiple timers are inadvertently started.Either synchronize the method or document the thread-safety guarantees you rely on:
-public void retry() { +public synchronized void retry() { boolean stillFailing = false; ...core/src/main/java/com/predic8/membrane/core/Router.java (1)
377-395: Consider reducing lock scope to avoid holding during potentially expensive operations.The method now correctly synchronizes access to the
runningfield (fixing the previous review concern). However, holding the lock duringproxy.init(this)at line 392 could block other threads if initialization performs expensive operations (I/O, network calls, etc.).🔎 Suggested pattern to minimize lock contention
public void add(Proxy proxy) throws IOException { log.debug("Adding proxy {}.", proxy.getName()); RuleManager ruleManager = getRuleManager(); + boolean isRunning; + boolean shouldInit; synchronized (lock) { + isRunning = running; + shouldInit = running; if (proxy instanceof SSLableProxy sp) { - if (running) { + if (isRunning) { ruleManager.addProxyAndOpenPortIfNew(sp, MANUAL); } else ruleManager.addProxy(sp, MANUAL); } else { ruleManager.addProxy(proxy, MANUAL); } - if (running) { - // init() has already been called - proxy.init(this); - } } + if (shouldInit) { + // init() has already been called + proxy.init(this); + } }core/src/test/java/com/predic8/membrane/core/config/ReadRulesConfigurationTest.java (1)
36-39: Consider adding a null check in tearDown.If
setUp()fails and throws an exception, therouterfield will remain null. When JUnit callstearDown(), therouter.shutdown()call will throw a NullPointerException, which may mask the original setup failure in test output.🔎 Suggested improvement
@AfterAll static void tearDown() { + if (router != null) { router.shutdown(); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/com/predic8/membrane/core/HttpRouter.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/RuleReinitializer.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.javacore/src/test/java/com/predic8/membrane/core/config/ReadRulesConfigurationTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/B2CMembrane.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.javacore/src/test/java/com/predic8/membrane/core/proxies/UnavailableSoapProxyTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java
✅ Files skipped from review due to trivial changes (1)
- core/src/test/java/com/predic8/membrane/core/proxies/UnavailableSoapProxyTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/HttpRouter.java
- core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/B2CMembrane.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/Router.java
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-157)core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
TestUtil(29-61)
core/src/test/java/com/predic8/membrane/core/config/ReadRulesConfigurationTest.java (1)
core/src/main/java/com/predic8/membrane/core/RouterBootstrap.java (1)
RouterBootstrap(28-65)
⏰ 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 (16)
core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (3)
34-38: LGTM!Clean delegation pattern for the public API entry point.
40-57: LGTM!Proper synchronization on the
lockobject, defensive guard against double-start, and clean use of pattern matching for theinstanceofcheck.
79-82: LGTM!Simple implementation appropriate for a default deployer that's always enabled.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java (1)
285-285: LGTM. The removal ofthrows Exceptionis correct—theoasi.init(router)call at line 297 invokesAbstractInterceptor.init(Router router), which does not declare checked exceptions.core/src/main/java/com/predic8/membrane/core/Router.java (9)
107-107: LGTM: Logger initialization follows best practices.The logger is now properly initialized using
LoggerFactory.getLogger(Router.class), which is the standard SLF4J pattern.
174-209: Well-structured initialization with proper lifecycle separation.The new
init()method correctly separates initialization fromstart(), usesregisterIfAbsentfor thread-safe singleton registration, and includes a guard against double initialization. The initialization order (registry components first, transport last) is appropriate.
289-295: Excellent fix: Race condition eliminated.The previous non-atomic check-then-act pattern has been properly replaced with
registerIfAbsent, which atomically ensures a singleRuleManagerinstance. This correctly addresses the previous review concern.
490-492: Excellent fix: Singleton pattern correctly implemented.The previous issue of creating multiple
FlowControllerinstances has been properly fixed usingregisterIfAbsent. This ensures thread-safe singleton registration. This correctly addresses the previous review concern.
303-305: Good fix: Uses accessor instead of direct field access.The method now correctly calls
getRegistry()instead of accessing theregistryfield directly, ensuring proper initialization and null-checking. This correctly addresses the previous review concern.
228-262: Well-structured start sequence with proper lifecycle management.The
start()method correctly:
- Separates initialization from startup (calls
init()if needed)- Uses proper synchronization for state checks
- Starts components in the right order (ports, JMX, hot deployer, reinitializer)
- Handles exceptions comprehensively with appropriate error messages
502-537: Improved observability and clearer action handling.The added debug logging enhances troubleshooting capabilities, and the refactored action handling using explicit
isAdded(),isDeleted(), andisModified()methods improves code readability.
589-617: Clear and user-friendly error handling.The extracted error handling methods provide well-formatted, actionable error messages for OpenAPI-related configuration issues. The use of text blocks for multi-line messages enhances readability.
155-155: Excellent addition of debug logging throughout lifecycle.The strategic placement of debug logs at key lifecycle points (creation, initialization, starting, proxy addition) significantly improves observability and troubleshooting capabilities without adding overhead in production (when debug logging is disabled).
Also applies to: 175-175, 212-214, 229-229, 270-270, 298-298, 378-378, 495-495
core/src/test/java/com/predic8/membrane/core/config/ReadRulesConfigurationTest.java (1)
48-48: No changes needed. The project targets Java 21 (confirmed in pom.xml:<javac.source>21</javac.source>and<javac.target>21</javac.target>), soList.getFirst()is a valid and appropriate method to use. Multiple other files in the core module already use this API.core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (2)
37-37: LGTM! Good visibility and API improvements.The static import for
Request.getenables cleaner test code, and tightening the visibility ofSPEC_LOCATIONandproxyto private follows the principle of least privilege.Also applies to: 44-45
62-63: LGTM! Cleaner request construction.The switch to package-private visibility is appropriate for JUnit 5, and the fluent
get("/foo")pattern is more readable than the builder pattern.
core/src/main/java/com/predic8/membrane/core/RuleReinitializer.java
Outdated
Show resolved
Hide resolved
...redic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java
Show resolved
Hide resolved
… and update test setup/teardown methods
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/RuleReinitializer.java (3)
30-32: Add null validation for the router parameter.The constructor does not validate that
routeris non-null. If a null router is passed, subsequent calls tostart()(line 48) orgetInactiveRules()(line 91) will throw aNullPointerException.🔎 Proposed fix
public RuleReinitializer(Router router) { + if (router == null) { + throw new IllegalArgumentException("router must not be null"); + } this.router = router; }
66-78: Do not replace the proxy when the new proxy is still inactive.The code unconditionally calls
replaceRule()at line 74, even after detecting thatnewProxy.isActive()is false. This replaces the old inactive proxy with a new inactive clone, which is not the intended behavior. The replacement should only occur if the new proxy successfully activates.🔎 Proposed fix
for (Proxy proxy : inactive) { try { log.info("Trying to start API {}.", proxy.getName()); Proxy newProxy = proxy.clone(); if (!newProxy.isActive()) { log.warn("New rule for API {} is still not active.", proxy.getName()); stillFailing = true; + } else { + router.getRuleManager().replaceRule(proxy, newProxy); } - router.getRuleManager().replaceRule(proxy, newProxy); } catch (CloneNotSupportedException e) { log.error("", e); } }
89-95: Avoid potentialConcurrentModificationExceptioningetInactiveRules().The method iterates over
router.getRuleManager().getRules(), which returns the liveVectordirectly rather than a defensive copy. WhileVectoris thread-safe for individual operations, iteration can throwConcurrentModificationExceptionif another thread adds or removes proxies concurrently.Consider taking a snapshot before iteration to avoid concurrent modification issues.
🔎 Proposed fix
List<Proxy> getInactiveRules() { + List<Proxy> rules = new ArrayList<>(router.getRuleManager().getRules()); ArrayList<Proxy> inactive = new ArrayList<>(); - for (Proxy proxy : router.getRuleManager().getRules()) + for (Proxy proxy : rules) if (!proxy.isActive()) inactive.add(proxy); return inactive; }
🧹 Nitpick comments (4)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
34-36: Consider renaming parameter to avoid shadowing.The parameter name
singletonshadows the field name, makingthis.singleton.set(singleton)harder to read. Consider renaming the parameter tovalueorbeanfor clarity.🔎 Proposed refactor
- public void setSingleton(Object singleton) { - this.singleton.set(singleton); + public void setSingleton(Object value) { + this.singleton.set(value); }core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java (1)
213-216: Good addition of test cleanup.Proper teardown ensures the router is shut down after each test, preventing resource leaks and test interference.
Consider adding defensive exception handling to prevent teardown failures from masking test issues:
🔎 Optional: Add error handling
@AfterEach void tearDown() { - router.shutdown(); + try { + router.shutdown(); + } catch (Exception e) { + // Log but don't fail the test due to cleanup issues + System.err.println("Error during router shutdown: " + e.getMessage()); + } }core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (2)
30-30: Consider making therouterfieldvolatilefor thread-safety.The
routerfield is assigned instart(Router)(line 36) without synchronization and read in multiple methods with inconsistent locking. Ifstart(Router)can be called concurrently with other methods, visibility issues may arise.Making
routervolatileensures all threads see the updated value immediately.🔎 Proposed fix
- private Router router; + private volatile Router router;
40-57: Consider makingstartInternal()idempotent to avoid exceptions when re-enabling.Currently, calling
setEnabled(true)when hot deployment is already running will triggerstartInternal(), which throwsIllegalStateExceptionbecausehdt != null. This makessetEnabled(true)non-idempotent.For better usability, consider returning early instead of throwing when
hdtis already initialized.🔎 Proposed fix
private void startInternal() { // Prevent multiple threads from starting hot deployment at the same time. synchronized (lock) { if (hdt != null) - throw new IllegalStateException("Hot deployment already started."); + return; // Already started, no-op if (!(router.getBeanFactory() instanceof TrackingApplicationContext tac)) { log.warn(""" ApplicationContext is not a TrackingApplicationContext. Please set <router hotDeploy="false">. """); return; } hdt = new HotDeploymentThread(router.getRef()); hdt.setFiles(tac.getFiles()); hdt.start(); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/RuleReinitializer.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.javacore/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/Router.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.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 (4)
core/src/main/java/com/predic8/membrane/core/RuleReinitializer.java (1)
34-56: LGTM: Lifecycle methods properly address thread-safety concerns.The
start()method correctly prevents multiple concurrent timers with the idempotency check at line 36-37, andstop()properly nullifies the timer reference at line 55. Both methods are synchronized, ensuring thread-safe access to thetimerfield.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2AuthorizationServerInterceptorBase.java (2)
290-303: Exception handling change is consistent with refactored init() API.The removal of
throws ExceptionfrominitOasi()is correct: OAuth2AuthorizationServerInterceptor.init() and Router.init() no longer declare checked exceptions in their signatures, making the removal of the throws clause in the test method appropriate.
204-211: Router lifecycle management is correct.The setup follows an explicit create → start → initialize pattern. The
router.start()call beforeinitOasi()andinitMas()is intentional and consistent with other OAuth2 integration tests in the codebase. The addition oftearDown()withrouter.shutdown()provides proper cleanup after each test.core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
72-77: Good fix for the NPE concern, but note the idempotency issue.The addition of
router != nullcheck at line 73 properly addresses the past review comment about potential NPE whensetEnabled(true)is called beforestart(Router).However, as noted in the
startInternal()review, callingsetEnabled(true)when hot deployment is already running will throwIllegalStateExceptiondue to the non-idempotent behavior ofstartInternal().
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java
Show resolved
Hide resolved
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java
Show resolved
Hide resolved
… threads in `Router` to align with modern threading practices
… initialization logic for consistency
…ing, and fix `BeanContainer` string formatting
…into router-refactor-to-beanfactory
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 (4)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java (1)
41-46: Add @AfterEach cleanup method to prevent resource leaks.The
HttpRouterinstantiated insetUp()requires explicit cleanup. Router'sshutdown()method closes transport resources and shuts down timer managers that are allocated during instantiation. Add an@AfterEachmethod callingrouter.shutdown()(as shown inAbstractTestWithRouter).core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
59-79: Fix inaccurate comment on line 74 — OpenAPIInterceptor is added during Spring configuration parsing, not during init().The comment "// Got added" on line 74 is misleading. Test 1 (
interceptorSequenceFromSpringConfiguration) expects OpenAPIInterceptor at position 0 without callinginit(), indicating the interceptor is already added by Spring configuration parsing. Test 2 has identical assertions becauseinit()only adds missing interceptors viainitOpenAPI()(lines 126–131 of APIProxy.java), and in this case, the interceptor is already present from Spring config.Test 3 (
noPublisherNoOpenAPIInterceptor) correctly demonstrates init() behavior — it omits<openapiPublisher/>from the config, so init() adds OpenAPIPublisherInterceptor and the order changes. The "// Was added" comment on line 87 accurately reflects this.Update the comment on line 74 to clarify that the interceptor came from Spring configuration, not init(). Alternatively, if the comment is meant to highlight a specific aspect of the test, consider a more precise message or remove it entirely.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java (1)
94-98: Inconsistent Router usage detected.Line 96 still instantiates a plain
Router()while the rest of the test has migrated toHttpRouter. This inconsistency could cause issues if the refactoring requires all instances to useHttpRouter.🔎 Proposed fix
void initializeEmptyRuleApiSpecsTest() { ApiDocsInterceptor adi = new ApiDocsInterceptor(); - adi.init(new Router()); + adi.init(new HttpRouter()); assertEquals(new HashMap<>(), adi.initializeRuleApiSpecs()); }core/src/test/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptorTest.java (1)
168-173: Inconsistent Router usage detected.Line 172 still instantiates a plain
Router()while the rest of the test has migrated toHttpRouter(lines 60, 64, 70). This inconsistency should be corrected to maintain uniformity across the refactoring.🔎 Proposed fix
void handleDuplicateApiKeys() { var dupeStore = new ApiKeyFileStore(); dupeStore.setLocation(getKeyfilePath("apikeys/duplicate-api-keys.txt")); - assertThrows(RuntimeException.class, () -> dupeStore.init(new Router())); + assertThrows(RuntimeException.class, () -> dupeStore.init(new HttpRouter())); }
♻️ Duplicate comments (5)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
42-45: Previous review feedback addressed.The
toString()method now correctly usessingleton.get()to display the actual bean value instead of theAtomicReferencewrapper.core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
63-72: Add null checks before accessing router and reinitializer.Line 68 calls
router.getReinitializer().stop()without verifying thatrouteris not null. Sincestop()can be invoked beforestart()is called,routermay be null, resulting in an NPE. Additionally,getReinitializer()should be checked for null as a defensive measure.🔎 Proposed fix
@Override public void stop() { synchronized (lock) { if (hdt == null) return; + if (router == null) + return; + + var reinitializer = router.getReinitializer(); + if (reinitializer != null) + reinitializer.stop(); - router.getReinitializer().stop(); hdt.stopASAP(); hdt = null; } }core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (1)
76-82: Inconsistent request builder pattern (duplicate issue).Line 77 still uses the old
new Request.Builder().get("/foo")pattern instead of the staticget("/foo")import introduced at line 37 and used consistently at line 68. Both test methods should use the same pattern.🔎 Apply the previously suggested fix
- Exchange exc = new Request.Builder().get("/foo").header("Authorization", "bearer " + getSignedJwt(privateKey, getJwtClaimsArrayScopesList())).buildExchange(); + Exchange exc = get("/foo").header("Authorization", "bearer " + getSignedJwt(privateKey, getJwtClaimsArrayScopesList())).buildExchange();core/src/main/java/com/predic8/membrane/core/Router.java (2)
375-386: Unsynchronized read ofrunningfield persists.The
runningfield is@GuardedBy("lock")(line 135-136), but this method reads it at line 379 without synchronization. This was flagged in a previous review and remains unaddressed.🔎 Proposed fix
public void add(Proxy proxy) throws IOException { log.debug("Adding proxy {}.", proxy.getName()); RuleManager ruleManager = getRuleManager(); + boolean isRunning; + synchronized (lock) { + isRunning = running; + } - if (running && proxy instanceof SSLableProxy sp) { + if (isRunning && proxy instanceof SSLableProxy sp) { sp.init(this); ruleManager.addProxyAndOpenPortIfNew(sp, MANUAL); } else { ruleManager.addProxy(proxy, MANUAL); } }
546-549:BeanRegistryImplementationcreated with null parameters.This was flagged in a previous review. Creating
BeanRegistryImplementationwithnullfor bothobserverandgrammarparameters may causeNullPointerExceptionwhen the registry invokes methods on these parameters during bean lifecycle operations.🔎 Proposed fix
Consider providing a no-op observer or ensuring
BeanRegistryImplementationhandles null gracefully:public BeanRegistry getRegistry() { if (registry == null) - registry = new BeanRegistryImplementation(null, this, null); + registry = new BeanRegistryImplementation(BeanCacheObserver.NO_OP, this, null); return registry; }Or add null-safety in
BeanRegistryImplementationfor observer calls.
🧹 Nitpick comments (3)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
17-18: Consider using an explicit import.Wildcard imports can be replaced with explicit imports for clarity. Only
AtomicReferenceis used from this package.🔎 Suggested change
-import java.util.concurrent.atomic.*; +import java.util.concurrent.atomic.AtomicReference;core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
22-22: Consider using specific static import instead of wildcard.Wildcard static imports (
import static ... *) can reduce code clarity and increase the risk of naming conflicts. Since onlygetPathFromResourceis used in this file, prefer the specific import.🔎 Proposed fix
-import static com.predic8.membrane.test.TestUtil.*; +import static com.predic8.membrane.test.TestUtil.getPathFromResource;core/src/main/java/com/predic8/membrane/core/HttpRouter.java (1)
42-49: Consider addressing the TODO or creating a tracking issue.The TODO comment indicates this method is brittle and test-only. If this is technical debt, consider either documenting a plan to refactor it or creating an issue to track the improvement.
Would you like me to open an issue to track this technical debt?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.javacore/src/main/java/com/predic8/membrane/core/HttpRouter.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/interceptor/Interceptor.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.javacore/src/test/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStoreTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPFaultTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/AbstractProxySpringConfigurationTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPI31Test.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/XMembraneExtensionSecurityTest.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/AbstractSecurityValidatorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPI31Test.java
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/Router.java
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
TestUtil(29-61)
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-157)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
TestUtil(29-61)
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-157)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptorTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-369)
⏰ 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 (35)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
24-36: LGTM! Thread-safety improvement is correctly implemented.The migration from
volatile ObjecttoAtomicReference<Object>is sound. Thefinalmodifier ensures the reference itself cannot be reassigned, and the atomic operations (get()/set()) provide thread-safe access. Compound operations requiring atomicity are handled externally via synchronization inBeanRegistryImplementation.core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (4)
19-19: LGTM! Good thread-safety improvement.The addition of a dedicated lock object and updated @GuardedBy annotation improve thread-safety by preventing external synchronization interference.
Also applies to: 28-28, 33-33
36-39: LGTM! Clear lifecycle API.The method rename to
start()and @NotNull annotation improve API clarity and compile-time safety. Delegation tostartInternal()properly separates public API from internal implementation.
41-60: LGTM! Well-structured initialization.The lock-based synchronization and defensive check for repeated starts (lines 44-47) ensure thread-safe initialization. The logging approach is appropriate for this lifecycle method.
75-80: LGTM! NPE prevention addressed.The null check for
routeron line 76 properly addresses the previous concern about potential NPE whensetEnabled(true)is called beforestart(Router).core/src/test/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptorTest.java (1)
35-35: LGTM! Clean refactor aligning with the HttpRouter migration.The instantiation now uses
HttpRouterwhile keeping the field type asRouter, which follows good practice by programming to the interface. This change is consistent with the broader architectural refactoring described in the PR objectives.core/src/main/java/com/predic8/membrane/core/interceptor/Interceptor.java (1)
56-59: Add Javadoc for the new init method signatures in the interface.The two new init method overloads at lines 56-58 lack documentation explaining their purpose, when each is invoked, and the relationship between them. While
AbstractInterceptorprovides implementations, the interface contract should be explicitly documented (e.g., which overload is called during what lifecycle phase, and why theProxyparameter variant exists).core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java (5)
37-37: Static import added for fluent request building.The static import for
getenables cleaner test code. Ensure it's used consistently across all test methods.
44-46: Improved encapsulation and lifecycle management.The visibility changes to
privateand the introduction of instance fields forrouterandproxyproperly support per-test lifecycle management and improve encapsulation.
50-59: HttpRouter usage aligns with refactoring objectives.The switch to
HttpRouter(line 52) and per-test router lifecycle management properly support the bean-factory/registry refactoring described in the PR objectives. The initialization logic is clear and correct.
61-64: Resource cleanup added, resolving past review concern.The
@AfterEachtearDown method properly shuts down the router, preventing resource leaks. This directly addresses the major issue flagged in previous reviews.
67-73: Improved request building with fluent API.The test method correctly uses the new fluent request-building pattern
get("/foo").header(...), which is cleaner and more readable than the previousRequest.Builder()approach.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java (1)
48-48: LGTM: HttpRouter migration in test setup.The test setup correctly instantiates
HttpRouterinstead ofRouter, aligning with the PR's refactoring goals.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
33-33: LGTM: HttpRouter migration.The test correctly instantiates
HttpRouterin the setup, consistent with the refactoring objectives.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java (1)
42-42: LGTM: HttpRouter migration.The test correctly instantiates
HttpRouterin the setup.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/XMembraneExtensionSecurityTest.java (1)
35-35: LGTM: HttpRouter migration.The test correctly instantiates
HttpRouterin the setup.core/src/test/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStoreTest.java (1)
103-103: LGTM: HttpRouter migration in test helper.The helper method correctly instantiates
HttpRouterfor store initialization.core/src/test/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptorTest.java (2)
16-16: LGTM: Import adjustment for HttpRouter.The wildcard import now includes
HttpRouterused throughout the test setup.
60-70: LGTM: HttpRouter migration in test setup.The test setup correctly instantiates
HttpRouterinstances for store and extractor initialization.core/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.java (3)
31-31: LGTM: Static import for cleaner test code.Adding the static import for
Request.getimproves readability in the test methods.
46-46: LGTM: HttpRouter migration.The test correctly instantiates
HttpRouterin the setup.
68-68: LGTM: Simplified request construction.Using the static imported
get()method makes the test code more concise and readable.Also applies to: 76-76
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/AbstractProxySpringConfigurationTest.java (1)
39-41: Explicit init() call is necessary—Spring context is not started in this test.The
ctx.refresh()call prepares the Spring beans but does not start the ApplicationContext lifecycle. In production code (RouterBootstrap),bf.start()is called aftergetBean(), which triggersrouter.start(), which automatically callsinit()if not already initialized. Since this test uses onlyrefresh()withoutstart(), the explicitrouter.init()call is intentional and necessary. This pattern is consistent with the codebase: direct initialization paths (direct instantiation or refresh-only contexts) require explicitinit()calls, while managed contexts that callstart()rely on the router's lifecycle callbacks.core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AbstractSecurityValidatorTest.java (1)
44-48: LGTM!The change from
RoutertoHttpRouteraligns with the PR's migration pattern. The return type remainsRouter, preserving API compatibility while using the concreteHttpRouterimplementation.core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java (1)
44-44: LGTM!Consistent migration to
HttpRouteracross all test methods. The initialization pattern is appropriately applied.Also applies to: 53-53, 84-84
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptorTest.java (1)
58-64: LGTM!Good use of static import for
post()which improves test readability. The migration toHttpRouteris consistent with the broader refactoring effort while maintaining theRoutertype for the field.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPI31Test.java (1)
41-53: LGTM!Clean refactoring that:
- Uses the
Request.get()factory method for improved readability- Properly declares
URISyntaxExceptionin the method signature- Migrates to
HttpRouterconsistently with other test filescore/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPFaultTest.java (1)
35-35: LGTM!Consistent migration to
HttpRouterwhile preserving theRoutertype declaration for the field.core/src/main/java/com/predic8/membrane/core/HttpRouter.java (1)
27-40: LGTM!Clean constructor refactoring:
- Proper delegation pattern with
this(null)- Null-safe proxy configuration handling
- Overridden
getTransport()provides stronger typing by returningHttpTransportThe static
createTransport()method is appropriate since it doesn't require instance state.core/src/main/java/com/predic8/membrane/core/Router.java (6)
52-80: Useful architectural documentation.The ADR-style comments clearly document design decisions and open questions. Consider extracting these to a separate ADR document if the project maintains architecture decision records.
290-296: Addresses previous review feedback.The
getRuleManager()method now usesregisterIfAbsent, making the get-or-create pattern atomic and thread-safe. This properly addresses the race condition flagged in a previous review.
304-306: UsesgetRegistry()accessor correctly.This addresses a previous review comment about using
registrydirectly. The code now properly callsgetRegistry()which handles lazy initialization.
481-483: Addresses previous review feedback.The
getFlowController()method now usesregisterIfAbsent, ensuring only one instance is created. This properly addresses the concern about multiple instances being created on each call.
580-608: Static error handlers improve code organization.These methods centralize error handling for OpenAPI-related startup failures with clear, user-friendly error messages.
348-350: DNSCache registration is properly handled ininit(), supporting theorElseThrow()change.The verification confirms that
DNSCacheis registered in theinit()method at line 196 usingregisterIfAbsent(DNSCache.class, DNSCache::new). All call sites ofgetDnsCache()are in transport handlers, proxies, and interceptors that execute during request processing, after the Router is initialized. This initialization order is implicit but follows the standard pattern.The change from
orElse(new DNSCache())toorElseThrow()is appropriate and correctly prevents multiple cache instances without requiring explicit guarantees.
…consistency across core components
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 (2)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
68-79: Clarify the misleading test name.The test name
noPublisherNoOpenAPIInterceptorrefers to the XML configuration (absence of the<openapiPublisher/>tag), but the test validates that bothOpenAPIPublisherInterceptorandOpenAPIInterceptorare automatically added at runtime (lines 74-75). This creates confusion about what the test is actually validating.Rename to better reflect the behavior being tested, such as
bothInterceptorsAddedDuringExplicitInitWhenNoExplicitPublisherorautomaticInterceptorsAddedOnInit, to clarify that the test is checking automatic addition during the explicitap.init(router)call.core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyStore.java (1)
22-29: Javadoc referencesRouterbut parameter isIRouter.The Javadoc on line 23 mentions
{@link Router}while the actual parameter type is nowIRouter. Update the documentation to reflect the new type.🔎 Proposed fix
/** - * Lifecycle hook invoked once to provide the {@link Router} context. + * Lifecycle hook invoked once to provide the {@link IRouter} context. * Default is a no-op to preserve backward compatibility; implementations may override. * - * @param router non-null router instance + * @param router non-null IRouter instance */ default void init(IRouter router) { }
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/Router.java (2)
370-381: Unsynchronized read ofrunningfield persists.The
runningfield is marked@GuardedBy("lock")at line 134, but line 374 reads it without synchronization. This could lead to visibility issues in multi-threaded scenarios.🔎 Proposed fix
public void add(Proxy proxy) throws IOException { log.debug("Adding proxy {}.", proxy.getName()); RuleManager ruleManager = getRuleManager(); + boolean isRunning; + synchronized (lock) { + isRunning = running; + } - if (running && proxy instanceof SSLableProxy sp) { + if (isRunning && proxy instanceof SSLableProxy sp) { sp.init(this); ruleManager.addProxyAndOpenPortIfNew(sp, MANUAL); } else { ruleManager.addProxy(proxy, MANUAL); } }
541-545:BeanRegistryImplementationcreated with nullobserverandgrammarparameters.The
getRegistry()method creates aBeanRegistryImplementationwithnullfor bothobserverandgrammarparameters. According to the past review, this can causeNullPointerExceptionduring bean lifecycle operations when observer methods are invoked.🔎 Proposed fix
Consider providing a no-op observer or ensuring
BeanRegistryImplementationhandles null gracefully:public BeanRegistry getRegistry() { if (registry == null) - registry = new BeanRegistryImplementation(null, this, null); + registry = new BeanRegistryImplementation(BeanCacheObserver.NOOP, this, new GrammarAutoGenerated()); return registry; }Alternatively, add null checks in
BeanRegistryImplementationbefore invoking observer methods.#!/bin/bash # Check BeanRegistryImplementation for null handling of observer and grammar cat annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java | head -100
🧹 Nitpick comments (11)
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/RoundRobinStrategy.java (1)
19-19: Consider using explicit imports instead of wildcard imports.Wildcard imports can reduce code clarity and potentially cause naming conflicts. Since this change was likely made just to import
IRouter, an explicit import would be cleaner.🔎 Suggested change
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/main/java/com/predic8/membrane/core/interceptor/balancer/SessionIdExtractor.java (1)
17-17: Consider using specific import instead of wildcard.Wildcard imports reduce code clarity and can introduce ambiguity. Replace with a specific import for
IRouter.🔎 Proposed refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java (1)
48-48: LGTM! Router instantiation updated to HttpRouter.The change from
new Router()tonew HttpRouter()aligns with the PR's refactoring objectives. The field type at line 41 remainsRouter, which works correctly through polymorphism.Optional consideration: If the broader refactoring moves toward the
IRouterabstraction (as mentioned in the PR summary), you might consider updating the field type fromRoutertoIRouterfor consistency:- Router router; + IRouter router;However, this is purely optional and depends on whether test code is migrating to the interface-based type as well.
core/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.java (1)
20-20: Consider using explicit imports instead of wildcard.The wildcard import
com.predic8.membrane.core.*reduces code clarity and can lead to naming conflicts. Explicit imports make it clearer which classes are being used.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
22-22: Consider using a specific import instead of a wildcard.The wildcard import reduces clarity since only
getPathFromResourceis used in this file. Specific imports make dependencies more explicit and prevent potential naming conflicts.🔎 Proposed fix
-import static com.predic8.membrane.test.TestUtil.*; +import static com.predic8.membrane.test.TestUtil.getPathFromResource;core/src/main/java/com/predic8/membrane/core/exchangestore/ElasticSearchExchangeStore.java (1)
510-515: Improved error handling with defensive null check.The addition of
indexRes.has("error")check before accessing the error object is a good defensive improvement. However, the code still assumes that if "error" exists, it contains a "root_cause" array with at least one element. While the outertry-catchat line 509 will handle unexpected structures, consider further hardening:🔎 Optional: More defensive error structure checking
- if(indexRes.has("error") && indexRes.getJSONObject("error").getJSONArray("root_cause").getJSONObject(0).getString("type").equals("resource_already_exists_exception")){ + if(indexRes.has("error")) { + JSONObject error = indexRes.getJSONObject("error"); + if(error.has("root_cause") && error.getJSONArray("root_cause").length() > 0 + && error.getJSONArray("root_cause").getJSONObject(0).has("type") + && error.getJSONArray("root_cause").getJSONObject(0).getString("type").equals("resource_already_exists_exception")){ log.info("Index already exists skipping index creation"); + return; + } + log.error("Error happened. Reply from elastic search is below"); + log.error(indexRes.toString()); } else { - log.error("Error happened. Reply from elastic search is below"); - log.error(indexRes.toString()); + // Unexpected response structure + log.warn("Unexpected response structure from Elasticsearch: {}", indexRes); }core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExtractor.java (1)
16-16: Consider using explicit imports instead of wildcard.The wildcard import (
import com.predic8.membrane.core.*;) reduces code readability and can lead to namespace pollution. Explicit imports make dependencies clearer.🔎 Suggested refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.java (1)
19-19: Consider using explicit imports instead of wildcard.The wildcard import (
import com.predic8.membrane.core.*;) can obscure which specific classes are being used and may cause future compilation issues if new classes are added to the package.🔎 Suggested refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/TokenGenerator.java (1)
16-16: Consider using explicit imports instead of wildcard.Using explicit imports improves code clarity and prevents potential naming conflicts.
🔎 Suggested refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/main/java/com/predic8/membrane/core/interceptor/session/MemcachedSessionManager.java (1)
20-20: Consider using explicit imports instead of wildcard.Wildcard imports reduce code readability and can lead to maintenance issues. Use explicit imports to make dependencies clear.
🔎 Suggested refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (1)
18-18: Consider using explicit imports instead of wildcard.Replace the wildcard import with explicit imports to improve code clarity and maintainability.
🔎 Suggested refactor
-import com.predic8.membrane.core.*; +import com.predic8.membrane.core.IRouter;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (87)
core/src/main/java/com/predic8/membrane/core/IRouter.javacore/src/main/java/com/predic8/membrane/core/Router.javacore/src/main/java/com/predic8/membrane/core/exchangestore/AbstractPersistentExchangeStore.javacore/src/main/java/com/predic8/membrane/core/exchangestore/ElasticSearchExchangeStore.javacore/src/main/java/com/predic8/membrane/core/exchangestore/ExchangeStore.javacore/src/main/java/com/predic8/membrane/core/exchangestore/MongoDBExchangeStore.javacore/src/main/java/com/predic8/membrane/core/graphql/GraphQLoverHttpValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/ApisJsonInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/Interceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/AbstractClientAddress.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControl.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControlInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/Any.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/Hostname.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/Ip.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/Resource.javacore/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminPageBuilder.javacore/src/main/java/com/predic8/membrane/core/interceptor/administration/RuleUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/MongoDBApiKeyStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/CachingUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/CustomStatementJdbcUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/EmailTokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/EmptyTokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JwtSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LDAPUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/SessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TOTPTokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TelekomSMSTokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/UnifyingUserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/UserDataProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/WhateverMobileSMSTokenProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/ByThreadStrategy.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/DispatchingStrategy.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/PriorityStrategy.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/RoundRobinStrategy.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/SessionIdExtractor.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/faultmonitoring/FaultMonitoringStrategy.javacore/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.javacore/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/InterceptorContainer.javacore/src/main/java/com/predic8/membrane/core/interceptor/javascript/GraalVMJavascriptLanguageAdapter.javacore/src/main/java/com/predic8/membrane/core/interceptor/javascript/LanguageAdapter.javacore/src/main/java/com/predic8/membrane/core/interceptor/javascript/RhinoJavascriptLanguageAdapter.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ClaimList.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ClientList.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ConsentPageFile.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/StaticClientList.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/DynamicRegistration.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/TokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/SessionAuthorizer.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/SchematronValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/InMemorySessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/MemcachedSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/RedisSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/SessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.javacore/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.javacore/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisher.javacore/src/main/java/com/predic8/membrane/core/transport/ws/WebSocketInterceptorInterface.javacore/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketLogInterceptor.javacore/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketSpringInterceptor.javacore/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketStompReassembler.javacore/src/test/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/session/FakeSyncSessionStoreManager.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.javadistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/Loadbalancing4XmlSessionExampleTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptorTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/Interceptor.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-15T18:38:23.728Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2322
File: core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java:18-26
Timestamp: 2025-11-15T18:38:23.728Z
Learning: In the membrane/api-gateway repository, the maintainer predic8 prefers fail-fast behavior where NullPointerExceptions are allowed to crash rather than being caught or having defensive null checks that silently return null, as it makes debugging easier by showing where problems originate.
Applied to files:
core/src/main/java/com/predic8/membrane/core/Router.java
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
TestUtil(29-61)
core/src/main/java/com/predic8/membrane/core/Router.java (5)
core/src/main/java/com/predic8/membrane/core/RuleManager.java (1)
RuleManager(35-344)core/src/main/java/com/predic8/membrane/core/util/DLPUtil.java (1)
DLPUtil(26-78)core/src/main/java/com/predic8/membrane/core/util/DNSCache.java (1)
DNSCache(27-78)core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
KubernetesWatcher(37-132)core/src/main/java/com/predic8/membrane/core/proxies/ApiInfo.java (1)
ApiInfo(27-86)
⏰ 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 (102)
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/RoundRobinStrategy.java (1)
74-76: LGTM!The signature change from
RoutertoIRoutercorrectly aligns with the PR's migration to the interface abstraction, improving testability and adhering to the dependency inversion principle. The empty implementation is appropriate since this strategy doesn't require router access.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/SessionIdExtractor.java (1)
23-24: LGTM! Interface correctly migrated to IRouter.The method signature change from
RoutertoIRouteraligns with the PR's refactoring objectives. SinceRouterimplementsIRouter, this maintains backward compatibility while moving to the abstraction layer.core/src/main/java/com/predic8/membrane/core/interceptor/ApisJsonInterceptor.java (1)
73-73: LGTM! Clean migration to IRouter abstraction.The parameter type change from
RoutertoIRouteraligns perfectly with the PR's repository-wide refactor objectives. The method body requires no changes since it only uses methods available on theIRouterinterface (getRuleManager(),getRules(), etc.).distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/Loadbalancing4XmlSessionExampleTest.java (3)
27-30: LGTM! Good refactoring to extract constants.Extracting the JSON body strings into named constants eliminates duplication and improves readability. The constant names clearly convey their purpose in the session-based load balancing test.
38-38: LGTM! Appropriate visibility for JUnit 5.The change from
publicto package-private visibility is correct for JUnit 5 tests, which do not require public access modifiers.
41-102: LGTM! Consistent usage of constants throughout the test.All inline JSON bodies have been correctly replaced with the appropriate constants (BODY_SESSION_1 and BODY_SESSION_2). The test logic remains functionally equivalent while being more maintainable.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java (1)
96-96: LGTM! Consistent instantiation change.The change to
new HttpRouter()is consistent with the setUp method change at line 48 and properly supports the isolated test scenario for empty rule specs.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ConsentPageFile.java (1)
44-51: Signature change correctly migrates parameter to IRouter interface.The parameter type change from
RoutertoIRouteris consistent with the PR's Router→IRouter migration. The implementation correctly usesIRoutermethods (getResolverMap(),getBaseLocation()), the logic remains unchanged, and call sites are already compatible since AbstractInterceptor inherited the IRouter-typed router field.core/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.java (1)
70-70: [Rewritten review comment]
[Classification tag]core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxySpringConfigurationTest.java (1)
55-66: LGTM!The test correctly validates the new bootstrap-driven initialization flow. The comment on line 60 helpfully clarifies that OpenAPIInterceptor is added automatically during bootstrap initialization.
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
44-44: LGTM!Clean parameter type migration to
IRouter. The method correctly delegates to interface methods (getBeanFactory(),getUriFactory()), maintaining the same functionality.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/ByThreadStrategy.java (1)
125-127: LGTM!The
init(IRouter router)signature aligns with theDispatchingStrategyinterface migration. No behavioral change since the method body is a no-op.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TelekomSMSTokenProvider.java (1)
98-100: LGTM!The parameter type migration to
IRouteris consistent with the broader refactor. The method continues to correctly obtain the HTTP client viarouter.getHttpClientFactory().core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStore.java (1)
62-68: LGTM!The
init(IRouter router)implementation correctly aligns with the updatedApiKeyStoreinterface. File reading logic usingrouter.getResolverMap()androuter.getBaseLocation()remains intact.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/DispatchingStrategy.java (1)
22-22: LGTM!The interface method signature update to
init(IRouter router)establishes the new contract for allDispatchingStrategyimplementations. This is the foundation for the consistent migration across implementing classes.core/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketSpringInterceptor.java (1)
58-60: LGTM!The
init(IRouter router)signature aligns with theWebSocketInterceptorInterfacemigration. The delegation toi.init(router)correctly propagates the router context to the referenced interceptor.core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/MongoDBApiKeyStore.java (1)
61-70: LGTM!The
init(IRouter router)implementation aligns with the updatedApiKeyStoreinterface. The router parameter is unused here since MongoDB initialization only requires the connection string, but this is acceptable for interface compliance.core/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.java (1)
63-70: LGTM! Clean migration to IRouter.The signature update from
RoutertoIRouteris consistent with the broader refactor. The wiring logic forProxyAwareinstances and delegation toinit(router)remains correct.core/src/main/java/com/predic8/membrane/core/interceptor/acl/Ip.java (1)
28-30: LGTM! Constructor signature correctly updated.The migration from
RoutertoIRouteris straightforward and the delegation to the parent constructor remains correct.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/SessionAuthorizer.java (1)
49-64: LGTM! Field and method signature correctly migrated.The change from
RoutertoIRouteris consistent throughout—field declaration, parameter, assignment, and usage (line 107) all align properly.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
37-53: LGTM! Field and method signature correctly updated.The migration from
RoutertoIRouteris straightforward and correct. The usage ofrouter.getBeanFactory()on line 82 is consistent with theIRouterinterface.core/src/main/java/com/predic8/membrane/core/interceptor/acl/AbstractClientAddress.java (2)
23-29: LGTM! Field and constructor correctly migrated.The change from
RoutertoIRouterfor thefinalfield is correct, and the constructor properly initializes it.
47-47: LGTM! Hook method signature correctly updated.The empty
initmethod serves as an optional hook for subclasses. The signature update toIRouteris consistent with the broader migration.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/DynamicRegistration.java (1)
44-51: IRouter interface correctly exposes all required methods.Verification confirms that
IRouterprovidesgetResolverMap(),getBaseLocation(),getHttpClientFactory(), andgetFlowController()at the locations used in DynamicRegistration (lines 47, 50, 76, 82). The migration is correct.core/src/main/java/com/predic8/membrane/core/interceptor/javascript/LanguageAdapter.java (1)
36-43: LGTM! Constructor parameter correctly migrated.Both
GraalVMJavascriptLanguageAdapter(line 28) andRhinoJavascriptLanguageAdapter(line 27) have constructors acceptingIRouterand properly callsuper(router), confirming the migration is complete and compatible.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerUtil.java (1)
25-25: LGTM! Clean migration to IRouter abstraction.All public method signatures consistently updated from
RoutertoIRouterparameter type. This refactoring improves modularity and aligns with interface-based programming practices.Also applies to: 37-37, 49-49, 61-61, 73-73, 85-85, 89-89, 93-93, 97-97, 101-101, 105-105, 109-109, 113-113, 117-117, 121-121
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/CustomStatementJdbcUserDataProvider.java (1)
18-18: LGTM! Consistent migration to IRouter.The import, field type, and init method signature cleanly migrated from
RoutertoIRouter. The usage pattern (accessingrouter.getBeanFactory()) remains unchanged.Also applies to: 33-33, 43-43
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.java (1)
36-36: LGTM! Field type updated to IRouter.The router field type cleanly migrated to
IRouter, maintaining immutability with thefinalmodifier. Constructor initialization and usage remain consistent.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/UnifyingUserDataProvider.java (1)
26-26: LGTM! Init signature updated to IRouter.The import and init method signature cleanly migrated to use
IRouter, properly delegating to nested user data providers.Also applies to: 66-66
core/src/main/java/com/predic8/membrane/core/interceptor/session/InMemorySessionManager.java (1)
41-41: LGTM! Init method signature updated to IRouter.Clean parameter type migration from
RoutertoIRouterwith no behavioral changes.core/src/main/java/com/predic8/membrane/core/interceptor/acl/Any.java (1)
17-17: LGTM! Constructor updated to accept IRouter.The import and constructor signature cleanly migrated to
IRouter, delegating properly to the parent class.Also applies to: 23-23
core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
39-39: LGTM! Init method updated to IRouter.The method signature cleanly migrated to accept
IRouter, properly passing it to theInterceptorAdapterconstructor.core/src/main/java/com/predic8/membrane/core/interceptor/administration/RuleUtil.java (1)
25-29: LGTM! Method signature updated to IRouter with explicit null return.The parameter type cleanly migrated to
IRouter. The explicitreturn null;at line 29 improves code clarity for the not-found case.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java (1)
34-38: LGTM! Clean migration to IRouter abstraction.The method signature correctly adopts the
IRouterinterface while preserving all existing logic. This aligns with the broader architectural refactoring described in the PR objectives.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/UserDataProvider.java (1)
22-22: LGTM! Interface contract updated to IRouter.The interface method signature correctly migrates to the
IRouterabstraction, maintaining consistency with the project-wide refactoring pattern.core/src/test/java/com/predic8/membrane/core/interceptor/session/FakeSyncSessionStoreManager.java (2)
16-16: Wildcard import adopted.The import statement changed from a specific
Routerimport to a wildcard import. This is a minor style change that accommodates theIRouterinterface.
30-30: LGTM! Test utility aligned with IRouter migration.The method signature correctly adopts the
IRouterinterface, maintaining consistency with the broader refactoring.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PriorityStrategy.java (1)
45-45: LGTM! Signature updated to IRouter.The method signature correctly migrates to the
IRouterabstraction. The no-op implementation suggests this strategy requires no router-specific initialization.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TokenProvider.java (2)
18-18: Wildcard import adopted.The import statement changed to accommodate the
IRouterinterface, consistent with the broader refactoring pattern.
22-24: LGTM! Interface contract migrated to IRouter.The
initmethod signature correctly adopts theIRouterabstraction. Other interface methods remain unchanged, preserving the existing contract for token operations.core/src/main/java/com/predic8/membrane/core/interceptor/session/RedisSessionManager.java (1)
50-52: LGTM! Init method signature updated to IRouter.The method signature correctly migrates to the
IRouterabstraction while preserving the no-op implementation. This aligns with the project-wide refactoring pattern.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/EmailTokenProvider.java (1)
74-75: LGTM! Signature migrated to IRouter.The method signature correctly adopts the
IRouterabstraction. The empty implementation indicates this provider requires no router-specific initialization.core/src/main/java/com/predic8/membrane/core/interceptor/javascript/RhinoJavascriptLanguageAdapter.java (1)
27-30: LGTM! Constructor parameter migrated to IRouter.The constructor signature correctly adopts the
IRouterabstraction and properly passes it to the parent class. The router field usage on Line 34 (router.isProduction()) confirms thatIRouterprovides the necessary interface methods.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerTokenGenerator.java (1)
17-17: LGTM!The import and
initmethod signature updates correctly align with theIRouterabstraction migration. Since theinitbody is a no-op, this change is purely mechanical with no functional impact.Also applies to: 68-70
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/StaticClientList.java (1)
18-18: LGTM!The
initsignature update toIRouteris consistent with theClientListinterface migration. The router parameter remains unused in this implementation, so no behavioral change.Also applies to: 32-34
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ClientList.java (1)
16-16: LGTM!The interface method signature update to
IRouterestablishes the contract that allClientListimplementations must follow. This is consistent with the broader migration pattern.Also applies to: 21-24
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/TOTPTokenProvider.java (1)
20-20: LGTM!The
initsignature update toIRouteris consistent with the migration. The no-op implementation means no behavioral change.Also applies to: 53-55
core/src/main/java/com/predic8/membrane/core/exchangestore/AbstractPersistentExchangeStore.java (1)
45-75: LGTM!The
initsignature update toIRouteris consistent with the migration, andsuper.init(router)correctly propagates the interface type to the parent class.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/CachingUserDataProvider.java (1)
22-22: LGTM!The
initsignature update toIRoutercorrectly propagates the interface type to the wrappeduserDataProvider. Cache initialization remains unchanged.Also applies to: 80-86
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ClaimList.java (1)
19-19: LGTM!The
initsignature update toIRouteraligns with the migration. The router parameter is unused here (init only triggers scope re-initialization), which is acceptable.Also applies to: 82-84
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.java (1)
87-102: The signature change toIRouteris correct and properly abstracted.All required methods are exposed by the
IRouterinterface:getUriFactory()(line 54),getResolverMap()(line 48), andgetBaseLocation()(line 46). The method body's usage is consistent with existing patterns throughout the codebase.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.java (1)
18-18: LGTM: Signature migrated to IRouter.The migration from
RoutertoIRouteraligns with the project-wide refactoring objective and maintains existing functionality.Also applies to: 53-53
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JwtSessionManager.java (1)
18-18: LGTM: Signature migrated to IRouter.The change is consistent with the IRouter abstraction migration and correctly passes the router to the parent class.
Also applies to: 59-59
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LDAPUserDataProvider.java (1)
39-39: LGTM: Signature migrated to IRouter.The migration is straightforward and maintains compatibility with the existing
getResolverMap()andgetBaseLocation()usage.Also applies to: 432-432
core/src/main/java/com/predic8/membrane/core/exchangestore/MongoDBExchangeStore.java (1)
25-25: LGTM: Signature migrated to IRouter.The change aligns with the broader migration to IRouter and correctly delegates to the parent class.
Also applies to: 54-54
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/faultmonitoring/FaultMonitoringStrategy.java (1)
19-19: LGTM: Signature migrated to IRouter with @OverRide annotation.Good addition of the
@Overrideannotation, which improves compile-time safety and makes the contract explicit.Also applies to: 125-126
core/src/main/java/com/predic8/membrane/core/transport/ws/WebSocketInterceptorInterface.java (1)
17-17: LGTM: Interface signature migrated to IRouter.The interface change is consistent with the project-wide IRouter migration. All implementations should be updated accordingly.
Also applies to: 24-24
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
17-17: LGTM: Signature migrated to IRouter.The change is straightforward and maintains compatibility with
InterceptorAdapterusage.Also applies to: 59-59
core/src/main/java/com/predic8/membrane/core/exchangestore/ElasticSearchExchangeStore.java (1)
79-79: LGTM: Signature migrated to IRouter.The init signature change aligns with the project-wide IRouter migration.
core/src/main/java/com/predic8/membrane/core/exchangestore/ExchangeStore.java (1)
60-60: LGTM! Clean interface migration to IRouter.The default method signature update from
RoutertoIRouteraligns well with the PR's objective to introduce a router abstraction layer. This change maintains backward compatibility through the default implementation while enabling future implementations to work with the IRouter interface.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java (1)
18-19: LGTM! Signature updated to IRouter abstraction.The import and method signature changes correctly align with the IRouter migration. The method implementation doesn't actually use the router parameter, so this is a safe type-level change.
Also applies to: 112-126
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/WhateverMobileSMSTokenProvider.java (1)
18-18: LGTM! IRouter migration with actual router usage.The signature change to
IRouteris correct. The method usesrouter.getHttpClientFactory(), which should be part of the IRouter interface contract. This change maintains the existing behavior while working with the new abstraction.Also applies to: 72-74
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
20-20: LGTM! Clean signature migration to IRouter.The import and signature updates align with the IRouter abstraction. The router parameter is not used in the method body, making this a straightforward type-level change.
Also applies to: 194-197
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/SessionManager.java (1)
64-67: LGTM! IRouter parameter type updated.The signature change from
RoutertoIRouteris consistent with the broader refactoring effort. The router parameter is not used in the method body, so this is a safe change.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/EmptyTokenProvider.java (1)
20-20: LGTM! No-op implementation correctly updated.The signature change to
IRouteris appropriate. Since the method body is a no-op, this change is entirely safe.Also applies to: 26-28
core/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketLogInterceptor.java (1)
19-19: LGTM! Interface update with empty implementation.The signature change to
IRouteraligns with the refactoring objectives. The empty method body makes this change risk-free.Also applies to: 40-42
core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.java (1)
35-50: LGTM! Constructor updated to use IRouter abstraction.The constructor parameter change from
RoutertoIRouteris correct. The implementation usesrouter.getResolverMap()androuter.getBaseLocation(), which should be part of the IRouter interface contract. The logic remains unchanged—these methods are called both in the main thread (line 41) and in the virtual thread (line 45) to create transformers.core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExtractor.java (1)
22-22: LGTM: Clean migration to IRouter abstraction.The change from
RoutertoIRouteraligns with the dependency inversion principle and improves testability and decoupling across the codebase.core/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.java (1)
62-83: LGTM: IRouter migration properly implemented.The method signature update to use
IRouteris well-integrated, and all router method calls (getResolverMap(), getBaseLocation()) are compatible with the IRouter abstraction.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/TokenGenerator.java (1)
22-22: LGTM: Interface updated to use IRouter.The migration to
IRouterin the interface definition is consistent with the broader refactoring and improves the abstraction layer.core/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControlInterceptor.java (1)
132-162: LGTM: IRouter parameter properly utilized.The parse method signature update to use
IRouteris well-integrated. All router method invocations (getResolverMap(), getBaseLocation()) are compatible with the IRouter abstraction.core/src/main/java/com/predic8/membrane/core/interceptor/session/MemcachedSessionManager.java (1)
46-48: LGTM: Clean IRouter migration.The method signature update is straightforward and consistent with the broader refactoring effort.
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/SchematronValidator.java (1)
57-88: LGTM: Constructor updated to use IRouter.The constructor parameter change from
RoutertoIRouteris well-integrated. The router.getResolverMap() call at line 73 is compatible with the IRouter interface, and the refactoring maintains the existing functionality.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisher.java (4)
90-106: LGTM: Consistent IRouter migration across methods.All method signatures have been consistently updated to use
IRouterinstead ofRouter. The router.getUriFactory() calls are compatible with the IRouter interface.
119-122: LGTM: returnHtmlOverview updated correctly.The method signature change to
IRouteris consistent with the class-wide refactoring.
136-141: LGTM: returnOpenApiAsYaml properly refactored.The IRouter parameter is correctly used to access getUriFactory() at line 138.
147-154: LGTM: renderOverviewTemplate updated correctly.The IRouter parameter is properly utilized to access router.getUriFactory() at line 152.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
72-72: LGTM: Field type updated to IRouter.The field type change from
RoutertoIRouterfollows the dependency inversion principle and improves testability.
94-109: LGTM: init method properly migrated to IRouter.The method signature update and all router method invocations (getResolverMap(), getBaseLocation(), getHttpClientFactory()) are compatible with the IRouter interface. The refactoring maintains existing functionality while improving abstraction.
core/src/main/java/com/predic8/membrane/core/transport/ws/interceptors/WebSocketStompReassembler.java (1)
42-45: LGTM!The
initmethod signature correctly updated to acceptIRouter, aligning with the project-wide migration to theIRouterabstraction. The delegation to contained interceptors remains unchanged.core/src/main/java/com/predic8/membrane/core/interceptor/javascript/GraalVMJavascriptLanguageAdapter.java (1)
28-31: LGTM!Constructor signature correctly updated to accept
IRouter, consistent with the parent classLanguageAdapterand the broader IRouter migration.core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.java (1)
34-34: LGTM!The
AbstractInterceptorbase class correctly migrated to useIRouter:
- Field type updated to
IRouter- Both
initmethod overloads acceptIRoutergetRouter()returnsIRouterThis foundational change properly enables the interface-based routing abstraction throughout the interceptor hierarchy.
Also applies to: 102-114
core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/InterceptorContainer.java (1)
31-49: LGTM!Method signatures correctly updated to use
IRouter. ThehandleInvocationProblemDetailsmethod now properly callsbuildAndSetResponse(exc)(line 48), ensuring that error responses are constructed and set on the exchange when plugin invocation fails. This is a good improvement over potentially leaving the exchange in an incomplete state.core/src/main/java/com/predic8/membrane/core/interceptor/acl/Resource.java (1)
39-46: LGTM!The
Resourceclass correctly migrated to useIRouterfor the field, constructor, andinitmethod. The child ACL address components (Ip,Hostname,Any) are properly wired with theIRouterinstance.Also applies to: 100-103
core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java (1)
77-84: LGTM!Both
InterceptorAdapterconstructors correctly updated to acceptIRouter, consistent with the parentAbstractInterceptorclass changes.core/src/main/java/com/predic8/membrane/core/interceptor/acl/Hostname.java (1)
59-61: LGTM!Constructor and
initmethod correctly updated to useIRouter. The added null-safety check on line 107 (router.getTransport() != null &&) is a good defensive improvement that prevents potential NPE when the transport is not configured.Also applies to: 105-108
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java (2)
102-107: LGTM!The
XenSessionManagerinterface correctly updated to acceptIRouterin theinitmethod signature, aligning with the project-wide abstraction migration.
114-115: LGTM!Both
InMemorySessionManagerandJwtSessionManagerimplementations correctly updated to acceptIRouter. TheJwtSessionManager.initproperly accessesIRoutermethods (getResolverMap(),getBaseLocation()) for JWK resolution.Also applies to: 143-149
core/src/main/java/com/predic8/membrane/core/graphql/GraphQLoverHttpValidator.java (1)
61-72: LGTM! Clean migration to IRouter interface.The field and constructor parameter types are correctly updated from
RoutertoIRouter. The only usage at line 245 (router.getUriFactory()) is properly supported by theIRouterinterface.core/src/main/java/com/predic8/membrane/core/interceptor/administration/AdminPageBuilder.java (1)
56-74: LGTM! Consistent migration to IRouter interface.The field and constructor parameter are correctly updated to use
IRouter. All router method calls throughout the file (getTransport(),getRuleManager(),getStatistics()) are properly defined in theIRouterinterface.core/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControl.java (2)
21-34: LGTM! Consistent migration to IRouter interface.The import, field, and constructor are correctly updated to use
IRouter. TheResourceclass (referenced at line 43) is also part of this migration as noted in the PR summary.
64-67: LGTM! Init method signature updated consistently.The
initmethod parameter type is correctly changed toIRouter, maintaining consistency with the field and constructor changes.core/src/main/java/com/predic8/membrane/core/IRouter.java (1)
29-69: Well-designed interface for router abstraction.The
IRouterinterface provides a clean abstraction over the router functionality, enabling better testability and decoupling. The API surface covers all essential router operations.A few observations:
- The TODO at lines 34-35 about
shutdown()vsstop()distinction is worth documenting in an ADR- The TODO at line 66 about moving
getRules()toRuleManageris a reasonable consideration for future refactoringcore/src/main/java/com/predic8/membrane/core/Router.java (9)
103-105: LGTM! Router now implements IRouter interface.The class declaration correctly implements
IRouterand the logger is updated to use the class reference.
136-137: Good addition ofinitializedguard with proper synchronization annotation.The new
@GuardedBy("lock")annotation oninitializedis consistent with the existingrunningfield pattern.
168-205: Well-structured initialization with registry-based component provisioning.The
init()method properly usesregisterIfAbsentfor thread-safe singleton registration of components. The guard against double initialization is appropriate.
285-291: Good fix:getRuleManager()now uses atomicregisterIfAbsentpattern.This addresses the previously identified race condition with the check-then-act pattern.
299-301: UsesgetRegistry()accessor instead ofregistryfield directly.This is correct - the code calls
getRegistry()which handles lazy initialization, addressing the concern from the past review.
476-478: Good fix:getFlowController()now uses atomicregisterIfAbsentpattern.This addresses the previously identified issue where a new
FlowControllerwas created on every call when absent from registry.
575-603: Good error handling helpers for OpenAPI configuration errors.The helper methods
handleOpenAPIParsingExceptionandhandleDuplicateOpenAPIPathsprovide clear, user-friendly error messages with formatting. The use ofSystem.erris appropriate for configuration errors, and throwingExitExceptionensures the application terminates cleanly.
571-573: New public accessor forRuleReinitializer.The
getReinitializer()method exposes the reinitializer instance. Note thatreinitializeris only set at the end ofinit()(line 204), so callers must ensureinit()has been called first, or handle a potentialnullreturn.
343-345: The implementation is correct and safe—no defensive approach is needed.All calls to
getDnsCache()occur in request/message handling code that executes only afterRouter.start()completes, which guaranteesinit()has run andDNSCacheis registered. TheorElseThrow()call is appropriate.
core/src/main/java/com/predic8/membrane/core/interceptor/session/SessionManager.java
Outdated
Show resolved
Hide resolved
…e `Router` with `IRouter` or specialized implementations, and align tests accordingly
… specialized implementations, and align related components and tests
…)` for improved naming consistency across core components and tests
…uce `DefaultMainComponents` to streamline router initialization and component management
…mplementation`, enhance initialization/logging in routers, and remove unused/deprecated methods
…tability under high load
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java
Show resolved
Hide resolved
rrayst
left a comment
rrayst
left a comment
There was a problem hiding this comment.
Approved. Before merging, we should check the test results.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Breaking
✏️ Tip: You can customize this high-level summary in your review settings.