Allow customization of underlying Netty HttpClient#737
Open
Periecle wants to merge 1 commit into
Open
Conversation
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces a pluggable customization mechanism for Netty HTTP clients in Feign Reactor. A new functional interface Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant Config as ReactiveFeignClientsConfiguration
participant Builder as WebReactiveFeign.Builder
participant ConnBuilder as NettyClientHttpConnectorBuilder
participant HttpClient as Netty HttpClient
User->>Config: reactiveFeignBuilder(..., customizers)
Config->>Builder: create builder
Config->>Builder: addCustomizer() for each customizer
Builder->>Builder: store customizers in list
User->>Builder: build()
Builder->>ConnBuilder: buildNettyClientHttpConnector(..., customizers)
ConnBuilder->>HttpClient: create HttpClient
ConnBuilder->>HttpClient: apply each customizer
HttpClient-->>ConnBuilder: modified HttpClient
ConnBuilder-->>Builder: ReactorClientHttpConnector
Builder-->>User: WebReactiveFeign client ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (1)
feign-reactor-webclient/src/test/java/reactivefeign/webclient/ClientCustomizerTest.java (1)
17-23: Consider adding coverage for the Spring bean-injection path.This test validates direct builder usage, but the PR also changes
ReactiveFeignClientsConfigurationto injectList<NettyHttpClientCustomizer>. A focused test for that wiring would reduce regression risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feign-reactor-webclient/src/test/java/reactivefeign/webclient/ClientCustomizerTest.java` around lines 17 - 23, Add a focused Spring context test that exercises the bean-injection path for ReactiveFeignClientsConfiguration by loading the application context (reuse ClientCustomizerTest/TestController and `@SpringBootTest`) and defining a `@TestConfiguration` that declares a NettyHttpClientCustomizer bean; in the test method autowire List<NettyHttpClientCustomizer> (or the reactivefeign configuration class) and assert the injected list contains the test bean to validate the new injection wiring in ReactiveFeignClientsConfiguration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feign-reactor-webclient/src/main/java/reactivefeign/webclient/NettyClientHttpConnectorBuilder.java`:
- Around line 135-138: Guard against a customizer returning null by adding an
explicit null-check after calling NettyHttpClientCustomizer.apply in
NettyClientHttpConnectorBuilder; after each httpClient =
customizer.apply(httpClient) verify httpClient != null and if it is null throw a
clear IllegalStateException (or similar) that includes the customizer's
class/name and context (e.g., "NettyHttpClientCustomizer X returned null from
apply"), so connector construction fails fast with a useful message.
In
`@feign-reactor-webclient/src/main/java/reactivefeign/webclient/WebReactiveFeign.java`:
- Around line 72-75: In Builder<T>.addCustomizer(NettyHttpClientCustomizer
customizer) validate input and fail fast by checking if customizer is null and
throwing a clear exception (e.g., NullPointerException or
IllegalArgumentException) instead of accepting null into
nettyHttpClientCustomizers; update the method so it performs the null check
before adding to the nettyHttpClientCustomizers collection and returns this as
before to preserve fluent API semantics.
---
Nitpick comments:
In
`@feign-reactor-webclient/src/test/java/reactivefeign/webclient/ClientCustomizerTest.java`:
- Around line 17-23: Add a focused Spring context test that exercises the
bean-injection path for ReactiveFeignClientsConfiguration by loading the
application context (reuse ClientCustomizerTest/TestController and
`@SpringBootTest`) and defining a `@TestConfiguration` that declares a
NettyHttpClientCustomizer bean; in the test method autowire
List<NettyHttpClientCustomizer> (or the reactivefeign configuration class) and
assert the injected list contains the test bean to validate the new injection
wiring in ReactiveFeignClientsConfiguration.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feign-reactor-spring-configuration/src/main/java/reactivefeign/spring/config/ReactiveFeignClientsConfiguration.javafeign-reactor-webclient/src/main/java/reactivefeign/webclient/NettyClientHttpConnectorBuilder.javafeign-reactor-webclient/src/main/java/reactivefeign/webclient/NettyHttpClientCustomizer.javafeign-reactor-webclient/src/main/java/reactivefeign/webclient/WebReactiveFeign.javafeign-reactor-webclient/src/test/java/reactivefeign/webclient/ClientCustomizerTest.java
Comment on lines
+135
to
+138
| if (customizers != null) { | ||
| for (NettyHttpClientCustomizer customizer : customizers) { | ||
| httpClient = customizer.apply(httpClient); | ||
| } |
There was a problem hiding this comment.
Guard against customizers returning null HttpClient.
A misbehaving customizer can return null and break connector construction with a non-obvious failure. Add an explicit null-check after apply.
Suggested fix
if (customizers != null) {
for (NettyHttpClientCustomizer customizer : customizers) {
- httpClient = customizer.apply(httpClient);
+ httpClient = Objects.requireNonNull(
+ customizer.apply(httpClient),
+ "NettyHttpClientCustomizer must not return null HttpClient");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (customizers != null) { | |
| for (NettyHttpClientCustomizer customizer : customizers) { | |
| httpClient = customizer.apply(httpClient); | |
| } | |
| if (customizers != null) { | |
| for (NettyHttpClientCustomizer customizer : customizers) { | |
| httpClient = Objects.requireNonNull( | |
| customizer.apply(httpClient), | |
| "NettyHttpClientCustomizer must not return null HttpClient"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feign-reactor-webclient/src/main/java/reactivefeign/webclient/NettyClientHttpConnectorBuilder.java`
around lines 135 - 138, Guard against a customizer returning null by adding an
explicit null-check after calling NettyHttpClientCustomizer.apply in
NettyClientHttpConnectorBuilder; after each httpClient =
customizer.apply(httpClient) verify httpClient != null and if it is null throw a
clear IllegalStateException (or similar) that includes the customizer's
class/name and context (e.g., "NettyHttpClientCustomizer X returned null from
apply"), so connector construction fails fast with a useful message.
Comment on lines
+72
to
+75
| public Builder<T> addCustomizer(NettyHttpClientCustomizer customizer){ | ||
| this.nettyHttpClientCustomizers.add(customizer); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Fail fast on null customizer input.
addCustomizer(null) is currently accepted and defers failure to connector creation time. Validate eagerly to keep error locality clear.
Suggested fix
public Builder<T> addCustomizer(NettyHttpClientCustomizer customizer){
- this.nettyHttpClientCustomizers.add(customizer);
+ this.nettyHttpClientCustomizers.add(
+ Objects.requireNonNull(customizer, "customizer must not be null"));
return this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public Builder<T> addCustomizer(NettyHttpClientCustomizer customizer){ | |
| this.nettyHttpClientCustomizers.add(customizer); | |
| return this; | |
| } | |
| public Builder<T> addCustomizer(NettyHttpClientCustomizer customizer){ | |
| this.nettyHttpClientCustomizers.add( | |
| Objects.requireNonNull(customizer, "customizer must not be null")); | |
| return this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feign-reactor-webclient/src/main/java/reactivefeign/webclient/WebReactiveFeign.java`
around lines 72 - 75, In Builder<T>.addCustomizer(NettyHttpClientCustomizer
customizer) validate input and fail fast by checking if customizer is null and
throwing a clear exception (e.g., NullPointerException or
IllegalArgumentException) instead of accepting null into
nettyHttpClientCustomizers; update the method so it performs the null check
before adding to the nettyHttpClientCustomizers collection and returns this as
before to preserve fluent API semantics.
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
1 participant
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.
Implemented feature request #690 to allow customization of the underlying Netty HttpClient.
This is achieved by adding a clientCustomizer field (Function<HttpClient, HttpClient>) to WebReactiveOptions and applying it in NettyClientHttpConnectorBuilder.
Users can now provide a customizer via WebReactiveOptions.Builder to tweak the HttpClient configuration (e.g., DNS resolver, timeouts, etc.) beyond what was previously exposed.
Added integration test ClientCustomizerTest to verify that the customizer is applied correctly.
Summary by CodeRabbit
New Features
Tests