Router: Refactoring move baselocation and isProduction to configuration #2525
Conversation
…N` with static import and minor cleanup in methods
📝 WalkthroughWalkthroughMoves baseLocation storage into Router.Configuration (adds Configuration.baseLocation with accessors), removes Router.getBaseLocation()/setBaseLocation(), updates call sites to use router.getConfiguration().getBaseLocation()/setBaseLocation(), and refactors JMX exporter/router JMX wiring and lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Router
participant JmxExporter
participant MBeanExporter
note right of Router: router.start() triggers JMX wiring
Router->>JmxExporter: JmxExporter.start(this)
alt exporter instance not yet created
JmxExporter->>MBeanExporter: create & configure exporter (assembler, policy)
MBeanExporter-->>JmxExporter: exporter ready
end
Router->>JmxExporter: addRouter(this)
JmxExporter->>JmxExporter: create namespaced JmxRouter and collect jmxBeans
JmxExporter->>MBeanExporter: setBeans(jmxBeans) and register (IGNORE_EXISTING)
MBeanExporter-->>JmxExporter: registration result
note right of JmxExporter: initAfterBeansAdded applies beans when registry populated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (3)
core/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.java (1)
27-27: Consider using a specific import instead of wildcard.Since only
get()is used in this test, a specific import would be clearer:-import static com.predic8.membrane.core.http.Request.*; +import static com.predic8.membrane.core.http.Request.get;core/src/main/java/com/predic8/membrane/core/router/Configuration.java (1)
45-45: LGTM - Consider enhancing javadoc.The addition of
baseLocationto Configuration is well-structured and follows existing patterns in the class. The field and accessors are properly annotated with@MCAttributefor configuration support.However, the javadoc at line 149 could be more descriptive:
📝 Suggested javadoc improvement
/** * @description Base location for the router's configuration file. - * @param baseLocation + * @param baseLocation The base directory path used to resolve relative file paths in configuration */ @MCAttribute() public void setBaseLocation(String baseLocation) {Also applies to: 147-158
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.java (1)
16-32: Optional: Import reorganization to wildcards.The imports have been reorganized to use wildcard imports. While this reduces line count, it can make it less obvious which specific classes are being used and may import unnecessary classes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControlInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStore.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/xen/XenAuthenticationInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ConsentPageFile.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/authorizationservice/MembraneAuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MicrosoftEntraIDAuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/stomp/STOMPClient.javacore/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.javacore/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SSLProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/SSLableProxy.javacore/src/main/java/com/predic8/membrane/core/router/Configuration.javacore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.javacore/src/main/java/com/predic8/membrane/core/router/Router.javacore/src/main/java/com/predic8/membrane/core/sslinterceptor/RouterIpResolverInterceptor.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/RewriteTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/XMembraneExtensionSecurityTest.javacore/src/test/java/com/predic8/membrane/core/router/DummyTestRouter.javacore/src/test/java/com/predic8/membrane/core/router/TestRouter.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.javacore/src/test/java/com/predic8/membrane/core/security/KeyStoreUtilTest.javacore/src/test/java/com/predic8/membrane/core/transport/ssl/SSLContextTest.javacore/src/test/java/com/predic8/membrane/core/transport/ssl/acme/AcmeStepTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java
💤 Files with no reviewable changes (3)
- core/src/test/java/com/predic8/membrane/core/router/TestRouter.java
- core/src/main/java/com/predic8/membrane/core/router/Router.java
- core/src/test/java/com/predic8/membrane/core/router/DummyTestRouter.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/JWSSigner.java (1)
JWSSigner(26-53)core/src/main/java/com/predic8/membrane/core/transport/ssl/PEMSupport.java (1)
PEMSupport(41-170)
core/src/test/java/com/predic8/membrane/core/transport/ssl/acme/AcmeStepTest.java (1)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-208)
⏰ 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 (51)
core/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.java (1)
53-55: Good refactoring to use DummyTestRouter.Replacing the mocked router with
DummyTestRouteraligns well with the PR's goal of centralizing configuration and provides a more realistic test setup.DummyTestRouterproperly initializes Configuration and all required dependencies forJwtAuthInterceptor.init()to execute successfully.core/src/test/java/com/predic8/membrane/core/security/KeyStoreUtilTest.java (1)
51-51: LGTM! Refactoring correctly applied.The base location access has been consistently updated to use
router.getConfiguration().getBaseLocation()across all test methods. The changes align with the PR objective of moving base location to the configuration object.Also applies to: 64-64, 82-82
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.java (1)
172-172: LGTM! Path resolution correctly updated.Both path resolution methods now correctly access base location through the configuration object. The refactoring maintains existing functionality while improving architectural consistency.
Also applies to: 204-204
core/src/main/java/com/predic8/membrane/core/interceptor/acl/AccessControlInterceptor.java (1)
140-140: LGTM! ACL file resolution correctly refactored.The ACL file path resolution has been properly updated to use the configuration-based base location. The change maintains existing behavior with improved architectural consistency.
core/src/main/java/com/predic8/membrane/core/proxies/SSLProxy.java (1)
332-332: LGTM! SSL context initialization correctly updated.The base location parameter for SSL context initialization has been properly refactored to use the configuration object. This maintains correct SSL certificate and key file resolution while improving code consistency.
core/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.java (1)
76-76: LGTM! JWK file resolution correctly refactored.The JWK file path resolution has been properly updated to access base location through the configuration object. The change maintains existing JWT session management behavior while following the new architectural pattern.
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
38-38: Refactoring looks correct - ensure Configuration is always initialized.The change correctly updates base location access to use the configuration object, consistent with the PR's refactoring objective. This is part of a systematic update across the codebase.
As noted in other files, please ensure
router.getConfiguration()is guaranteed to return a non-null Configuration object during theinit()lifecycle phase. The same verification script from the STOMPClient review applies here.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.java (1)
83-83: Consistent refactoring - ensure null safety.The change correctly migrates base location access to the configuration object, consistent with the systematic refactoring across the codebase.
As with the other files in this PR, verify that
router.getConfiguration()always returns a non-null Configuration during initialization (see verification scripts in STOMPClient review).core/src/main/java/com/predic8/membrane/core/interceptor/stomp/STOMPClient.java (1)
104-104: The refactoring is safe;router.getConfiguration()cannot return null.The codebase consistently uses
router.getConfiguration().getBaseLocation()without null checks across dozens of locations (SSLProxy, RouterIpResolverInterceptor, SSLableProxy, SOAPProxy, WebServerInterceptor, etc.). The DefaultRouter implementation always returns a non-nullconfigurationfield set during initialization. The Router interface has no @nullable annotation, confirming getConfiguration() is guaranteed to return a non-null Configuration object.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
136-138: Remove unnecessary null check - Configuration is always initialized.The
router.getConfiguration()call cannot return null. Configuration is initialized in the DefaultRouter constructor and the getter simply returns the initialized field. Throughout the codebase,getConfiguration()is called without null checks in hundreds of locations, demonstrating the established convention that it is always non-null when the router is not null.The current null safety check is correct as-is and requires no changes.
core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java (2)
189-189: LGTM!The base location is now correctly sourced from the configuration object when constructing the WSDL resource path.
212-212: LGTM!The base location is now correctly sourced from the configuration object when creating the
RelativePathRewriter.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LDAPUserDataProvider.java (1)
445-445: LGTM!The SSL context initialization now correctly uses the configuration-based base location, consistent with the refactoring pattern applied across the codebase.
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
179-183: LGTM!The WSDL parser context now correctly uses the configuration-based base location for resolving WSDL resources.
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.java (1)
257-259: LGTM! Improved test setup withDummyTestRouter.Replacing the Mockito-based router initialization with
DummyTestRouterprovides a more realistic test environment and aligns with the configuration-based refactoring. TheDummyTestRouterproperly implements the full router interface with a workingConfigurationobject, avoiding mock complexities. The removal of the checked exception from the method signature is appropriate sinceinitInterceptordoesn't throw checked exceptions itself.core/src/main/java/com/predic8/membrane/core/proxies/SSLableProxy.java (1)
128-132: Code is safe; null safety concern resolved.The
router.getConfiguration()method is guaranteed to be non-null. TheConfigurationfield is always initialized tonew Configuration()at declaration inDefaultRouter, and the pattern is used extensively throughout the codebase without defensive null checks (including inAbstractProxy.init()at line 84). However, consider adding@NotNullannotations toRouter.getConfiguration()and theconfigurationfield inDefaultRouterfor explicit null-safety documentation.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/RewriteTest.java (1)
54-54: LGTM: Base location configuration correctly migrated.The change to access
setBaseLocationviarouter.getConfiguration()is consistent with the PR's refactoring objective to move base location management to the Configuration object.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordTest.java (1)
34-34: LGTM: Configuration-based base location setup.The migration to configuration-based base location access is correct and aligns with the refactoring pattern applied across the codebase.
core/src/test/java/com/predic8/membrane/core/transport/ssl/SSLContextTest.java (1)
63-63: LGTM: Base location retrieval correctly updated.The change to retrieve base location via
router.getConfiguration().getBaseLocation()for SSL context initialization is correct and consistent with the PR's refactoring objective.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.java (1)
56-56: LGTM: Base location setup migrated to configuration.The configuration-based approach for setting base location is correct and follows the established refactoring pattern.
core/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.java (1)
98-98: LGTM: WSDL resolution updated to use configuration-based base location.The change correctly updates WSDL path resolution to retrieve base location from the configuration object, maintaining the same resolution logic while aligning with the new architecture.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (1)
124-124: LGTM! Consistent refactoring.The change correctly updates the base location retrieval to use the new configuration-based accessor.
core/src/test/java/com/predic8/membrane/core/transport/ssl/acme/AcmeStepTest.java (1)
153-153: LGTM! Test updated correctly.The SSL context initialization now uses the configuration-based base location accessor, consistent with the refactoring across the codebase.
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java (1)
73-78: LGTM! JWT initialization updated correctly.Both the JWK initialization and parsing now use the configuration-based base location accessor, maintaining consistency with the broader refactoring.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ConsentPageFile.java (1)
50-50: LGTM! OAuth consent page initialization updated.The consent page file resolution now correctly uses the configuration-based base location accessor.
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.java (1)
55-55: LGTM! JWT signing initialization updated.The JWK parameter loading now uses the configuration-based base location accessor, consistent with the refactoring.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/XMembraneExtensionSecurityTest.java (1)
36-36: LGTM! Test setup correctly updated.The test setup now uses the configuration-based base location, consistent with the refactoring across the codebase.
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginDialog.java (1)
92-101: LGTM! Base location access consistently updated.The init method now correctly retrieves base location from the router's configuration for index.html resolution and error reporting. All three usages are consistent.
core/src/main/java/com/predic8/membrane/core/interceptor/cache/CacheInterceptor.java (1)
106-106: LGTM! Cache directory resolution updated correctly.The FileStore initialization now uses the configuration-based base location for computing the cache directory path.
core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (1)
111-111: LGTM! Application context integration updated correctly.The setter now assigns base location through the configuration object, maintaining proper Spring integration while following the new pattern.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (3)
97-98: LGTM! JWT authentication setup updated correctly.The initialization of the JWSSigner now retrieves the base location from the router's configuration for resolving private key and certificate paths. This is critical for JWT-based client authentication.
103-103: LGTM! SSL context initialization updated correctly.The StaticSSLContext creation now uses the configuration-based base location, ensuring correct SSL certificate and key resolution.
93-108: Remove this review comment—the concern is unfounded.The
router.getConfiguration()call is guaranteed to return a non-null Configuration object. DefaultRouter initializes the Configuration field withnew Configuration()at declaration, and the codebase contains nosetConfiguration(null)calls. All code throughout the system assumesgetConfiguration()is non-null (no null checks exist anywhere), confirming this is a safe pattern.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/ApiKeyFileStore.java (1)
64-64: LGTM: Base location access updated correctly.The change correctly delegates base location retrieval to the configuration object, consistent with the broader refactoring.
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
164-164: LGTM: Base location setter updated correctly.The base location is now properly set via the configuration object during YAML router initialization.
core/src/main/java/com/predic8/membrane/core/sslinterceptor/RouterIpResolverInterceptor.java (1)
118-118: LGTM: SSL context initialization updated correctly.The base location is correctly retrieved from the configuration object for SSL context setup.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
34-34: LGTM: Test setup updated correctly.The test properly configures base location via the configuration object, maintaining consistency with the refactored API.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.java (2)
62-62: LGTM: JWK resolution updated correctly.The base location is properly retrieved from the configuration object for JWK JSON parsing.
62-62: Verify complete migration of base location API.Since this refactoring moves
baseLocationfrom the Router interface to the Configuration object across approximately 35 files, ensure all usages have been migrated and no references to the old API remain. Specifically:
- No remaining calls to
router.getBaseLocation()- No remaining calls to
router.setBaseLocation(...)- Router interface no longer declares these methods
- DefaultRouter class no longer implements these methods
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/AcmeRenewTest.java (1)
109-109: LGTM! Base location correctly sourced from configuration.The update to use
router.getConfiguration().getBaseLocation()aligns with the refactoring objective to centralize base location in the Configuration object.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java (1)
84-84: LGTM! Configuration-based base location resolution.The change correctly updates the OpenID configuration resolution to use the router's configuration object for base location, consistent with the refactoring pattern across the codebase.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/DynamicRegistration.java (1)
47-47: LGTM! SSL context initialization updated correctly.The StaticSSLContext now correctly receives the base location from the router's configuration object, maintaining consistency with the refactoring approach.
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java (1)
144-144: LGTM! JWK resolution correctly updated.The JWT key resolution now properly uses the configuration-based base location, aligning with the refactoring to centralize base location management.
core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTTransformer.java (1)
41-45: LGTM! Transformer initialization consistently updated.Both transformer creation calls (initial and parallel loop) now correctly use
router.getConfiguration().getBaseLocation(), ensuring consistent base location resolution for stylesheet resources.core/src/main/java/com/predic8/membrane/core/interceptor/server/WebServerInterceptor.java (4)
118-131: LGTM! Consistent refactoring across multiple call sites.Both base location references in
handleRequestInternal()have been correctly updated to access the configuration object. The logic for path resolution and response creation remains functionally identical.
196-196: LGTM! Index resource resolution correctly updated.The base location accessor in
tryToReceiveResource()has been properly updated to use the Configuration object, maintaining the same path resolution logic for index files.
211-211: LGTM! Directory listing path resolution updated correctly.The base location reference for retrieving directory children has been properly migrated to use the Configuration object.
291-291: LGTM! Path normalization helper correctly updated.The base location accessor in
getNewPath()has been appropriately updated to use the Configuration object, maintaining the same path combination logic.core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (2)
53-65: LGTM! Documentation improvements enhance clarity.The updated comments better describe the Router's responsibilities and current state, making the codebase more maintainable.
89-451: Migration of removed Router methods to Configuration is complete.The removal of
getBaseLocation(),setBaseLocation(), andisProduction()from DefaultRouter is a public API change that has been properly handled. All three methods are now present in the Configuration class, and no remaining direct calls to these methods exist in the codebase outside of tests. The migration across the ~35 affected files is complete.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MicrosoftEntraIDAuthorizationService.java (1)
59-59: router.getBaseLocation() does not exist on the Router interface—this change is a necessary fix, not a refactoring.The Router interface only exposes
getConfiguration(), notgetBaseLocation(). The original code usingrouter.getBaseLocation()would not compile. The change torouter.getConfiguration().getBaseLocation()is required. Additionally, ResolverMap.combine() safely handles null baseLocation by returning the child URL as-is, so the call is null-safe.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/router/Configuration.java
Outdated
Show resolved
Hide resolved
- Replace `DefaultRouter` with `Router` in `JmxRouter` for enhanced abstraction. - Introduce `JMX_NAMESPACE` for improved consistency in bean names. - Enhance `JmxExporter` with static factory methods and improved `Router` integration. - Update `HotDeployer` to provide debug logging for unsupported YAML configurations.
- Replace `DefaultRouter` with `Router` in `JmxRouter` for enhanced abstraction. - Introduce `JMX_NAMESPACE` for improved consistency in bean names. - Enhance `JmxExporter` with static factory methods and improved `Router` integration. - Update `HotDeployer` to provide debug logging for unsupported YAML configurations.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/router/Configuration.java (1)
151-154: @MCAttribute annotation is appropriately applied; consider a minor documentation enhancement for clarity.The evidence shows
baseLocationis legitimately user-configurable in multiple scenarios:
- CLI usage (RouterCLI.java:164) passes a location parameter from user input
- Configuration override via @MCAttribute allows users to set this in Membrane config files
- Framework initialization (DefaultMainComponents, Spring context) auto-determines it when running in container
- Testing explicitly sets custom values for different test scenarios
The @MCAttribute annotation is justified. The existing Javadoc (lines 148-150) explains its purpose adequately, but optionally expanding it with an example scenario—such as "useful when the configuration file location differs from the default deployment directory"—would improve clarity for users deciding whether to override this setting.
core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.java (2)
30-30: Design issue: Class extends MBeanExporter but also composes one.
JmxExporterextendsMBeanExporter(line 30) but also holds a privateMBeanExporter exporterfield (line 39) that's actually used. This is confusing - the inheritedMBeanExporterfunctionality is unused.Consider either:
- Remove the
extends MBeanExporterand only compose, or- Use the inherited functionality directly instead of composing
Also applies to: 39-39
37-37: Thread safety concern: unsynchronized HashMap.
jmxBeansis a plainHashMapaccessed byaddBean(),stop(), andinitAfterBeansAdded(). If these methods can be called from different threads (e.g., during Spring context refresh), this could lead to race conditions.Consider using
ConcurrentHashMapor synchronizing access if concurrent modification is possible.🔎 Proposed fix
- private final HashMap<String, Object> jmxBeans = new HashMap<>(); + private final Map<String, Object> jmxBeans = new ConcurrentHashMap<>();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MicrosoftEntraIDAuthorizationService.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.javacore/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.javacore/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.javacore/src/main/java/com/predic8/membrane/core/jmx/JmxRouter.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/router/Configuration.javacore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.javacore/src/main/java/com/predic8/membrane/core/router/MainComponents.javacore/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/router/DummyTestRouter.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.javacore/src/test/java/com/predic8/membrane/core/transport/ssl/SSLContextTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/com/predic8/membrane/core/transport/ssl/SSLContextTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MicrosoftEntraIDAuthorizationService.java
- core/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/JWSSigner.java (1)
JWSSigner(26-53)core/src/main/java/com/predic8/membrane/core/transport/ssl/PEMSupport.java (1)
PEMSupport(41-170)
⏰ 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 (17)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
83-84: LGTM! Base location correctly sourced from configuration.The refactoring to use
router.getConfiguration().getBaseLocation()for JWSSigner initialization is consistent with the broader migration pattern.
89-89: LGTM! Base location correctly sourced from configuration.The StaticSSLContext initialization now properly uses the configuration-based base location.
core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (2)
69-69: LGTM! Constructor reference is more idiomatic.The change from lambda to constructor reference
HttpClientConfiguration::newis cleaner and more concise.
111-111: LGTM! Base location correctly set on configuration.The refactoring to set base location via
router.getConfiguration().setBaseLocation(...)aligns with the configuration-based architecture.core/src/main/java/com/predic8/membrane/core/interceptor/session/JwtSessionManager.java (2)
76-76: LGTM! Base location correctly sourced from configuration.The JWK parsing now properly uses
router.getConfiguration().getBaseLocation(), consistent with the broader refactoring.
71-74: LGTM! Log message formatting improved.Consolidating the log message to a single line improves readability while maintaining the same content.
core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java (1)
180-180: LGTM! Base location correctly sourced from configuration.The WSDL parser context initialization now properly uses
router.getConfiguration().getBaseLocation(), aligning with the configuration-based architecture refactoring.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2server/LoginDialog2.java (3)
83-83: LGTM! Base location access updated consistently.The change from
router.getBaseLocation()torouter.getConfiguration().getBaseLocation()aligns with the PR's refactoring objective to move base location to the Configuration class.
348-355: LGTM! Exception declaration correctly removed.The method no longer declares
throws Exception, which is appropriate since the method body usesURLEncoder.encodewithStandardCharsets.UTF_8(which doesn't throw checked exceptions) and the Response builder pattern.
311-317: No throws declarations needed in these methods.The
OAuth2Util.urldecode()method does not declare throws. It usesURLDecoder.decode(String, Charset)withStandardCharsets.UTF_8, which does not throw checked exceptions. The entire method call chain (prepareClaimsFromSession/prepareScopesFromSession→decodeClaimsFromSession/decodeScopesFromSession→OAuth2Util.urldecode) contains no methods that throw checked exceptions, so removing the throws declarations is safe.core/src/test/java/com/predic8/membrane/core/router/DummyTestRouter.java (1)
119-122: LGTM! Consistent with other unsupported operations.The
getRegistry()override appropriately throwsUnsupportedOperationException, which is consistent with other unsupported operations in this test router (e.g.,getKubernetesClientFactory()on line 136).core/src/main/java/com/predic8/membrane/core/router/hotdeploy/DefaultHotDeployer.java (1)
49-64: LGTM! Improved handling of different configuration types.The refactored logic now properly handles three scenarios:
- XML configuration with TrackingApplicationContext → starts hot deployment
- XML configuration without TrackingApplicationContext → logs warning
- YAML configuration (beanFactory is null) → logs debug message
The defensive type checking and clear logging improve the robustness of the hot deployment initialization.
core/src/main/java/com/predic8/membrane/core/jmx/JmxRouter.java (2)
29-33: LGTM! Improved abstraction with Router interface.Changing the constructor parameter from
DefaultRoutertoRouterimproves abstraction and aligns with the commit message objective to replace DefaultRouter with Router for better abstraction in JMX components.
49-50: LGTM! Using JMX_NAMESPACE constant improves maintainability.Replacing the hardcoded JMX prefix string with the
JMX_NAMESPACEconstant centralizes the namespace definition and improves maintainability.core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.java (1)
253-260: LGTM! Clean test simplification.Replacing Mockito-based router mocking with
DummyTestRouterreduces test complexity and aligns with the PR-wide configuration-based base location changes. The removal ofthrows Exceptionis appropriate since the new initialization path doesn't throw checked exceptions.core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (1)
160-160: LGTM! JMX startup centralized in JmxExporter.Delegating JMX startup to
JmxExporter.start(this)cleanly separates concerns and aligns with the refactoring to centralize JMX management.core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.java (1)
86-107: LGTM! Static helper methods provide clean router-aware JMX initialization.The
start(Router)andgetExporter(Router)static methods provide a clean entry point for JMX initialization. The fallback chain (registry → bean factory) handles both YAML and XML configuration paths appropriately.
core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java
Outdated
Show resolved
Hide resolved
- Remove unused methods and imports from `DefaultRouter`, `JmxRouter`, and `Configuration`. - Simplify `JmxExporter` by replacing `MBeanExporter` inheritance and improving lifecycle checks with `exporter` nullability. - Optimize `addRouter()` in `JmxExporter` by removing redundant exception handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (1)
30-30: Consider using specific imports instead of wildcard.The wildcard import
import com.predic8.membrane.core.proxies.RuleManager.*;is less explicit than importing specific nested classes or members. Consider replacing it with explicit imports for the specific types being used from RuleManager.core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.java (1)
67-75: Track the lifecycle workaround as technical debt.The workaround on lines 68-70 masks a lifecycle management gap where YAML configuration doesn't properly call
start()on components. While this defensive code is pragmatic, consider:
- Creating a tracking issue for the root cause (ensuring YAML properly initializes lifecycle)
- Adding this workaround to your technical debt backlog with a timeline for removal
- Ensuring the workaround doesn't hide other initialization issues
Would you like me to open an issue to track removing this workaround once YAML lifecycle initialization is fixed?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.javacore/src/main/java/com/predic8/membrane/core/jmx/JmxRouter.javacore/src/main/java/com/predic8/membrane/core/router/Configuration.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/router/Configuration.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 (11)
core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (5)
52-58: LGTM - Documentation improvements.The updated comments provide clearer context about the Router's responsibilities and lifecycle management.
101-101: LGTM - Javadoc improvements.The documentation updates improve clarity around initialization lifecycle and the behavior of
add(Proxy)when called afterstart().Also applies to: 116-117, 259-262
406-406: LGTM - Good practice to add @OverRide.Adding the
@Overrideannotation improves type safety and makes the interface implementation explicit.
428-430: Verify all call sites updated for removed public methods.The PR removes
getBaseLocation(),setBaseLocation(), andisProduction()from DefaultRouter and moves them to Configuration. Verify that all call sites throughout the codebase have been updated to userouter.getConfiguration().getBaseLocation()pattern instead of the direct method calls.
159-159: JMX initialization refactoring is complete and correct.The consolidation to
JmxExporter.start(Router router)has been properly implemented. The method exists at line 83 ofJmxExporter.java, correctly handles initialization with graceful fallback when no exporter is configured, and all references to the previousstartJmx()method have been removed from the codebase.core/src/main/java/com/predic8/membrane/core/jmx/JmxRouter.java (2)
20-21: LGTM: Clean static import.The static import of
JMX_NAMESPACEimproves readability and centralizes the namespace constant definition.
28-28: No action needed—Router interface provides all required methods.The
Routerinterface extendsMainComponents, which definesgetRuleManager()(line 43 of MainComponents.java), andRouterdirectly definesgetConfiguration()(line 28 of Router.java). Both methods are available and the parameter type change fromDefaultRoutertoRouteris valid.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/jmx/JmxExporter.java (4)
16-30: LGTM: Clean refactoring to composition.The shift from extending
MBeanExporterto implementingLifecyclewith a composedMBeanExporterinstance is a solid architectural improvement. This reduces tight coupling to Spring's MBean infrastructure and provides clearer lifecycle management.
34-35: LGTM: Well-defined constants.The constants
JMX_EXPORTER_NAMEandJMX_NAMESPACEcentralize configuration and improve maintainability.
42-61: LGTM: Lifecycle methods correctly implemented.The lifecycle methods properly manage the
MBeanExporterinstance:
start()creates and configures the exporter with appropriate registration policystop()cleanly destroys the exporter and clears stateisRunning()correctly reflects lifecycle state via null checkThis resolves the previous issue where
isRunning()always returned false.
77-104: LGTM: Clean static helper methods.The refactoring properly addresses the previous concern about exception handling:
addRouter()no longer has the misplacedNoSuchBeanDefinitionExceptioncatchgetExporter()correctly handles the exception where it can actually occur (bean factory lookup)- The two-tier lookup strategy (registry first, then bean factory) provides good flexibility
…MX namespace in roadmap
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (1)
89-95: Inconsistent timeout configuration.The test method has
@Timeout(60)(60 seconds), but theawaitTerminationcall at line 105 now uses 120 seconds. JUnit will terminate the test after 60 seconds, so the 120-second timeout will never be reached. If the test legitimately requires more time, update the@Timeoutannotation to at least 120 seconds (or higher to account for setup overhead).🔎 Proposed fix to align timeouts
-@Timeout(60) // seconds +@Timeout(130) // seconds (120 + buffer for setup/teardown) public void run() throws Exception {
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.java (1)
105-105: Consider investigating the root cause of timeout issues.Doubling the
awaitTerminationtimeout from 60 to 120 seconds suggests the test was experiencing timeouts. With 500 virtual threads and 1-second delays per request, 60 seconds should theoretically be sufficient. Consider investigating whether there are underlying issues such as resource constraints, thread pool saturation, or network delays that are causing the slowdown.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/com/predic8/membrane/integration/withoutinternet/MassivelyParallelTest.javadocs/ROADMAP.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
docs/ROADMAP.md (1)
94-94: ✓ Clear and properly documented breaking change.The JMX namespace change is correctly recorded in the Breaking Changes section with consistent formatting. This aligns with the broader refactoring of JMX-related code in the PR.
…N` with static import and minor cleanup in methods
Summary by CodeRabbit
Refactor
Breaking Changes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.