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

Add FhirEngine interface method 'withTransaction' #2535

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
857f112
Add FhirEngine interface method 'withTransaction'
LZRS May 6, 2024
5f2e163
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS May 26, 2024
72be126
Only allow FHIREngine CRUD api for withTransaction
LZRS May 26, 2024
e7bff9c
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS May 30, 2024
868877f
Rename BaseFhirEngine to CrudFhirEngine
LZRS May 30, 2024
75b41b5
Move count method back to FhirEngine class
LZRS May 30, 2024
7b769c1
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS Jun 1, 2024
22f0e57
Merge branch 'master' into 2531-fhirengine-interface
LZRS Jun 3, 2024
f048d86
Merge branch 'master' into 2531-fhirengine-interface
LZRS Jun 3, 2024
4b1e808
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS Jun 8, 2024
f336d2d
Merge branch 'master' into 2531-fhirengine-interface
LZRS Jun 13, 2024
c1aad5e
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS Jun 14, 2024
892ed27
Update 'withTransaction' tests
LZRS Jun 14, 2024
ddc3ecb
Merge branch 'master' into 2531-fhirengine-interface
LZRS Jun 26, 2024
2d2eec0
Merge branch 'master' into 2531-fhirengine-interface
LZRS Jul 1, 2024
52d4147
Merge branch 'master' into 2531-fhirengine-interface
LZRS Aug 22, 2024
ff0f0e1
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS Oct 4, 2024
99d3eb0
Merge remote-tracking branch 'upstream/master' into 2531-fhirengine-i…
LZRS Oct 4, 2024
b520d30
Revert to using FhirEngine interface
LZRS Oct 4, 2024
fe0cee6
Merge branch 'master' into 2531-fhirengine-interface
LZRS Oct 8, 2024
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
114 changes: 66 additions & 48 deletions engine/src/main/java/com/google/android/fhir/FhirEngine.kt
Original file line number Diff line number Diff line change
Expand Up @@ -58,54 +58,7 @@ import org.hl7.fhir.r4.model.ResourceType
* val fhirEngine = FhirEngineProvider.getInstance(this)
* ```
*/
interface FhirEngine {
/**
* Creates one or more FHIR [Resource]s in the local storage. FHIR Engine requires all stored
* resources to have a logical [Resource.id]. If the `id` is specified in the resource passed to
* [create], the resource created in `FhirEngine` will have the same `id`. If no `id` is
* specified, `FhirEngine` will generate a UUID as that resource's `id` and include it in the
* returned list of IDs.
*
* @param resource The FHIR resources to create.
* @return A list of logical IDs of the newly created resources.
*/
suspend fun create(vararg resource: Resource): List<String>

/**
* Loads a FHIR resource given its [ResourceType] and logical ID.
*
* @param type The type of the resource to load.
* @param id The logical ID of the resource.
* @return The requested FHIR resource.
* @throws ResourceNotFoundException if the resource is not found.
*/
@Throws(ResourceNotFoundException::class)
suspend fun get(type: ResourceType, id: String): Resource

/**
* Updates one or more FHIR [Resource]s in the local storage.
*
* @param resource The FHIR resources to update.
*/
suspend fun update(vararg resource: Resource)

/**
* Removes a FHIR resource given its [ResourceType] and logical ID.
*
* @param type The type of the resource to delete.
* @param id The logical ID of the resource.
*/
suspend fun delete(type: ResourceType, id: String)

/**
* Searches the database and returns a list of resources matching the [Search] specifications.
*
* @param search The search criteria to apply.
* @return A list of [SearchResult] objects containing the matching resources and any included
* references.
*/
suspend fun <R : Resource> search(search: Search): List<SearchResult<R>>

interface FhirEngine : CrudFhirEngine {
/**
* Synchronizes upload results with the database.
*
Expand Down Expand Up @@ -204,6 +157,71 @@ interface FhirEngine {
suspend fun purge(type: ResourceType, ids: Set<String>, forcePurge: Boolean = false)
}

interface CrudFhirEngine {
/**
* Creates one or more FHIR [Resource]s in the local storage. FHIR Engine requires all stored
* resources to have a logical [Resource.id]. If the `id` is specified in the resource passed to
* [create], the resource created in `FhirEngine` will have the same `id`. If no `id` is
* specified, `FhirEngine` will generate a UUID as that resource's `id` and include it in the
* returned list of IDs.
*
* @param resource The FHIR resources to create.
* @return A list of logical IDs of the newly created resources.
*/
suspend fun create(vararg resource: Resource): List<String>

/**
* Loads a FHIR resource given its [ResourceType] and logical ID.
*
* @param type The type of the resource to load.
* @param id The logical ID of the resource.
* @return The requested FHIR resource.
* @throws ResourceNotFoundException if the resource is not found.
*/
@Throws(ResourceNotFoundException::class)
suspend fun get(type: ResourceType, id: String): Resource

/**
* Updates one or more FHIR [Resource]s in the local storage.
*
* @param resource The FHIR resources to update.
*/
suspend fun update(vararg resource: Resource)

/**
* Removes a FHIR resource given its [ResourceType] and logical ID.
*
* @param type The type of the resource to delete.
* @param id The logical ID of the resource.
*/
suspend fun delete(type: ResourceType, id: String)

/**
* Searches the database and returns a list of resources matching the [Search] specifications.
*
* Example:
* ```
* fhirEngine.search<Patient> {
* filter(Patient.GIVEN, {
* value = "Kiran"
* modifier = StringFilterModifier.MATCHES_EXACTLY
* })
* }
* ```
*
* @param search The search criteria to apply.
* @return A list of [SearchResult] objects containing the matching resources and any included
* references.
*/
suspend fun <R : Resource> search(search: Search): List<SearchResult<R>>
MJ1998 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Adds support for performing actions on `FhirEngine` as a single atomic transaction where the
* entire set of changes succeed or fail as a single entity
*/
suspend fun withTransaction(block: suspend CrudFhirEngine.() -> Unit)
}

/**
* Retrieves a FHIR resource of type [R] with the given [id] from the local storage.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.android.fhir.impl

import android.content.Context
import com.google.android.fhir.CrudFhirEngine
import com.google.android.fhir.FhirEngine
import com.google.android.fhir.FhirEngineProvider
import com.google.android.fhir.LocalChange
Expand Down Expand Up @@ -106,6 +107,10 @@ internal class FhirEngineImpl(private val database: Database, private val contex
}
}

override suspend fun withTransaction(block: suspend CrudFhirEngine.() -> Unit) {
database.withTransaction { block.invoke(this@FhirEngineImpl) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this work?

Suggested change
database.withTransaction { block.invoke(this@FhirEngineImpl) }
database.withTransaction { this.block() }

}

private suspend fun saveResolvedResourcesToDatabase(resolved: List<Resource>?) {
resolved?.let {
database.deleteUpdates(it)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import androidx.work.Data
import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.context.FhirVersionEnum
import ca.uhn.fhir.parser.IParser
import com.google.android.fhir.CrudFhirEngine
import com.google.android.fhir.FhirEngine
import com.google.android.fhir.LocalChange
import com.google.android.fhir.LocalChangeToken
Expand Down Expand Up @@ -176,6 +177,8 @@ internal object TestFhirEngineImpl : FhirEngine {
download().collect()
}

override suspend fun withTransaction(block: suspend CrudFhirEngine.() -> Unit) {}

override suspend fun count(search: Search): Long {
return 0
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import com.google.android.fhir.FhirServices.Companion.builder
import com.google.android.fhir.LocalChange
import com.google.android.fhir.LocalChange.Type
import com.google.android.fhir.db.ResourceNotFoundException
import com.google.android.fhir.delete
import com.google.android.fhir.get
import com.google.android.fhir.lastUpdated
import com.google.android.fhir.logicalId
Expand Down Expand Up @@ -783,6 +784,87 @@ class FhirEngineImplTest {
assertThat(services.database.getLocalChangesCount()).isEqualTo(0)
}

@Test
fun `withTransaction saves all changes successfully in order`() = runTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you simplify this test case?

I think you should keep this test case to use at most 2 resources. For example a patient and an observation. this is because when you add more resources to reference each other it's not really testing anything new.

Even just create a patient and then an observation that references the patient in my opinion is enough.

The point of the transation API isn't that things happen in order.... things will be executed in order without transation. The point is really that a transaction is atomic and will be reverted as a whole. So this test case should just be simply testing that the transation api runs multiple operations. the next test case is more important.

val patient01ID = "patient-01"
val patient01 =
Patient().apply {
id = patient01ID
gender = Enumerations.AdministrativeGender.FEMALE
}
val patient02ID = "patient-02"
val patient02 =
Patient().apply {
id = patient02ID
gender = Enumerations.AdministrativeGender.MALE
}
val patient03ID = "patient-03"
val patient03 =
Patient().apply {
id = patient03ID
gender = Enumerations.AdministrativeGender.FEMALE
}

fhirEngine.create(patient01)
assertThat(fhirEngine.get<Patient>(patient01ID).gender)
.isEqualTo(Enumerations.AdministrativeGender.FEMALE)
fhirEngine.withTransaction {
fhirEngine.create(patient02, patient03)
patient01.gender = Enumerations.AdministrativeGender.FEMALE
fhirEngine.update(patient01)
fhirEngine.delete<Patient>(patient01ID)
MJ1998 marked this conversation as resolved.
Show resolved Hide resolved
}
assertResourceEquals(patient02, fhirEngine.get<Patient>(patient02ID))
assertResourceEquals(patient03, fhirEngine.get<Patient>(patient03ID))
assertThrows(ResourceNotFoundException::class.java) {
runBlocking { fhirEngine.get<Patient>(patient01ID) }
}
}

@Test
fun `withTransaction reverts all changes when an error occurs`() = runTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test case can be done using 2 resources.

before transation:
create a patient

in a transation:
create an encoutner for the patient, update the encouter to reference a non-existent patient.

check that the encounter is also rolled back.

val patient01ID = "patient-01"
val patient01 =
Patient().apply {
id = patient01ID
gender = Enumerations.AdministrativeGender.FEMALE
}
val patient02ID = "patient-02"
val patient02 =
Patient().apply {
id = patient02ID
gender = Enumerations.AdministrativeGender.MALE
}
val patient03ID = "patient-03"
val patient03 =
Patient().apply {
id = patient03ID
gender = Enumerations.AdministrativeGender.FEMALE
}
val patient03Updated =
patient03.copy().apply { gender = Enumerations.AdministrativeGender.MALE }

fhirEngine.create(patient03)
try {
fhirEngine.withTransaction {
this.create(patient01)
this.create(patient02)
this.update(patient03Updated)
this.get(ResourceType.Patient, "non_existent_patient_id")
as Patient // Force ResourceNotFoundException
}
} catch (_: ResourceNotFoundException) {}

assertThrows(ResourceNotFoundException::class.java) {
runBlocking { fhirEngine.get<Patient>(patient01ID) }
}
assertThrows(ResourceNotFoundException::class.java) {
runBlocking { fhirEngine.get<Patient>(patient02ID) }
}
assertResourceEquals(patient03, fhirEngine.get<Patient>(patient03ID))
assertResourceNotEquals(patient03Updated, fhirEngine.get<Patient>(patient03ID))
}

companion object {
private const val TEST_PATIENT_1_ID = "test_patient_1"
private var TEST_PATIENT_1 =
Expand Down
Loading