Ignoring all corrupt opportunities to show #2995
Conversation
📝 WalkthroughWalkthroughThe changes enhance the error handling within the Sequence Diagram(s)sequenceDiagram
participant PS as processSuccess
participant JI as JSON Iterator
participant JP as JSON Parser
participant EL as Error Logger
participant OC as Outer Catch
PS->>JI: Start iterating over JSON objects
loop For each JSON object
JI->>JP: Attempt to parse JSON object
alt Parsing succeeds
JP-->>JI: Parsed object returned
else Parsing fails (JSONException/ParseException)
JP--xJI: Exception thrown
JI->>EL: Log individual parsing error
end
end
alt Global error occurs outside iteration
PS->>OC: Exception caught in outer catch
OC-->>PS: Log general error (parsing or database)
end
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:
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: 0
🧹 Nitpick comments (2)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
140-140: Consider removing or updating the TODO comment.Since you've implemented a solution that keeps the outer try-catch but adds inner try-catch blocks for individual records, the TODO comment about possibly removing the try-catch is no longer relevant and might be confusing for future developers.
- //TODO: Sounds like we don't want a try-catch here, better to crash. Verify before changing + // We maintain the outer try-catch for JSON array parsing while handling individual record parse errors separately
158-159: Consider adding error count tracking.Since you're now handling individual record parsing errors, it might be useful to track how many records failed to parse and log this information or consider exposing it to the user if the failure rate is significant.
//Store retrieved jobs totalJobs = jobs.size(); + int failedParseCount = json.length() - jobs.size(); + if (failedParseCount > 0) { + Logger.log("ERROR", "Failed to parse " + failedParseCount + " out of " + json.length() + " opportunities"); + } newJobs = ConnectDatabaseHelper.storeJobs(getContext(), jobs, true); setJobListData(jobs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (1)
app/src/org/commcare/connect/ConnectDatabaseHelper.java (1) (1)
ConnectDatabaseHelper(56-1024)
🔇 Additional comments (2)
app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java (2)
149-154: Good enhancement to error isolation when parsing job opportunities.Adding individual try-catch blocks around each JSON object processing is a solid approach. This ensures that a single corrupt record won't prevent the entire batch from being processed, improving the application's resilience.
162-163: Appropriate update to exception handling and logging message.The updated catch block and error message better reflects the potential sources of errors - either in parsing the overall JSON array or in database operations. Removing ParseException here makes sense as it's now handled at the individual record level.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
@Jignesh-dimagi Our strategy in cases like this to have been crash hard and get to know about the errors as soon as possible rather than have them lingering into the codebase and silently affecting users. This is also because we don't always have a great connect with users and struggle to hear about issues happening out into the field until late.
I do see the tradeoff in why sometimes not crashing the app and handling gracefully seems like a more convenient/better UX but with our user demographics, we would rather crash hard and fix our code sooner than have lingering bugs in our app.
That said we are not rigid and will be curious to hear your thoughts here as well.
|
@shubham1g5 I do understand that knowing the problem is very important but whenever it effects business, we should not compromise. User have 10 opportunities but due to 1 corrupt opportunity, he/she is not able to use remaining 9 and it effects business. I also agree with @OrangeAndGreen that we should inform users that one of the opportunity is corrupt and so not showing here. But as discussed with @OrangeAndGreen we will find some better way for such messaging going forward. I am open to your suggestions. |
Agree that It does seems intuitive in this case for user to be able to continue with other opportunities but also strongly thing that we would never have found this issue if we were not hard crashing potentially causing a lot more users to risk not getting new opportunities. Also a lot of times catching an exception may introduce some very hard to debug app states which are preferrable to prevent upfront with crashing the app. Plus I think we can fix these errors quite quickly on web side once we get to know them and add better server side validations to protect against more of such failures. As such I think I still prefer crashing and fixing things early specially since we don't have a good error/crash monitoring process at the moment. Also inviting opinions from @OrangeAndGreen @ctsims on the same. |
|
@shubham1g5 App was not crashing previously also but was just showing blank connect screen. |
|
@shubham1g5 Strong agreement on a few points:
One thing that's kind of funny in this case is that we were actually in the worst of all worlds, in that we both were overly fault tolerant (IE: The app didn't crash or show a visible error that would have allowed the user to know something was wrong) and under-tolerant (failure of one model prevented the use of another model uncoupled with it). There are two other considerations that come to mind for me in how we think about this going forward.
I think that we should consider a few different approaches in this case, some of which are probably worth doing simultaneously.
|
|
Thanks @ctsims for the detailed thoughts here.
That's a good catch. Something we should try to get more attention to as a followup here.
We have been changing the current behaviour to hard crash by removing a lot of JsonException catches during the merge work. I think we will continue to assume failing hard and fast is still a generic rule in most scenarios for the merge work.
+1 to this approach specifically in context of this issue.
Reg. error reporting, We do have a good mechanism for errors to reported on Firebase be it hard crashes or non fatals. The problem there is the amount of non-fatals can be quite high as we get almost most catched exceptions reported over there and unless a particular issue is very frequent, it's unlikely for it to be stand out in that reporting. With an issue rare enough like this, I am quite sure we would not notice it in both hard crash and non fatals reporting unless we try to look for it intentionally in response to a raised bug. But I do think crashing still gives this issue higher probability to get reported sooner from our users given it's a more disrupting experience for the user. |
|
App will show the corrupt opportunities along with other
|
| if(viewType==NON_CORRUPT_JOB_VIEW){ | ||
| ItemLoginConnectHomeAppsBinding binding = ItemLoginConnectHomeAppsBinding.inflate( | ||
| LayoutInflater.from(parent.getContext()), parent, false); | ||
| return new NonCorruptJobViewHolder(binding); | ||
| }else{ | ||
| ItemLoginConnectHomeCorruptAppsBinding binding = ItemLoginConnectHomeCorruptAppsBinding.inflate( | ||
| LayoutInflater.from(parent.getContext()), parent, false); | ||
| return new CorruptJobViewHolder(binding); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: code spacing seems off (format all new code with style file guide)
There was a problem hiding this comment.
@shubham1g5 I am already using this style
There was a problem hiding this comment.
Think you will need to manually format the new code in Android Studio once you apply that style.
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java
Show resolved
Hide resolved
| try { | ||
| obj = (JSONObject)json.get(i); | ||
| jobs.add(ConnectJobRecord.fromJson(obj)); | ||
| }catch (JSONException | ParseException e) { |
There was a problem hiding this comment.
It seems to me that ParseException is getting thrown uncessarily from underlying code and think would be great to be removed.
There was a problem hiding this comment.
@shubham1g5 Its required ConnectJobRecord.fromJson is throwing this ParseException
There was a problem hiding this comment.
I think we can remove it from there as well. The underlying method is throwing this in method signature without it actually being thrown ever from the code.
There was a problem hiding this comment.
@shubham1g5 Underlying method is throwing this error when it tries to parse payment related stuff.
| Logger.exception("Parsing return from Opportunities request", e); | ||
| } catch (IOException | JSONException e) { | ||
| Logger.exception("Parsing / database error return from Opportunities request", e); | ||
| handleCorruptJobs(); |
There was a problem hiding this comment.
think we need to still crash altogether from here as coming here means that the complete response/Json is incorrect and user won't be able to see any opportunitities in this case.
There was a problem hiding this comment.
@shubham1g5 done, app will crash after showing message to user
There was a problem hiding this comment.
I think we want to crash instantly without doing any special handling for exception here. IMO there is not any solid benefit of user seeing a page for sometime that will crash in 3 seconds to do this special handling.
Also we would always want to crash in these cases even in future as user can't meanigfully proceed in the app workflow once they are on this page with no opportunities.
There was a problem hiding this comment.
@shubham1g5 We are not handling this scenario gracefully as of now so I believe lets at least show user some message before crashing so that they are aware what is happening with their account. That's my view point.
There was a problem hiding this comment.
Getting back to the discussion we had I think we still prefer crashing instead of any graceful handling in scenarios where user can't do anything else even if we handle them gracefully. Crashing already gives a strong and self sufficent signal to the user that something is wrong with the app and I am wary of introducing timed exceptions at various places to the code if they don't have a value addition.
There was a problem hiding this comment.
@shubham1g5 App is crashing and not handling gracefully. handleCorruptJobs is crashing it. Its just showing message to user before crashing. I can make it immediate crash if you still insist.
There was a problem hiding this comment.
Thanks, I do think we should crash immediately as there is not much value added by user messaging just before crash, in both cases we are conveying the same thing that we are unexpectedly closing.
…tunities' into jignesh/fix/ignore_corrupt_opportunities # Conflicts: # app/src/org/commcare/fragments/connect/ConnectJobsListsFragment.java
| } catch (IOException | JSONException e) { | ||
| Logger.exception("Parsing / database error return from Opportunities request", e); | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
I think we only want to crash for JSONException and not IOException here. IOException doesn't necessarily mean a code error and can happen due to reasons outside the control of our app.
There was a problem hiding this comment.
@shubham1g5 You are right for other cases but here IOException is raised if any error while reading the network response so I think we should include it.
There was a problem hiding this comment.
I think that signifies an error in network stream which should not be related to our application logic.
|
|
||
| public void refreshData() { | ||
| corruptJobs.clear(); | ||
| ApiConnect.getConnectOpportunities(getContext(), new IApiCallback() { |
There was a problem hiding this comment.
Can we call retrieveOpportunities directly here, seems like a lot of code duplication here to me.
There was a problem hiding this comment.
@shubham1g5 Sorry I didn't get you. Which retrieveOpportunities are you referring?
There was a problem hiding this comment.
ConnectUnlockFragment.retrieveOpportunities
There was a problem hiding this comment.
They both are in different fragment so not sure if we can use that. Already code is separate originally.
| ConnectJobRecord job = ConnectJobRecord.fromJson(obj); | ||
| jobs.add(job); | ||
| }catch (JSONException e) { | ||
| Logger.exception("Parsing return from Opportunities request", e); |
There was a problem hiding this comment.
does this mean we would swallow exception without showing any errors to the user ?
There was a problem hiding this comment.
@shubham1g5 Here, there is no UI to display and code is just retrieving opportunities only to store locally. Also, local store should only have good opportunities and not corrupt ones.
There was a problem hiding this comment.
Agree on DB containing only good opportunities. I am trying to think if it's possible for user to not see an error on jobs list page when there is a corrupt opportunity in the response here ? For eg. it would be confusing if the corrupt opportunity shows only some times on UI and not all times due to us reusing the stored jobs list from DB that doesn't have corrupt opportunities.
There was a problem hiding this comment.
In both cases, DB has good jobs only. ConnectUnlockFragment is not showing any UI for opportunities.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Offlined and approving based on flowwing discussion -
-
We can't show UI indicators for background tasks, so it's ok for user to just see the error when user navigates to the job list page itself.
-
we are crashing even
IOExceptionhere as it's a result of bad response characters instead of network errors.
Product Description
No change in UI as such
Technical Summary
Part of https://dimagi.atlassian.net/browse/CCCT-825
Feature Flag
Bug fixes
Safety Assurance
Safety story
Tested on prod server