Fix sync state exception handling #1669

ellykits · 2022-10-17T13:26:04Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Description
Surround the call site of com.google.android.fhir.impl.FhirEngineImpl#saveRemoteResourcesToDatabase and com.google.android.fhir.impl.FhirEngineImpl#saveResolvedResourcesToDatabase in try-catch block.

Alternative(s) considered
The exception we were getting was a NullPointerException

java.lang.NullPointerException: coding.code must not be null
2022-10-18 10:38:10.162  7941-7985  FhirEngine...ed$collect org.smartregister.fhircore.map       E  	at com.google.android.fhir.index.ResourceIndexer.tokenIndex(ResourceIndexer.kt:319)
2022-10-18 10:38:10.162  7941-7985  FhirEngine...ed$collect org.smartregister.fhircore.map       E  	at com.google.android.fhir.index.ResourceIndexer.extractIndexValues(ResourceIndexer.kt:92)
2022-10-18 10:38:10.162  7941-7985  FhirEngine...ed$collect org.smartregister.fhircore.map       E  	at com.google.android.fhir.index.ResourceIndexer.index(ResourceIndexer.kt:71)
2022-10-18 10:38:10.162  7941-7985  FhirEngine...ed$collect org.smartregister.fhircore.map       E  	at com.google.android.fhir.db.impl.dao.ResourceDao.insertResource(ResourceDao.kt:194)

Solution would be to add the null check before using the property on this line com/google/android/fhir/index/ResourceIndexer.kt:317. This will however only address this type of exception and ignore any other exceptions that may happen.

Type
Choose one: Bug fix

Checklist

I have read and acknowledged the Code of conduct.
I have read the Contributing page.
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

Remove state emission from catch section of try-catch block. This violates flow exception handling. Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

When an exception occurs while attempting to save a resource, the rest of the process should not be affected. Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

maimoonak · 2022-10-17T14:22:07Z

engine/src/main/java/com/google/android/fhir/sync/download/DownloaderImpl.kt

@@ -53,7 +53,7 @@ internal class DownloaderImpl(
          )
        )
      } catch (exception: Exception) {
-        emit(DownloadState.Failure(ResourceSyncException(resourceTypeToDownload, exception)))


we may not want to remove this line. sync should emit failure when resource syncing fails.

I see. I have reverted this line so this state can be emitted.

But this needs to be handled not only by the implementers but also in FHirEngineImpl where the download function is called.

emitting events in catch block is however not recommended because of such unexpected behaviors when the exception is not correctly handle where we are collecting the states for the flow.

maimoonak · 2022-10-17T14:25:26Z

engine/src/main/java/com/google/android/fhir/impl/FhirEngineImpl.kt

+            saveResolvedResourcesToDatabase(resolved)
+          }
+        } catch (exception: Exception) {
+          Timber.e(exception, "Save remote and resolved conflicting resources to database failed")


we may not want to just catch exception for resources which have failed to save. it should either show this error to user, or it should do something to log error somewhere to resolve the issue with DB. this can lead to suppress app issues with data collection without getting noticed.

I agree we may need to do more with this error log but do we really want to show the user this error? Is there a strategy in SDK for handling/logging errors?

Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

- With unmerged PR google#1669 branch

- With unmerged PR #1 - With unmerged PR google#1917 - With unmerged PR google#1978 - With unmerged PR google#1907 - With unmerged PR google#2016 - With unmerged PR google#2032 - With unmerged PR google#1669

- With unmerged PR #1 - With unmerged PR google#1917 - With unmerged PR google#1978 - With unmerged PR google#1907 - With unmerged PR google#2032 - With unmerged PR google#1669 - With unmerged PR google#2047

- With unmerged PR google#2032 - With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2132 - With unmerged PR #9

- With unmerged PR google#2032 - With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2132

- With unmerged PR google#2032 - With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2132 - With unmerged PR #9 - With unmerged PR google#2076 - With unmerged PR #10

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR #9 - With unmerged PR google#2076 - With unmerged PR #10

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR #9 - With unmerged PR #10

jingtang10 · 2023-09-14T15:07:40Z

hey @ellykits - sorry for not having closed this earlier. looking at this PR again i'm not confident this is the route we want to take to fix the problem. what is done here is wrapping the problem in a try catch instead of addressing the exception. for example, if it's network issue, we might need to try again, or if this is a failure we can't recover from we need a way for the application to know about that.

additionally, @santosh-pingle is working on #2142 which is very relevant to this problem.

so i'll close this PR - but we should still keep the issue open to continue exploring the correct solution.

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2178 - With unmerged PR #9 - With unmerged PR #10

- With unmerged PR google#1669 - Wup google#2178 - Wup #9 - Wup #10 - Wup google#2076

- With unmerged PR #11 - Wup google#1669 - Wup google#2076 - Wup #9 - Wup google#2178 - Wup #10

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178 - Wup #10 - Wup google#2230 - Wup google#2262

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

PR google#1669

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

- With unmerged PR google#11 - Wup google#1669 - Wup google#9 - Wup google#2178

ellykits added 2 commits October 17, 2022 16:18

Fix IllegalStateException

52ceb2d

Remove state emission from catch section of try-catch block. This violates flow exception handling. Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

Handle exceptions emitted by sync download

71ca622

When an exception occurs while attempting to save a resource, the rest of the process should not be affected. Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

ellykits requested review from aditya-07, deepankarb, jingtang10, jjtswan, kevinmost, ktarasenko, omarismail94, stevenckngaa, vvikramraj, wantingzhang77 and yigit as code owners October 17, 2022 13:26

ellykits added 2 commits October 17, 2022 16:30

Run spotlessApply

65f8e44

Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

Move try-catch block inside flow collect operator

5a6f8b7

Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

maimoonak reviewed Oct 17, 2022

View reviewed changes

Revert emit ResourceSyncException

0913137

Signed-off-by: Elly Kitoto <junkmailstoelly@gmail.com>

ellykits force-pushed the fix-sync-state-excepiton branch from 3b345f5 to 0913137 Compare October 17, 2022 15:04

ellykits and others added 5 commits October 18, 2022 16:26

Merge branch 'master' into fix-sync-state-excepiton

fcc0dbd

Merge branch 'master' into fix-sync-state-excepiton

116df94

Merge branch 'master' into fix-sync-state-excepiton

5f8c07d

Merge branch 'master' into fix-sync-state-excepiton

cfd11ae

Merge branch 'master' into fix-sync-state-excepiton

3f25e49

jingtang10 self-assigned this Dec 4, 2022

ndegwamartin and others added 4 commits December 7, 2022 11:58

Merge branch 'master' into fix-sync-state-excepiton

cee67c2

Merge branch 'master' into fix-sync-state-excepiton

5e6a522

Merge branch 'master' into fix-sync-state-excepiton

f10d8e8

Merge branch 'master' into fix-sync-state-excepiton

59852af

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Feb 24, 2023

SDK Release: Engine, SDC

10e46a0

- With unmerged PR google#1669 branch

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 1, 2023

SDK Release: Barcode

fb3b13b

- With unmerged PR google#1669 branch

Merge branch 'master' into fix-sync-state-excepiton

adcba26

ndegwamartin mentioned this pull request Aug 23, 2023

Migrate FHIR Core to the latest FHIR SDK artifacts opensrp/fhircore#2708

Closed

3 tasks

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Aug 23, 2023

Engine, SDC, Workflow, Common, Knowledge

a6b0ec2

- With unmerged PR google#2032 - With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2132 - With unmerged PR #9

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Aug 23, 2023

Engine, SDC, Workflow, Common, Knowledger

1c93f90

- With unmerged PR google#2032 - With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2132

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 13, 2023

Engine, SDC, Workflow, Common, Knowledge

314b479

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR #9 - With unmerged PR google#2076 - With unmerged PR #10

Merge branch 'master' into fix-sync-state-excepiton

b84237e

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 13, 2023

Engine, SDC, Workflow, Common, Knowledge

edb2842

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR #9 - With unmerged PR #10

jingtang10 closed this Sep 14, 2023

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 18, 2023

Engine, SDC, Workflow, Knowledge

80b2d69

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2178 - With unmerged PR #9 - With unmerged PR #10

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 18, 2023

Engine, SDC, Workflow, Knowledge

283ad6e

- With unmerged PR #1 - With unmerged PR google#1669 - With unmerged PR google#2178 - With unmerged PR #9 - With unmerged PR #10

ndegwamartin mentioned this pull request Sep 20, 2023

Database migration exception force closes the app #2170

Closed

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 21, 2023

Engine, SDC

4abd9cc

- With unmerged PR google#1669 - Wup google#2178 - Wup #9 - Wup #10 - Wup google#2076

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Sep 21, 2023

Engine, SDC

658f9c4

- With unmerged PR #11 - Wup google#1669 - Wup google#2076 - Wup #9 - Wup google#2178 - Wup #10

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 13, 2023

Engine, SDC, Workflow, Common, Knowledge

ac92d70

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178 - Wup #10 - Wup google#2230 - Wup google#2262

ndegwamartin mentioned this pull request Oct 19, 2023

Migrate FHIR Core to the latest FHIR SDK artifacts opensrp/fhircore#2816

Closed

5 tasks

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 30, 2023

Engine, SDC, Workflow, Common, Knowledge

afc72b1

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Oct 30, 2023

Engine, SDC, Workflow, Common, Knowledge

111c662

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

Rkareko added a commit to opensrp/android-fhir that referenced this pull request Nov 24, 2023

Engine, SDC, Workflow, Common, Knowledge

5276c82

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

Rkareko added a commit to opensrp/android-fhir that referenced this pull request Nov 24, 2023

Fix sync state exception handling

09a637c

PR google#1669

Rkareko added a commit to opensrp/android-fhir that referenced this pull request Dec 19, 2023

Engine, SDC, Workflow, Common, Knowledge

d03017a

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

Rkareko added a commit to opensrp/android-fhir that referenced this pull request Jan 9, 2024

Engine, SDC, Workflow, Common, Knowledge, Datacapture, Contrib, Catalog

dfe6232

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

Rkareko added a commit to opensrp/android-fhir that referenced this pull request Jan 10, 2024

Engine, Workflow

219cd36

- With unmerged PR #11 - Wup google#1669 - Wup #9 - Wup google#2178

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jan 22, 2024

SDC, Engine, Workflow

69c2ebf

- With unmerged PR google#11 - Wup google#1669 - Wup google#9 - Wup google#2178

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jan 22, 2024

SDC, Engine, Workflow

64ffbae

- With unmerged PR google#11 - Wup google#1669 - Wup google#9 - Wup google#2178

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sync state exception handling #1669

Fix sync state exception handling #1669

ellykits commented Oct 17, 2022 •

edited

Loading

maimoonak Oct 17, 2022

ellykits Oct 17, 2022

ellykits Oct 17, 2022

ellykits Oct 17, 2022

maimoonak Oct 17, 2022

ellykits Oct 17, 2022

jingtang10 commented Sep 14, 2023

Fix sync state exception handling #1669

Fix sync state exception handling #1669

Conversation

ellykits commented Oct 17, 2022 • edited Loading

maimoonak Oct 17, 2022

Choose a reason for hiding this comment

ellykits Oct 17, 2022

Choose a reason for hiding this comment

ellykits Oct 17, 2022

Choose a reason for hiding this comment

ellykits Oct 17, 2022

Choose a reason for hiding this comment

maimoonak Oct 17, 2022

Choose a reason for hiding this comment

ellykits Oct 17, 2022

Choose a reason for hiding this comment

jingtang10 commented Sep 14, 2023

ellykits commented Oct 17, 2022 •

edited

Loading