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 14 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
@@ -1,5 +1,5 @@
/*
* Copyright 2021-2023 Google LLC
* Copyright 2021-2024 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -85,16 +85,23 @@ internal sealed class FilterCriteria(
* This function takes care of wrapping the conditions in brackets so that they are evaluated as
* intended.
*/
private fun List<ConditionParam<*>>.toQueryString(operation: Operation) =
this.joinToString(
separator = " ${operation.logicalOperator} ",
prefix = if (size > 1) "(" else "",
postfix = if (size > 1) ")" else "",
) {
if (it.params.size > 1) {
"(${it.condition})"
} else {
it.condition
}
private fun List<ConditionParam<*>>.toQueryString(operation: Operation): String {
if (this.size <= 1) {
LZRS marked this conversation as resolved.
Show resolved Hide resolved
val queryString =
firstOrNull()?.let {
if (it.params.size > 1) {
"(${it.condition})"
} else {
it.condition
}
LZRS marked this conversation as resolved.
Show resolved Hide resolved
}
return queryString ?: ""
}

val mid = this.size / 2
val left = this.subList(0, mid).toQueryString(operation)
val right = this.subList(mid, this.size).toQueryString(operation)

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
Loading
Loading