Improved Connect exception handling #3158
Conversation
…IOException properly.
Proper handling for JSONException and IOException. Catching more specific than Exception where possible. Got rid of catch-log-rethrow pattern.
📝 WalkthroughWalkthroughThis change refactors exception handling across several classes by removing or narrowing try-catch blocks and updating method signatures. Specifically, Sequence Diagram(s)sequenceDiagram
participant API
participant App
participant Database
API->>App: Successful API response
App->>App: processSuccess()
alt JSON parsing error
App->>App: throw RuntimeException(JSONException)
else IO error
App->>App: Log IOException
else Success
App->>Database: Store/Update data
Database-->>App: Acknowledge
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
70-75: Consistent error handling pattern applied.The same crash-on-JSON-error pattern is applied here in
processFailure. This maintains consistency with theprocessSuccessmethod changes.app/src/org/commcare/connect/ConnectManager.java (1)
366-371: JSON parsing errors will now crash instead of being logged.This change aligns with the pattern seen in other files where
JSONExceptionis escalated as aRuntimeException. Ensure this crash behavior is intentional and that appropriate crash reporting is in place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java(1 hunks)app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java(1 hunks)app/src/org/commcare/connect/ConnectManager.java(3 hunks)app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(4 hunks)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java(2 hunks)app/src/org/commcare/connect/database/JobStoreManager.java(2 hunks)app/src/org/commcare/connect/network/PersonalIdApiHandler.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (4)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
ConnectDatabaseHelper(35-135)app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt (1)
context(12-28)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
DatabaseConnectOpenHelper(38-164)app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(155-1260)
🔇 Additional comments (22)
app/src/org/commcare/connect/PersonalIdManager.java (2)
510-514: LGTM! Improved exception handling precision.The separation of
JSONExceptionandIOExceptionhandling is well-designed:
JSONExceptionescalated toRuntimeExceptionfor API response format issues (fail-fast)IOExceptionlogged for recoverable network/IO issuesThis change aligns with the PR's goal of more precise exception handling.
510-514: LGTM: Improved exception handling aligns with PR objectives.The separation of
JSONExceptionandIOExceptionhandling is a good improvement. ConvertingJSONExceptiontoRuntimeExceptionensures JSON parsing errors are not silently ignored, which may help expose previously masked server errors as intended by this PR.app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (2)
101-106: LGTM! Consistent exception handling pattern.The exception handling follows the same improved pattern as other files:
JSONExceptionescalated toRuntimeException(fail-fast for API issues)IOExceptionlogged with user-friendly toast message (appropriate for UI context)Good consistency across the codebase.
101-106: LGTM: Consistent exception handling pattern.The exception handling follows the same improved pattern as other files in this PR - escalating
JSONExceptionasRuntimeExceptionwhile loggingIOExceptionwith appropriate user feedback via toast.app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (4)
171-175: LGTM! Consistent exception handling improvement.The exception handling follows the established pattern:
JSONExceptionescalated toRuntimeExceptionfor API format issuesIOExceptionlogged for network/IO issuesMaintains consistency with other files in this PR.
426-439: LGTM! Simplified exception handling.Removing the generic try-catch block allows exceptions to propagate naturally, eliminating the catch-log-rethrow pattern as intended by this PR. This improves error visibility and aligns with the goal of more precise exception handling.
171-175: LGTM: Consistent exception handling pattern.The exception handling follows the same improved pattern established across this PR - escalating
JSONExceptionasRuntimeExceptionwhile loggingIOException.
426-439: LGTM: Removal of generic exception handling.Removing the try-catch block allows exceptions to propagate naturally, which aligns with the PR objective of eliminating catch-log-rethrow patterns. This change may expose previously masked errors, which is the intended behavior.
app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java (2)
53-53:Details
❓ Verification inconclusive
Verify the impact of removing ParseException from method signature.
The
ParseExceptionwas removed from the throws clause, butDateUtils.parseDateTime()on line 59 can still throw it. This meansParseExceptionwould now be wrapped in aRuntimeExceptionand propagated.This aligns with the fail-fast approach for data format issues, but ensure that callers can handle the potential
RuntimeExceptionappropriately.
🏁 Script executed:
#!/bin/bash # Search for callers of ConnectJobLearningRecord.fromJson to verify exception handling ast-grep --pattern 'ConnectJobLearningRecord.fromJson($$$)'Length of output: 232
🏁 Script executed:
#!/bin/bash # Show context around the fromJson call to see how exceptions are handled rg "ConnectJobLearningRecord.fromJson" -n -C5 app/src/org/commcare/connect/ConnectManager.javaLength of output: 857
🏁 Script executed:
#!/bin/bash # Display 20 lines of context around the fromJson call to check for surrounding try/catch rg "ConnectJobLearningRecord.fromJson" -n -C20 app/src/org/commcare/connect/ConnectManager.javaLength of output: 2704
🏁 Script executed:
#!/bin/bash # Show the fromJson method to see how ParseException is handled rg "public static ConnectJobLearningRecord fromJson" -n -C30 app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java # Find parseDateTime definition to confirm its throws signature rg "parseDateTime(" -n .Length of output: 1963
Verify removal of
ParseExceptionfrom fromJson signatureThe
fromJson(JSONObject, int)method no longer declaresParseException, yet it still callsDateUtils.parseDateTime(...). Confirm whether:
DateUtils.parseDateTime(String)no longer throws a checkedParseException(e.g., it now handles and wraps errors internally),- or if removing this declaration changes compile-time or runtime behavior in any caller.
Key locations to check:
app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.javaat thefromJsonsignature (line 53).app/src/org/commcare/connect/ConnectManager.javaaround line 420, wherefromJsonis invoked inside atry { … } catch (JSONException | IOException)block but doesn’t catch unchecked exceptions.Please verify that callers either don’t need to handle a checked
ParseExceptionor are prepared to deal with a potentialRuntimeExceptionfrom date parsing.
53-53:Details
❓ Verification inconclusive
Verify that ParseException removal doesn't cause compilation issues.
The method signature no longer declares
ParseExceptionbut still callsDateUtils.parseDateTime(json.getString(META_DATE))which may throwParseException. Please verify that this change doesn't introduce compilation errors.
🏁 Script executed:
#!/bin/bash # Description: Check if DateUtils.parseDateTime throws ParseException and verify compilation # Expected: Either DateUtils was changed to not throw ParseException, or this should cause compilation errors # Search for DateUtils.parseDateTime method declaration ast-grep --pattern 'parseDateTime($_) { $$$ }' # Search for any ParseException handling in DateUtils rg -A 5 -B 5 "ParseException" --type java | rg "DateUtils" # Check for other usages of DateUtils.parseDateTime to see how ParseException is handled rg -A 3 -B 3 "DateUtils\.parseDateTime" --type javaLength of output: 6688
Verify external
DateUtils.parseDateTimesignatureThe
fromJsonmethod in
app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java
no longer declaresParseExceptionyet still invokes
DateUtils.parseDateTime(json.getString(META_DATE)), which comes from the external libraryorg.javarosa.core.model.utils.DateUtils. Please confirm:
- That the version of
org.javarosa:corein your build no longer declaresthrows ParseExceptiononparseDateTime.- That all other callers of
DateUtils.parseDateTime(e.g., inConnectJobDeliveryRecord,ConnectJobPaymentRecord, etc.) compile cleanly under this dependency version.This will ensure you haven’t introduced a hidden compilation error.
app/src/org/commcare/connect/network/PersonalIdApiHandler.java (1)
51-56:Details
❓ Verification inconclusive
Ensure JSONException crash handling is appropriate for your error recovery strategy.
The change to rethrow
JSONExceptionas aRuntimeExceptionwill cause the app to crash on JSON parsing errors instead of gracefully handling them viaonFailure. This is a significant behavioral change from the previous implementation where JSON parsing errors were logged and handled through the failure callback.Consider whether crashing is the desired behavior for JSON parsing errors, especially since network responses can be unpredictable.
🏁 Script executed:
#!/bin/bash # Check if there are any crash reporting or error recovery mechanisms in place for RuntimeExceptions rg -A 5 'RuntimeException.*PersonalIdApiHandler' # Check if there are any try-catch blocks that might catch these RuntimeExceptions higher up ast-grep --pattern 'try { $$$ PersonalIdApiHandler $$$ } catch (RuntimeException $_) { $$$ }'Length of output: 154
Confirm JSON parsing error handling
We didn’t find any higher-levelcatch (RuntimeException)aroundPersonalIdApiHandlerinvocations. RethrowingJSONExceptionas aRuntimeExceptionwill crash the app rather than invoking your failure callback.Please verify:
- That an unhandled
RuntimeExceptionhere is intentional (e.g., to surface crashes via your crash-reporting system).- Your crash-reporting or global error handler will capture these exceptions.
- If you want graceful recovery instead, consider calling
rather than throwing.onFailure(PersonalIdApiErrorCodes.JSON_PARSING_ERROR);app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java (1)
55-67:Details
❓ Verification inconclusive
Verify that DateUtils.parseDateTime doesn't throw ParseException.
The removal of
ParseExceptionfrom the method signature could be problematic ifDateUtils.parseDateTime(line 61) throws a checkedParseException. This would result in a compilation error.
🏁 Script executed:
#!/bin/bash # Check the signature of DateUtils.parseDateTime to see if it throws ParseException ast-grep --pattern 'parseDateTime($$$) throws $$$' # Also check with ripgrep for the method definition rg -A 3 'parseDateTime.*\(' --type java # Check if ParseException is a checked or unchecked exception in this codebase fd -e java -x grep -l "class ParseException" {} \;Length of output: 4801
#!/bin/bash # Locate DateUtils class file(s) date_utils_files=$(rg -l 'class DateUtils' -t java) echo "Found DateUtils files:" echo "$date_utils_files" # Inspect parseDateTime method signature within DateUtils.java for file in $date_utils_files; do echo -e "\n--- $file: parseDateTime occurrences ---" rg -n 'parseDateTime' -C2 "$file" echo -e "\n--- $file: throws clauses ---" rg -n 'throws' -C2 "$file" done
#!/bin/bash # Locate DateUtils.java in the repo echo "Searching for DateUtils.java..." date_utils_files=$(fd -t f DateUtils.java) if [ -z "$date_utils_files" ]; then echo "No DateUtils.java found" exit 1 fi echo "Found DateUtils file(s):" echo "$date_utils_files" # Inspect parseDateTime method signatures in each DateUtils file for file in $date_utils_files; do echo -e "\n--- $file: parseDateTime definition(s) ---" rg -n 'parseDateTime' -C3 "$file" done
Confirm DateUtils.parseDateTime exception signature
I wasn’t able to locate
DateUtils.parseDateTime’s definition in the repo. Please verify whether it throws a checkedParseException. If it does, re-introduce it in thefromJsonthrows clause or catch it to avoid compilation errors.app/src/org/commcare/connect/ConnectManager.java (2)
438-442: Consistent exception handling pattern maintained.Both
updateLearningProgressandupdateDeliveryProgresscallbacks follow the same pattern of rethrowingJSONExceptionasRuntimeException. This maintains consistency across the codebase.Also applies to: 564-569
420-421:Details
❓ Verification inconclusive
Note: Potential ParseException from date parsing methods.
The code calls
ConnectJobLearningRecord.fromJsonandConnectJobAssessmentRecord.fromJsonwhich previously threwParseException. Since these method signatures were changed to removeParseException, ensure that any date parsing within these methods is handled appropriately.Also applies to: 431-432
🏁 Script executed:
#!/bin/bash # Check if ConnectJobLearningRecord.fromJson has similar ParseException removal rg -B 2 -A 10 'fromJson.*ConnectJobLearningRecord' --type java # Check DateUtils methods used in the codebase for exception handling ast-grep --pattern 'DateUtils.$_($$$)'Length of output: 6486
🏁 Script executed:
#!/bin/bash # Inspect fromJson implementation in ConnectJobLearningRecord rg -B5 -A20 "fromJson" app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java # Inspect fromJson implementation in ConnectJobAssessmentRecord rg -B5 -A20 "fromJson" app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.javaLength of output: 1585
🏁 Script executed:
#!/bin/bash # Locate parseDateTime method definitions in the codebase rg -n "static Date parseDateTime" -R app/src # Display the full implementation around each definition rg -B3 -A30 "static Date parseDateTime" app/srcLength of output: 501
🏁 Script executed:
#!/bin/bash # Locate the DateUtils class file DATE_UTILS_FILE=$(find app/src -type f -name "DateUtils.java") echo "Found DateUtils at: $DATE_UTILS_FILE" # Show the full implementation of parseDateTime and related parseDate methods rg -B3 -A20 "parseDateTime" "$DATE_UTILS_FILE" rg -B3 -A20 "parseDate(" "$DATE_UTILS_FILE"Length of output: 287
🏁 Script executed:
#!/bin/bash # Locate DateUtils.java in the repo DATE_UTILS_FILE=$(rg -l "class DateUtils" --type java) echo "DateUtils found at: $DATE_UTILS_FILE" # Show the implementation of parseDateTime and parseDate methods rg -B3 -A20 "parseDateTime" "$DATE_UTILS_FILE" rg -B3 -A20 "parseDate(" "$DATE_UTILS_FILE"Length of output: 281
🏁 Script executed:
#!/bin/bash # Locate DateUtils.java in the repository find app/src -type f -iname "DateUtils.java" # Search for parseDateTime definitions and usages rg -n "parseDateTime" --glob "*.java" app/src # Search for parseDate( usages to find related methods rg -n "parseDate(" --glob "*.java" app/srcLength of output: 1567
Ensure Runtime Safety for Date Parsing
The
fromJsonmethods in both ConnectJobLearningRecord and ConnectJobAssessmentRecord no longer declare a checkedParseException(they only throwJSONException) yet still invokeDateUtils.parseDateTime(...). We couldn’t locate theDateUtilsimplementation to confirm its error handling, so please verify that invalid or unexpected date strings won’t cause uncaught runtime exceptions.• Locations to review:
- app/src/org/commcare/connect/ConnectManager.java lines 420–421 and 431–432
- app/src/org/commcare/android/database/connect/models/ConnectJobLearningRecord.java
- app/src/org/commcare/android/database/connect/models/ConnectJobAssessmentRecord.java
• Actions:
- Confirm that
DateUtils.parseDateTime(and anyparseDate) internally catches parsing errors or returns a safe fallback.- If it propagates unchecked exceptions (e.g. JodaTime or
IllegalArgumentException), either wrap calls toparseDateTimein try/catch or enhanceDateUtilsto handle malformed input gracefully.app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (4)
8-8: LGTM: Required import for specific exception handling.The import addition is necessary to support the more specific exception handling implemented in the methods below.
40-42: LGTM: Improved exception specificity.The change from catching generic
ExceptiontoEncryptionUtils.EncryptionExceptionis more precise and aligns with the PR objective of improving exception handling specificity.
57-59: LGTM: Appropriate specific exception handling.Catching
Base64DecoderExceptionspecifically is correct sinceBase64.decode()is the only operation that can throw this exception in this method.
89-91: LGTM: Multi-catch for specific exceptions is well-structured.The multi-catch block properly handles both
Base64DecoderException(fromBase64.decode()) andEncryptionUtils.EncryptionException(fromEncryptionUtils.decrypt()), making the error handling more precise than the previous genericExceptioncatch.app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (3)
17-25: LGTM: Exception propagation aligns with PR objectives.Removing the try-catch block allows database-related exceptions to propagate to callers, which is consistent with the PR's goal of eliminating catch-log-rethrow patterns. The method correctly checks for database existence before attempting operations.
35-35: LGTM: Simplified exception handling.The removal of exception handling allows database write exceptions to propagate naturally, following the PR's approach of letting exceptions bubble up rather than being caught and logged locally.
43-47: LGTM: Clean exception propagation for cleanup operations.Removing exception handling from database cleanup operations is appropriate as any failures during user data deletion should be visible to calling code rather than silently logged.
app/src/org/commcare/connect/database/JobStoreManager.java (1)
93-115: LGTM: Exception handling removal is appropriate for non-transactional operations.The removal of exception handling from
storeOrUpdateJoballows database exceptions to propagate naturally, which aligns with the PR objectives. Since this method doesn't manage transactions directly, letting exceptions bubble up is the correct approach.
| } | ||
| } catch (IOException | JSONException e) { | ||
| } catch (JSONException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (IOException e) { | ||
| Logger.exception("Parsing return from DB key request", e); |
There was a problem hiding this comment.
this needs to be PR'ed to master
| ConnectDatabaseHelper.connectDatabase.setTransactionSuccessful(); | ||
| ConnectDatabaseHelper.connectDatabase.endTransaction(); |
There was a problem hiding this comment.
transaction management should follow the try..finally pattern similar to how we use in databse updates.
Product Description
No user-visible changes
Technical Summary
Proper handling of JSONException and IOException.
Catching more specific than just Exception where possible.
Got rid of catch-log-rethrow pattern (crashes are already logged).
Feature Flag
Connect
Safety Assurance
Safety story
Tested locally.
Automated test coverage
None
QA Plan
This should be thoroughly tested to verify that error behavior is as we expect across the product.
Some code that would cover up server errors was removed, so it's possible this will cause a server error to newly crash the app.