Add HttpClientConfiguration to configuration and refactor HttpClient creation and usage#2842
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes HttpClientConfiguration in Configuration, creates/exposes a shared HttpClient from DefaultMainComponents/DefaultRouter, and re-wires resolvers and interceptors to accept an injected HttpClient instead of using factory + post-construction configuration. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant Router as DefaultRouter
participant Main as DefaultMainComponents
participant Factory as HttpClientFactory
participant ResolverMap as ResolverMap
participant Resolver as HTTPSchemaResolver
participant Interceptor as HTTPClientInterceptor
Config->>Router: getHttpClientConfig()
Router->>Main: init() / request httpClientConfig
Main->>Factory: createClient(httpClientConfig)
Factory-->>Main: HttpClient instance
Main->>ResolverMap: new ResolverMap(HttpClient, kubernetesFactory)
ResolverMap->>Resolver: construct with injected HttpClient
Router->>Interceptor: construct (inject HttpClient) -- conditional for DefaultRouter
Note over Resolver,Interceptor: runtime calls use the same injected HttpClient
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 1
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/HTTPClientInterceptor.java (1)
61-65:⚠️ Potential issue | 🟠 MajorLocal interceptor config is overridden and global config is mutated
Line 61 unconditionally replaces any interceptor-local
httpClientConfig, and Line 64 then mutates the shared global retry handler. This breaks local precedence and can leak one interceptor’sfailOverOn5XXinto others.Please keep local config when explicitly provided, and apply per-interceptor overrides on an interceptor-local effective config (not the shared resolver instance).
🤖 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/HTTPClientInterceptor.java` around lines 61 - 65, The code in HTTPClientInterceptor unconditionally assigns httpClientConfig from router.getResolverMap().getHTTPSchemaResolver().getHttpClientConfig() and then mutates its shared retry handler via getRetryHandler().setFailOverOn5XX(failOverOn5XX), which overwrites local configs and leaks settings; change the logic in HTTPClientInterceptor so that if an interceptor-local httpClientConfig is present keep it, otherwise obtain the global config but create an interceptor-local copy/clone before applying per-interceptor overrides like failOverOn5XX (i.e. do not call setFailOverOn5XX on the shared instance returned by getHTTPSchemaResolver().getHttpClientConfig(), instead clone it into a new httpClientConfig and apply getRetryHandler().setFailOverOn5XX on that clone).
🤖 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/router/Configuration.java`:
- Around line 156-158: The setter setHttpClientConfig(HttpClientConfiguration
httpClientConfig) currently allows null which can make
configuration.getHttpClientConfig() return null later; update
setHttpClientConfig in class Configuration to guard against null by either
throwing an IllegalArgumentException when httpClientConfig is null or
normalizing to a default HttpClientConfiguration instance before assignment, and
ensure any callers rely on non-null values thereafter (reference
setHttpClientConfig and the getHttpClientConfig usage sites to implement the
chosen behavior).
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java`:
- Around line 61-65: The code in HTTPClientInterceptor unconditionally assigns
httpClientConfig from
router.getResolverMap().getHTTPSchemaResolver().getHttpClientConfig() and then
mutates its shared retry handler via
getRetryHandler().setFailOverOn5XX(failOverOn5XX), which overwrites local
configs and leaks settings; change the logic in HTTPClientInterceptor so that if
an interceptor-local httpClientConfig is present keep it, otherwise obtain the
global config but create an interceptor-local copy/clone before applying
per-interceptor overrides like failOverOn5XX (i.e. do not call setFailOverOn5XX
on the shared instance returned by
getHTTPSchemaResolver().getHttpClientConfig(), instead clone it into a new
httpClientConfig and apply getRetryHandler().setFailOverOn5XX on that clone).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.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.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/resolver/HTTPSchemaResolver.java (1)
108-120:⚠️ Potential issue | 🔴 CriticalConstructor migration left resolver in an invalid runtime state
After Line 108–109,
httpClientFactoryis never initialized, but the watcher still uses it, soobserveChange()can fail with NPE. Also, Line 120 dereferenceshttpClientdirectly despite nullable construction.🔧 Proposed fix
-import com.predic8.membrane.core.transport.http.HttpClientFactory; +import java.util.Objects; @@ - private HttpClientFactory httpClientFactory; @@ - public HTTPSchemaResolver(`@Nullable` HttpClient httpClient) { - this.httpClient = httpClient; + public HTTPSchemaResolver(`@Nullable` HttpClient httpClient) { + this.httpClient = Objects.requireNonNull(httpClient, "httpClient must not be null"); } @@ - try (HttpClient client = httpClientFactory.createClient(null)) { + HttpClient client = this.httpClient; + try { while (!watchedUrlMd5s.isEmpty()) {🤖 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/resolver/HTTPSchemaResolver.java` around lines 108 - 120, The constructor HTTPSchemaResolver(`@Nullable` HttpClient httpClient) leaves httpClientFactory uninitialized and allows httpClient to be null; fix by initializing the resolver's httpClientFactory (or accepting/setting a non-null factory) inside the constructor so observeChange() never sees a null factory, and add a null-check/guard in resolve(String url) before calling httpClient.call(...) (or fall back to creating/obtaining an HttpClient from httpClientFactory) to avoid dereferencing a nullable httpClient; update createResolveExchange/resolve logic to consistently use the initialized httpClientFactory rather than the possibly-null httpClient.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (1)
50-53: Consider deferring placeholderHttpClient/ResolverMapcreationLine 50 and Line 53 instantiate defaults that are immediately replaced in
init(). Lazy/deferred construction would avoid unnecessary allocations and temporary pre-init state.🤖 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/router/DefaultMainComponents.java` around lines 50 - 53, The fields httpClient and resolverMap are pre-initialized then overwritten in init(), causing unnecessary allocations; change their declarations in DefaultMainComponents to not instantiate defaults (set to null) and move construction into init() where httpClient is created (using httpClientFactory) and resolverMap is assigned, and ensure KubernetesClientFactory is constructed after httpClientFactory is available (or lazily created in init()) so no temporary pre-init objects exist; update any getters to handle lazy initialization if they may be called before init().
🤖 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/jwt/Jwks.java`:
- Around line 137-141: The code creates a new HTTP client on each getJwk() call
by calling router.getHttpClientFactory().createClient(httpClientConfig) and
wrapping it in a new HTTPSchemaResolver, which leaks resources; change this to
reuse a shared or cached client/HTTPSchemaResolver instead (e.g., create and
store a single HTTPSchemaResolver or HttpClient per unique httpClientConfig at
class construction or in a concurrent cache) and have getJwk() re-use that
resolver rather than creating one every call; ensure any cached client is
properly closed on component shutdown to avoid resource leaks while keeping the
existing rm.clone() and rm.addSchemaResolver(httpSR) usage.
In `@core/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.java`:
- Around line 80-83: The code creates a new HttpClient each time init() runs
(via router.getHttpClientFactory().createClient(httpClientConfig)) but never
closes prior instances; add a field on SOAPProxy (e.g., customHttpClient or
wsdlHttpClient) to hold the created client and reuse it across re-inits or, if
replacing it, explicitly close the previous client before assigning a new one;
ensure HTTPSchemaResolver is constructed with that field instead of creating
inline, and also close that client in the class shutdown/destroy path (e.g.,
destroy()/close()) to avoid resource leaks while keeping
resolverMap.addSchemaResolver(httpSR) behavior intact.
In `@core/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.java`:
- Around line 61-68: The ResolverMap constructor currently forwards a
possibly-null HttpClient into new HTTPSchemaResolver, so change the wiring in
the ResolverMap(HttpClient httpClient, KubernetesClientFactory
kubernetesClientFactory) constructor to guard against null: only call
addSchemaResolver(new HTTPSchemaResolver(httpClient)) when httpClient != null
(or else provide a safe default/no-op HttpClient) so the map never contains an
HTTPSchemaResolver constructed with a null HttpClient; keep the existing
addSchemaResolver, ClasspathSchemaResolver, and KubernetesSchemaResolver wiring
intact.
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/resolver/HTTPSchemaResolver.java`:
- Around line 108-120: The constructor HTTPSchemaResolver(`@Nullable` HttpClient
httpClient) leaves httpClientFactory uninitialized and allows httpClient to be
null; fix by initializing the resolver's httpClientFactory (or accepting/setting
a non-null factory) inside the constructor so observeChange() never sees a null
factory, and add a null-check/guard in resolve(String url) before calling
httpClient.call(...) (or fall back to creating/obtaining an HttpClient from
httpClientFactory) to avoid dereferencing a nullable httpClient; update
createResolveExchange/resolve logic to consistently use the initialized
httpClientFactory rather than the possibly-null httpClient.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java`:
- Around line 50-53: The fields httpClient and resolverMap are pre-initialized
then overwritten in init(), causing unnecessary allocations; change their
declarations in DefaultMainComponents to not instantiate defaults (set to null)
and move construction into init() where httpClient is created (using
httpClientFactory) and resolverMap is assigned, and ensure
KubernetesClientFactory is constructed after httpClientFactory is available (or
lazily created in init()) so no temporary pre-init objects exist; update any
getters to handle lazy initialization if they may be called before init().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/proxies/SOAPProxy.javacore/src/main/java/com/predic8/membrane/core/resolver/HTTPSchemaResolver.javacore/src/main/java/com/predic8/membrane/core/resolver/ResolverMap.javacore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java
- core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java
Collaborator
Author
|
/ok-to-test |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/HTTPClientInterceptor.java (1)
65-74:⚠️ Potential issue | 🟡 MinorInjected HttpClient bypasses
streamPumpStatsconfiguration.When an
HttpClientis provided via the new constructor, the early return at line 65 skips line 73 wherestreamPumpStatsis set. This means statistics tracking for stream pumps won't work for the injected client path.If the caller is expected to configure
streamPumpStatsbefore injection, consider documenting this requirement. Otherwise, the stats should be set ininit()regardless of the injection path.🐛 Proposed fix to ensure streamPumpStats is always set
`@Override` public void init() { super.init(); - if (hc != null) return; + if (hc != null) { + hc.setStreamPumpStats(getRouter().getStatistics().getStreamPumpStats()); + return; + } httpClientConfig = router.getHttpClientConfig(); // Overwrite httpClientConfiguration with local value if (failOverOn5XX != null) { httpClientConfig.getRetryHandler().setFailOverOn5XX(failOverOn5XX); } hc = router.getHttpClientFactory().createClient(httpClientConfig); hc.setStreamPumpStats(getRouter().getStatistics().getStreamPumpStats()); }🤖 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/HTTPClientInterceptor.java` around lines 65 - 74, The early-return in init() skips setting streamPumpStats for an injected HttpClient (hc), so ensure setStreamPumpStats(...) is called regardless of whether hc was injected or created: remove or adjust the "if (hc != null) return;" behavior in init() so that after obtaining or using the existing hc you always call hc.setStreamPumpStats(getRouter().getStatistics().getStreamPumpStats());; alternatively, if injection must require pre-configuration, document that callers who pass an HttpClient via the constructor must call setStreamPumpStats themselves (mentioning the hc field and init() method and the setStreamPumpStats call) — but prefer changing init() to always set the stats on hc.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/transport/Transport.java (1)
63-64: Redundant instanceof check - router is alreadyDefaultRouter.Since the method signature now accepts
DefaultRouterdirectly (line 54), theinstanceof DefaultRoutercheck is always true. This can be simplified.♻️ Proposed simplification
- if (router instanceof DefaultRouter dr) - dr.getRegistry().getBean(GlobalInterceptor.class).ifPresent(i -> interceptors.add(i )); + router.getRegistry().getBean(GlobalInterceptor.class).ifPresent(interceptors::add);🤖 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/transport/Transport.java` around lines 63 - 64, The instanceof pattern matching is redundant because the method parameter is already a DefaultRouter; remove the "if (router instanceof DefaultRouter dr)" check and call router.getRegistry().getBean(GlobalInterceptor.class).ifPresent(i -> interceptors.add(i)); directly (reference: DefaultRouter, getRegistry, GlobalInterceptor, interceptors).
🤖 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/HTTPClientInterceptor.java`:
- Around line 57-59: The new HTTPClientInterceptor(HttpClient httpClient)
constructor omits the default initialization (setting name and applied flow), so
update that constructor to perform the same setup as the no-arg constructor:
either chain to the default constructor or explicitly set the interceptor name
(name) and call setAppliedFlow(REQUEST_FLOW) after assigning hc; ensure the
constructor in class HTTPClientInterceptor mirrors the default constructor's
initialization to avoid inconsistencies during introspection or flow checks.
In `@core/src/main/java/com/predic8/membrane/core/transport/Transport.java`:
- Around line 54-55: The method signature change in Transport.init from the
Router interface to the concrete DefaultRouter breaks callers like TestRouter
and DummyTestRouter; restore the parameter type to the Router interface (use
Router router) and only cast to DefaultRouter inside the method when you need
DefaultRouter-specific behavior (e.g., to call
getRegistry().getBean(GlobalInterceptor.class)); remove the redundant instanceof
check that assumed DefaultRouter when the parameter was already DefaultRouter,
and ensure callers using AbstractRouter continue compiling without modifying
their classes.
In `@core/src/test/java/com/predic8/membrane/core/router/TestRouter.java`:
- Line 92: The line constructing ResolverMap uses a truncated identifier
`getHtt` and must be replaced with a proper HttpClient instance; change the call
that constructs ResolverMap to pass an HttpClient created from
httpClientFactory.createClient(httpClientConfiguration) instead of `getHtt`,
i.e. use httpClientFactory.createClient(httpClientConfiguration) as the first
argument to new ResolverMap(...), keeping kubernetesClientFactory as the second
argument (mirror the pattern used in DefaultMainComponents.init()).
---
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java`:
- Around line 65-74: The early-return in init() skips setting streamPumpStats
for an injected HttpClient (hc), so ensure setStreamPumpStats(...) is called
regardless of whether hc was injected or created: remove or adjust the "if (hc
!= null) return;" behavior in init() so that after obtaining or using the
existing hc you always call
hc.setStreamPumpStats(getRouter().getStatistics().getStreamPumpStats());;
alternatively, if injection must require pre-configuration, document that
callers who pass an HttpClient via the constructor must call setStreamPumpStats
themselves (mentioning the hc field and init() method and the setStreamPumpStats
call) — but prefer changing init() to always set the stats on hc.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/Transport.java`:
- Around line 63-64: The instanceof pattern matching is redundant because the
method parameter is already a DefaultRouter; remove the "if (router instanceof
DefaultRouter dr)" check and call
router.getRegistry().getBean(GlobalInterceptor.class).ifPresent(i ->
interceptors.add(i)); directly (reference: DefaultRouter, getRegistry,
GlobalInterceptor, interceptors).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.javacore/src/main/java/com/predic8/membrane/core/transport/Transport.javacore/src/test/java/com/predic8/membrane/core/router/TestRouter.java
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/router/TestRouter.java (1)
150-153: AligngetHttpClientConfig()withDefaultRouterandDummyTestRouterpatterns.
TestRouter.getHttpClientConfig()returns a localhttpClientConfigurationfield, while bothDefaultRouterandDummyTestRouterdelegate toconfiguration.getHttpClientConfig(). This inconsistency meansTestRouter.getConfiguration().getHttpClientConfig()returns a different instance thanTestRouter.getHttpClientConfig(), which is confusing and inconsistent with the rest of the codebase.Change to delegate to the configuration object:
♻️ Proposed fix
`@Override` public HttpClientConfiguration getHttpClientConfig() { - return httpClientConfiguration; + return configuration.getHttpClientConfig(); }Then remove the now-unused field:
- private final HttpClientConfiguration httpClientConfiguration = new HttpClientConfiguration();🤖 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/router/TestRouter.java` around lines 150 - 153, TestRouter currently returns its local httpClientConfiguration in getHttpClientConfig(), which diverges from DefaultRouter and DummyTestRouter; change getHttpClientConfig() to delegate to configuration.getHttpClientConfig() (i.e., return configuration.getHttpClientConfig()), and remove the now-unused httpClientConfiguration field and any associated initialization to keep behavior consistent with DefaultRouter and DummyTestRouter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/test/java/com/predic8/membrane/core/router/TestRouter.java`:
- Around line 150-153: TestRouter currently returns its local
httpClientConfiguration in getHttpClientConfig(), which diverges from
DefaultRouter and DummyTestRouter; change getHttpClientConfig() to delegate to
configuration.getHttpClientConfig() (i.e., return
configuration.getHttpClientConfig()), and remove the now-unused
httpClientConfiguration field and any associated initialization to keep behavior
consistent with DefaultRouter and DummyTestRouter.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.javacore/src/main/java/com/predic8/membrane/core/transport/Transport.javacore/src/test/java/com/predic8/membrane/core/proxies/UnavailableSoapProxyTest.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/transport/http/client/HttpClientConfigurationTest.java
💤 Files with no reviewable changes (1)
- core/src/test/java/com/predic8/membrane/core/transport/http/client/HttpClientConfigurationTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java
- core/src/main/java/com/predic8/membrane/core/transport/Transport.java
predic8
requested changes
Feb 26, 2026
…lity and consistency
…dlHttpClientConfig`.
…nstructors to improve dependency management.
…entConfiguration` handling.
…ctory` parameter from `ResolverMap` and `HTTPSchemaResolver`.
…er factory handling
…olverMap` constructor
predic8
previously requested changes
Feb 27, 2026
…t` method in `HTTPClientInterceptor`.
…thod in `HTTPClientInterceptor`.
Closed
rrayst
approved these changes
Mar 2, 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
3 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
Tests