Clean error code handling for Personal ID and logs it to firebase #3362
Conversation
📝 WalkthroughWalkthroughThe PR standardizes API failure handling across the connect stack by changing IApiCallback.processFailure to accept (responseCode, url, errorCode, errorSubCode), removing the InputStream error body. BaseApi now parses error JSON to extract error_code and error_sub_code and logs them via an updated logFailedResponse. ConnectNetworkHelper and all call sites (MessageManager flows and others) are updated to pass the new arguments, defaulting to empty strings when unavailable. BaseApiCallback and PersonalIdApiHandler adapt to the new signature; PersonalIdApiHandler now consumes errorCode/errorSubCode directly and removes local JSON parsing. Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller (e.g., MessageManager)
participant A as BaseApi
participant S as Server
participant H as IApiCallback implementation<br/>(BaseApiCallback/PersonalIdApiHandler)
C->>A: executeRequest(url, payload)
A->>S: HTTP request
S-->>A: HTTP response
alt HTTP 2xx
A->>H: onSuccess(data)
else HTTP error
rect rgba(240, 248, 255, 0.6)
note right of A: New behavior: parse JSON error<br/>fields error_code, error_sub_code
A->>A: Parse error body (JSON) -> (errorCode, errorSubCode)<br/>fallback: ""
end
A->>H: processFailure(responseCode, url, errorCode, errorSubCode)
H->>H: Handle status-specific logic<br/>(401/403/429/400/5xx, etc.)
end
sequenceDiagram
autonumber
participant N as ConnectNetworkHelper
participant H as IApiCallback
N->>H: processFailure(responseCode, url, "", "")
note over N,H: Fallback path when no error body available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1)
343-377: Fix processFailure argument orderAll four calls swap the
url,errorCode, anderrorSubCodepositions, so downstream handlers see blank URLs and receive the endpoint string as the error subcode. That breaks the new logging intent and can mask real error codes. Please pass the arguments in the contract order.- handler.processFailure(responseCode, "", "", url); + handler.processFailure(responseCode, url, "", ""); ... - handler.processFailure(responseCode, "", "", url); + handler.processFailure(responseCode, url, "", ""); ... - handler.processFailure(responseCode, "", "", url); + handler.processFailure(responseCode, url, "", ""); ... - handler.processFailure(-1, "", "", url); + handler.processFailure(-1, url, "", "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/src/org/commcare/connect/MessageManager.java(5 hunks)app/src/org/commcare/connect/network/ConnectNetworkHelper.java(4 hunks)app/src/org/commcare/connect/network/IApiCallback.java(1 hunks)app/src/org/commcare/connect/network/base/BaseApi.kt(4 hunks)app/src/org/commcare/connect/network/base/BaseApiCallback.kt(2 hunks)app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/src/org/commcare/connect/MessageManager.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java (2)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)app/src/org/commcare/connect/network/base/BaseApiHandler.kt (1)
onFailure(16-16)
app/src/org/commcare/connect/network/ConnectNetworkHelper.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
app/src/org/commcare/connect/network/IApiCallback.java (1)
app/src/org/commcare/connect/network/base/BaseApiCallback.kt (1)
processFailure(20-53)
| Logger.exception("Error reading response stream", e); | ||
| // Handle error when reading the stream | ||
| callback.processFailure(response.code(), null, endPoint) | ||
| callback.processFailure(response.code(), "", "", endPoint) | ||
| } | ||
| } else { | ||
| // Handle validation errors | ||
| logFailedResponse(response, endPoint) | ||
| val stream = if (response.errorBody() != null) response.errorBody()!! | ||
| .byteStream() else null | ||
| callback.processFailure(response.code(), stream, endPoint) | ||
| var errorCode = "" | ||
| var errorSubCode = "" | ||
| try { | ||
| if (stream != null) { | ||
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | ||
| val jsonStr = String(errorBytes, StandardCharsets.UTF_8) | ||
| val json = JSONObject(jsonStr) | ||
| errorCode = json.optString("error_code", ""); | ||
| errorSubCode = json.optString("error_sub_code", ""); | ||
| } | ||
| } catch (e: Exception) { | ||
| Logger.exception("Error parsing error_code", e); | ||
| } | ||
| logFailedResponse(response, endPoint, errorCode, errorSubCode) | ||
| callback.processFailure(response.code(), errorCode, errorSubCode, endPoint) | ||
| } |
There was a problem hiding this comment.
Ensure processFailure receives (responseCode, url, errorCode, errorSubCode)
Both invocations hand off the parameters in the wrong order (errorCode becomes the URL, etc.), so the callback loses the endpoint context and the parsed error codes. This defeats the PR’s goal of surfacing error codes to Firebase.
- callback.processFailure(response.code(), "", "", endPoint)
+ callback.processFailure(response.code(), endPoint, "", "")
...
- callback.processFailure(response.code(), errorCode, errorSubCode, endPoint)
+ callback.processFailure(response.code(), endPoint, errorCode, errorSubCode)📝 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.
| Logger.exception("Error reading response stream", e); | |
| // Handle error when reading the stream | |
| callback.processFailure(response.code(), null, endPoint) | |
| callback.processFailure(response.code(), "", "", endPoint) | |
| } | |
| } else { | |
| // Handle validation errors | |
| logFailedResponse(response, endPoint) | |
| val stream = if (response.errorBody() != null) response.errorBody()!! | |
| .byteStream() else null | |
| callback.processFailure(response.code(), stream, endPoint) | |
| var errorCode = "" | |
| var errorSubCode = "" | |
| try { | |
| if (stream != null) { | |
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | |
| val jsonStr = String(errorBytes, StandardCharsets.UTF_8) | |
| val json = JSONObject(jsonStr) | |
| errorCode = json.optString("error_code", ""); | |
| errorSubCode = json.optString("error_sub_code", ""); | |
| } | |
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e); | |
| } | |
| logFailedResponse(response, endPoint, errorCode, errorSubCode) | |
| callback.processFailure(response.code(), errorCode, errorSubCode, endPoint) | |
| } | |
| Logger.exception("Error reading response stream", e); | |
| // Handle error when reading the stream | |
| callback.processFailure(response.code(), endPoint, "", "") | |
| } | |
| } else { | |
| // Handle validation errors | |
| val stream = if (response.errorBody() != null) response.errorBody()!! | |
| .byteStream() else null | |
| var errorCode = "" | |
| var errorSubCode = "" | |
| try { | |
| if (stream != null) { | |
| val errorBytes = StreamsUtil.inputStreamToByteArray(stream) | |
| val jsonStr = String(errorBytes, StandardCharsets.UTF_8) | |
| val json = JSONObject(jsonStr) | |
| errorCode = json.optString("error_code", ""); | |
| errorSubCode = json.optString("error_sub_code", ""); | |
| } | |
| } catch (e: Exception) { | |
| Logger.exception("Error parsing error_code", e); | |
| } | |
| logFailedResponse(response, endPoint, errorCode, errorSubCode) | |
| callback.processFailure(response.code(), endPoint, errorCode, errorSubCode) | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/base/BaseApi.kt around lines 43 to 66,
the two callback.processFailure invocations pass parameters in the wrong order
(endpoint is last) — change them to match the required signature (responseCode,
url, errorCode, errorSubCode). Specifically, replace the first call to pass
endPoint as the second argument (response.code(), endPoint, "", "") and the
second call to pass endPoint as the second argument as well (response.code(),
endPoint, errorCode, errorSubCode).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit details to see the changes made
gh api repos/dimagi/commcare-android/commits/b5e000c8b76a74e46c2016b3ddebad06c61c0896 --jq '.commit.message'
# Get the file changes in this commit for BaseApi.kt
gh api repos/dimagi/commcare-android/commits/b5e000c8b76a74e46c2016b3ddebad06c61c0896 --jq '.files[] | select(.filename == "app/src/org/commcare/connect/network/base/BaseApi.kt") | .patch'
Length of output: 1020
@shubham1g5, perfect! ✅ The parameter order is now correct in both processFailure calls. The error codes will now be properly passed through to Firebase for better debugging of 400 errors. Thank you for the quick fix!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| public void processFailure(int responseCode, String url, String errorCode, String errorSubCode) { | ||
| if (!handleErrorCodeIfPresent(errorCode, errorSubCode, sessionData)) { | ||
| super.processFailure(responseCode, errorCode, errorSubCode, url); | ||
| } |
There was a problem hiding this comment.
Pass params to super.processFailure in new order
Here the errorCode is being sent as the url, and the actual url ends up as the errorSubCode. This prevents BaseApiCallback from logging the endpoint correctly and drops the error code that Firebase is supposed to capture.
- super.processFailure(responseCode, errorCode, errorSubCode, url);
+ super.processFailure(responseCode, url, errorCode, errorSubCode);📝 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.
| public void processFailure(int responseCode, String url, String errorCode, String errorSubCode) { | |
| if (!handleErrorCodeIfPresent(errorCode, errorSubCode, sessionData)) { | |
| super.processFailure(responseCode, errorCode, errorSubCode, url); | |
| } | |
| public void processFailure(int responseCode, String url, String errorCode, String errorSubCode) { | |
| if (!handleErrorCodeIfPresent(errorCode, errorSubCode, sessionData)) { | |
| super.processFailure(responseCode, url, errorCode, errorSubCode); | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java
around lines 56 to 59, the call to super.processFailure passes parameters in the
wrong order (errorCode is passed as url and errorSubCode as errorCode), causing
incorrect logging and dropped error info; change the call to pass responseCode,
then url, then errorCode, then errorSubCode so it matches the superclass
signature (super.processFailure(responseCode, url, errorCode, errorSubCode)).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the commit details to verify the fix
gh api repos/dimagi/commcare-android/commits/b5e000c8b76a74e46c2016b3ddebad06c61c0896 --jq '.files[] | select(.filename == "app/src/org/commcare/connect/network/connectId/PersonalIdApiHandler.java") | .patch'
Length of output: 532
@shubham1g5 Perfect! ✅ The fix looks great. The parameters are now in the correct order:
super.processFailure(responseCode, url, errorCode, errorSubCode);This ensures that:
- The
urlis correctly logged as the endpoint - The
errorCodeanderrorSubCodeare properly captured for Firebase reporting
Would you like me to resolve this comment thread?
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@damagatchi retest this please |
| else -> "Unexpected Error: $message" | ||
| } | ||
|
|
||
| errorMessage += " for url ${endPoint ?: "url not found"}" |
There was a problem hiding this comment.
@shubham1g5 Can we make it unknown url as url not found seems like 404 error
|
|
||
| @Override | ||
| public void processClientError(int responseCode) { | ||
| public void processClientError(int responseCode, InputStream errorStream) { |
There was a problem hiding this comment.
Why there is change in this method? I cannot see its being used further.
There was a problem hiding this comment.
this is the same interface used by ConnectNetworkHelper
There was a problem hiding this comment.
it's the same interface used by ConnectNetworkHelper
| public interface IApiCallback { | ||
| void processSuccess(int responseCode, InputStream responseData); | ||
| void processFailure(int responseCode, @Nullable InputStream errorResponse, String url); | ||
| void processFailure(int responseCode, String url, String errorCode, String errorSubCode); |
There was a problem hiding this comment.
Currently it has requirement of code and sub code but in future it can have multiple codes, just thinking to have Map<String, String> here to support many? Your thoughts?
There was a problem hiding this comment.
that is plausible for sure, but decided to keep as it is for now and not over-enginner.
| Pair<String, String> errorCodes = NetworkUtils.getErrorCodes(errorStream); | ||
| NetworkUtils.logFailedResponse("", responseCode, url, errorCodes.getFirst(), | ||
| errorCodes.getSecond()); | ||
| handler.processFailure(responseCode, url, errorCodes.getFirst(), | ||
| errorCodes.getSecond()); |
There was a problem hiding this comment.
This code will get removed whenever migration is done to retrofit for remaining methods in ApiPersonalId.java. processClientError(int responseCode, InputStream errorStream) is un necessarily getting changed here and causing changes in other files like ModernHttpTask.java. If we ignore the functionality for connect messages (which doesn't have sub error code right now) and only focus on personalId sign up, I think these changes are not required. Connect message will be automatically covered for sub error code whenever retrofit migration is done in ApiPersonalId.java.
|
@Jignesh-dimagi I am no longer planning these changes for 2.60 given QA has already completed and there is some more thought I need to give to this PR around logging right things. I have instead separated out the most critical change from this PR here instead that would be nice to get in 2.60 |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1789
Mostly motivated by this ticket and debugging 400 errors on mobile, currently we see 400 errors in firebase like this without much info on error, this change should add more details to these exceptions.
The second change the PR makes is it corrects the json field from
sub_codetoerror_sub_codeas mentioned hereLabels and Review
cross-request: dimagi/commcare-core#1505