Skip to content

Commit 17029eb

Browse files
ndegwamartinndegwamartin
authored
Refactor usages of Json Parser for thread safety (#2749)
* Refactor usages of Json Parser * Fix failing tests ✅ * Update JsonParser usages for Dao classes Spotless clean * Fix failing unit test ✅
1 parent f42e804 commit 17029eb

File tree

11 files changed

+62
-76
lines changed

11 files changed

+62
-76
lines changed

‎engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ class DatabaseImplTest {
118118
@JvmField @Parameterized.Parameter(0) var encrypted: Boolean = false
119119

120120
private val context: Context = ApplicationProvider.getApplicationContext()
121+
private val parser = FhirContext.forR4Cached().newJsonParser()
121122
private lateinit var services: FhirServices
122123
private lateinit var database: Database
123124

@@ -202,7 +203,7 @@ class DatabaseImplTest {
202203
fun getLocalChanges_withSingleLocaleChange_shouldReturnSingleLocalChanges() = runBlocking {
203204
val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json")
204205
database.insert(patient)
205-
val patientString = services.parser.encodeResourceToString(patient)
206+
val patientString = parser.encodeResourceToString(patient)
206207
val resourceLocalChanges = database.getLocalChanges(patient.resourceType, patient.logicalId)
207208
assertThat(resourceLocalChanges.size).isEqualTo(1)
208209
with(resourceLocalChanges[0]) {
@@ -269,7 +270,7 @@ class DatabaseImplTest {
269270
fun clearDatabase_shouldClearAllTablesData() = runBlocking {
270271
val patient: Patient = readFromFile(Patient::class.java, "/date_test_patient.json")
271272
database.insert(patient)
272-
val patientString = services.parser.encodeResourceToString(patient)
273+
val patientString = parser.encodeResourceToString(patient)
273274
val resourceLocalChanges = database.getLocalChanges(patient.resourceType, patient.logicalId)
274275
assertThat(resourceLocalChanges.size).isEqualTo(1)
275276
with(resourceLocalChanges[0]) {
@@ -393,7 +394,7 @@ class DatabaseImplTest {
393394

394395
@Test
395396
fun insert_shouldAddInsertLocalChange() = runBlocking {
396-
val testPatient2String = services.parser.encodeResourceToString(TEST_PATIENT_2)
397+
val testPatient2String = parser.encodeResourceToString(TEST_PATIENT_2)
397398
database.insert(TEST_PATIENT_2)
398399
val resourceLocalChanges =
399400
database.getAllLocalChanges().filter { it.resourceId.equals(TEST_PATIENT_2_ID) }
@@ -481,7 +482,7 @@ class DatabaseImplTest {
481482
database.insert(patient)
482483
patient = readFromFile(Patient::class.java, "/update_test_patient_1.json")
483484
database.update(patient)
484-
services.parser.encodeResourceToString(patient)
485+
parser.encodeResourceToString(patient)
485486
val localChangeTokenIds =
486487
database
487488
.getAllLocalChanges()
@@ -4101,7 +4102,7 @@ class DatabaseImplTest {
41014102
val observationLocalChange = updatedObservationLocalChanges[0]
41024103
assertThat(observationLocalChange.type).isEqualTo(LocalChange.Type.INSERT)
41034104
val observationLocalChangePayload =
4104-
services.parser.parseResource(observationLocalChange.payload) as Observation
4105+
parser.parseResource(observationLocalChange.payload) as Observation
41054106
assertThat(observationLocalChangePayload.subject.reference)
41064107
.isEqualTo("Patient/$remotelyCreatedPatientResourceId")
41074108
}

‎engine/src/androidTest/java/com/google/android/fhir/db/impl/EncryptedDatabaseErrorTest.kt‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2023 Google LLC
2+
* Copyright 2023-2024 Google LLC
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -22,7 +22,6 @@ import androidx.test.core.app.ApplicationProvider
2222
import androidx.test.ext.junit.runners.AndroidJUnit4
2323
import androidx.test.filters.MediumTest
2424
import ca.uhn.fhir.context.FhirContext
25-
import ca.uhn.fhir.context.FhirVersionEnum
2625
import ca.uhn.fhir.util.FhirTerser
2726
import com.google.android.fhir.DatabaseErrorStrategy.RECREATE_AT_OPEN
2827
import com.google.android.fhir.DatabaseErrorStrategy.UNSPECIFIED
@@ -49,8 +48,7 @@ import org.junit.runner.RunWith
4948
@RunWith(AndroidJUnit4::class)
5049
class EncryptedDatabaseErrorTest {
5150
private val context: Context = ApplicationProvider.getApplicationContext()
52-
private val parser = FhirContext.forR4().newJsonParser()
53-
private val terser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
51+
private val terser = FhirTerser(FhirContext.forR4Cached())
5452
private val resourceIndexer = ResourceIndexer(SearchParamDefinitionsProviderImpl())
5553

5654
@After
@@ -66,7 +64,6 @@ class EncryptedDatabaseErrorTest {
6664
// GIVEN an unencrypted database.
6765
DatabaseImpl(
6866
context,
69-
parser,
7067
terser,
7168
DatabaseConfig(
7269
inMemory = false,
@@ -84,7 +81,6 @@ class EncryptedDatabaseErrorTest {
8481
// THEN it should throw SQLiteException
8582
DatabaseImpl(
8683
context,
87-
parser,
8884
terser,
8985
DatabaseConfig(
9086
inMemory = false,
@@ -115,7 +111,6 @@ class EncryptedDatabaseErrorTest {
115111
// GIVEN an unencrypted database.
116112
DatabaseImpl(
117113
context,
118-
parser,
119114
terser,
120115
DatabaseConfig(
121116
inMemory = false,
@@ -139,7 +134,6 @@ class EncryptedDatabaseErrorTest {
139134
// THEN it should throw SQLiteException
140135
DatabaseImpl(
141136
context,
142-
parser,
143137
terser,
144138
DatabaseConfig(
145139
inMemory = false,
@@ -169,7 +163,6 @@ class EncryptedDatabaseErrorTest {
169163
// GIVEN an unencrypted database.
170164
DatabaseImpl(
171165
context,
172-
parser,
173166
terser,
174167
DatabaseConfig(
175168
inMemory = false,
@@ -193,7 +186,6 @@ class EncryptedDatabaseErrorTest {
193186
// THEN it should recreate the database
194187
DatabaseImpl(
195188
context,
196-
parser,
197189
terser,
198190
DatabaseConfig(
199191
inMemory = false,
@@ -226,7 +218,6 @@ class EncryptedDatabaseErrorTest {
226218
// GIVEN an encrypted database.
227219
DatabaseImpl(
228220
context,
229-
parser,
230221
terser,
231222
DatabaseConfig(
232223
inMemory = false,
@@ -244,7 +235,6 @@ class EncryptedDatabaseErrorTest {
244235
// THEN it should recreate database.
245236
DatabaseImpl(
246237
context,
247-
parser,
248238
terser,
249239
DatabaseConfig(
250240
inMemory = false,

‎engine/src/androidTest/java/com/google/android/fhir/db/impl/dao/LocalChangeDaoTest.kt‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ import org.junit.runner.RunWith
4949
class LocalChangeDaoTest {
5050
private lateinit var database: ResourceDatabase
5151
private lateinit var localChangeDao: LocalChangeDao
52+
private val iParser = FhirContext.forR4Cached().newJsonParser()
5253

5354
@Before
5455
fun setupDatabase() {
@@ -62,7 +63,6 @@ class LocalChangeDaoTest {
6263

6364
localChangeDao =
6465
database.localChangeDao().also {
65-
it.iParser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser()
6666
it.fhirTerser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
6767
}
6868
}
@@ -97,8 +97,7 @@ class LocalChangeDaoTest {
9797
assertThat(carePlanLocalChange1.resourceUuid).isEqualTo(carePlanResourceUuid)
9898
assertThat(carePlanLocalChange1.resourceId).isEqualTo(carePlan.id)
9999
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
100-
assertThat(carePlanLocalChange1.payload)
101-
.isEqualTo(localChangeDao.iParser.encodeResourceToString(carePlan))
100+
assertThat(carePlanLocalChange1.payload).isEqualTo(iParser.encodeResourceToString(carePlan))
102101
val carePlanLocalChange1Id = carePlanLocalChange1.id
103102

104103
val localChangeResourceReferences =
@@ -150,7 +149,7 @@ class LocalChangeDaoTest {
150149
resourceId = originalCarePlan.logicalId,
151150
resourceType = originalCarePlan.resourceType,
152151
resourceUuid = carePlanResourceUuid,
153-
serializedResource = localChangeDao.iParser.encodeResourceToString(originalCarePlan),
152+
serializedResource = iParser.encodeResourceToString(originalCarePlan),
154153
),
155154
updatedResource = modifiedCarePlan,
156155
timeOfLocalChange = carePlanUpdateTime,
@@ -163,7 +162,7 @@ class LocalChangeDaoTest {
163162
assertThat(carePlanLocalChange1.resourceId).isEqualTo(originalCarePlan.id)
164163
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
165164
assertThat(carePlanLocalChange1.payload)
166-
.isEqualTo(localChangeDao.iParser.encodeResourceToString(originalCarePlan))
165+
.isEqualTo(iParser.encodeResourceToString(originalCarePlan))
167166

168167
val carePlanLocalChange2 = carePlanLocalChanges[1]
169168
assertThat(carePlanLocalChange2.resourceUuid).isEqualTo(carePlanResourceUuid)
@@ -224,8 +223,7 @@ class LocalChangeDaoTest {
224223
assertThat(carePlanLocalChange1.resourceUuid).isEqualTo(carePlanResourceUuid)
225224
assertThat(carePlanLocalChange1.resourceId).isEqualTo(carePlan.id)
226225
assertThat(carePlanLocalChange1.type).isEqualTo(LocalChangeEntity.Type.INSERT)
227-
assertThat(carePlanLocalChange1.payload)
228-
.isEqualTo(localChangeDao.iParser.encodeResourceToString(carePlan))
226+
assertThat(carePlanLocalChange1.payload).isEqualTo(iParser.encodeResourceToString(carePlan))
229227

230228
val carePlanLocalChange2 = carePlanLocalChanges[1]
231229
assertThat(carePlanLocalChange2.resourceUuid).isEqualTo(carePlanResourceUuid)
@@ -285,7 +283,7 @@ class LocalChangeDaoTest {
285283
resourceId = originalCarePlan.logicalId,
286284
resourceType = originalCarePlan.resourceType,
287285
resourceUuid = carePlanResourceUuid,
288-
serializedResource = localChangeDao.iParser.encodeResourceToString(originalCarePlan),
286+
serializedResource = iParser.encodeResourceToString(originalCarePlan),
289287
),
290288
updatedResource = modifiedCarePlan,
291289
timeOfLocalChange = carePlanUpdateTime,
@@ -318,7 +316,7 @@ class LocalChangeDaoTest {
318316
activityFirstRep.detail.performer.add(Reference("Patient/$updatedPatientId"))
319317
}
320318
assertThat(carePlanLocalChange1.payload)
321-
.isEqualTo(localChangeDao.iParser.encodeResourceToString(updatedReferencesCarePlan))
319+
.isEqualTo(iParser.encodeResourceToString(updatedReferencesCarePlan))
322320
val carePlanLocalChange1Id = carePlanLocalChange1.id
323321
// assert that LocalChangeReferences are updated as well
324322
val localChange1ResourceReferences =

‎engine/src/main/java/com/google/android/fhir/FhirServices.kt‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@ package com.google.android.fhir
1818

1919
import android.content.Context
2020
import ca.uhn.fhir.context.FhirContext
21-
import ca.uhn.fhir.context.FhirVersionEnum
22-
import ca.uhn.fhir.parser.IParser
2321
import ca.uhn.fhir.util.FhirTerser
2422
import com.google.android.fhir.db.Database
2523
import com.google.android.fhir.db.impl.DatabaseConfig
@@ -38,7 +36,6 @@ import timber.log.Timber
3836

3937
internal data class FhirServices(
4038
val fhirEngine: FhirEngine,
41-
val parser: IParser,
4239
val database: Database,
4340
val remoteDataSource: DataSource? = null,
4441
val fhirDataStore: FhirDataStore,
@@ -74,15 +71,13 @@ internal data class FhirServices(
7471
}
7572

7673
fun build(): FhirServices {
77-
val parser = FhirContext.forCached(FhirVersionEnum.R4).newJsonParser()
78-
val terser = FhirTerser(FhirContext.forCached(FhirVersionEnum.R4))
74+
val terser = FhirTerser(FhirContext.forR4Cached())
7975
val searchParamMap =
8076
searchParameters?.asMapOfResourceTypeToSearchParamDefinitions() ?: emptyMap()
8177
val provider = SearchParamDefinitionsProviderImpl(searchParamMap)
8278
val db =
8379
DatabaseImpl(
8480
context = context,
85-
iParser = parser,
8681
fhirTerser = terser,
8782
DatabaseConfig(inMemory, enableEncryption, databaseErrorStrategy),
8883
resourceIndexer = ResourceIndexer(provider),
@@ -100,7 +95,6 @@ internal data class FhirServices(
10095
}
10196
return FhirServices(
10297
fhirEngine = engine,
103-
parser = parser,
10498
database = db,
10599
remoteDataSource = remoteDataSource,
106100
fhirDataStore = FhirDataStore(context),

‎engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import androidx.room.Room
2222
import androidx.room.withTransaction
2323
import androidx.sqlite.db.SimpleSQLiteQuery
2424
import ca.uhn.fhir.context.FhirContext
25-
import ca.uhn.fhir.parser.IParser
2625
import ca.uhn.fhir.util.FhirTerser
2726
import com.google.android.fhir.DatabaseErrorStrategy
2827
import com.google.android.fhir.LocalChange
@@ -56,7 +55,6 @@ import org.hl7.fhir.r4.model.ResourceType
5655
@Suppress("UNCHECKED_CAST")
5756
internal class DatabaseImpl(
5857
private val context: Context,
59-
private val iParser: IParser,
6058
private val fhirTerser: FhirTerser,
6159
databaseConfig: DatabaseConfig,
6260
private val resourceIndexer: ResourceIndexer,
@@ -122,18 +120,9 @@ internal class DatabaseImpl(
122120
.build()
123121
}
124122

125-
private val resourceDao by lazy {
126-
db.resourceDao().also {
127-
it.iParser = iParser
128-
it.resourceIndexer = resourceIndexer
129-
}
130-
}
123+
private val resourceDao by lazy { db.resourceDao().also { it.resourceIndexer = resourceIndexer } }
131124

132-
private val localChangeDao =
133-
db.localChangeDao().also {
134-
it.iParser = iParser
135-
it.fhirTerser = fhirTerser
136-
}
125+
private val localChangeDao = db.localChangeDao().also { it.fhirTerser = fhirTerser }
137126

138127
override suspend fun <R : Resource> insert(vararg resource: R): List<String> {
139128
val logicalIds = mutableListOf<String>()
@@ -191,18 +180,21 @@ internal class DatabaseImpl(
191180
db.withTransaction {
192181
resourceDao.getResourceEntity(oldResourceId, resourceType)?.let { oldResourceEntity ->
193182
val updatedResource =
194-
(iParser.parseResource(oldResourceEntity.serializedResource) as Resource).apply {
195-
idElement = IdType(newResourceId)
196-
updateMeta(versionId, lastUpdatedRemote)
197-
}
183+
(FhirContext.forR4Cached()
184+
.newJsonParser()
185+
.parseResource(oldResourceEntity.serializedResource) as Resource)
186+
.apply {
187+
idElement = IdType(newResourceId)
188+
updateMeta(versionId, lastUpdatedRemote)
189+
}
198190
updateResourceAndReferences(oldResourceId, updatedResource)
199191
}
200192
}
201193
}
202194

203195
override suspend fun select(type: ResourceType, id: String): Resource {
204196
return resourceDao.getResource(resourceId = id, resourceType = type)?.let {
205-
iParser.parseResource(it) as Resource
197+
FhirContext.forR4Cached().newJsonParser().parseResource(it) as Resource
206198
}
207199
?: throw ResourceNotFoundException(type.name, id)
208200
}
@@ -317,7 +309,10 @@ internal class DatabaseImpl(
317309
) {
318310
db.withTransaction {
319311
val currentResourceEntity = selectEntity(updatedResource.resourceType, currentResourceId)
320-
val oldResource = iParser.parseResource(currentResourceEntity.serializedResource) as Resource
312+
val oldResource =
313+
FhirContext.forR4Cached()
314+
.newJsonParser()
315+
.parseResource(currentResourceEntity.serializedResource) as Resource
321316
val resourceUuid = currentResourceEntity.resourceUuid
322317
updateResourceEntity(resourceUuid, updatedResource)
323318

@@ -375,6 +370,7 @@ internal class DatabaseImpl(
375370
val updatedReferenceValue = "${updatedResource.resourceType.name}/${updatedResource.logicalId}"
376371
referringResourcesUuids.forEach { resourceUuid ->
377372
resourceDao.getResourceEntity(resourceUuid)?.let {
373+
val iParser = FhirContext.forR4Cached().newJsonParser()
378374
val referringResource = iParser.parseResource(it.serializedResource) as Resource
379375
val updatedReferringResource =
380376
addUpdatedReferenceToResource(

‎engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import androidx.room.Insert
2121
import androidx.room.OnConflictStrategy
2222
import androidx.room.Query
2323
import androidx.room.Transaction
24+
import ca.uhn.fhir.context.FhirContext
2425
import ca.uhn.fhir.parser.IParser
2526
import ca.uhn.fhir.util.FhirTerser
2627
import ca.uhn.fhir.util.ResourceReferenceInfo
@@ -50,8 +51,6 @@ import timber.log.Timber
5051
*/
5152
@Dao
5253
internal abstract class LocalChangeDao {
53-
54-
lateinit var iParser: IParser
5554
lateinit var fhirTerser: FhirTerser
5655

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

7574
val localChangeEntity =
7675
LocalChangeEntity(
@@ -128,6 +127,7 @@ internal abstract class LocalChangeDao {
128127
"Unexpected DELETE when updating $resourceType/$resourceId. UPDATE failed.",
129128
)
130129
}
130+
val iParser = FhirContext.forR4Cached().newJsonParser()
131131
val oldResource = iParser.parseResource(oldEntity.serializedResource) as Resource
132132
val jsonDiff = diff(iParser, oldResource, updatedResource)
133133
if (jsonDiff.length() == 0) {
@@ -475,6 +475,7 @@ internal abstract class LocalChangeDao {
475475
oldReference: String,
476476
updatedReference: String,
477477
): LocalChangeEntity {
478+
val iParser = FhirContext.forR4Cached().newJsonParser()
478479
return when (localChange.type) {
479480
LocalChangeEntity.Type.INSERT -> {
480481
val insertResourcePayload = iParser.parseResource(localChange.payload) as Resource

0 commit comments

Comments
 (0)