Quarantine forms due to malformed xml as unrecoverable #2859
Conversation
…ess errored form" This reverts commit ee48781.
|
@damagatchi retest this please |
| @@ -126,7 +127,7 @@ protected ResultAndError<SaveStatus> doTaskBackground(Void... nothing) { | |||
| // Likely a user level issue, so send error to HQ as a app build error | |||
| XPathErrorLogger.INSTANCE.logErrorToCurrentApp(cleanedMessage); | |||
|
|
|||
| return new ResultAndError<>(SaveStatus.SAVE_ERROR, cleanedMessage); | |||
| return new ResultAndError<>(SaveStatus.SAVE_UNRECOVERABLE_ERROR, cleanedMessage); | |||
There was a problem hiding this comment.
think all the places where we are throwing SaveStatus.SAVE_ERROR above should be SaveStatus.SAVE_UNRECOVERABLE_ERROR with this change.
| @@ -126,7 +127,7 @@ protected ResultAndError<SaveStatus> doTaskBackground(Void... nothing) { | |||
| // Likely a user level issue, so send error to HQ as a app build error | |||
| XPathErrorLogger.INSTANCE.logErrorToCurrentApp(cleanedMessage); | |||
|
|
|||
| return new ResultAndError<>(SaveStatus.SAVE_ERROR, cleanedMessage); | |||
| return new ResultAndError<>(SaveStatus.SAVE_UNRECOVERABLE_ERROR, cleanedMessage); | |||
There was a problem hiding this comment.
Also would you be able to provide more details around the error pathway you think the crash linked in the PR will surface as in this method.
hi @shubham1g5 I have added more details to the description of the PR, including the way I tested the changes. |
Where exactly in code are we quarantining the forms erroring due to |
Here, based on the |
@avazirna How is that related to |
Using the same method where the NPE was thrown as example, the exception is caught by this generic catch which then wraps the exception in a |
| if (!CommCareApplication.instance().isConsumerApp()) { | ||
| new UserfacingErrorHandling<>().createErrorDialog(this, errorMessage, | ||
| Localization.get("notification.formentry.save_error.title"), | ||
| FormEntryConstants.EXIT); | ||
| } | ||
| quarantineRecordOnError(errorMessage); | ||
| String reasonType = (saveStatus == SAVE_UNRECOVERABLE_ERROR) ? QuarantineReason_LOCAL_PROCESSING_ERROR | ||
| : QuarantineReason_RECORD_ERROR ; |
|
@damagatchi retest this please |
Summary
Errors that occur when saving the forms to disk are categorized under a single umbrella status SAVE_ERROR, which causes forms to be quarantined due to LOCAL_PROCESSING_ERROR motive, which in turn prevents forms from being sent back to the queue for reprocessing. This PR adds a new
SaveStatus,SAVE_UNRECOVERABLE_ERROR, to be used when quarantining forms due to malformed or corrupted form XML, leavingSAVE_ERRORto any other errors that are deemed recoverable. These forms are quarantine asRECORD_ERRORand can be added to the submissions queue to be reprocessed.This PR was motivated by an NPE that occurred when sending a data update broadcast during the Save to disk process, fixed in this PR, which caused the form to be quarantined in an unrecoverable manner. With this change, and while not applicable anymore due to the PR above, in case the same were to occur, the expectation is that the exception will bubble up as it did before, get caught by the generic catch clause in CommCareTask.doInBackground(), return
nulland subsequently save the form with a SAVE_ERROR status, the difference is that this status will now cause the form to be quarantined as RECORD_ERROR.cross-request: dimagi/commcare-core#1437
Safety Assurance
Safety story
Forms can be quarantined for various reasons and in order to test a few of the scenarios covered by this change,
IllegalStateExceptionandRuntimeExceptionwere purposely thrown at specific areas to simulate common real issues. Those quarantined due toRuntimeExceptionwere then sent back to the queue and submitted to HQ while theIllegalStateExceptionones didn't have the option to do so.