Replaced IOException during body access with custom ReadingBodyException & WritingBodyException#2800
Open
Replaced IOException during body access with custom ReadingBodyException & WritingBodyException#2800
IOException during body access with custom ReadingBodyException & WritingBodyException#2800Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request systematically converts checked IOException exceptions to unchecked exceptions (ReadingBodyException and WritingBodyException) throughout the HTTP body handling and related code. Public methods in AbstractBody and its subclasses no longer declare IOException; instead, IOExceptions are wrapped into runtime exceptions. Callers across interceptors and utilities are updated to handle the new exception types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Member
Author
|
/ok-to-test |
Member
Author
|
@coderabbitai plz review |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
core/src/main/java/com/predic8/membrane/core/interceptor/ntlm/NtlmInterceptor.java (1)
78-78:⚠️ Potential issue | 🟠 MajorUnguarded
ReadingBodyExceptionat thehandleResponsecall site will propagate as an uncaught runtime exception.The previous code wrapped
getContent()in a try/catch that swallowed/loggedIOException. Now that the method throws the uncheckedReadingBodyException, the call at line 78 is completely unprotected. If the body read fails, the exception escapeshandleResponse()entirely, bypassing NTLM logic and likely resulting in a generic 500 rather than a controlled response.The call at line 112 (inside
authenticate) happens to be safe because the surroundingcatch (Exception e)on line 94 catchesRuntimeExceptionsubtypes — but note the error detail"Could not authenticate!"would be misleading if the actual failure was a body-read error.🛡️ Proposed fix: guard both call sites appropriately
- private void prepareStreamByEmptyingIt(Exchange exc) { - exc.getResponse().getBody().getContent(); - } + private void prepareStreamByEmptyingIt(Exchange exc) { + try { + exc.getResponse().getBody().getContent(); + } catch (ReadingBodyException e) { + log.debug("Failed to empty response body stream: {}", e.getMessage(), e); + } + }Based on learnings, body reading failures in interceptors should be caught and logged at
debuglevel rather than allowed to propagate, to avoid cluttering error logs with common stream-reading issues while still surfacing them when debug logging is enabled.Also applies to: 140-142
🤖 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/ntlm/NtlmInterceptor.java` at line 78, handle the unchecked ReadingBodyException thrown when reading the message body so it doesn't escape handleResponse()/authenticate(): wrap calls to prepareStreamByEmptyingIt(exc) and any direct getContent() body reads in a try/catch that catches ReadingBodyException (or RuntimeException if ReadingBodyException is not importable) and log the failure at debug level (using the class logger) instead of letting it propagate; after catching, proceed with the NTLM logic (e.g., return/continue as existing flow expects) so authentication paths still run and error messages (like "Could not authenticate!") remain accurate.core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java (1)
62-72:⚠️ Potential issue | 🟠 MajorCatch block does not handle
ReadingBodyExceptionthrown bygetBodyAsStreamDecoded()andsetBodyContent().The catch block only catches
IOException, but:
msg.getBodyAsStreamDecoded()throwsReadingBodyException(unchecked)msg.setBodyContent(...)throwsReadingBodyException(unchecked)Both are runtime exceptions that will propagate unhandled, breaking the "swallow and continue" contract.
Add a catch for
ReadingBodyException:🔧 Proposed fix
- } catch (IOException e) { + } catch (ReadingBodyException e) { + // Body I/O failed; beautification not possible – continue as-is + log.info("Could not beautify message body: {}", e.getMessage()); + } catch (IOException e) { // If it is not possible to beautify, to nothing // Cause will be often user input => do not log stacktrace log.info("Could not beautify message body: {}", e.getMessage()); }🤖 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/beautifier/BeautifierInterceptor.java` around lines 62 - 72, The catch currently only handles IOException but msg.getBodyAsStreamDecoded() and msg.setBodyContent(...) can throw the unchecked ReadingBodyException, so update BeautifierInterceptor to also catch ReadingBodyException (either as a separate catch or combined) around the try that calls getPrettifier(...), msg.getBodyAsStreamDecoded() and msg.setBodyContent(...); in that catch log the same info-level message as for IOException (no stacktrace) and ensure the method still returns CONTINUE so beautifier failures are swallowed as intended.core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (1)
88-95:⚠️ Potential issue | 🟡 MinorWrap body retrieval in try-catch to prevent
ReadingBodyExceptionfrom silently swallowing theMismatchedInputException.If
getBodyAsStringDecoded()(line 89) throwsReadingBodyException, it will escape the currentMismatchedInputExceptionhandler and be caught by the broadcatch (Exception e)at line 96, causing the original exception context to be lost.Proposed fix
} catch (MismatchedInputException e) { - String body = exchange.getMessage(flow).getBodyAsStringDecoded(); - if (body == null || body.isEmpty()) { - log.info("Error evaluating Jsonpath {}. Body is empty!", expression); - } else { - log.info("Error evaluating Jsonpath {}. Body is: {}", expression, truncateAfter(body, 200)); + try { + String body = exchange.getMessage(flow).getBodyAsStringDecoded(); + if (body == null || body.isEmpty()) { + log.info("Error evaluating Jsonpath {}. Body is empty!", expression); + } else { + log.info("Error evaluating Jsonpath {}. Body is: {}", expression, truncateAfter(body, 200)); + } + } catch (ReadingBodyException rbe) { + log.debug("Could not read body for error context: {}", ExceptionUtil.concatMessageAndCauseMessages(rbe)); } throw new ExchangeExpressionException(expression, e); }🤖 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/lang/jsonpath/JsonpathExchangeExpression.java` around lines 88 - 95, In JsonpathExchangeExpression (inside the catch (MismatchedInputException e) block) wrap the call to exchange.getMessage(flow).getBodyAsStringDecoded() in its own try-catch that catches ReadingBodyException so that a ReadingBodyException thrown while retrieving the body does not escape and replace the original MismatchedInputException; if ReadingBodyException occurs, log a message indicating the body could not be read (including the ReadingBodyException details) and then continue to throw the ExchangeExpressionException constructed from the original MismatchedInputException (e) so the original parsing error context is preserved.core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java (1)
199-201:⚠️ Potential issue | 🟡 Minor
analyseSOAPMessagesilently swallowsReadingBodyException, inconsistent withisSOAP.
isSOAP(lines 56–57) now explicitly rethrowsReadingBodyException, butanalyseSOAPMessagelets the same exception fall into the genericcatch (Exception e)and returnsNO_SOAP_RESULT. A body-read failure is indistinguishable from a non-SOAP message, potentially causing incorrect routing decisions downstream.Apply the same rethrow guard used in
isSOAP:🐛 Proposed fix
+ } catch (ReadingBodyException e) { + throw e; } catch (Exception e) { log.warn("Ignoring exception: ", e); }🤖 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/util/SOAPUtil.java` around lines 199 - 201, The catch-all in analyseSOAPMessage currently swallows ReadingBodyException making body-read failures indistinguishable from non-SOAP results; update analyseSOAPMessage to detect and rethrow ReadingBodyException just like isSOAP does (i.e., if (e instanceof ReadingBodyException) throw e), so ReadingBodyException is propagated up instead of returning NO_SOAP_RESULT; locate the catch(Exception e) in analyseSOAPMessage and add the same rethrow guard referencing ReadingBodyException and the analyseSOAPMessage method.core/src/main/java/com/predic8/membrane/core/http/Message.java (2)
136-142:⚠️ Potential issue | 🟠 Major
getBodyAsStringDecoded()will double-wrapReadingBodyException.
MessageUtil.getContent(this)can throwReadingBodyException(unchecked), which thecatch (Exception e)on line 139 would intercept and re-wrap into a newReadingBodyException(e). This creates a redundant wrapper chain.The sibling method
getBodyAsStreamDecoded()(line 116-117) correctly handles this by re-throwingReadingBodyExceptionbefore the generalcatch (Exception e). Apply the same pattern here.🐛 Proposed fix
public String getBodyAsStringDecoded() throws ReadingBodyException { try { return new String(MessageUtil.getContent(this), getCharsetOrDefault()); + } catch (ReadingBodyException e) { + throw e; } catch (Exception e) { throw new ReadingBodyException(e); } }🤖 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/http/Message.java` around lines 136 - 142, The method getBodyAsStringDecoded currently catches Exception and always wraps it in a new ReadingBodyException, causing double-wrapping when MessageUtil.getContent(this) already throws ReadingBodyException; change the catch handling to first re-throw ReadingBodyException unchanged and only wrap other Exceptions (mirror the pattern used in getBodyAsStreamDecoded): detect if the caught exception is an instance of ReadingBodyException and re-throw it, otherwise wrap the exception in new ReadingBodyException before throwing, referencing getBodyAsStringDecoded, MessageUtil.getContent(this), and ReadingBodyException.
397-409:⚠️ Potential issue | 🟡 MinorRedundant wrapping:
ReadingBodyExceptionis already aRuntimeException.Line 401-402 catches
ReadingBodyExceptionand wraps it innew RuntimeException(e). SinceReadingBodyExceptionalready extendsRuntimeException, this adds a needless wrapper layer that obscures the original exception type in stack traces.🐛 Proposed fix — rethrow directly
public void bodyComplete(AbstractBody body2) { try { result.setBody(getBody(body2)); } catch (ReadingBodyException e) { - throw new RuntimeException(e); + throw e; }🤖 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/http/Message.java` around lines 397 - 409, In bodyComplete (in Message.java) avoid wrapping ReadingBodyException in a new RuntimeException; since ReadingBodyException already extends RuntimeException, either remove the try/catch around result.setBody(getBody(body2)) or rethrow the caught exception directly (throw e) so the original exception type and stack trace are preserved; locate the bodyComplete method that calls result.setBody(getBody(body2)) and replace the wrapping new RuntimeException(e) with a direct rethrow of the ReadingBodyException.
🧹 Nitpick comments (7)
core/src/main/java/com/predic8/membrane/core/proxies/StatisticCollector.java (1)
94-94: Log level and exception representation don't match the established pattern.Two issues with the updated log call:
- Root cause silently dropped —
e.getMessage()only surfaces theReadingBodyExceptionmessage; if it wraps an underlyingIOException(the typical conversion pattern), the root cause is swallowed entirely.warnlevel is too loud — body-read failures during statistics counting are likely common/benign. The repo's established pattern islog.debug()for these cases.💡 Proposed fix aligning with the established pattern
- } catch (ReadingBodyException e) { - log.warn("Could not count statistic: {}", e.getMessage()); + } catch (ReadingBodyException e) { + log.debug("Could not count statistic: {}", ExceptionUtil.concatMessageAndCauseMessages(e)); }Also add the import if not already present via a wildcard:
import com.predic8.membrane.core.util.ExceptionUtil; // adjust package as neededBased on learnings: In Java exception handling for body logging, use
log.debug()level withExceptionUtil.concatMessageAndCauseMessages(e)to log body reading failures, preventing clutter in info/warn logs while still providing full error chain details when debug logging is enabled.🤖 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/proxies/StatisticCollector.java` at line 94, In StatisticCollector (the place with the log.warn("Could not count statistic: {}", e.getMessage())), change the logging to use debug level and include the full exception chain by calling ExceptionUtil.concatMessageAndCauseMessages(e) instead of e.getMessage(); also ensure ExceptionUtil is imported (e.g. import com.predic8.membrane.core.util.ExceptionUtil) so the debug log reads the full concatenated message and avoids using warn for benign body-read failures.core/src/main/java/com/predic8/membrane/core/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java (2)
29-31: Downgrade log level todebugand avoid double-wrapping the uncheckedReadingBodyException.Two issues with the catch block after this PR's changes:
Log level:
log.warnfor a body-reading failure clutters operational logs. Based on learnings from PR#1977,log.debug()withExceptionUtil.concatMessageAndCauseMessages(e)is the established pattern for body-reading failures in this codebase.Redundant re-wrap:
MessageUtil.getContent(...)now throwsReadingBodyException, which is already aRuntimeException. Catching it and re-wrapping withnew RuntimeException(e)adds an extra, unhelpful layer to the stack trace. The simplest fix is to let the unchecked exception propagate naturally and remove the try/catch entirely; alternatively, rethrow the caught exception directly.♻️ Proposed refactor
- try { - return new String(MessageUtil.getContent(body.getMessage()), body.getMessage().getCharsetOrDefault()); - } catch (Exception e) { - log.warn("Cannot log body content", e); - throw new RuntimeException(e); - } + var message = body.getMessage(); + return new String(MessageUtil.getContent(message), message.getCharsetOrDefault());If logging on failure is still desired:
try { - return new String(MessageUtil.getContent(body.getMessage()), body.getMessage().getCharsetOrDefault()); - } catch (Exception e) { - log.warn("Cannot log body content", e); - throw new RuntimeException(e); + var message = body.getMessage(); + return new String(MessageUtil.getContent(message), message.getCharsetOrDefault()); + } catch (ReadingBodyException e) { + log.debug("Cannot convert body to String: {}", ExceptionUtil.concatMessageAndCauseMessages(e)); + throw e; }Based on learnings from PR
#1977: body reading failures should be logged atlog.debug()level usingExceptionUtil.concatMessageAndCauseMessages(e)to avoid cluttering operational logs.🤖 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/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java` around lines 29 - 31, The catch block in SpELBodyToStringTypeConverter that catches Exception around MessageUtil.getContent should be changed: remove the try/catch so the unchecked ReadingBodyException can propagate (do not wrap it in new RuntimeException), or if you must log, replace log.warn(...) with log.debug(ExceptionUtil.concatMessageAndCauseMessages(e)) and rethrow the caught exception directly (throw e); reference MessageUtil.getContent(...), ReadingBodyException and ExceptionUtil.concatMessageAndCauseMessages(...) when applying the change.
28-28: Extractbody.getMessage()to a local variable to avoid the double call.
body.getMessage()is invoked twice on the same line. A local variable improves readability and avoids any potential double evaluation.♻️ Proposed refactor
- return new String(MessageUtil.getContent(body.getMessage()), body.getMessage().getCharsetOrDefault()); + var message = body.getMessage(); + return new String(MessageUtil.getContent(message), message.getCharsetOrDefault());🤖 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/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java` at line 28, In SpELBodyToStringTypeConverter (the method that returns the String), avoid calling body.getMessage() twice: extract body.getMessage() into a local variable (e.g., Message msg) and then use MessageUtil.getContent(msg) and msg.getCharsetOrDefault() to build the new String; this improves readability and prevents potential double evaluation.core/src/main/java/com/predic8/membrane/core/interceptor/log/access/AccessLogInterceptorService.java (1)
134-142: Redundant innertry/catch— consider removing or adding a debug log.The inner
try { … } catch (ReadingBodyException e) { return defaultValue; }is now redundant: sinceReadingBodyExceptionis unchecked, any unhandled instance will propagate up to the enclosingsafe(Supplier<Object>)call (lines 148–154), which already catchesExceptionand returns the samedefaultValue. Both paths produce identical results.If the explicit handling block is kept purely for clarity, consider adding a
log.debug()call to preserve visibility into body-read failures without cluttering info logs — consistent with the team's convention for body reading failures.♻️ Option A — remove the redundant inner try-catch
private Supplier<Object> getPayLoadSize(Message msg) { - return () -> { - try { - return msg.getBody().getLength(); - } catch (ReadingBodyException e) { - return defaultValue; - } - }; + return () -> msg.getBody().getLength(); }♻️ Option B — keep explicit catch, add debug log
private Supplier<Object> getPayLoadSize(Message msg) { return () -> { try { return msg.getBody().getLength(); } catch (ReadingBodyException e) { + log.debug("Could not read body length: {}", ExceptionUtil.concatMessageAndCauseMessages(e)); return defaultValue; } }; }Based on learnings, the team convention for body-read failures in logging paths is to use
log.debug()withExceptionUtil.concatMessageAndCauseMessages(e)to avoid cluttering info logs while retaining diagnostic detail.🤖 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/log/access/AccessLogInterceptorService.java` around lines 134 - 142, The inner try/catch inside getPayLoadSize is redundant because ReadingBodyException is unchecked and safe(Supplier<Object>) already catches exceptions; either remove the inner try/catch entirely from getPayLoadSize (leaving it to return msg.getBody().getLength() directly) or, if you want to keep explicit handling for clarity, add a debug log in the catch that uses log.debug(ExceptionUtil.concatMessageAndCauseMessages(e)) before returning defaultValue so body-read failures remain visible at debug level; update the Supplier<Object> returned by getPayLoadSize accordingly.core/src/main/java/com/predic8/membrane/core/interceptor/flow/CallInterceptor.java (1)
123-128:ReadingBodyExceptionfromsetRequestBodyescapeshandleInternalunhandled — consider catching it.
getNewRequest(exc)is called at line 80, before either try-catch block inhandleInternal. IfReadingBodyExceptionis thrown at line 127, it propagates all the way out ofhandleInternalwithout anABORToutcome or a structured error response being set on the exchange.This gap existed before (a
RuntimeExceptionwrapper escaped the same way), so it is not a regression introduced by this PR. That said, transitioning to a typed unchecked exception makes this a natural moment to close it.♻️ Proposed fix: move request construction inside a guarded block
private Outcome handleInternal(Exchange exc) { final String dest = exchangeExpression.evaluate(exc, REQUEST, String.class); log.debug("Calling {}", dest); - final Exchange newExc = createNewExchange(dest, getNewRequest(exc)); - - try (HttpClient client = new HttpClient()) { + final Exchange newExc; + try { + newExc = createNewExchange(dest, getNewRequest(exc)); + } catch (Exception e) { + log.error("Error building request for {}: {}", dest, e.getMessage(), e); + internal(router.isProduction(), getDisplayName()) + .addSubSee("internal-calling") + .detail("Internal call") + .exception(e) + .buildAndSetResponse(exc); + return ABORT; + } + + try (HttpClient client = new HttpClient()) { client.call(newExc); } catch (Exception e) {🤖 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/CallInterceptor.java` around lines 123 - 128, The call to getNewRequest(exc) can throw ReadingBodyException from setRequestBody and currently escapes handleInternal; to fix, move the request construction into the guarded try-catch inside handleInternal (or add a specific catch for ReadingBodyException around getNewRequest) so the exception is caught, the exchange outcome is set to ABORT, and an appropriate error response is set on the Exchange; update references to setRequestBody, getNewRequest, and handleInternal accordingly so ReadingBodyException is handled before it propagates.core/src/main/java/com/predic8/membrane/core/http/ChunkedBody.java (1)
227-256: Good separation of read vs. write exceptions, samemarkAsRead()concern asBody.java.The nested try-catch structure correctly routes read-path IOExceptions to
ReadingBodyExceptionand write-path IOExceptions toWritingBodyException. The comment on line 247 is helpful.Same note as in
Body.java: if either exception is thrown,markAsRead()(line 255) is skipped and observers are never notified. If this is intentional, consider adding a brief comment. If not, consider afinallyblock formarkAsRead().🤖 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/http/ChunkedBody.java` around lines 227 - 256, The method writeStreamed currently skips markAsRead() (and thus never notifies observers) when a ReadingBodyException or WritingBodyException is thrown; update writeStreamed (reference method writeStreamed in ChunkedBody) to ensure markAsRead() always runs by placing it in a finally block (or add a clear comment if skipping is intentional), and preserve the existing exception mapping (ReadingBodyException for read IO errors and WritingBodyException for write IO errors) so behavior of read/write exception handling (readChunkSize, readByteArray, out.write, out.finish) remains unchanged; also ensure observers.bodyChunk calls remain inside the read loop.core/src/main/java/com/predic8/membrane/core/http/Body.java (1)
155-184: Consider ensuringmarkAsRead()is called even when writing fails.If
WritingBodyExceptionis thrown (lines 172-173 or 180-181),markAsRead()on line 183 is never reached. This means observers are never notified of body completion. While the connection is likely torn down on error, any registeredMessageObserverwill be left dangling.If the intent is to always signal completion (even on failure),
markAsRead()could be moved into afinallyblock. If the current behavior is intentional (don't signal on write failure), a brief comment would help clarify.Also, the negated condition on line 163 is harder to parse than the original while-loop form. A minor readability nit — consider extracting it:
♻️ Readability suggestion for the read loop
- while (true) { - try { - if (!((this.length > totalLength || this.length == -1) && (length = inputStream.read(buffer)) > 0)) - break; - } catch (IOException e) { - throw new ReadingBodyException(e); - } + while (hasMoreToRead(totalLength)) { + try { + length = inputStream.read(buffer); + } catch (IOException e) { + throw new ReadingBodyException(e); + } + if (length <= 0) + break;Where
hasMoreToReadis:private boolean hasMoreToRead(long totalLength) { return this.length > totalLength || this.length == -1; }🤖 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/http/Body.java` around lines 155 - 184, The writeStreamed method can fail with WritingBodyException before markAsRead() is reached, leaving observers un-notified; ensure markAsRead() is always invoked by moving it into a finally block that surrounds the entire read/write/finish sequence (i.e., wrap the loop, out.finish(null) and associated try/catches in try { ... } finally { markAsRead(); }) and rethrow any caught WritingBodyException/ReadingBodyException so behavior remains unchanged, and optionally simplify the loop condition by extracting the total-length check into a helper like hasMoreToRead(long totalLength) to improve readability; refer to writeStreamed, markAsRead, out.finish, WritingBodyException, and ReadingBodyException when making changes.
🤖 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/balancer/BalancerHealthMonitor.java`:
- Around line 138-141: In BalancerHealthMonitor where ReadingBodyException is
caught (catch (ReadingBodyException e) { ... }), replace the current log.debug
call that uses e.getMessage() with log.debug and
ExceptionUtil.concatMessageAndCauseMessages(e) so the full cause chain is
logged; i.e., call log.debug("Calling health endpoint failed: {} {}", exc,
ExceptionUtil.concatMessageAndCauseMessages(e)) to retain deeper cause messages
for diagnostics.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java`:
- Around line 57-59: In BeautifierInterceptor.java, inside the catch block that
catches ReadingBodyException (the handler that currently calls log.error("Could
not beautify body ({}), but continuing flow.", e.getMessage()); and returns
CONTINUE), change the logging level from error to debug to follow the project's
convention for body-reading failures; keep the message text and return behavior
unchanged so the catch still returns CONTINUE and only the logging call is
downgraded to log.debug(...).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java`:
- Around line 110-113: Replace the current catch block that calls
writeLog("Error accessing body: " + e.getMessage()) with a DEBUG-level log that
includes the full exception chain: in LogInterceptor's catch
(ReadingBodyException e) use logger.debug("Error accessing body while checking
isBodyEmpty()", e) (or equivalent logger.debug call that passes the exception)
so the message is logged at DEBUG and the stack/cause chain is preserved; ensure
the class imports/uses the existing SLF4J logger instance rather than writeLog.
In `@core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java`:
- Around line 133-135: The catch in XML2HTTP (method handling unwrap/rewrap
operations) currently only catches XMLStreamException and XML2HTTPException, so
ReadingBodyException and WritingBodyException thrown from calls like
message.getBody().getLength(), message.getBodyAsStreamDecoded(), and
message.setBodyContent() will escape; update the catch to also handle
ReadingBodyException and WritingBodyException and log them (use debug level to
match existing body-failure logging) so the exceptions are swallowed and
recorded as before, or if you intend propagation, add a clear comment in
XML2HTTP explaining callers (e.g., SOAP2RESTInterceptor.unwrapMessageIfNecessary
and REST2SOAPInterceptor.unwrapMessageIfNecessary) must handle these runtime
exceptions.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java`:
- Around line 155-156: In ValidatorInterceptor's catch block for
ReadingBodyException, change the log call to pass the exception as the final
SLF4J argument so the stack trace is preserved; keep the existing message
template (e.g., "Validation failed: {}") but pass e.getMessage() as the
replacement and then pass e (the Throwable) as the last parameter so SLF4J
prints the full stack trace of ReadingBodyException and its cause.
In
`@core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java`:
- Around line 73-75: Replace the error-level logging in
JsonpathExchangeExpression's catch(ReadingBodyException e) block with
debug-level logging and include full cause-chain text: call log.debug(...)
instead of log.error(...) and pass
ExceptionUtil.concatMessageAndCauseMessages(e) as the message/detail; leave the
existing return resultForNoEvaluation(type) behavior unchanged so failures
remain non-fatal.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.java`:
- Around line 14-19: The ArchRule declaration in BodyDoesntThrowIOExceptionTest
has a typo in the description string and a non-descriptive field name; update
the .as(...) text to "Public instance methods of Body and its subclasses should
not throw IOException." and rename the static field adr to a clearer name like
bodyMethodsShouldNotThrowIOException (update all references) so the rule for
methods declared in classes assignable to AbstractBody that are public,
non-static, should not declare IOException remains correct and readable.
---
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/http/Message.java`:
- Around line 136-142: The method getBodyAsStringDecoded currently catches
Exception and always wraps it in a new ReadingBodyException, causing
double-wrapping when MessageUtil.getContent(this) already throws
ReadingBodyException; change the catch handling to first re-throw
ReadingBodyException unchanged and only wrap other Exceptions (mirror the
pattern used in getBodyAsStreamDecoded): detect if the caught exception is an
instance of ReadingBodyException and re-throw it, otherwise wrap the exception
in new ReadingBodyException before throwing, referencing getBodyAsStringDecoded,
MessageUtil.getContent(this), and ReadingBodyException.
- Around line 397-409: In bodyComplete (in Message.java) avoid wrapping
ReadingBodyException in a new RuntimeException; since ReadingBodyException
already extends RuntimeException, either remove the try/catch around
result.setBody(getBody(body2)) or rethrow the caught exception directly (throw
e) so the original exception type and stack trace are preserved; locate the
bodyComplete method that calls result.setBody(getBody(body2)) and replace the
wrapping new RuntimeException(e) with a direct rethrow of the
ReadingBodyException.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java`:
- Around line 62-72: The catch currently only handles IOException but
msg.getBodyAsStreamDecoded() and msg.setBodyContent(...) can throw the unchecked
ReadingBodyException, so update BeautifierInterceptor to also catch
ReadingBodyException (either as a separate catch or combined) around the try
that calls getPrettifier(...), msg.getBodyAsStreamDecoded() and
msg.setBodyContent(...); in that catch log the same info-level message as for
IOException (no stacktrace) and ensure the method still returns CONTINUE so
beautifier failures are swallowed as intended.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ntlm/NtlmInterceptor.java`:
- Line 78: handle the unchecked ReadingBodyException thrown when reading the
message body so it doesn't escape handleResponse()/authenticate(): wrap calls to
prepareStreamByEmptyingIt(exc) and any direct getContent() body reads in a
try/catch that catches ReadingBodyException (or RuntimeException if
ReadingBodyException is not importable) and log the failure at debug level
(using the class logger) instead of letting it propagate; after catching,
proceed with the NTLM logic (e.g., return/continue as existing flow expects) so
authentication paths still run and error messages (like "Could not
authenticate!") remain accurate.
In
`@core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java`:
- Around line 88-95: In JsonpathExchangeExpression (inside the catch
(MismatchedInputException e) block) wrap the call to
exchange.getMessage(flow).getBodyAsStringDecoded() in its own try-catch that
catches ReadingBodyException so that a ReadingBodyException thrown while
retrieving the body does not escape and replace the original
MismatchedInputException; if ReadingBodyException occurs, log a message
indicating the body could not be read (including the ReadingBodyException
details) and then continue to throw the ExchangeExpressionException constructed
from the original MismatchedInputException (e) so the original parsing error
context is preserved.
In `@core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java`:
- Around line 199-201: The catch-all in analyseSOAPMessage currently swallows
ReadingBodyException making body-read failures indistinguishable from non-SOAP
results; update analyseSOAPMessage to detect and rethrow ReadingBodyException
just like isSOAP does (i.e., if (e instanceof ReadingBodyException) throw e), so
ReadingBodyException is propagated up instead of returning NO_SOAP_RESULT;
locate the catch(Exception e) in analyseSOAPMessage and add the same rethrow
guard referencing ReadingBodyException and the analyseSOAPMessage method.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/http/Body.java`:
- Around line 155-184: The writeStreamed method can fail with
WritingBodyException before markAsRead() is reached, leaving observers
un-notified; ensure markAsRead() is always invoked by moving it into a finally
block that surrounds the entire read/write/finish sequence (i.e., wrap the loop,
out.finish(null) and associated try/catches in try { ... } finally {
markAsRead(); }) and rethrow any caught
WritingBodyException/ReadingBodyException so behavior remains unchanged, and
optionally simplify the loop condition by extracting the total-length check into
a helper like hasMoreToRead(long totalLength) to improve readability; refer to
writeStreamed, markAsRead, out.finish, WritingBodyException, and
ReadingBodyException when making changes.
In `@core/src/main/java/com/predic8/membrane/core/http/ChunkedBody.java`:
- Around line 227-256: The method writeStreamed currently skips markAsRead()
(and thus never notifies observers) when a ReadingBodyException or
WritingBodyException is thrown; update writeStreamed (reference method
writeStreamed in ChunkedBody) to ensure markAsRead() always runs by placing it
in a finally block (or add a clear comment if skipping is intentional), and
preserve the existing exception mapping (ReadingBodyException for read IO errors
and WritingBodyException for write IO errors) so behavior of read/write
exception handling (readChunkSize, readByteArray, out.write, out.finish) remains
unchanged; also ensure observers.bodyChunk calls remain inside the read loop.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/flow/CallInterceptor.java`:
- Around line 123-128: The call to getNewRequest(exc) can throw
ReadingBodyException from setRequestBody and currently escapes handleInternal;
to fix, move the request construction into the guarded try-catch inside
handleInternal (or add a specific catch for ReadingBodyException around
getNewRequest) so the exception is caught, the exchange outcome is set to ABORT,
and an appropriate error response is set on the Exchange; update references to
setRequestBody, getNewRequest, and handleInternal accordingly so
ReadingBodyException is handled before it propagates.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/log/access/AccessLogInterceptorService.java`:
- Around line 134-142: The inner try/catch inside getPayLoadSize is redundant
because ReadingBodyException is unchecked and safe(Supplier<Object>) already
catches exceptions; either remove the inner try/catch entirely from
getPayLoadSize (leaving it to return msg.getBody().getLength() directly) or, if
you want to keep explicit handling for clarity, add a debug log in the catch
that uses log.debug(ExceptionUtil.concatMessageAndCauseMessages(e)) before
returning defaultValue so body-read failures remain visible at debug level;
update the Supplier<Object> returned by getPayLoadSize accordingly.
In
`@core/src/main/java/com/predic8/membrane/core/lang/spel/typeconverters/SpELBodyToStringTypeConverter.java`:
- Around line 29-31: The catch block in SpELBodyToStringTypeConverter that
catches Exception around MessageUtil.getContent should be changed: remove the
try/catch so the unchecked ReadingBodyException can propagate (do not wrap it in
new RuntimeException), or if you must log, replace log.warn(...) with
log.debug(ExceptionUtil.concatMessageAndCauseMessages(e)) and rethrow the caught
exception directly (throw e); reference MessageUtil.getContent(...),
ReadingBodyException and ExceptionUtil.concatMessageAndCauseMessages(...) when
applying the change.
- Line 28: In SpELBodyToStringTypeConverter (the method that returns the
String), avoid calling body.getMessage() twice: extract body.getMessage() into a
local variable (e.g., Message msg) and then use MessageUtil.getContent(msg) and
msg.getCharsetOrDefault() to build the new String; this improves readability and
prevents potential double evaluation.
In
`@core/src/main/java/com/predic8/membrane/core/proxies/StatisticCollector.java`:
- Line 94: In StatisticCollector (the place with the log.warn("Could not count
statistic: {}", e.getMessage())), change the logging to use debug level and
include the full exception chain by calling
ExceptionUtil.concatMessageAndCauseMessages(e) instead of e.getMessage(); also
ensure ExceptionUtil is imported (e.g. import
com.predic8.membrane.core.util.ExceptionUtil) so the debug log reads the full
concatenated message and avoids using warn for benign body-read failures.
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/beautifier/BeautifierInterceptor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/log/LogInterceptor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/rest/XML2HTTP.java
Show resolved
Hide resolved
...c/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/interceptor/BodyDoesntThrowIOExceptionTest.java
Outdated
Show resolved
Hide resolved
Member
Author
|
@coderabbitai configuration |
Contributor
|
Configuration used: defaults CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
inheritance: false
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_instructions: ''
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
review_details: false
commit_status: true
fail_commit_status: false
collapse_walkthrough: true
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
enable_prompt_for_ai_agents: true
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
description_keyword: ''
auto_incremental_review: true
auto_pause_after_reviewed_commits: 5
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
custom: []
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
trufflehog:
enabled: true
checkov:
enabled: true
tflint:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
fortitudeLint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
opengrep:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
trivy:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
stylelint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
blinter:
enabled: true
psscriptanalyzer:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
issue_enrichment:
auto_enrich:
enabled: false
planning:
enabled: true
auto_planning:
enabled: true
labels: []
labeling:
labeling_instructions: []
auto_apply_labels: false
|
…ingBodyException` and `WritingBodyException` across body handling methods.
rrayst
force-pushed
the
reading-body-exception
branch
from
497ed21 to
5afd227
Compare
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.
…ingBodyException
andWritingBodyException` across body handling methods.Summary by CodeRabbit
Release Notes
Refactoring
Tests