Show error message when mobile report exceeds limit #3135
Conversation
avazirna
force-pushed
the
show-error-message-when-mobile-report-exceeds-limit
branch
from
9c2a955 to
d3f625f
Compare
📝 WalkthroughWalkthroughThe changes introduce a new error message string key, Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpUtils
participant Server
Client->>Server: Send HTTP request
Server-->>Client: Return error response (JSON or XML)
Client->>HttpUtils: parseUserVisibleError(response)
alt Response is JSON
HttpUtils->>HttpUtils: parseJsonErrorResponseBody(responseStr)
HttpUtils-->>Client: Extracted error message
else Response is XML
HttpUtils->>HttpUtils: parseXmlErrorResponseBody(responseStr)
HttpUtils-->>Client: Extracted error message
end
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (3)
✨ 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 (
|
avazirna
force-pushed
the
show-error-message-when-mobile-report-exceeds-limit
branch
from
d3f625f to
2487da6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/org/commcare/network/HttpUtils.java (1)
123-123: Consider making helper methods package-private.The new helper methods
parseJsonErrorResponseBodyandparseXmlErrorResponseBodyare currently public but appear to be implementation details that don't need external access.- public static Map<String, String> parseJsonErrorResponseBody(String responseStr) throws JSONException { + static Map<String, String> parseJsonErrorResponseBody(String responseStr) throws JSONException {
🛑 Comments failed to post (3)
app/src/org/commcare/network/HttpUtils.java (3)
123-131: 🛠️ Refactor suggestion
Enhance JSON parsing error handling and remove redundant null check.
The current implementation has several issues:
- The null check on
jsonObjectis redundant since the constructor throws an exception for invalid inputgetString()calls will throwJSONExceptionif keys don't exist, which should be handled gracefullypublic static Map<String, String> parseJsonErrorResponseBody(String responseStr) throws JSONException { Map<String, String> map = new HashMap<>(); JSONObject jsonObject = new JSONObject(responseStr); - if (jsonObject != null) { - map.put("error", jsonObject.getString("error")); - map.put("default_response", jsonObject.getString("default_response")); - } + map.put("error", jsonObject.optString("error", "")); + map.put("default_response", jsonObject.optString("default_response", "")); return map; }📝 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 static Map<String, String> parseJsonErrorResponseBody(String responseStr) throws JSONException { Map<String, String> map = new HashMap<>(); JSONObject jsonObject = new JSONObject(responseStr); map.put("error", jsonObject.optString("error", "")); map.put("default_response", jsonObject.optString("default_response", "")); return map; }🤖 Prompt for AI Agents
In app/src/org/commcare/network/HttpUtils.java around lines 123 to 131, remove the redundant null check on jsonObject since the constructor throws JSONException on invalid input. Instead, enhance error handling by checking if the keys "error" and "default_response" exist in the JSONObject before calling getString(), and handle missing keys gracefully to avoid throwing exceptions.
99-104: 🛠️ Refactor suggestion
Improve content type checking robustness.
The current content type comparison using
.equals()may fail if the response includes additional parameters like charset (e.g., "application/json; charset=utf-8").- if (response.errorBody().contentType().equals("application/json")) { + if (response.errorBody().contentType().toString().contains("application/json")) {Alternatively, use a more robust approach:
- if (response.errorBody().contentType().equals("application/json")) { + if ("application".equals(response.errorBody().contentType().type()) && + "json".equals(response.errorBody().contentType().subtype())) {🤖 Prompt for AI Agents
In app/src/org/commcare/network/HttpUtils.java around lines 99 to 104, the content type check uses equals() which fails if the content type has parameters like charset. Replace the equals() check with a more robust approach such as using startsWith("application/json") or parsing the media type properly to handle additional parameters, ensuring the content type check correctly identifies JSON responses even with charset or other parameters.
147-168: 🛠️ Refactor suggestion
Add input validation and consider making the method package-private.
The XML parsing method needs input validation and the rigid XML structure parsing might fail with slight variations.
- public static Map<String, String> parseXmlErrorResponseBody(String responseStr) + static Map<String, String> parseXmlErrorResponseBody(String responseStr) throws IOException, InvalidStructureException, UnfullfilledRequirementsException, XmlPullParserException { + if (responseStr == null || responseStr.trim().isEmpty()) { + return new HashMap<>(); + }Consider adding more flexible XML parsing that can handle structure variations, or add better error messages when the expected structure is not found.
📝 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.static Map<String, String> parseXmlErrorResponseBody(String responseStr) throws IOException, InvalidStructureException, UnfullfilledRequirementsException, XmlPullParserException { if (responseStr == null || responseStr.trim().isEmpty()) { return new HashMap<>(); } KXmlParser baseParser = ElementParser.instantiateParser( new ByteArrayInputStream(responseStr.getBytes(StandardCharsets.UTF_8))); ElementParser<Map<String, String>> responseParser = new ElementParser<>(baseParser) { @Override public Map<String, String> parse() throws InvalidStructureException, IOException, XmlPullParserException { Map<String, String> map = new HashMap<>(); checkNode("OpenRosaResponse"); nextTag("message"); nextTag("error"); map.put("error", parser.nextText()); nextTag("default_response"); map.put("default_response", parser.nextText()); return map; } }; return responseParser.parse(); }🤖 Prompt for AI Agents
In app/src/org/commcare/network/HttpUtils.java around lines 147 to 168, the parseXmlErrorResponseBody method lacks input validation and assumes a rigid XML structure that may cause failures with slight variations. Add input validation to check if responseStr is null or empty before parsing. Modify the XML parsing logic to be more flexible by checking for the presence of expected tags before accessing them, and provide clear error messages if the structure is not as expected. Also, consider changing the method's visibility to package-private if it is not used outside the package.
|
@damagatchi retest this please |
|
@damagatchi retest this please |
Product Description
During the first login a full data pull is triggered, HQ now responds with a 406 when the restore file contains at least one mobile report with more 100K records, more info on this HQ PR. This PR adds a new user message and ensures parsing of XML-formatted response bodies.
Ticket: https://dimagi.atlassian.net/browse/SAAS-16814
Safety Assurance
Safety story
Successfully tested locally. This shouldn't cause any crash, worse case scenario the user will see an
Unknown issuemessage or an inconveniently JSON/XML-formatted text.Labels and Review