Fix sync failure state exception handling #1656
Conversation
Signed-off-by: Elly Kitoto <[email protected]>
Signed-off-by: Elly Kitoto <[email protected]>
ellykits
requested review from
aditya-07,
deepankarb,
jingtang10,
jjtswan,
kevinmost,
ktarasenko,
omarismail94,
stevenckngaa,
vvikramraj,
wantingzhang77 and
yigit
as code owners
| @@ -30,16 +30,17 @@ import org.hl7.fhir.r4.model.ResourceType | |||
| class DownloadWorkManagerImpl : DownloadWorkManager { | |||
| private val resourceTypeList = ResourceType.values().map { it.name } | |||
| private val urls = LinkedList(listOf("Patient?address-city=NAIROBI")) | |||
| override var nextRequestUrl: String? = null | |||
There was a problem hiding this comment.
can we make it private. ideally this should not be modifiable outside this class
There was a problem hiding this comment.
I see. This was exposed to be used in com.google.android.fhir.sync.download.DownloaderImpl#download to get the resource type that is currently being synced. The catch operator can only access the flow instance and not the internal states within the flow { } closure.
There was a problem hiding this comment.
Do we need the resource type in ResourceSyncException data class? Most instance creation of that class have ResourceType.Bundle provided as the resource type.
There was a problem hiding this comment.
I guess it does emit the failed resource type. the test DownloaderImplTest.downloader with patient and observations should continue to download observations if patient download fail has an assertion for this.
There was a problem hiding this comment.
Investigating the failing tests.
There was a problem hiding this comment.
I figured the cause of the filing tests. So apparently the catch operator completes the flow thus the remaining states are not emitted.
There was a problem hiding this comment.
The test was verifying whether emissions still happens even when there is a failure. Which I think should be the correct behavior. I will revert and remove the emission in the catch section of the try-catch block.
| .catch { throwable: Throwable -> | ||
| Timber.e(throwable, "Error saving remote resource to database") | ||
| } | ||
| .collect() |
There was a problem hiding this comment.
we are using on each and doing the processing above. do we need collect now?
There was a problem hiding this comment.
Yes collect operator is required to process all emitted values and handle exceptions that might occur in the upstream flow or during processing.
|
@maimoonak I will have to revert the commits; then create a new PR instead of maintaining the commit history of reverted code. |
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #1654
Description
Clear and concise code change description.
Type
Choose one: Bug fix
Checklist
./gradlew spotlessApplyand./gradlew spotlessCheckto check my code follows the style guide of this project../gradlew checkand./gradlew connectedCheckto test my changes locally.