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 all 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
6 changes: 6 additions & 0 deletions engine/src/main/java/com/google/android/fhir/FhirEngine.kt
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,12 @@ interface FhirEngine {
* back and no record is purged.
*/
suspend fun purge(type: ResourceType, ids: Set<String>, forcePurge: Boolean = false)

/**
* 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 FhirEngine.() -> Unit)
LZRS marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ internal class FhirEngineImpl(private val database: Database, private val contex
}
}

override suspend fun withTransaction(block: suspend FhirEngine.() -> 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 @@ -176,6 +176,8 @@ internal object TestFhirEngineImpl : FhirEngine {
download().collect()
}

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

override suspend fun count(search: Search): Long {
return 0
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,18 @@ import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.runTest
import org.hl7.fhir.exceptions.FHIRException
import org.hl7.fhir.r4.model.Address
import org.hl7.fhir.r4.model.Appointment
import org.hl7.fhir.r4.model.CanonicalType
import org.hl7.fhir.r4.model.Coding
import org.hl7.fhir.r4.model.DateTimeType
import org.hl7.fhir.r4.model.Encounter
import org.hl7.fhir.r4.model.Enumerations
import org.hl7.fhir.r4.model.HumanName
import org.hl7.fhir.r4.model.Meta
import org.hl7.fhir.r4.model.Observation
import org.hl7.fhir.r4.model.Patient
import org.hl7.fhir.r4.model.Practitioner
import org.hl7.fhir.r4.model.Reference
import org.hl7.fhir.r4.model.ResourceType
import org.junit.Assert.assertThrows
import org.junit.Before
Expand Down Expand Up @@ -783,6 +788,132 @@ 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 patient01AppointmentID = "appointment-01"
val patient01Appointment =
Appointment().apply {
id = patient01AppointmentID
status = Appointment.AppointmentStatus.BOOKED
addParticipant(
Appointment.AppointmentParticipantComponent().apply {
actor = Reference("${patient01.resourceType}/$patient01ID")
},
)
}
fhirEngine.create(patient01, patient01Appointment)

// Fulfill appointment with related Encounter/Observation
val patient01AppointmentEncounterID = "enc-01"
val patient01AppointmentEncounter =
Encounter().apply {
id = patient01AppointmentEncounterID
subject = Reference("${patient01.resourceType}/$patient01ID")
addAppointment(Reference("${patient01Appointment.resourceType}/$patient01AppointmentID"))
}
val patient01AppointmentEncounterObservationID = "obs-01"
val patient01AppointmentEncounterObservation =
Observation().apply {
id = patient01AppointmentEncounterObservationID
encounter =
Reference(
"${patient01AppointmentEncounter.resourceType}/$patient01AppointmentEncounterID",
)
}
val updatedAppointment =
patient01Appointment.copy().apply { status = Appointment.AppointmentStatus.FULFILLED }

fhirEngine.withTransaction {
this.create(patient01AppointmentEncounter, patient01AppointmentEncounterObservation)
this.update(updatedAppointment)
}

assertThat(
fhirEngine.get<Encounter>(patient01AppointmentEncounterID).appointmentFirstRep.reference,
)
.isEqualTo("Appointment/$patient01AppointmentID")
assertThat(
fhirEngine.get<Observation>(patient01AppointmentEncounterObservationID).encounter.reference,
)
.isEqualTo("Encounter/$patient01AppointmentEncounterID")
assertThat(fhirEngine.get<Appointment>(patient01AppointmentID).status)
.isEqualTo(Appointment.AppointmentStatus.FULFILLED)
}

@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 patient01AppointmentID = "appointment-01"
val patient01Appointment =
Appointment().apply {
id = patient01AppointmentID
status = Appointment.AppointmentStatus.BOOKED
addParticipant(
Appointment.AppointmentParticipantComponent().apply {
actor = Reference("${patient01.resourceType}/$patient01ID")
},
)
}
fhirEngine.create(patient01, patient01Appointment)

// Fulfill appointment with related Encounter/Observation
val patient01AppointmentEncounterID = "enc-01"
val patient01AppointmentEncounter =
Encounter().apply {
id = patient01AppointmentEncounterID
subject = Reference("${patient01.resourceType}/$patient01ID")
addAppointment(Reference("${patient01Appointment.resourceType}/$patient01AppointmentID"))
}
val patient01AppointmentEncounterObservationID = "obs-01"
val patient01AppointmentEncounterObservation =
Observation().apply {
id = patient01AppointmentEncounterObservationID
encounter =
Reference(
"${patient01AppointmentEncounter.resourceType}/$patient01AppointmentEncounterID",
)
}

try {
fhirEngine.withTransaction {
this.create(patient01AppointmentEncounter, patient01AppointmentEncounterObservation)
// Get non-existent practitioner to force ResourceNotFoundException
val nonExistentPractitioner =
this.get(ResourceType.Practitioner, "non_existent_practitioner_id") as Practitioner
val updatedAppointment =
patient01Appointment.copy().apply {
status = Appointment.AppointmentStatus.FULFILLED
addParticipant(
Appointment.AppointmentParticipantComponent().apply {
actor = Reference("Practitioner/${nonExistentPractitioner.logicalId}")
},
)
}
this.update(updatedAppointment)
}
} catch (_: ResourceNotFoundException) {}

assertThrows(ResourceNotFoundException::class.java) {
runBlocking { fhirEngine.get<Encounter>(patient01AppointmentEncounterID) }
}
assertThrows(ResourceNotFoundException::class.java) {
runBlocking { fhirEngine.get<Observation>(patient01AppointmentEncounterObservationID) }
}
assertThat(fhirEngine.get<Appointment>(patient01AppointmentID).status)
.isEqualTo(Appointment.AppointmentStatus.BOOKED)
}

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