Status code change in response from 4XX to 500#2949
Open
predic8 wants to merge 3 commits into
Open
Conversation
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)
📝 WalkthroughWalkthroughThis PR centralizes response-status handling: ProblemDetails adds a helper that maps certain 4XX response codes to 500 in response/Exchange contexts; Xml2JsonInterceptor now always sets 400 on transform errors; XML parse-error detection is broadened via substring matches; tests and imports are cleaned up accordingly. ChangesError Handling and XML Detection Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
127-141: ⚡ Quick winConsider case-insensitive matching for cleaner intent.
Using
"rolog"and"ontent"as substring fragments works but is fragile and unclear. Since the helper method centralizes the logic, updating it to use case-insensitive matching would be cleaner:♻️ Suggested fix
private static boolean causeMessageContains(Throwable t, String fragment) { for (Throwable c = t; c != null; c = c.getCause()) { String m = c.getMessage(); - if (m != null && m.contains(fragment)) + if (m != null && m.toLowerCase().contains(fragment.toLowerCase())) return true; } return false; }Then use the full words:
- // Matches: prolog and Prolog - if (causeMessageContains(e, "rolog")) { + if (causeMessageContains(e, "prolog")) { ... - // Matches: Content and content - if (causeMessageContains(e, "ontent")) { + if (causeMessageContains(e, "content")) {🤖 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/lang/xpath/XPathExchangeExpression.java` around lines 127 - 141, The current checks in XPathExchangeExpression use fragile substrings ("rolog"/"ontent"); update the logic to be case-insensitive and use full words instead: change the calls in XPathExchangeExpression to check for "prolog" and "content" (instead of "rolog"/"ontent") and modify the helper causeMessageContains to perform a case-insensitive contains (e.g., normalize both the exception message and the needle to lower-case) so ExchangeExpressionException creation (expression, e), body(...) calls (msg.getBodyAsStringDecoded(), truncateAfter, tail) continue to work unchanged but the matching is robust and clearer.core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.java (1)
82-92: ⚡ Quick winConsider case-insensitive matching instead of partial substring.
Using
"rolog"as a substring match is fragile and could theoretically match unintended words. A cleaner approach that explicitly conveys the case-insensitive intent:cause.getMessage().toLowerCase().contains("prolog")Also note the inconsistency: this check now uses a short fragment, but line 92 still uses the full phrase
"is not allowed in trailing section". If parser variations are a concern for prolog messages, the same likely applies to trailing section messages.♻️ Suggested fix using case-insensitive matching
- // rolog matches Prolog and prolog - if (cause.getMessage() != null && cause.getMessage().contains("rolog")) { + if (cause.getMessage() != null && cause.getMessage().toLowerCase().contains("prolog")) {And for consistency at line 92:
- if (cause.getMessage() != null && cause.getMessage().contains("is not allowed in trailing section")) { + if (cause.getMessage() != null && cause.getMessage().toLowerCase().contains("trailing section")) {🤖 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/xslt/XSLTInterceptor.java` around lines 82 - 92, The current fragile substring check in XSLTInterceptor using cause.getMessage().contains("rolog") should be made case-insensitive and explicit by checking cause.getMessage() != null and using toLowerCase().contains("prolog") (update the condition around the prolog-handling branch that builds the user(...) response), and make the trailing-section check consistent by similarly normalizing case for the "is not allowed in trailing section" check; ensure you only alter the conditional expressions (around cause.getMessage()) and leave the user(...).title(...).detail(...).internal(...).stacktrace(false).buildAndSetResponse(exc) flow intact.
🤖 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/exceptions/ProblemDetails.java`:
- Around line 371-373: The current check in correctStatusCodeForResponse uses
exc.getResponse() != null to detect response flow which is too broad; change the
condition to detect actual response flow (e.g., use an explicit
Exchange.isResponseFlow() or equivalent flow flag/property) instead of presence
of a Response object, and only up-convert 4xx to 500 when that true
response-flow check indicates we're in the response path; if Exchange lacks such
a helper, add/consume a clear boolean (e.g., isResponseFlow or a RESPONSE_FLOW
exchange property) and update correctStatusCodeForResponse to rely on it rather
than getResponse() != null.
- Around line 354-355: The problem is that createMap() is using the original
status while you call statusCode(correctStatusCodeForResponse(...)) afterward,
causing HTTP status and the "status" field to diverge; fix by computing the
corrected status first (call correctStatusCodeForResponse(exchange, status) once
into a local correctedStatus), use that correctedStatus when creating the
builder (statusCode(correctedStatus)) and ensure createMap() is invoked
with/after that correctedStatus (or accept correctedStatus as a parameter) so
the map's "status" field matches the HTTP response status; update references
around statusCode(...), correctStatusCodeForResponse(...), createMap(), and the
builder variable accordingly.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.java`:
- Around line 82-92: The current fragile substring check in XSLTInterceptor
using cause.getMessage().contains("rolog") should be made case-insensitive and
explicit by checking cause.getMessage() != null and using
toLowerCase().contains("prolog") (update the condition around the
prolog-handling branch that builds the user(...) response), and make the
trailing-section check consistent by similarly normalizing case for the "is not
allowed in trailing section" check; ensure you only alter the conditional
expressions (around cause.getMessage()) and leave the
user(...).title(...).detail(...).internal(...).stacktrace(false).buildAndSetResponse(exc)
flow intact.
In
`@core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java`:
- Around line 127-141: The current checks in XPathExchangeExpression use fragile
substrings ("rolog"/"ontent"); update the logic to be case-insensitive and use
full words instead: change the calls in XPathExchangeExpression to check for
"prolog" and "content" (instead of "rolog"/"ontent") and modify the helper
causeMessageContains to perform a case-insensitive contains (e.g., normalize
both the exception message and the needle to lower-case) so
ExchangeExpressionException creation (expression, e), body(...) calls
(msg.getBodyAsStringDecoded(), truncateAfter, tail) continue to work unchanged
but the matching is robust and clearer.
🪄 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: a61f8891-990c-4b8c-bf8f-82387fad832b
📒 Files selected for processing (9)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.javacore/src/main/java/com/predic8/membrane/core/interceptor/xml/Xml2JsonInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptor.javacore/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.javacore/src/test/java/com/predic8/membrane/core/interceptor/xml/Xml2JsonInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/xslt/XSLTInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/exceptions/ExceptionInterceptorTest.java
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java
Comment on lines
+354
to
355
| var builder = statusCode(correctStatusCodeForResponse(exchange,status)); | ||
| try { |
Contributor
There was a problem hiding this comment.
HTTP status and problem payload status can diverge after remapping.
Line 354 applies corrected HTTP status, but createMap() already wrote the original status field. This can return HTTP 500 with body "status": 400, which is inconsistent for Problem Details consumers.
Proposed fix
- private Response createContent(Map<String, Object> root, Exchange exchange) {
- var builder = statusCode(correctStatusCodeForResponse(exchange,status));
+ private Response createContent(Map<String, Object> root, Exchange exchange) {
+ var effectiveStatus = correctStatusCodeForResponse(exchange, status);
+ root.put(STATUS, effectiveStatus);
+ var builder = statusCode(effectiveStatus);
try {🤖 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/exceptions/ProblemDetails.java`
around lines 354 - 355, The problem is that createMap() is using the original
status while you call statusCode(correctStatusCodeForResponse(...)) afterward,
causing HTTP status and the "status" field to diverge; fix by computing the
corrected status first (call correctStatusCodeForResponse(exchange, status) once
into a local correctedStatus), use that correctedStatus when creating the
builder (statusCode(correctedStatus)) and ensure createMap() is invoked
with/after that correctedStatus (or accept correctedStatus as a parameter) so
the map's "status" field matches the HTTP response status; update references
around statusCode(...), correctStatusCodeForResponse(...), createMap(), and the
builder variable accordingly.
Member
Author
There was a problem hiding this comment.
statusCode(..) sets the statusCode on the builder
Contributor
There was a problem hiding this comment.
@predic8, you're correct that statusCode(correctStatusCodeForResponse(exchange, status)) properly sets the HTTP response line (e.g., HTTP/1.1 500). My concern is about a different thing — the "status" field inside the JSON problem-details body.
createMap() is called before createContent, and it writes:
root.put(STATUS, status); // e.g. 400 — the *original* field valueThen createContent receives that already-populated map and sets the corrected HTTP status on the builder. The result is:
HTTP/1.1 500 Internal Server Error ← builder: corrected to 500 ✅
Content-Type: application/problem+json
{
"status": 400, ← body field: still original 400 ❌
...
}
Per RFC 9457 §3.1, the "status" member "is advisory; it conveys the HTTP status code used for the convenience of the consumer", so it should match the actual HTTP status. A Problem Details consumer relying on that field would see an inconsistency here.
The minimal fix is to update the "status" entry in root inside createContent after the remapping:
private Response createContent(Map<String, Object> root, Exchange exchange) {
- var builder = statusCode(correctStatusCodeForResponse(exchange, status));
+ var effectiveStatus = correctStatusCodeForResponse(exchange, status);
+ root.put(STATUS, effectiveStatus); // keep body in sync with HTTP status
+ var builder = statusCode(effectiveStatus);
try {Would you like to address this, or should I open a follow-up issue for it?
🧠 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 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.
Summary by CodeRabbit
Bug Fixes
Tests