Refactor remove proxy aware#2884
Merged
Merged
Conversation
- Eliminated `ProxyAware` interface, simplifying codebase and reducing unused complexity. - Updated `AbstractInterceptor`, `Interceptor`, and related implementations to directly manage proxy references via `getProxy()` and `init()`. - Refactored interceptors to remove unused `Proxy` fields and redundant `setProxy()` methods. - Adjusted roadmap to reflect removal of `ProxyAware` and associated refactoring tasks.
- Simplified `AbstractFlowInterceptor`, `OAuth2AuthorizationServerInterceptor`, `WebServiceExplorerInterceptor`, and `RuleManager` by removing unused fields, imports, and redundant logic. - Cleaned up code to improve maintainability and readability across core interceptor and proxy-related classes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved the ProxyAware interface and centralized proxy storage/propagation: AbstractInterceptor now stores the Proxy and exposes getProxy(); Interceptor gained getProxy(); initialization now passes Proxy to interceptors via init(router, proxy); RuleManager removed listener and per-source tracking; Flow interceptors propagate router+proxy to nested interceptors. Changes
Sequence Diagram(s)sequenceDiagram
participant AbstractProxy
participant Interceptor
participant AbstractInterceptor
participant AbstractFlowInterceptor
participant ChildInterceptor
AbstractProxy->>Interceptor: init(router, proxy)
Note right of Interceptor: Concrete interceptor\ncalls super.init(router, proxy)
Interceptor->>AbstractInterceptor: init(router, proxy)
AbstractInterceptor->>AbstractInterceptor: this.proxy = proxy
AbstractFlowInterceptor->>AbstractFlowInterceptor: iterate getFlow()
AbstractFlowInterceptor->>ChildInterceptor: child.init(router, proxy)
ChildInterceptor->>AbstractInterceptor: child stores proxy via init
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/interceptor/flow/AbstractFlowInterceptor.java (1)
39-44:⚠️ Potential issue | 🔴 CriticalNested interceptors won't receive the proxy reference.
Line 43 calls
i.init(router)for child interceptors, but should calli.init(router, proxy)to propagate the proxy reference. Without this, any interceptor nested inside a flow interceptor (e.g., inside<if>,<request>, etc.) will havegetProxy()returnnull.The parent
AbstractProxy.init(Router)correctly callsi.init(router, this)for top-level interceptors, so this flow interceptor should do the same for its children.🐛 Proposed fix to propagate proxy to nested interceptors
`@Override` public void init() { super.init(); for (Interceptor i : interceptors) - i.init(router); + i.init(router, proxy); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.java` around lines 39 - 44, AbstractFlowInterceptor.init currently calls i.init(router) for child interceptors so nested interceptors never receive the proxy reference; change the call to pass the proxy (call i.init(router, getProxy()) or i.init(router, this.getProxy())) so nested interceptors get the same proxy as the parent; update AbstractFlowInterceptor.init to invoke Interceptor.init(Router, Proxy) for each interceptor, mirroring AbstractProxy.init(Router) which uses i.init(router, this).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.java`:
- Around line 114-116: In AbstractInterceptor.init(Router router, Proxy proxy)
assign this.proxy = proxy before calling init(router) so subclasses' overridden
init(Router) see a non-null proxy; i.e., move the this.proxy assignment to
precede the call to init(router) (affects AbstractInterceptor.init(Router,
Proxy) and ensures subclass init(Router) methods that compute values from proxy
get the proper proxy reference).
In `@docs/ROADMAP.md`:
- Line 55: Fix the typo in the ROADMAP entry referencing ReplaceInterceptor:
change "tansformation" to "transformation" so the line reads
"ReplaceInterceptor: Support Languages, tutorial/transformation" (or
"tutorial/transformation" if you prefer a slash) — update the text where
ReplaceInterceptor is mentioned in the ROADMAP.md.
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.java`:
- Around line 39-44: AbstractFlowInterceptor.init currently calls i.init(router)
for child interceptors so nested interceptors never receive the proxy reference;
change the call to pass the proxy (call i.init(router, getProxy()) or
i.init(router, this.getProxy())) so nested interceptors get the same proxy as
the parent; update AbstractFlowInterceptor.init to invoke
Interceptor.init(Router, Proxy) for each interceptor, mirroring
AbstractProxy.init(Router) which uses i.init(router, this).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 179a0c1d-ceea-4b40-8729-dbc05b977238
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/config/ProxyAware.javacore/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/Interceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/LoginInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/groovy/GroovyInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2AuthorizationServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/soap/WebServiceExplorerInterceptor.javacore/src/main/java/com/predic8/membrane/core/proxies/AbstractProxy.javacore/src/main/java/com/predic8/membrane/core/proxies/RuleManager.javadocs/ROADMAP.md
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/config/ProxyAware.java
Contributor
|
No open human review comments were found in this PR to create a plan for. |
…nterceptor - Improved proxy initialization in `AbstractInterceptor` and `AbstractFlowInterceptor` to ensure consistent propagation. - Updated `init()` logic to handle `proxy` references appropriately across interceptors. - Added `AbstractFlowInterceptorTest` to validate proxy propagation and router integration. - Minor corrections in `ROADMAP.md` and cleanup of unused fields.
…o refactor-remove-proxy-aware
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.java (1)
14-24: Consider using the public API for cleaner test setup.The test directly accesses the protected
interceptorsfield. While this works (same package), using the publicsetFlow()method would make the test more resilient to internal refactoring and better document the intended usage.♻️ Suggested refactor using public API
void propagationOfProxy() { var r = new TestRouter(); var p = new ServiceProxy(); var ri = new RequestInterceptor(); var ti = new LogInterceptor(); - ri.interceptors.add(ti); + ri.setFlow(java.util.List.of(ti)); ri.init(r,p); assertSame(r,ti.getRouter()); assertSame(p,ti.getProxy()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.java` around lines 14 - 24, The test directly mutates the protected field ri.interceptors; replace that with the public API by calling RequestInterceptor.setFlow(...) to install the child interceptor(s). Concretely, remove ri.interceptors.add(ti) and call ri.setFlow(Collections.singletonList(ti)) (or Arrays.asList(ti)) so the test uses the public setFlow method on RequestInterceptor and keeps the rest of the assertions (assertSame(r, ti.getRouter()), assertSame(p, ti.getProxy())) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.java`:
- Line 7: Remove the unused Groovy logging import from the test class: delete
the line "import groovy.util.logging.*" in AbstractFlowInterceptorTest so the
class no longer contains an unused import; verify compilation/tests still pass
after removing it.
---
Nitpick comments:
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.java`:
- Around line 14-24: The test directly mutates the protected field
ri.interceptors; replace that with the public API by calling
RequestInterceptor.setFlow(...) to install the child interceptor(s). Concretely,
remove ri.interceptors.add(ti) and call
ri.setFlow(Collections.singletonList(ti)) (or Arrays.asList(ti)) so the test
uses the public setFlow method on RequestInterceptor and keeps the rest of the
assertions (assertSame(r, ti.getRouter()), assertSame(p, ti.getProxy()))
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7e12ef5-9139-400f-b7f0-072e436032ad
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptor.javacore/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.javadocs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/ROADMAP.md
- core/src/main/java/com/predic8/membrane/core/interceptor/AbstractInterceptor.java
… and router - Renamed test method for clarity and updated logic to use `setFlow` with `List.of`. - Verified proper initialization of nested interceptors with proxy and router.
Member
Author
|
/ok-to-test |
rrayst
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Documentation
Tests