Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sync state exception handling #1669

Closed
wants to merge 18 commits into from
Closed

Fix sync state exception handling #1669

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
52ceb2d
Fix IllegalStateException
ellykits Oct 17, 2022
71ca622
Handle exceptions emitted by sync download
ellykits Oct 17, 2022
65f8e44
Run spotlessApply
ellykits Oct 17, 2022
5a6f8b7
Move try-catch block inside flow collect operator
ellykits Oct 17, 2022
0913137
Revert emit ResourceSyncException
ellykits Oct 17, 2022
fcc0dbd
Merge branch 'master' into fix-sync-state-excepiton
ellykits Oct 18, 2022
116df94
Merge branch 'master' into fix-sync-state-excepiton
ellykits Oct 19, 2022
5f8c07d
Merge branch 'master' into fix-sync-state-excepiton
f-odhiambo Oct 21, 2022
cfd11ae
Merge branch 'master' into fix-sync-state-excepiton
ellykits Oct 31, 2022
3f25e49
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Nov 11, 2022
cee67c2
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Dec 7, 2022
5e6a522
Merge branch 'master' into fix-sync-state-excepiton
dubdabasoduba Jan 19, 2023
f10d8e8
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Feb 23, 2023
59852af
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Feb 24, 2023
a0326af
Merge branch 'master' into fix-sync-state-excepiton
f-odhiambo Mar 17, 2023
c60eeba
Merge branch 'master' into fix-sync-state-excepiton
ellykits Jun 20, 2023
adcba26
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Aug 22, 2023
b84237e
Merge branch 'master' into fix-sync-state-excepiton
ndegwamartin Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions engine/src/main/java/com/google/android/fhir/impl/FhirEngineImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,20 +88,24 @@ internal class FhirEngineImpl(private val database: Database, private val contex
download: suspend (SyncDownloadContext) -> Flow<List<Resource>>
) {
download(
object : SyncDownloadContext {
override suspend fun getLatestTimestampFor(type: ResourceType) = database.lastUpdate(type)
}
)
object : SyncDownloadContext {
override suspend fun getLatestTimestampFor(type: ResourceType) = database.lastUpdate(type)
}
)
.collect { resources ->
database.withTransaction {
val resolved =
resolveConflictingResources(
resources,
getConflictingResourceIds(resources),
conflictResolver
)
saveRemoteResourcesToDatabase(resources)
saveResolvedResourcesToDatabase(resolved)
try {
database.withTransaction {
val resolved =
resolveConflictingResources(
resources,
getConflictingResourceIds(resources),
conflictResolver
)
saveRemoteResourcesToDatabase(resources)
saveResolvedResourcesToDatabase(resolved)
}
} catch (exception: Exception) {
Timber.e(exception, "Save remote and resolved conflicting resources to database failed")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}
Expand All @@ -115,9 +119,11 @@ internal class FhirEngineImpl(private val database: Database, private val contex

private suspend fun saveRemoteResourcesToDatabase(resources: List<Resource>) {
val timeStamps =
resources.groupBy { it.resourceType }.entries.map {
SyncedResourceEntity(it.key, it.value.maxOf { it.meta.lastUpdated }.toTimeZoneString())
}
resources
.groupBy { it.resourceType }
.entries.map {
SyncedResourceEntity(it.key, it.value.maxOf { it.meta.lastUpdated }.toTimeZoneString())
}
database.insertSyncedResources(timeStamps, resources)
}

Expand Down Expand Up @@ -207,7 +213,8 @@ internal class FhirEngineImpl(private val database: Database, private val contex
*/
private val Bundle.BundleEntryResponseComponent.resourceIdAndType: Pair<String, ResourceType>?
get() =
location?.split("/")?.takeIf { it.size > 3 }?.let {
it[it.size - 3] to ResourceType.fromCode(it[it.size - 4])
}
location
?.split("/")
?.takeIf { it.size > 3 }
?.let { it[it.size - 3] to ResourceType.fromCode(it[it.size - 4]) }
}
6 changes: 3 additions & 3 deletions engine/src/main/java/com/google/android/fhir/sync/download/DownloaderImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 Google LLC
* Copyright 2022 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -21,10 +21,10 @@ import com.google.android.fhir.sync.DataSource
import com.google.android.fhir.sync.DownloadState
import com.google.android.fhir.sync.DownloadWorkManager
import com.google.android.fhir.sync.Downloader
import com.google.android.fhir.sync.ResourceSyncException
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
import org.hl7.fhir.r4.model.ResourceType
import timber.log.Timber

/**
* Implementation of the [Downloader]. It orchestrates the pre & post processing of resources via
Expand Down Expand Up @@ -53,7 +53,7 @@ internal class DownloaderImpl(
)
)
} catch (exception: Exception) {
emit(DownloadState.Failure(ResourceSyncException(resourceTypeToDownload, exception)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Timber.e(exception, "Resource type $resourceTypeToDownload failed to sync")
}

url = downloadWorkManager.getNextRequestUrl(context)
Expand Down