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

Split and limit variables to 999 when using 'IN(...)' #2562

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.google.android.fhir.SearchParamName
import com.google.android.fhir.SearchResult
import com.google.android.fhir.db.Database
import com.google.android.fhir.db.ResourceNotFoundException
import com.google.android.fhir.db.impl.dao.LocalChangeDao
import com.google.android.fhir.logicalId
import com.google.android.fhir.search.LOCAL_LAST_UPDATED_PARAM
import com.google.android.fhir.search.Operation
Expand Down Expand Up @@ -83,6 +84,7 @@ import org.hl7.fhir.r4.model.ResourceType
import org.hl7.fhir.r4.model.RiskAssessment
import org.hl7.fhir.r4.model.SearchParameter
import org.hl7.fhir.r4.model.StringType
import org.hl7.fhir.r4.model.Task
import org.json.JSONArray
import org.json.JSONObject
import org.junit.After
Expand Down Expand Up @@ -4086,6 +4088,31 @@ class DatabaseImplTest {
assertThat(searchedObservations[0].logicalId).isEqualTo(locallyCreatedObservationResourceId)
}

@Test
LZRS marked this conversation as resolved.
Show resolved Hide resolved
fun getLocalChangeResourceReferences_shouldSafelyReturnReferencesAboveSQLiteInOpLimit() =
runBlocking {
val patientsCount = LocalChangeDao.SQLITE_LIMIT_MAX_VARIABLE_NUMBER * 7
val locallyCreatedPatients =
(1..patientsCount).map {
Patient().apply {
id = "local-patient-id$it"
name = listOf(HumanName().setFamily("Family").setGiven(listOf(StringType("$it"))))
}
}
database.insert(*locallyCreatedPatients.toTypedArray())
val locallyCreatedPatientTasks =
locallyCreatedPatients.mapIndexed { index, patient ->
Task().apply {
`for` = Reference("Patient/${patient.logicalId}")
id = "local-observation-$index"
}
}
database.insert(*locallyCreatedPatientTasks.toTypedArray())
val localChangeIds = database.getAllLocalChanges().flatMap { it.token.ids }
val localChangeResourceReferences = database.getLocalChangeResourceReferences(localChangeIds)
assertThat(localChangeResourceReferences.size).isEqualTo(locallyCreatedPatients.size)
}

private companion object {
const val mockEpochTimeStamp = 1628516301000
const val TEST_PATIENT_1_ID = "test_patient_1"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import org.hl7.fhir.r4.model.Enumerations
import org.hl7.fhir.r4.model.Observation
import org.hl7.fhir.r4.model.Patient
import org.hl7.fhir.r4.model.Reference
import org.hl7.fhir.r4.model.Task
import org.junit.After
import org.junit.Before
import org.junit.Test
Expand Down Expand Up @@ -358,6 +359,38 @@ class LocalChangeDaoTest {
.isEqualTo(practitionerReference)
}

@Test
LZRS marked this conversation as resolved.
Show resolved Hide resolved
fun updateResourceIdAndReferences_shouldSafelyUpdateLocalChangesReferencesAboveSQLiteInOpLimit() =
runBlocking {
val localPatientId = "local-patient-id"
val patientResourceUuid = UUID.randomUUID()
val localPatient = Patient().apply { id = localPatientId }
val patientCreationTime = Instant.now()
localChangeDao.addInsert(localPatient, patientResourceUuid, patientCreationTime)

val countAboveLimit = LocalChangeDao.SQLITE_LIMIT_MAX_VARIABLE_NUMBER * 10
(1..countAboveLimit).forEach {
val taskResourceUuid = UUID.randomUUID()
val task =
Task().apply {
id = "local-task-$it"
`for` = Reference("Patient/$localPatientId")
}
val taskCreationTime = Instant.now()
localChangeDao.addInsert(task, taskResourceUuid, taskCreationTime)
}

val updatedPatientId = "synced-patient-id"
val updatedLocalPatient = localPatient.copy().apply { id = updatedPatientId }
val updatedReferences =
localChangeDao.updateResourceIdAndReferences(
patientResourceUuid,
oldResource = localPatient,
updatedResource = updatedLocalPatient,
)
assertThat(updatedReferences.size).isEqualTo(countAboveLimit)
}

@Test
fun getReferencesForLocalChanges_should_return_all_changes(): Unit = runBlocking {
listOf(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import com.google.android.fhir.db.ResourceNotFoundException
import com.google.android.fhir.db.ResourceWithUUID
import com.google.android.fhir.db.impl.DatabaseImpl.Companion.UNENCRYPTED_DATABASE_NAME
import com.google.android.fhir.db.impl.dao.ForwardIncludeSearchResult
import com.google.android.fhir.db.impl.dao.LocalChangeDao.Companion.SQLITE_LIMIT_MAX_VARIABLE_NUMBER
import com.google.android.fhir.db.impl.dao.ReverseIncludeSearchResult
import com.google.android.fhir.db.impl.entities.ResourceEntity
import com.google.android.fhir.index.ResourceIndexer
Expand Down Expand Up @@ -408,12 +409,14 @@ internal class DatabaseImpl(
override suspend fun getLocalChangeResourceReferences(
localChangeIds: List<Long>,
): List<LocalChangeResourceReference> {
return localChangeDao.getReferencesForLocalChanges(localChangeIds).map {
LocalChangeResourceReference(
it.localChangeId,
it.resourceReferenceValue,
it.resourceReferencePath,
)
return localChangeIds.chunked(SQLITE_LIMIT_MAX_VARIABLE_NUMBER).flatMap { chunk ->
localChangeDao.getReferencesForLocalChanges(chunk).map {
LocalChangeResourceReference(
it.localChangeId,
it.resourceReferenceValue,
it.resourceReferencePath,
)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,10 @@ internal abstract class LocalChangeDao {
val updatedReferenceValue = "${updatedResource.resourceType.name}/${updatedResource.logicalId}"
val referringLocalChangeIds =
getLocalChangeReferencesWithValue(oldReferenceValue).map { it.localChangeId }.distinct()
val referringLocalChanges = getLocalChanges(referringLocalChangeIds)
val referringLocalChanges =
referringLocalChangeIds.chunked(SQLITE_LIMIT_MAX_VARIABLE_NUMBER).flatMap {
getLocalChanges(it)
}

referringLocalChanges.forEach { existingLocalChangeEntity ->
val updatedLocalChangeEntity =
Expand Down Expand Up @@ -498,6 +501,12 @@ internal abstract class LocalChangeDao {

companion object {
const val DEFAULT_ID_VALUE = 0L

/**
* Represents SQLite limit on the size of parameters that can be passed in an IN(..) query See
* https://issuetracker.google.com/issues/192284727 See https://www.sqlite.org/limits.html
*/
LZRS marked this conversation as resolved.
Show resolved Hide resolved
const val SQLITE_LIMIT_MAX_VARIABLE_NUMBER = 999
}
}

Expand Down
Loading