Improved PersonalID API Error Handling #3167
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,70 +27,103 @@ public abstract class PersonalIdApiHandler { | |
| public enum PersonalIdApiErrorCodes { | ||
| NETWORK_ERROR, | ||
| OLD_API_ERROR, | ||
| FORBIDDEN_ERROR, | ||
| TOKEN_UNAVAILABLE_ERROR, | ||
| TOKEN_DENIED_ERROR, | ||
| INVALID_RESPONSE_ERROR, | ||
| JSON_PARSING_ERROR, | ||
| FAILED_AUTH_ERROR, | ||
| SERVER_ERROR, | ||
| RATE_LIMIT_EXCEEDED_ERROR; | ||
|
|
||
| public boolean shouldAllowRetry(){ | ||
| return this == NETWORK_ERROR || this == TOKEN_UNAVAILABLE_ERROR || this == SERVER_ERROR | ||
| || this == RATE_LIMIT_EXCEEDED_ERROR; | ||
| } | ||
| } | ||
|
|
||
| private IApiCallback createCallback(PersonalIdSessionData sessionData, | ||
| PersonalIdApiResponseParser parser) { | ||
| return new IApiCallback() { | ||
| @Override | ||
| public void processSuccess(int responseCode, InputStream responseData) { | ||
| if (parser != null) { | ||
| try (InputStream in = responseData) { | ||
| JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(in))); | ||
| parser.parse(json, sessionData); | ||
| } catch (JSONException e) { | ||
| Logger.exception("JSON error parsing API response", e); | ||
| onFailure(PersonalIdApiErrorCodes.JSON_PARSING_ERROR, e); | ||
| } catch (IOException e) { | ||
| Logger.exception("Error parsing API response", e); | ||
| onFailure(PersonalIdApiErrorCodes.NETWORK_ERROR, e); | ||
| } | ||
| } | ||
| onSuccess(sessionData); | ||
| } | ||
|
|
||
| @Override | ||
| public void processFailure(int responseCode, @Nullable InputStream errorResponse) { | ||
| if(responseCode == 401) { | ||
| onFailure(PersonalIdApiErrorCodes.FAILED_AUTH_ERROR, null); | ||
| return; | ||
| } | ||
|
|
||
| if(responseCode == 403) { | ||
| onFailure(PersonalIdApiErrorCodes.FORBIDDEN_ERROR, null); | ||
| return; | ||
| } | ||
|
Comment on lines
+72
to
+75
Contributor
There was a problem hiding this comment. we should incorporate all critical reponse codes here - 401 - Auth failure
Contributor
Author
There was a problem hiding this comment. 9868847
Contributor
There was a problem hiding this comment. I do think we should have different messaging here on the lines of - |
||
|
|
||
| if(responseCode == 429 || responseCode == 503) { | ||
| onFailure(PersonalIdApiErrorCodes.RATE_LIMIT_EXCEEDED_ERROR, null); | ||
| return; | ||
| } | ||
|
|
||
| if(responseCode == 500) { | ||
| onFailure(PersonalIdApiErrorCodes.SERVER_ERROR, null); | ||
| return; | ||
| } | ||
|
|
||
| StringBuilder info = new StringBuilder("Response " + responseCode); | ||
| if (errorResponse != null) { | ||
| try (InputStream in = errorResponse) { | ||
| JSONObject json = new JSONObject(new String(StreamsUtil.inputStreamToByteArray(in))); | ||
| if (json.has("error")) { | ||
| info.append(": ").append(json.optString("error")); | ||
| Toast.makeText(CommCareApplication.instance(), json.optString("error"), | ||
| Toast.LENGTH_LONG).show(); | ||
| } | ||
| } catch (JSONException e) { | ||
| Logger.exception("JSON error parsing API error response", e); | ||
| onFailure(PersonalIdApiErrorCodes.JSON_PARSING_ERROR, e); | ||
| return; | ||
| } catch (IOException e) { | ||
| Logger.exception("Error parsing API error response", e); | ||
| onFailure(PersonalIdApiErrorCodes.NETWORK_ERROR, e); | ||
| return; | ||
| } | ||
|
Comment on lines
+96
to
104
Contributor
There was a problem hiding this comment. Error in response handling of error body should not propogate same as error in response hadling of successful response body, think it's fine to eat these errors instead and have the
Contributor
Author
There was a problem hiding this comment. I'm not sure I understand. Are you saying to create an UNKNOWN_ERROR enum with a different message, and get rid of the code that attempts to retrieve the "error" from the response?
Contributor
There was a problem hiding this comment.
Not really, we should have that code but any failures in that code shuld just return as "UNKNOWN ERROR" (enum should be fine) instead of |
||
| } | ||
| onFailure(PersonalIdApiErrorCodes.INVALID_RESPONSE_ERROR, new Exception(info.toString())); | ||
| } | ||
|
|
||
| @Override | ||
| public void processNetworkFailure() { | ||
| onFailure(PersonalIdApiErrorCodes.NETWORK_ERROR, null); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenUnavailableError() { | ||
| onFailure(PersonalIdApiErrorCodes.TOKEN_UNAVAILABLE_ERROR, null); | ||
| } | ||
|
|
||
| @Override | ||
| public void processTokenRequestDeniedError() { | ||
| onFailure(PersonalIdApiErrorCodes.TOKEN_DENIED_ERROR, null); | ||
| } | ||
|
|
||
| @Override | ||
| public void processOldApiError() { | ||
| onFailure(PersonalIdApiErrorCodes.OLD_API_ERROR, null); | ||
| } | ||
| }; | ||
| } | ||
|
|
@@ -102,40 +135,35 @@ public void makeStartConfigurationCall(Activity activity, | |
| PersonalIdSessionData sessionData = new PersonalIdSessionData(); | ||
| ApiPersonalId.startConfiguration(activity, body, integrityToken, requestHash, | ||
| createCallback(sessionData, | ||
| new StartConfigurationResponseParser())); | ||
| } | ||
| public void validateFirebaseIdToken(Activity activity, String firebaseIdToken,PersonalIdSessionData sessionData) { | ||
| ApiPersonalId.validateFirebaseIdToken(sessionData.getToken(),activity,firebaseIdToken, | ||
| createCallback(sessionData, | ||
| null)); | ||
| } | ||
|
|
||
| public void addOrVerifyNameCall(Activity activity, String name, PersonalIdSessionData sessionData) { | ||
| ApiPersonalId.addOrVerifyName(activity, name, sessionData.getToken(), | ||
| createCallback(sessionData, | ||
| new AddOrVerifyNameParser())); | ||
| } | ||
|
|
||
| public void confirmBackupCode(Activity activity, String backupCode, PersonalIdSessionData sessionData) { | ||
| ApiPersonalId.confirmBackupCode(activity, backupCode, sessionData.getToken(), | ||
| createCallback(sessionData, | ||
| new ConfirmBackupCodeResponseParser())); | ||
| } | ||
|
|
||
| public void completeProfile(Context context, String userName, | ||
| String photoAsBase64, String backupCode, String token,PersonalIdSessionData sessionData) { | ||
| ApiPersonalId.setPhotoAndCompleteProfile(context, userName, photoAsBase64, backupCode, token, | ||
| createCallback(sessionData, | ||
| new CompleteProfileResponseParser())); | ||
| } | ||
|
|
||
|
|
||
| protected abstract void onSuccess(PersonalIdSessionData sessionData); | ||
|
|
||
| protected abstract void onFailure(PersonalIdApiErrorCodes errorCode, Throwable t); | ||
| } | ||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider safer default error handling.
The default case throws a
RuntimeExceptionwhich will crash the app. Consider returning a generic error message string instead to maintain app stability while still logging the unexpected error.📝 Committable suggestion
🤖 Prompt for AI Agents