-
Notifications
You must be signed in to change notification settings - Fork 288
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
6b3d4c4
8c720e2
c73037d
0264816
6a45013
d764115
d509f53
ab9a4a6
432f7af
b5e67df
b9708a6
821feab
a304c19
112344f
ed8d74b
999fe93
5a96ef8
7aacdde
d83e139
e7dde67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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" } } | ||
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 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can also limit this list to 2000 or 3000. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I couldn't set limit the test to either 3000 or 2000, ran to another sqlite error on android
It's related to https://issuetracker.google.com/issues/73634057 For now, I just used a limit of 990 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
LZRS marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
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.