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

Refactor usages of Json Parser for thread safety #2749

Merged
merged 4 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class DatabaseImplTest {
@JvmField @Parameterized.Parameter(0) var encrypted: Boolean = false

private val context: Context = ApplicationProvider.getApplicationContext()
private val parser = FhirContext.forR4Cached().newJsonParser()
private lateinit var services: FhirServices
private lateinit var database: Database

Expand Down Expand Up @@ -202,7 +203,7 @@ class DatabaseImplTest {
fun getLocalChanges_withSingleLocaleChange_shouldReturnSingleLocalChanges() = runBlocking {
val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json")
database.insert(patient)
val patientString = services.parser.encodeResourceToString(patient)
val patientString = parser.encodeResourceToString(patient)
val resourceLocalChanges = database.getLocalChanges(patient.resourceType, patient.logicalId)
assertThat(resourceLocalChanges.size).isEqualTo(1)
with(resourceLocalChanges[0]) {
Expand Down Expand Up @@ -269,7 +270,7 @@ class DatabaseImplTest {
fun clearDatabase_shouldClearAllTablesData() = runBlocking {
val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json")
database.insert(patient)
val patientString = services.parser.encodeResourceToString(patient)
val patientString = parser.encodeResourceToString(patient)
val resourceLocalChanges = database.getLocalChanges(patient.resourceType, patient.logicalId)
assertThat(resourceLocalChanges.size).isEqualTo(1)
with(resourceLocalChanges[0]) {
Expand Down Expand Up @@ -393,7 +394,7 @@ class DatabaseImplTest {

@Test
fun insert_shouldAddInsertLocalChange() = runBlocking {
val testPatient2String = services.parser.encodeResourceToString(TEST_PATIENT_2)
val testPatient2String = parser.encodeResourceToString(TEST_PATIENT_2)
database.insert(TEST_PATIENT_2)
val resourceLocalChanges =
database.getAllLocalChanges().filter { it.resourceId.equals(TEST_PATIENT_2_ID) }
Expand Down Expand Up @@ -481,7 +482,7 @@ class DatabaseImplTest {
database.insert(patient)
patient = readFromFile(Patient::class.java, "/update_test_patient_1.json")
database.update(patient)
services.parser.encodeResourceToString(patient)
parser.encodeResourceToString(patient)
val localChangeTokenIds =
database
.getAllLocalChanges()
Expand Down Expand Up @@ -4101,7 +4102,7 @@ class DatabaseImplTest {
val observationLocalChange = updatedObservationLocalChanges[0]
assertThat(observationLocalChange.type).isEqualTo(LocalChange.Type.INSERT)
val observationLocalChangePayload =
services.parser.parseResource(observationLocalChange.payload) as Observation
parser.parseResource(observationLocalChange.payload) as Observation
assertThat(observationLocalChangePayload.subject.reference)
.isEqualTo("Patient/$remotelyCreatedPatientResourceId")
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023 Google LLC
* Copyright 2023-2024 Google LLC
aditya-07 marked this conversation as resolved.
Show resolved Hide resolved
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -22,7 +22,6 @@ import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.MediumTest
import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.context.FhirVersionEnum
import ca.uhn.fhir.util.FhirTerser
import com.google.android.fhir.DatabaseErrorStrategy.RECREATE_AT_OPEN
import com.google.android.fhir.DatabaseErrorStrategy.UNSPECIFIED
Expand All @@ -49,8 +48,7 @@ import org.junit.runner.RunWith
@RunWith(AndroidJUnit4::class)
class EncryptedDatabaseErrorTest {
private val context: Context = ApplicationProvider.getApplicationContext()
private val parser = FhirContext.forR4().newJsonParser()
private val terser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
private val terser = FhirTerser(FhirContext.forR4Cached())
private val resourceIndexer = ResourceIndexer(SearchParamDefinitionsProviderImpl())

@After
Expand All @@ -66,7 +64,6 @@ class EncryptedDatabaseErrorTest {
// GIVEN an unencrypted database.
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand All @@ -84,7 +81,6 @@ class EncryptedDatabaseErrorTest {
// THEN it should throw SQLiteException
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand Down Expand Up @@ -115,7 +111,6 @@ class EncryptedDatabaseErrorTest {
// GIVEN an unencrypted database.
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand All @@ -139,7 +134,6 @@ class EncryptedDatabaseErrorTest {
// THEN it should throw SQLiteException
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand Down Expand Up @@ -169,7 +163,6 @@ class EncryptedDatabaseErrorTest {
// GIVEN an unencrypted database.
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand All @@ -193,7 +186,6 @@ class EncryptedDatabaseErrorTest {
// THEN it should recreate the database
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand Down Expand Up @@ -226,7 +218,6 @@ class EncryptedDatabaseErrorTest {
// GIVEN an encrypted database.
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand All @@ -244,7 +235,6 @@ class EncryptedDatabaseErrorTest {
// THEN it should recreate database.
DatabaseImpl(
context,
parser,
terser,
DatabaseConfig(
inMemory = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import org.junit.runner.RunWith
class LocalChangeDaoTest {
private lateinit var database: ResourceDatabase
private lateinit var localChangeDao: LocalChangeDao
private val iParser = FhirContext.forR4Cached().newJsonParser()

@Before
fun setupDatabase() {
Expand All @@ -62,7 +63,6 @@ class LocalChangeDaoTest {

localChangeDao =
database.localChangeDao().also {
it.iParser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser()
it.fhirTerser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
}
}
Expand Down Expand Up @@ -97,8 +97,7 @@ class LocalChangeDaoTest {
assertThat(carePlanLocalChange1.resourceUuid).isEqualTo(carePlanResourceUuid)
assertThat(carePlanLocalChange1.resourceId).isEqualTo(carePlan.id)
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
assertThat(carePlanLocalChange1.payload)
.isEqualTo(localChangeDao.iParser.encodeResourceToString(carePlan))
assertThat(carePlanLocalChange1.payload).isEqualTo(iParser.encodeResourceToString(carePlan))
val carePlanLocalChange1Id = carePlanLocalChange1.id

val localChangeResourceReferences =
Expand Down Expand Up @@ -150,7 +149,7 @@ class LocalChangeDaoTest {
resourceId = originalCarePlan.logicalId,
resourceType = originalCarePlan.resourceType,
resourceUuid = carePlanResourceUuid,
serializedResource = localChangeDao.iParser.encodeResourceToString(originalCarePlan),
serializedResource = iParser.encodeResourceToString(originalCarePlan),
),
updatedResource = modifiedCarePlan,
timeOfLocalChange = carePlanUpdateTime,
Expand All @@ -163,7 +162,7 @@ class LocalChangeDaoTest {
assertThat(carePlanLocalChange1.resourceId).isEqualTo(originalCarePlan.id)
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
assertThat(carePlanLocalChange1.payload)
.isEqualTo(localChangeDao.iParser.encodeResourceToString(originalCarePlan))
.isEqualTo(iParser.encodeResourceToString(originalCarePlan))

val carePlanLocalChange2 = carePlanLocalChanges[1]
assertThat(carePlanLocalChange2.resourceUuid).isEqualTo(carePlanResourceUuid)
Expand Down Expand Up @@ -224,8 +223,7 @@ class LocalChangeDaoTest {
assertThat(carePlanLocalChange1.resourceUuid).isEqualTo(carePlanResourceUuid)
assertThat(carePlanLocalChange1.resourceId).isEqualTo(carePlan.id)
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
assertThat(carePlanLocalChange1.payload)
.isEqualTo(localChangeDao.iParser.encodeResourceToString(carePlan))
assertThat(carePlanLocalChange1.payload).isEqualTo(iParser.encodeResourceToString(carePlan))

val carePlanLocalChange2 = carePlanLocalChanges[1]
assertThat(carePlanLocalChange2.resourceUuid).isEqualTo(carePlanResourceUuid)
Expand Down Expand Up @@ -285,7 +283,7 @@ class LocalChangeDaoTest {
resourceId = originalCarePlan.logicalId,
resourceType = originalCarePlan.resourceType,
resourceUuid = carePlanResourceUuid,
serializedResource = localChangeDao.iParser.encodeResourceToString(originalCarePlan),
serializedResource = iParser.encodeResourceToString(originalCarePlan),
),
updatedResource = modifiedCarePlan,
timeOfLocalChange = carePlanUpdateTime,
Expand Down Expand Up @@ -318,7 +316,7 @@ class LocalChangeDaoTest {
activityFirstRep.detail.performer.add(Reference("Patient/$updatedPatientId"))
}
assertThat(carePlanLocalChange1.payload)
.isEqualTo(localChangeDao.iParser.encodeResourceToString(updatedReferencesCarePlan))
.isEqualTo(iParser.encodeResourceToString(updatedReferencesCarePlan))
val carePlanLocalChange1Id = carePlanLocalChange1.id
// assert that LocalChangeReferences are updated as well
val localChange1ResourceReferences =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ package com.google.android.fhir

import android.content.Context
import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.context.FhirVersionEnum
import ca.uhn.fhir.parser.IParser
import ca.uhn.fhir.util.FhirTerser
import com.google.android.fhir.db.Database
import com.google.android.fhir.db.impl.DatabaseConfig
Expand All @@ -38,7 +36,6 @@ import timber.log.Timber

internal data class FhirServices(
val fhirEngine: FhirEngine,
val parser: IParser,
val database: Database,
val remoteDataSource: DataSource? = null,
val fhirDataStore: FhirDataStore,
Expand Down Expand Up @@ -74,15 +71,13 @@ internal data class FhirServices(
}

fun build(): FhirServices {
val parser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser()
val terser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
val terser = FhirTerser(FhirContext.forR4Cached())
val searchParamMap =
searchParameters?.asMapOfResourceTypeToSearchParamDefinitions() ?: emptyMap()
val provider = SearchParamDefinitionsProviderImpl(searchParamMap)
val db =
DatabaseImpl(
context = context,
iParser = parser,
fhirTerser = terser,
DatabaseConfig(inMemory, enableEncryption, databaseErrorStrategy),
resourceIndexer = ResourceIndexer(provider),
Expand All @@ -100,7 +95,6 @@ internal data class FhirServices(
}
return FhirServices(
fhirEngine = engine,
parser = parser,
database = db,
remoteDataSource = remoteDataSource,
fhirDataStore = FhirDataStore(context),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import androidx.room.Room
import androidx.room.withTransaction
import androidx.sqlite.db.SimpleSQLiteQuery
import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.parser.IParser
import ca.uhn.fhir.util.FhirTerser
import com.google.android.fhir.DatabaseErrorStrategy
import com.google.android.fhir.LocalChange
Expand Down Expand Up @@ -56,7 +55,6 @@ import org.hl7.fhir.r4.model.ResourceType
@Suppress("UNCHECKED_CAST")
internal class DatabaseImpl(
private val context: Context,
private val iParser: IParser,
private val fhirTerser: FhirTerser,
databaseConfig: DatabaseConfig,
private val resourceIndexer: ResourceIndexer,
Expand Down Expand Up @@ -122,18 +120,9 @@ internal class DatabaseImpl(
.build()
}

private val resourceDao by lazy {
db.resourceDao().also {
it.iParser = iParser
it.resourceIndexer = resourceIndexer
}
}
private val resourceDao by lazy { db.resourceDao().also { it.resourceIndexer = resourceIndexer } }

private val localChangeDao =
db.localChangeDao().also {
it.iParser = iParser
it.fhirTerser = fhirTerser
}
private val localChangeDao = db.localChangeDao().also { it.fhirTerser = fhirTerser }

override suspend fun <R : Resource> insert(vararg resource: R): List<String> {
val logicalIds = mutableListOf<String>()
Expand Down Expand Up @@ -191,18 +180,21 @@ internal class DatabaseImpl(
db.withTransaction {
resourceDao.getResourceEntity(oldResourceId, resourceType)?.let { oldResourceEntity ->
val updatedResource =
(iParser.parseResource(oldResourceEntity.serializedResource) as Resource).apply {
idElement = IdType(newResourceId)
updateMeta(versionId, lastUpdatedRemote)
}
(FhirContext.forR4Cached()
.newJsonParser()
.parseResource(oldResourceEntity.serializedResource) as Resource)
.apply {
idElement = IdType(newResourceId)
updateMeta(versionId, lastUpdatedRemote)
}
updateResourceAndReferences(oldResourceId, updatedResource)
}
}
}

override suspend fun select(type: ResourceType, id: String): Resource {
return resourceDao.getResource(resourceId = id, resourceType = type)?.let {
iParser.parseResource(it) as Resource
FhirContext.forR4Cached().newJsonParser().parseResource(it) as Resource
}
?: throw ResourceNotFoundException(type.name, id)
}
Expand Down Expand Up @@ -317,7 +309,10 @@ internal class DatabaseImpl(
) {
db.withTransaction {
val currentResourceEntity = selectEntity(updatedResource.resourceType, currentResourceId)
val oldResource = iParser.parseResource(currentResourceEntity.serializedResource) as Resource
val oldResource =
FhirContext.forR4Cached()
.newJsonParser()
.parseResource(currentResourceEntity.serializedResource) as Resource
val resourceUuid = currentResourceEntity.resourceUuid
updateResourceEntity(resourceUuid, updatedResource)

Expand Down Expand Up @@ -375,6 +370,7 @@ internal class DatabaseImpl(
val updatedReferenceValue = "${updatedResource.resourceType.name}/${updatedResource.logicalId}"
referringResourcesUuids.forEach { resourceUuid ->
resourceDao.getResourceEntity(resourceUuid)?.let {
val iParser = FhirContext.forR4Cached().newJsonParser()
val referringResource = iParser.parseResource(it.serializedResource) as Resource
val updatedReferringResource =
addUpdatedReferenceToResource(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import androidx.room.Transaction
import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.parser.IParser
import ca.uhn.fhir.util.FhirTerser
import ca.uhn.fhir.util.ResourceReferenceInfo
Expand Down Expand Up @@ -50,8 +51,6 @@ import timber.log.Timber
*/
@Dao
internal abstract class LocalChangeDao {

lateinit var iParser: IParser
lateinit var fhirTerser: FhirTerser

@Insert(onConflict = OnConflictStrategy.REPLACE)
Expand All @@ -70,7 +69,7 @@ internal abstract class LocalChangeDao {
open suspend fun addInsert(resource: Resource, resourceUuid: UUID, timeOfLocalChange: Instant) {
val resourceId = resource.logicalId
val resourceType = resource.resourceType
val resourceString = iParser.encodeResourceToString(resource)
val resourceString = FhirContext.forR4Cached().newJsonParser().encodeResourceToString(resource)

val localChangeEntity =
LocalChangeEntity(
Expand Down Expand Up @@ -128,6 +127,7 @@ internal abstract class LocalChangeDao {
"Unexpected DELETE when updating $resourceType/$resourceId. UPDATE failed.",
)
}
val iParser = FhirContext.forR4Cached().newJsonParser()
val oldResource = iParser.parseResource(oldEntity.serializedResource) as Resource
val jsonDiff = diff(iParser, oldResource, updatedResource)
if (jsonDiff.length() == 0) {
Expand Down Expand Up @@ -475,6 +475,7 @@ internal abstract class LocalChangeDao {
oldReference: String,
updatedReference: String,
): LocalChangeEntity {
val iParser = FhirContext.forR4Cached().newJsonParser()
return when (localChange.type) {
LocalChangeEntity.Type.INSERT -> {
val insertResourcePayload = iParser.parseResource(localChange.payload) as Resource
Expand Down
Loading
Loading