Correct error handling for network requests #3195
Merged
Conversation
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
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
pm-dimagi
and removed request for
Jignesh-dimagi
📝 WalkthroughWalkthroughThe change updates the error handling logic in the Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| } | ||
| } | ||
| onFailure(PersonalIdApiErrorCodes.INVALID_RESPONSE_ERROR, new Exception(info.toString())); | ||
| onFailure(PersonalIdApiErrorCodes.UNKNOWN_ERROR, new Exception(info.toString())); |
There was a problem hiding this comment.
Mismatch between error code and retry policy
The PR switches to UNKNOWN_ERROR for unclassified HTTP responses, but UNKNOWN_ERROR is not included in PersonalIdApiErrorCodes.shouldAllowRetry(). Therefore, these failures won’t be retryable despite the objective to allow retries on unhandled codes.
Please update the retry policy to include UNKNOWN_ERROR, or choose a retryable code.
Apply this diff to include UNKNOWN_ERROR in the retry check:
public boolean shouldAllowRetry(){
- return this == NETWORK_ERROR || this == TOKEN_UNAVAILABLE_ERROR
- || this == SERVER_ERROR || this == RATE_LIMIT_EXCEEDED_ERROR;
+ return this == NETWORK_ERROR || this == TOKEN_UNAVAILABLE_ERROR
+ || this == SERVER_ERROR || this == RATE_LIMIT_EXCEEDED_ERROR
+ || this == UNKNOWN_ERROR;
}🤖 Prompt for AI Agents
In app/src/org/commcare/connect/network/PersonalIdApiHandler.java at line 103,
the error code UNKNOWN_ERROR is used for unclassified HTTP responses but is not
included in the retry policy method PersonalIdApiErrorCodes.shouldAllowRetry().
To fix this, update the shouldAllowRetry() method to include UNKNOWN_ERROR as a
retryable code so that these failures will be retried as intended.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the `shouldAllowRetry` method to verify if `UNKNOWN_ERROR` is now included.
rg -n "shouldAllowRetry" -A 4 -B 2 app/src/org/commcare/connect/network/PersonalIdApiHandler.java
Length of output: 359
@shubham1g5 great, I just checked the latest commit (c06189c…) and the shouldAllowRetry() method now contains UNKNOWN_ERROR, so unclassified HTTP responses will be retriable as intended:
public boolean shouldAllowRetry(){
return this == NETWORK_ERROR
|| this == TOKEN_UNAVAILABLE_ERROR
|| this == SERVER_ERROR
|| this == UNKNOWN_ERROR;
}With this change in place the retry behaviour now matches the goal of the PR.
Thanks for addressing the feedback so quickly!
🧬 Code Graph Analysis Results
Relevant snippets for app/src/org/commcare/connect/network/PersonalIdApiHandler.java
File: app/src/org/commcare/connect/network/parser/AddOrVerifyNameParser.java
Lines: 12-25
public class AddOrVerifyNameParser implements PersonalIdApiResponseParser {
/**
* Parses and sets values on the given PersonalIdSessionData instance.
*
* @param sessionData the instance to populate
* @throws JSONException if a parsing error occurs
*/
@Override
public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
sessionData.setAccountExists(json.optBoolean("account_exists", false));
sessionData.setPhotoBase64(JsonExtensions.optStringSafe(json, "photo", null));
}
}File: app/src/org/commcare/connect/network/parser/CompleteProfileResponseParser.java
Lines: 14-27
public class CompleteProfileResponseParser implements PersonalIdApiResponseParser {
/**
* Parses and sets values on the given PersonalIdSessionData instance.
*
* @param sessionData the instance to populate
* @throws JSONException if a parsing error occurs
*/
@Override
public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
sessionData.setPersonalId(Objects.requireNonNull(JsonExtensions.optStringSafe(json, "username", null)));
sessionData.setDbKey(Objects.requireNonNull(JsonExtensions.optStringSafe(json, "db_key", null)));
sessionData.setOauthPassword(Objects.requireNonNull(JsonExtensions.optStringSafe(json, "password", null)));
}
}File: app/src/org/commcare/connect/network/parser/ConfirmBackupCodeResponseParser.java
Lines: 12-28
public class ConfirmBackupCodeResponseParser implements PersonalIdApiResponseParser{
/**
* Parses and sets values on the given PersonalIdSessionData instance.
*
* @param sessionData the instance to populate
* @throws JSONException if a parsing error occurs
*/
@Override
public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
sessionData.setPersonalId(JsonExtensions.optStringSafe(json, "username", null));
sessionData.setDbKey(JsonExtensions.optStringSafe(json, "db_key", null));
if (json.has("attempts_left")) {
sessionData.setAttemptsLeft(json.getInt("attempts_left"));
}
sessionData.setOauthPassword(JsonExtensions.optStringSafe(json, "password", null));
}
}File: app/src/org/commcare/connect/network/parser/StartConfigurationResponseParser.java
Lines: 12-28
public class StartConfigurationResponseParser implements PersonalIdApiResponseParser {
/**
* Parses and sets values on the given PersonalIdSessionData instance.
*
* @param sessionData the instance to populate
* @throws JSONException if a parsing error occurs
*/
@Override
public void parse(JSONObject json, PersonalIdSessionData sessionData) throws JSONException {
sessionData.setRequiredLock(JsonExtensions.optStringSafe(json, "required_lock", null));
sessionData.setDemoUser(json.optBoolean("demo_user", false));
sessionData.setToken(JsonExtensions.optStringSafe(json, "token", null));
sessionData.setSessionFailureCode(JsonExtensions.optStringSafe(json, "failure_code", null));
sessionData.setSessionFailureSubcode(JsonExtensions.optStringSafe(json, "failure_subcode", null));
}
}File: app/src/org/commcare/connect/network/ApiPersonalId.java
Lines: 48-366 (summarized)
/**
* Provides static methods to make network API calls related to Personal ID functionality.
* Methods include:
* - startConfiguration: starts configuration with integrity token and request hash.
* - validateFirebaseIdToken: validates a Firebase ID token with the server.
* - addOrVerifyName: sends a name to be added or verified.
* - confirmBackupCode: confirms a backup code.
* - setPhotoAndCompleteProfile: sets user photo and completes profile.
*
* Each method constructs appropriate HTTP requests using Retrofit ApiService,
* handles authentication tokens, and uses a common callApi method to enqueue calls.
*
* callApi method:
* - Shows a progress dialog.
* - Enqueues the Retrofit call asynchronously.
* - On success, passes InputStream of response body to callback.processSuccess.
* - On failure, logs error and passes error stream or null to callback.processFailure.
* - On network failure, logs and calls callback.processNetworkFailure.
*
* Error handling methods log network and HTTP errors.
*/
public class ApiPersonalId {
// Constants for API versions and client IDs.
// Various static methods for API calls, e.g.:
public static void startConfiguration(Context context, Map<String, String> body, String integrityToken,
String requestHash, IApiCallback callback) {
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.startConfiguration(integrityToken, requestHash, body);
callApi(context, call, callback);
}
public static void validateFirebaseIdToken(String token, Context context, String firebaseIdToken, IApiCallback callback) {
HashMap<String, String> params = new HashMap<>();
params.put("token", firebaseIdToken);
AuthInfo authInfo = new AuthInfo.TokenAuth(token);
String tokenAuth = HttpUtils.getCredential(authInfo);
Objects.requireNonNull(tokenAuth);
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.validateFirebaseIdToken(tokenAuth, params);
callApi(context, call, callback);
}
public static void addOrVerifyName(Context context, String name, String token, IApiCallback callback) {
HashMap<String, String> params = new HashMap<>();
params.put("name", name);
AuthInfo authInfo = new AuthInfo.TokenAuth(token);
String tokenAuth = HttpUtils.getCredential(authInfo);
Objects.requireNonNull(tokenAuth);
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.checkName(tokenAuth, params);
callApi(context, call, callback);
}
public static void confirmBackupCode(Context context,
String backupCode, String token, IApiCallback callback) {
HashMap<String, String> params = new HashMap<>();
params.put("recovery_pin", backupCode);
AuthInfo authInfo = new AuthInfo.TokenAuth(token);
String tokenAuth = HttpUtils.getCredential(authInfo);
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.confirmBackupCode(tokenAuth, params);
callApi(context, call, callback);
}
public static void setPhotoAndCompleteProfile(Context context, String userName,
String photoAsBase64, String backupCode, String token, IApiCallback callback) {
Objects.requireNonNull(photoAsBase64);
Objects.requireNonNull(userName);
AuthInfo authInfo = new AuthInfo.TokenAuth(token);
String tokenAuth = HttpUtils.getCredential(authInfo);
Objects.requireNonNull(tokenAuth);
HashMap<String, String> params = new HashMap<>();
params.put("photo", photoAsBase64);
params.put("name", userName);
params.put("recovery_pin", backupCode);
ApiService apiService = ApiClient.getClientApi();
Call<ResponseBody> call = apiService.completeProfile(tokenAuth, params);
callApi(context, call, callback);
}
// Internal helper methods for logging and error handling.
}These snippets provide context on:
- How the
PersonalIdApiHandleruses various parsers to parse API responses intoPersonalIdSessionData. - The API calls made via
ApiPersonalIdstatic methods, which correspond to the calls made inPersonalIdApiHandler. - The callback mechanism and error handling used in the network calls.
- The parsing logic for different API responses.
This context is directly relevant to understanding the implementation and behavior of PersonalIdApiHandler.java.
Jignesh-dimagi
approved these changes
Jun 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Product Description
Any network response codes we are not handling specifically crashes today but instead they should be failing with an error message and should allow user to retry the network request. Therefore I am failing them with
UNKNOWN_ERRORinstead.Labels and Review