Handling a JSON Exception properly. #3166
Conversation
📝 WalkthroughWalkthroughThe exception handling in the Sequence Diagram(s)sequenceDiagram
participant Caller
participant PersonalIdManager
participant Logger
Caller->>PersonalIdManager: processSuccess()
alt Success
PersonalIdManager-->>Caller: (normal return)
else JSONException occurs
PersonalIdManager->>PersonalIdManager: Catch JSONException
PersonalIdManager-->>Caller: Throw RuntimeException
else IOException occurs
PersonalIdManager->>Logger: Log IOException
PersonalIdManager-->>Caller: (handle/log error)
end
✨ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/connect/PersonalIdManager.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
| } 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.
💡 Verification agent
🧩 Analysis chain
Technical implementation is correct, but consider the user experience impact.
The change successfully achieves the PR objective of escalating JSONException to crash the application instead of silently logging. The catch block ordering is correct (more specific exception first).
However, consider these aspects:
- This will now crash the app during database passphrase retrieval if the JSON response is malformed
- No graceful fallback or user-friendly error message before crashing
- The crash occurs in a network callback context where users might not expect sudden termination
Does this crash-on-JSON-error approach align with your overall error handling strategy for network operations?
🏁 Script executed:
#!/bin/bash
# Verify the crash handling strategy is consistent across the codebase
echo "Searching for other JSONException handling patterns in the codebase..."
rg "catch.*JSONException" -A 3 -B 1
echo -e "\nSearching for RuntimeException throws to understand crash patterns..."
rg "throw new RuntimeException" -A 1 -B 1
echo -e "\nSearching for similar API callback error handling..."
rg "processSuccess.*JSONException|JSONException.*processSuccess" -A 5 -B 5Length of output: 60632
Consistent JSON parsing error handling in PersonalIdManager
In app/src/org/commcare/connect/PersonalIdManager.java (lines 510–514), you’ve split out JSONException to rethrow as a RuntimeException, which will crash the app on malformed JSON. In other Connect network callbacks (for example PersonalIdApiHandler.java), JSON parsing errors are logged and delivered via an onFailure callback rather than crashing.
Please update this catch to match those patterns:
• File: app/src/org/commcare/connect/PersonalIdManager.java, lines 510–514
• Replace
} catch (JSONException e) {
throw new RuntimeException(e);
} catch (IOException e) {
Logger.exception("Parsing return from DB key request", e);
}with something like
} catch (JSONException e) {
Logger.exception("Parsing return from DB key request", e);
onFailure(PersonalIdApiErrorCodes.JSON_PARSING_ERROR);
} catch (IOException e) {
Logger.exception("Parsing return from DB key request", e);
}(or the equivalent error-callback in this context)
This will prevent unexpected crashes on bad JSON and keep the user-experience consistent with your other network flows.
🤖 Prompt for AI Agents
In app/src/org/commcare/connect/PersonalIdManager.java around lines 510 to 514,
the JSONException catch block currently throws a RuntimeException causing the
app to crash on malformed JSON. To align with the consistent error handling
strategy used elsewhere, replace the throw statement with logging the exception
and invoking the appropriate onFailure callback method with a JSON parsing error
code. Keep the IOException catch block as is. This change will prevent
unexpected crashes and provide a graceful failure path for JSON errors.
There was a problem hiding this comment.
@OrangeAndGreen This looks good and I know its not in this scope but still can we close the inputstream similar to here
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Oh very cool, I'll go ahead and merge this PR but will be working in this same area again in another ticket, I'll add this there.
Fixed a spot where we were only logging a JSONException when we want to crash instead.