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

Modify toQueryString to prevent SQLite expression tree from exceeding depth of 1000 #2565

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
6b3d4c4
Modify toQueryString to chunk large list of ConditionParam
LZRS Jun 8, 2024
8c720e2
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Jun 21, 2024
c73037d
Add test for condition params chunking and wrapping in brackets
LZRS Jun 21, 2024
0264816
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Jun 24, 2024
6a45013
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Jun 26, 2024
d764115
Add support for chunkSize param in SearchDsl filters
LZRS Jun 26, 2024
d509f53
Update workflow engine dependency to use latest
LZRS Jun 27, 2024
ab9a4a6
Merge branch 'master' into 2561-fix-sqlite-crash
LZRS Jul 1, 2024
432f7af
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Aug 22, 2024
b5e67df
Refactor remove `chunkSize` parameter
LZRS Aug 22, 2024
b9708a6
Recursively bifurcate expression tree to reduce depth
LZRS Aug 22, 2024
821feab
Revert touched files not relevant for the PR
LZRS Aug 22, 2024
a304c19
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Sep 11, 2024
112344f
Refactor toQueryString
LZRS Sep 11, 2024
ed8d74b
Update tests to include base cases for toQueryString
LZRS Oct 8, 2024
999fe93
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Oct 8, 2024
5a96ef8
Refactor and update related test cases
LZRS Oct 9, 2024
7aacdde
Merge remote-tracking branch 'upstream/master' into 2561-fix-sqlite-c…
LZRS Oct 9, 2024
d83e139
Merge branch 'master' into 2561-fix-sqlite-crash
LZRS Oct 15, 2024
e7dde67
Refactor test to make filter strict
LZRS Oct 16, 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
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import com.google.android.fhir.search.Order
import com.google.android.fhir.search.Search
import com.google.android.fhir.search.StringFilterModifier
import com.google.android.fhir.search.execute
import com.google.android.fhir.search.filter.ReferenceParamFilterCriterion
import com.google.android.fhir.search.getQuery
import com.google.android.fhir.search.has
import com.google.android.fhir.search.include
Expand Down Expand Up @@ -5169,6 +5170,29 @@ class DatabaseImplTest {
assertThat(localChangeResourceReferences.size).isEqualTo(locallyCreatedPatients.size)
}

@Test
fun searchTasksForManyPatientsReturnCorrectly() = runBlocking {
val patients = (0..5001).map { Patient().apply { id = "task-patient-index-$it" } }
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are 5002 patients and 5001 tasks. Is this test case deliberately checking this? I feel like if you really want to test the filter clause, it's better to query tasks linked to patient 0-4999 so at least you're filtering out 1 patient to know that the filter clause works.

database.insert(*patients.toTypedArray())
val tasks =
(0..5000).map {
Task().apply {
id = "patient-$it-task"
`for` = Reference(patients[it])
}
}
database.insert(*tasks.toTypedArray())

val patientsSearchIdList =
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can also limit this list to 2000 or 3000.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can also limit this list to 2000 or 3000.

I couldn't set limit the test to either 3000 or 2000, ran to another sqlite error on android

android.database.sqlite.SQLiteException: too many SQL variables (code 1)

It's related to https://issuetracker.google.com/issues/73634057

For now, I just used a limit of 990

Copy link
Collaborator

Choose a reason for hiding this comment

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

but 5000 worked? this is odd...

can you please do some more testing to see what's goign on here?

this is not blocking the pr though.

Copy link
Collaborator Author

@LZRS LZRS Oct 16, 2024

Choose a reason for hiding this comment

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

Initially the way they'd passed for the 5000, I had generated the bifurcated queries and ran them in separate sqlite db browser. That might be the difference, assuming the sqlite version for Android may be limited

patients.map<Patient, ReferenceParamFilterCriterion.() -> Unit> { { value = it.logicalId } }
val searchQuery =
Search(ResourceType.Task)
.apply { filter(Task.SUBJECT, *patientsSearchIdList.toTypedArray()) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

this filter is checking that tasks are referencing patients 0-990 (inclusive) so that's 991 patients.

but you only have tasks for patients 0-989 (990 in total).

so the filter criteria is not really being tests because the filter criteria is broader than what you have in the database.

if you want to test it, make the filter more strict. for example, create N patients, and in the filter list N-1 IDs. and check that you only get N-1 results.

.getQuery()
val searchResults = database.search<Task>(searchQuery)
assertThat(searchResults.size).isEqualTo(5001)
}

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 @@ -685,6 +685,8 @@ internal val DateTimeType.rangeEpochMillis

data class ConditionParam<T>(val condition: String, val params: List<T>) {
constructor(condition: String, vararg params: T) : this(condition, params.asList())

val queryString = if (params.size > 1) "($condition)" else condition
}

private enum class SortTableInfo(val tableName: String, val columnName: String) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,28 +86,19 @@ internal sealed class FilterCriteria(
* intended.
*/
private fun List<ConditionParam<*>>.toQueryString(operation: Operation): String {
if (this.size <= 1) {
return map {
if (it.params.size > 1) {
"(${it.condition})"
} else {
it.condition
}
}
.firstOrNull()
?: ""
}

val mid = this.size / 2
val left = this.subList(0, mid).toQueryString(operation)
val right = this.subList(mid, this.size).toQueryString(operation)
when {
this.isEmpty() ->
return "true" // In case empty, return true to match other conditions in the where clause
LZRS marked this conversation as resolved.
Show resolved Hide resolved
this.size == 1 -> {
return first().queryString
}
else -> {
val mid = this.size / 2
val left = this.subList(0, mid).toQueryString(operation)
val right = this.subList(mid, this.size).toQueryString(operation)

return listOf(left, right)
.filter { it.isNotBlank() }
.joinToString(
separator = " ${operation.logicalOperator} ",
prefix = "(",
postfix = ")",
)
return "($left ${operation.logicalOperator} $right)"
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.android.fhir.impl

import androidx.test.core.app.ApplicationProvider
import ca.uhn.fhir.rest.gclient.TokenClientParam
import ca.uhn.fhir.rest.param.ParamPrefixEnum
import com.google.android.fhir.FhirServices.Companion.builder
import com.google.android.fhir.LocalChange
Expand All @@ -26,6 +27,8 @@ import com.google.android.fhir.get
import com.google.android.fhir.lastUpdated
import com.google.android.fhir.logicalId
import com.google.android.fhir.search.LOCAL_LAST_UPDATED_PARAM
import com.google.android.fhir.search.filter.TokenParamFilterCriterion
import com.google.android.fhir.search.getQuery
import com.google.android.fhir.search.search
import com.google.android.fhir.sync.AcceptLocalConflictResolver
import com.google.android.fhir.sync.AcceptRemoteConflictResolver
Expand Down Expand Up @@ -723,6 +726,47 @@ class FhirEngineImplTest {
assertThat(result.map { it.resource.logicalId }).containsExactly("patient-id-create").inOrder()
}

@Test
fun `testing search many`() = runTest {
val patient1 = Patient().apply { id = "patient-1" }
val patient2 = Patient().apply { id = "patient-2" }
val patient3 = Patient().apply { id = "patient-45" }
val patient4 = Patient().apply { id = "patient-4355" }
val patient5 = Patient().apply { id = "patient-899" }
val patient6 = Patient().apply { id = "patient-883376" }
fhirEngine.create(patient1, patient2, patient3, patient4, patient5, patient6)

val result1 =
fhirEngine.search<Patient> {
filter(TokenClientParam("_id"), { value = of(patient3.logicalId) })
}
assertThat(result1).isNotEmpty()
assertThat(result1.size).isEqualTo(1)
LZRS marked this conversation as resolved.
Show resolved Hide resolved

val filterValues =
listOf(patient2, patient3, patient1, patient5, patient4, patient6).map<
Patient,
TokenParamFilterCriterion.() -> Unit,
> {
{ value = of(it.logicalId) }
}
val result2 =
fhirEngine.search<Patient> {
filter(TokenClientParam("_id"), *filterValues.toTypedArray())
println(getQuery().query)
println(getQuery().args)
LZRS marked this conversation as resolved.
Show resolved Hide resolved
}
assertThat(result2.map { it.resource.logicalId })
.containsExactly(
"patient-2",
"patient-45",
"patient-1",
"patient-4355",
"patient-899",
"patient-883376",
)
}

@Test
fun `update should allow patient search with LOCAL_LAST_UPDATED_PARAM and update local entity`() =
runBlocking {
Expand Down
215 changes: 215 additions & 0 deletions engine/src/test/java/com/google/android/fhir/search/SearchTest.kt
LZRS marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2791,6 +2791,221 @@ class SearchTest {
.inOrder()
}

@Test
fun `search CarePlan filter with no reference`() {
val query = Search(ResourceType.CarePlan).apply { filter(CarePlan.SUBJECT) }.getQuery()

val queryString = query.query
assertThat(queryString)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND true
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
)
.inOrder()
}

@Test
fun `search CarePlan filter with one patient reference`() {
val query =
Search(ResourceType.CarePlan)
.apply { filter(CarePlan.SUBJECT, { value = "Patient/patient-0" }) }
.getQuery()

val queryString = query.query
assertThat(queryString)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND index_value = ?
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
"Patient/patient-0",
)
.inOrder()
}

@Test
fun `search CarePlan filter with two patient references`() {
val query =
Search(ResourceType.CarePlan)
.apply {
filter(CarePlan.SUBJECT, { value = "Patient/patient-0" }, { value = "Patient/patient-1" })
}
.getQuery()

val queryString = query.query
assertThat(queryString)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND (index_value = ? OR index_value = ?)
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
"Patient/patient-0",
"Patient/patient-1",
)
.inOrder()
}

@Test
fun `search CarePlan filter with three patient references`() {
val query =
Search(ResourceType.CarePlan)
.apply {
filter(
CarePlan.SUBJECT,
{ value = "Patient/patient-0" },
{ value = "Patient/patient-1" },
{ value = "Patient/patient-4" },
)
}
.getQuery()

val queryString = query.query
assertThat(queryString)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND (index_value = ? OR (index_value = ? OR index_value = ?))
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
"Patient/patient-0",
"Patient/patient-1",
"Patient/patient-4",
)
.inOrder()
}

@Test
fun `search CarePlan filter with four patient references`() {
val query =
Search(ResourceType.CarePlan)
.apply {
filter(
CarePlan.SUBJECT,
{ value = "Patient/patient-0" },
{ value = "Patient/patient-1" },
{ value = "Patient/patient-4" },
{ value = "Patient/patient-7" },
)
}
.getQuery()

val queryString = query.query
assertThat(queryString)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND ((index_value = ? OR index_value = ?) OR (index_value = ? OR index_value = ?))
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
"Patient/patient-0",
"Patient/patient-1",
"Patient/patient-4",
"Patient/patient-7",
)
.inOrder()
}

@Test
fun `search CarePlan filter with 8 patient references`() {
val patientIdReferenceList = (0..7).map { "Patient/patient-$it" }
val patientIdList =
patientIdReferenceList.map<String, ReferenceParamFilterCriterion.() -> Unit> {
{ value = it }
}
val query =
Search(ResourceType.CarePlan)
.apply { filter(CarePlan.SUBJECT, *patientIdList.toTypedArray()) }
.getQuery()

assertThat(query.query)
.isEqualTo(
"""
SELECT a.resourceUuid, a.serializedResource
FROM ResourceEntity a
WHERE a.resourceType = ?
AND a.resourceUuid IN (
SELECT resourceUuid FROM ReferenceIndexEntity
WHERE resourceType = ? AND index_name = ? AND (((index_value = ? OR index_value = ?) OR (index_value = ? OR index_value = ?)) OR ((index_value = ? OR index_value = ?) OR (index_value = ? OR index_value = ?)))
)
"""
.trimIndent(),
)

assertThat(query.args)
.containsExactly(
"CarePlan",
"CarePlan",
"subject",
*patientIdReferenceList.toTypedArray(),
)
.inOrder()
}

private companion object {
const val mockEpochTimeStamp = 1628516301000
const val APPROXIMATION_COEFFICIENT = 0.1
Expand Down
Loading