Changes to facilititate more error response logging on mobile #1505
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe HttpResponseProcessor interface now requires a ResponseStreamAccessor when handling client errors. ResponseStreamAccessor adds getErrorResponseStream(). ModernHttpRequester implements and exposes error stream access, and passes the accessor into processClientError during client-error responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ModernHttpRequester as ModernHttpRequester
participant Server
participant Processor as HttpResponseProcessor
participant Accessor as ResponseStreamAccessor
Caller->>ModernHttpRequester: makeRequestAndProcess(...)
ModernHttpRequester->>Server: HTTP request
Server-->>ModernHttpRequester: HTTP response
rect rgba(200,240,255,0.3)
note over ModernHttpRequester: Build ResponseStreamAccessor<br/>(getResponseStream, getErrorResponseStream)
ModernHttpRequester->>Accessor: create anonymous accessor
end
alt 2xx Success
ModernHttpRequester->>Processor: processSuccess(streamAccessor)
else 4xx Client Error
ModernHttpRequester->>Processor: processClientError(code, streamAccessor)
note over Accessor: getErrorResponseStream()<br/>reads error body stream
else 5xx/Other
ModernHttpRequester->>Processor: processServerFailure(code)
end
Processor-->>Caller: processing result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/commcare/core/interfaces/HttpResponseProcessor.java(1 hunks)src/main/java/org/commcare/core/interfaces/ResponseStreamAccessor.java(1 hunks)src/main/java/org/commcare/core/network/ModernHttpRequester.java(4 hunks)
| public interface ResponseStreamAccessor { | ||
| InputStream getResponseStream() throws IOException; | ||
|
|
||
| InputStream getErrorResponseStream(); |
There was a problem hiding this comment.
Align error stream accessor contract with I/O realities
Error bodies are optional and may require disk/network I/O to materialize. Without @Nullable and a checked exception, implementors must either fabricate values or silently swallow failures, which breaks parity with getResponseStream(). Please strengthen the contract so callers can both detect “no error body” and surface I/O issues.
import java.io.IOException;
import java.io.InputStream;
+import javax.annotation.Nullable;
public interface ResponseStreamAccessor {
InputStream getResponseStream() throws IOException;
- InputStream getErrorResponseStream();
+ @Nullable
+ InputStream getErrorResponseStream() throws IOException;📝 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.
| InputStream getErrorResponseStream(); | |
| import java.io.IOException; | |
| import java.io.InputStream; | |
| import javax.annotation.Nullable; | |
| public interface ResponseStreamAccessor { | |
| InputStream getResponseStream() throws IOException; | |
| @Nullable | |
| InputStream getErrorResponseStream() throws IOException; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/commcare/core/interfaces/ResponseStreamAccessor.java around
line 9, the getErrorResponseStream method lacks nullable and checked-I/O
semantics; update its signature to mirror getResponseStream by annotating the
return as @Nullable and declaring it throws IOException (and add the necessary
imports), so implementors can return null for absent error bodies and surface
I/O failures instead of fabricating values or swallowing exceptions.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit details to see what changes were made
gh api repos/dimagi/commcare-core/commits/36d172699825c3d048ca6609a0cb7b55686cb456 --jq '.commit.message, .files[].patch'
Length of output: 2254
@shubham1g5, I see you've taken a different approach that centralizes error handling! 👍
Instead of propagating IOException through the interface, you're:
- ✅ Added
@NullabletogetErrorResponseStream()(as suggested) - ✅ Handling exceptions at the call site in
ModernHttpRequester.processResponse()with a try-catch-finally block - ✅ Managing stream lifecycle (opening and closing) in the caller
- ✅ Simplified
HttpResponseProcessor.processClientError()to takeInputStreamdirectly instead ofResponseStreamAccessor
This design centralizes error handling and resource management, which is a valid alternative to what I originally suggested. The core concern—handling absent error bodies and I/O failures gracefully—is now addressed.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| } | ||
| } else if (responseCode >= 400 && responseCode < 500) { | ||
| responseProcessor.processClientError(responseCode); | ||
| responseProcessor.processClientError(responseCode, streamAccessor); |
There was a problem hiding this comment.
Ensure error streams are closed after client handling
We now expose an InputStream for error bodies, but unlike the success path we never close it. That leaves file descriptors (or cached files) dangling if the processor only reads part of the stream or forgets to close it. Wrap the accessor so we can close the cached stream after processClientError returns.
- } else if (responseCode >= 400 && responseCode < 500) {
- responseProcessor.processClientError(responseCode, streamAccessor);
+ } else if (responseCode >= 400 && responseCode < 500) {
+ final InputStream[] errorStreamHolder = new InputStream[1];
+ ResponseStreamAccessor closingAccessor = new ResponseStreamAccessor() {
+ @Override
+ public InputStream getResponseStream() throws IOException {
+ return streamAccessor.getResponseStream();
+ }
+
+ @Nullable
+ @Override
+ public InputStream getErrorResponseStream() throws IOException {
+ if (errorStreamHolder[0] == null) {
+ errorStreamHolder[0] = streamAccessor.getErrorResponseStream();
+ }
+ return errorStreamHolder[0];
+ }
+
+ @Override
+ public String getApiVersion() {
+ return streamAccessor.getApiVersion();
+ }
+ };
+
+ try {
+ responseProcessor.processClientError(responseCode, closingAccessor);
+ } finally {
+ StreamsUtil.closeStream(errorStreamHolder[0]);
+ }🤖 Prompt for AI Agents
In src/main/java/org/commcare/core/network/ModernHttpRequester.java around line
184, the error-path exposes an InputStream via the provided stream accessor but
never closes it; wrap the accessor usage so the cached/error InputStream is
closed after responseProcessor.processClientError returns. Concretely, obtain
the InputStream from the accessor (or create a small wrapper accessor), call
processClientError inside a try block, and in a finally block close the
retrieved/cached stream (or call the accessor's close method) to ensure file
descriptors/cached files are released while preserving the existing contract
with responseProcessor.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit details using GitHub CLI
gh api repos/dimagi/commcare-core/commits/36d172699825c3d048ca6609a0cb7b55686cb456 --jq '.commit.message, .files[].patch'
# Get the full diff for this commit
gh api repos/dimagi/commcare-core/commits/36d172699825c3d048ca6609a0cb7b55686cb456 --jq '.files[] | select(.filename | contains("ModernHttpRequester.java")) | .patch'
Length of output: 3770
@shubham1g5, perfect! The fix looks great. You've taken a cleaner approach than my suggestion by:
- Obtaining the stream in the parent caller (
processResponse) rather than wrapping the accessor - Closing it in a finally block to guarantee cleanup, even if
processClientErrorthrows an exception - Simplifying the API by passing the
InputStreamdirectly instead of theResponseStreamAccessor - Adding proper exception handling with
Logger.exceptionto catch any errors during processing
This approach is now consistent with how the success path handles streams (both obtain the stream, process it, and close it in a finally block). The resource management is solid and the code is easier to understand.
🐰✨
^_^
(•‿•)
🎯 <
Nice fix!
🧬 Code Graph Analysis Results
<src/main/java/org/commcare/util/NetworkStatus.java> (lines 16-30)
public class NetworkStatus {
public static boolean isCaptivePortal() {
String captivePortalURL = "http://www.commcarehq.org/serverup.txt";
CommCareNetworkService commCareNetworkService =
CommCareNetworkServiceGenerator.createNoAuthCommCareNetworkService();
try {
Response<ResponseBody> response =
commCareNetworkService.makeGetRequest(captivePortalURL, new HashMap<>()).execute();
return response.code() == 200 && !"success".equals(response.body().string());
} catch (IOException e) {
Logger.log(LogTypes.TYPE_WARNING_NETWORK, "Detecting captive portal failed with exception" + e.getMessage());
return false;
}
}
}
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Glad to see this improvement!
| try { | ||
| errorStream = streamAccessor.getErrorResponseStream(); | ||
| responseProcessor.processClientError(responseCode, errorStream); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Curious about the generic Exception catch here, is there something more specific that we're expecting?
There was a problem hiding this comment.
no, I just took a fail safe approach for anything related to error handling as it's not critical for us to know about these failures to justify crashing and also a catch here not have more side-effects than showing a non-specific error.
Product Description
Network Interface changes to facilitate more specific error handling
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit
New Features
Bug Fixes