Handle specific exceptions in HTTPClientInterceptor with detailed res…#2945
Handle specific exceptions in HTTPClientInterceptor with detailed res…#2945predic8 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughHTTPClientInterceptor is updated to use explicit imports instead of wildcards and to refine exception handling in the ChangesHTTPClientInterceptor robustness improvement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java`:
- Around line 123-127: The 504 response builder call
(internal(router.getConfiguration().isProduction(),
getDisplayName()).title("Gateway
Timeout").status(504).addSubSee("socket-timeout").buildAndSetResponse(exc)) is
missing a .detail(...) message; update that chain to include a concise timeout
detail (e.g., explain that the backend did not respond within the socket timeout
and suggest retrying or checking upstream) so clients receive actionable
information—modify the builder invocation in HTTPClientInterceptor to call
.detail(...) before buildAndSetResponse(exc).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 505c2723-b31c-4e05-8f2d-e03dd5d49854
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java
| internal(router.getConfiguration().isProduction(), getDisplayName()) | ||
| .title("Gateway Timeout") | ||
| .status(504) | ||
| .addSubSee("socket-timeout") | ||
| .detail("Target %s is not reachable.".formatted(exc.getDestinations())) | ||
| .buildAndSetResponse(exc); |
There was a problem hiding this comment.
Add timeout detail text to the 504 response.
Line 123–127 currently omits .detail(...), which makes timeout failures less actionable for clients than the other new handlers.
Suggested patch
} catch (SocketTimeoutException e) {
// Details are logged further down in the HTTPClient
log.info("Target {} is not reachable.",exc.getDestinations());
internal(router.getConfiguration().isProduction(), getDisplayName())
.title("Gateway Timeout")
.status(504)
.addSubSee("socket-timeout")
+ .detail("Timed out while waiting for a response from target %s.".formatted(exc.getDestinations()))
.buildAndSetResponse(exc);
return ABORT;📝 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.
| internal(router.getConfiguration().isProduction(), getDisplayName()) | |
| .title("Gateway Timeout") | |
| .status(504) | |
| .addSubSee("socket-timeout") | |
| .detail("Target %s is not reachable.".formatted(exc.getDestinations())) | |
| .buildAndSetResponse(exc); | |
| internal(router.getConfiguration().isProduction(), getDisplayName()) | |
| .title("Gateway Timeout") | |
| .status(504) | |
| .addSubSee("socket-timeout") | |
| .detail("Timed out while waiting for a response from target %s.".formatted(exc.getDestinations())) | |
| .buildAndSetResponse(exc); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java`
around lines 123 - 127, The 504 response builder call
(internal(router.getConfiguration().isProduction(),
getDisplayName()).title("Gateway
Timeout").status(504).addSubSee("socket-timeout").buildAndSetResponse(exc)) is
missing a .detail(...) message; update that chain to include a concise timeout
detail (e.g., explain that the backend did not respond within the socket timeout
and suggest retrying or checking upstream) so clients receive actionable
information—modify the builder invocation in HTTPClientInterceptor to call
.detail(...) before buildAndSetResponse(exc).
There was a problem hiding this comment.
@coderabbitai should we really reveal the backend addresses to the client?
There was a problem hiding this comment.
@predic8 That's a valid security concern — exposing backend addresses leaks internal network topology to potentially untrusted clients, which is generally undesirable in production environments.
I also notice that the PR deliberately removed the previous .detail("Target %s is not reachable.".formatted(exc.getDestinations())) from the SocketTimeoutException handler, which suggests this was likely intentional.
There are two reasonable paths:
-
Keep no detail (current state) — clients get a
504 Gateway Timeoutwith thesocket-timeoutsubtype, which is actionable enough without revealing internals. Thelog.info(...)already captures the destination server-side. -
Use a generic, address-free message — if a detail is still desired for client usability:
} catch (SocketTimeoutException e) {
log.info("Target {} is not reachable.", exc.getDestinations());
internal(router.getConfiguration().isProduction(), getDisplayName())
.title("Gateway Timeout")
.status(504)
.addSubSee("socket-timeout")
+ .detail("The backend did not respond within the timeout period.")
.buildAndSetResponse(exc);
return ABORT;It may also be worth reviewing the SocketException handler at line 107–112, which does include exc.getDestinations() in its detail — the same concern applies there.
✏️ Learnings added
Learnt from: predic8
Repo: membrane/api-gateway PR: 2945
File: core/src/main/java/com/predic8/membrane/core/interceptor/HTTPClientInterceptor.java:123-127
Timestamp: 2026-05-14T08:54:25.466Z
Learning: In membrane/api-gateway (HTTPClientInterceptor.java), the PR owner (predic8) considers it a security concern to expose backend addresses (exc.getDestinations()) in HTTP error response detail messages, as it leaks internal network topology to clients. Prefer generic messages (e.g., "The backend did not respond within the timeout period.") over destination-formatted strings in ProblemDetails `.detail(...)` calls.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rrayst
Repo: membrane/api-gateway PR: 2863
File: core/src/main/java/com/predic8/membrane/core/interceptor/server/WSDLPublisherInterceptor.java:107-107
Timestamp: 2026-03-11T21:11:47.936Z
Learning: When reviewing refactors that extract logic from a synchronized block into a new method, verify that the synchronization guard (the GuardedBy target, e.g., the lock on paths) is carried into the extracted method and not dropped. Ensure shared fields annotated with GuardedBy(
|
This pull request needs "/ok-to-test" from an authorized committer. |
1 participant
…ponses
Summary by CodeRabbit