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.

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

Catch and emit exceptions from downloading invalid resources #1917

Closed
wants to merge 8 commits into from
Closed

Catch and emit exceptions from downloading invalid resources #1917

wants to merge 8 commits into from

Conversation

omarismail94
Copy link
Contributor

@omarismail94 omarismail94 commented Mar 17, 2023

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

Fixes #1654

Description

  • This catches any error when processing resources that were successfully dowloaded, but cannot be synced into the db as they are invalid (e.g. missing field)
  • Add unit tests for the FhirSynchronizer class

Alternative(s) considered
See PR #1669

Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)

Screenshots (if applicable)

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).

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Mar 21, 2023
- With unmerged PR #1
- With unmerged PR google#1669
- With unmerged PR google#1927
- With unmerged PR google#1917
@ellykits
Copy link
Collaborator

@omarismail94 Testing this implementation against our application.

@ellykits
Copy link
Collaborator

I will close my previous PR in favor of this. I can see the error logs and sync progress is not interrupted.

@ellykits ellykits mentioned this pull request Mar 22, 2023
7 tasks
}
}
}
.catch { exceptions.add(ResourceSyncException(ResourceType.Bundle, Exception(it))) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

discussed - if the exception is actually raised in the code where the already downloaded resources are being saved to the db, shouldn't the exception handling code be there instead of here where the resources are being downloaded?

perhaps - and only just perhaps - fhir engine's syncdownload function should return a flow... and you handle the exceptions here... but i'm not entirely sure that's the right solution here.

allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 14, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 17, 2023
  - With unmerged PR google#1973
  - With unmerged PR #1
  - With unmerged PR google#1917
allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 26, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
allan-on added a commit to opensrp/android-fhir that referenced this pull request Apr 26, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
@ellykits
Copy link
Collaborator

Deleted my previous comment. Our issue is still not resolved yet. I had tested with my previous implementation instead.

Sync job finishes with a failure when an exception is encountered, ignores the rest urls.

java.lang.NullPointerException: coding.code must not be null
                                                                                                    	at com.google.android.fhir.index.ResourceIndexer.tokenIndex(ResourceIndexer.kt:322)
                                                                                                    	at com.google.android.fhir.index.ResourceIndexer.extractIndexValues(ResourceIndexer.kt:95)
                                                                                                    	at com.google.android.fhir.index.ResourceIndexer.index(ResourceIndexer.kt:73)
                                                                                                    	at com.google.android.fhir.db.impl.dao.ResourceDao.insertResource(ResourceDao.kt:174)
                                                                                                    	at com.google.android.fhir.db.impl.dao.ResourceDao.insertAll$suspendImpl(ResourceDao.kt:75)
                                                                                                    	at com.google.android.fhir.db.impl.dao.ResourceDao.insertAll(Unknown Source:0)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertRemote$2.invokeSuspend(DatabaseImpl.kt:120)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertRemote$2.invoke(Unknown Source:8)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertRemote$2.invoke(Unknown Source:2)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invokeSuspend(RoomDatabase.kt:58)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invoke(Unknown Source:8)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invoke(Unknown Source:4)
                                                                                                    	at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:89)
                                                                                                    	at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:169)
                                                                                                    	at kotlinx.coroutines.BuildersKt.withContext(Unknown Source:1)
                                                                                                    	at androidx.room.RoomDatabaseKt.withTransaction(RoomDatabase.kt:51)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl.insertRemote(DatabaseImpl.kt:120)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertSyncedResources$2.invokeSuspend(DatabaseImpl.kt:159)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertSyncedResources$2.invoke(Unknown Source:8)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl$insertSyncedResources$2.invoke(Unknown Source:2)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invokeSuspend(RoomDatabase.kt:58)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invoke(Unknown Source:8)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invoke(Unknown Source:4)
                                                                                                    	at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:89)
                                                                                                    	at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:169)
                                                                                                    	at kotlinx.coroutines.BuildersKt.withContext(Unknown Source:1)
                                                                                                    	at androidx.room.RoomDatabaseKt.withTransaction(RoomDatabase.kt:51)
                                                                                                    	at com.google.android.fhir.db.impl.DatabaseImpl.insertSyncedResources(DatabaseImpl.kt:159)
                                                                                                    	at com.google.android.fhir.impl.FhirEngineImpl$syncDownload$2$1.invokeSuspend(FhirEngineImpl.kt:126)
                                                                                                    	at com.google.android.fhir.impl.FhirEngineImpl$syncDownload$2$1.invoke(Unknown Source:8)
                                                                                                    	at com.google.android.fhir.impl.FhirEngineImpl$syncDownload$2$1.invoke(Unknown Source:2)
                                                                                                    	at androidx.room.RoomDatabaseKt$withTransaction$2.invokeSuspend(RoomDatabase.kt:58)
                                                                                                    	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                                                                                                    	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
                                                                                                    	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:284)
                                                                                                    	at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:85)
                                                                                                    	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
                                                                                                    	at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source:1)
                                                                                                    	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
                                                                                                    	at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source:1)
                                                                                                    	at androidx.room.RoomDatabaseKt$acquireTransactionThread$2$2.run(RoomDatabase.kt:121)
                                                                                                    	at androidx.room.TransactionExecutor$1.run(TransactionExecutor.java:47)
                                                                                                    	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
                                                                                                    	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
                                                                                                    	at java.lang.Thread.run(Thread.java:920)

@ellykits
Copy link
Collaborator

I understand the bug can be fixed by adding a null check before using the Code or by using valid data but this will not fix the underlying problem. If an exception happens when downloading data for a given URL, the remaining URL downloads should continue regardless.

ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 11, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1963
  - With unmerged PR google#1907
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request May 23, 2023
  - With unmerged PR #1
  - With unmerged PR google#1917
  - With unmerged PR google#1978
  - With unmerged PR google#1964
  - With unmerged PR google#1907
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 15, 2023
  - 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
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2023
      - 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
ndegwamartin added a commit to opensrp/android-fhir that referenced this pull request Jun 26, 2023
   - 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
@aditya-07
Copy link
Collaborator

@omarismail94 Do we have any update in this PR?

@omarismail94 omarismail94 deleted the mimicexception branch December 20, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download fails to complete when exception is encountered
4 participants