diff --git a/engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt b/engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt index 705b3c8a58..da3caad8af 100644 --- a/engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt +++ b/engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt @@ -67,6 +67,7 @@ import org.hl7.fhir.r4.model.DecimalType import org.hl7.fhir.r4.model.Encounter import org.hl7.fhir.r4.model.Enumerations import org.hl7.fhir.r4.model.Extension +import org.hl7.fhir.r4.model.Group import org.hl7.fhir.r4.model.HumanName import org.hl7.fhir.r4.model.Identifier import org.hl7.fhir.r4.model.Immunization @@ -75,6 +76,8 @@ import org.hl7.fhir.r4.model.Observation import org.hl7.fhir.r4.model.Organization import org.hl7.fhir.r4.model.Patient import org.hl7.fhir.r4.model.Period +import org.hl7.fhir.r4.model.Person +import org.hl7.fhir.r4.model.Person.PersonLinkComponent import org.hl7.fhir.r4.model.Practitioner import org.hl7.fhir.r4.model.Quantity import org.hl7.fhir.r4.model.Reference @@ -3680,6 +3683,124 @@ class DatabaseImplTest { .inOrder() } + @Test + fun search_patient_and_revinclude_person_should_map_common_person_to_all_matching_patients() = + runBlocking { + val person1 = + Person().apply { + id = "person-1" + addName( + HumanName().apply { + family = "Person" + addGiven("First") + }, + ) + addLink(PersonLinkComponent(Reference("Patient/pa-01"))) + addLink(PersonLinkComponent(Reference("Patient/pa-02"))) + } + + val person2 = + Person().apply { + id = "person-2" + addName( + HumanName().apply { + family = "Person" + addGiven("Second") + }, + ) + addLink(PersonLinkComponent(Reference("Patient/pa-02"))) + addLink(PersonLinkComponent(Reference("Patient/pa-03"))) + } + + val person3 = + Person().apply { + id = "person-3" + addName( + HumanName().apply { + family = "Person" + addGiven("Third") + }, + ) + addLink(PersonLinkComponent(Reference("Patient/pa-01"))) + addLink(PersonLinkComponent(Reference("Patient/pa-03"))) + } + + val patient01 = + Patient().apply { + id = "pa-01" + addName( + HumanName().apply { + addGiven("James") + family = "Gorden" + }, + ) + } + + val patient02 = + Patient().apply { + id = "pa-02" + addName( + HumanName().apply { + addGiven("James") + family = "Bond" + }, + ) + } + + val patient03 = + Patient().apply { + id = "pa-03" + addName( + HumanName().apply { + addGiven("Jamie") + family = "Bond" + }, + ) + } + + database.insert(person1, person2, person3, patient01, patient02, patient03) + + val result = + Search(ResourceType.Patient) + .apply { + filter( + Patient.GIVEN, + { + value = "Jam" + modifier = StringFilterModifier.STARTS_WITH + }, + ) + + revInclude(ResourceType.Person, Person.LINK) { sort(Person.NAME, Order.ASCENDING) } + } + .execute(database) + + assertThat(result) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult( + patient01, + included = null, + revIncluded = + mapOf(Pair(ResourceType.Person, Person.LINK.paramName) to listOf(person1, person3)), + ), + SearchResult( + patient02, + included = null, + revIncluded = + mapOf(Pair(ResourceType.Person, Person.LINK.paramName) to listOf(person1, person2)), + ), + SearchResult( + patient03, + included = null, + revIncluded = + mapOf(Pair(ResourceType.Person, Person.LINK.paramName) to listOf(person2, person3)), + ), + ) + .inOrder() + } + @Test fun search_patient_and_revInclude_encounters_sorted_by_date_descending(): Unit = runBlocking { val patient01 = @@ -4086,6 +4207,802 @@ class DatabaseImplTest { assertThat(searchedObservations[0].logicalId).isEqualTo(locallyCreatedObservationResourceId) } + @Test // https://github.com/google/android-fhir/issues/2512 + fun included_results_sort_ascending_should_have_distinct_resources() = runBlocking { + /** + * This tests that the search query does not return duplicated resources as a result of sorting + * by a field that has multiple index values. For example, searching a group of patients sorted + * by Patient.GIVEN should return a single copy of each patient even if some of them might have + * multiple given names indexed. + * + * Whilst sorting, the resource table and the relevant index table are joined, which could + * result in multiple rows for a single resource if there are multiple index values for the + * particular index used in the sorting criteria. This is prevented by adding the `GROUP BY` + * with `HAVING` clause in the generated SQL query. See `MoreSearch.generateGroupAndOrderQuery` + * for additional info. + */ + val group = + Group().apply { + id = "group" + addMember(Group.GroupMemberComponent(Reference("Patient/p1"))) + addMember(Group.GroupMemberComponent(Reference("Patient/p2"))) + } + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + } + database.insert(group, p1, p2) + + val ascendingResult = + Search(ResourceType.Group) + .apply { include(Group.MEMBER) { sort(Patient.GIVEN, Order.ASCENDING) } } + .execute(database) + + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .contains(SearchResult(group, mapOf(Group.MEMBER.paramName to listOf(p1, p2)), null)) + } + + @Test // https://github.com/google/android-fhir/issues/2512 + fun included_results_sort_descending_should_have_distinct_resources() = runBlocking { + /** + * This tests that the search query does not return duplicated resources as a result of sorting + * by a field that has multiple index values. For example, searching a group of patients sorted + * by Patient.GIVEN should return a single copy of each patient even if some of them might have + * multiple given names indexed. + * + * Whilst sorting, the resource table and the relevant index table are joined, which could + * result in multiple rows for a single resource if there are multiple index values for the + * particular index used in the sorting criteria. This is prevented by adding the `GROUP BY` + * with `HAVING` clause in the generated SQL query. See `MoreSearch.generateGroupAndOrderQuery` + * for additional info. + */ + val group = + Group().apply { + id = "group" + addMember(Group.GroupMemberComponent(Reference("Patient/p1"))) + addMember(Group.GroupMemberComponent(Reference("Patient/p2"))) + } + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + } + database.insert(group, p1, p2) + + val descendingResult = + Search(ResourceType.Group) + .apply { include(Group.MEMBER) { sort(Patient.GIVEN, Order.DESCENDING) } } + .execute(database) + + assertThat(descendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .contains(SearchResult(group, mapOf(Group.MEMBER.paramName to listOf(p2, p1)), null)) + } + + @Test + fun revIncluded_results_sort_ascending_should_have_distinct_resources() = runBlocking { + val practitioner = + Practitioner().apply { + id = "practitioner-1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("James") + }, + ) + } + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + + addGeneralPractitioner(Reference("Practitioner/practitioner-1")) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + addGeneralPractitioner(Reference("Practitioner/practitioner-1")) + } + + database.insert(practitioner, p1, p2) + val ascendingResult = + Search(ResourceType.Practitioner) + .apply { + revInclude(Patient.GENERAL_PRACTITIONER) { sort(Patient.GIVEN, Order.ASCENDING) } + } + .execute(database) + + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .contains( + SearchResult( + practitioner, + null, + mapOf( + Pair(ResourceType.Patient, Patient.GENERAL_PRACTITIONER.paramName) to listOf(p1, p2), + ), + ), + ) + } + + @Test + fun revIncluded_results_sort_descending_should_have_distinct_resources() = runBlocking { + val practitioner = + Practitioner().apply { + id = "practitioner-1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("James") + }, + ) + } + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + + addGeneralPractitioner(Reference("Practitioner/practitioner-1")) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + addGeneralPractitioner(Reference("Practitioner/practitioner-1")) + } + + database.insert(practitioner, p1, p2) + val descendingResult = + Search(ResourceType.Practitioner) + .apply { + revInclude(Patient.GENERAL_PRACTITIONER) { + sort(Patient.GIVEN, Order.DESCENDING) + } + } + .execute(database) + + assertThat(descendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .contains( + SearchResult( + practitioner, + null, + mapOf( + Pair(ResourceType.Patient, Patient.GENERAL_PRACTITIONER.paramName) to listOf(p2, p1), + ), + ), + ) + } + + @Test + fun included_and_revIncluded_results_should_have_distinct_resources() = runBlocking { + // A person has multiple first names and encounter has multiple location + // Searching a group including Patient and revIncluding encounter with results sorted by + // Patient.GIVEN and Encounter.LOCATION_PERIOD should return single copies of the resources. + + val group = + Group().apply { + id = "group" + addMember(Group.GroupMemberComponent(Reference("Patient/multiple-first-names"))) + } + val patient = + Patient().apply { + id = "multiple-first-names" + + addName( + HumanName().apply { + family = "LastName" + addGiven("FirstName-01") + addGiven("FirstName-02") + addGiven("FirstName-03") + }, + ) + } + + val encounter = + Encounter().apply { + id = "encounter-multiple-locations" + + subject = Reference("Group/group") + + addLocation().apply { + location = Reference("Location/1") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T10:00:00-05:30") + endElement = DateTimeType("2024-03-13T10:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/2") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T11:00:00-05:30") + endElement = DateTimeType("2024-03-13T11:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/3") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T09:00:00-05:30") + endElement = DateTimeType("2024-03-13T09:30:00-05:30") + } + } + } + database.insert(group, patient, encounter) + + val result = + Search(ResourceType.Group) + .apply { + include(Group.MEMBER) { + filter( + Patient.GIVEN, + { + value = "FirstName" + modifier = StringFilterModifier.STARTS_WITH + }, + ) + + sort(Patient.GIVEN, Order.ASCENDING) + } + + revInclude(Encounter.SUBJECT) { + sort(Encounter.LOCATION_PERIOD, Order.ASCENDING) + } + } + .execute(database) + + assertThat(result) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .contains( + SearchResult( + group, + mapOf( + Group.MEMBER.paramName to listOf(patient), + ), + mapOf(Pair(ResourceType.Encounter, Encounter.SUBJECT.paramName) to listOf(encounter)), + ), + ) + } + + @Test + fun sort_ascending_repeated_values_with_string_param_should_return_distinct_values() = + runBlocking { + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + } + database.insert(p1, p2) + + val ascendingResult = + Search(ResourceType.Patient) + .apply { + filter( + Patient.FAMILY, + { + value = "Cooper" + modifier = StringFilterModifier.MATCHES_EXACTLY + }, + ) + sort(Patient.GIVEN, Order.ASCENDING) + } + .execute(database) + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(p1, null, null), + SearchResult(p2, null, null), + ) + .inOrder() + } + + @Test + fun sort_descending_repeated_values_with_string_param_should_return_distinct_values() = + runBlocking { + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("1") + }, + ) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("4") + }, + ) + } + database.insert(p1, p2) + + val descendingResult = + Search(ResourceType.Patient) + .apply { + filter( + Patient.FAMILY, + { + value = "Cooper" + modifier = StringFilterModifier.MATCHES_EXACTLY + }, + ) + sort(Patient.GIVEN, Order.DESCENDING) + } + .execute(database) + + assertThat(descendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(p2, null, null), + SearchResult(p1, null, null), + ) + .inOrder() + } + + @Test + fun sort_ascending_repeated_values_with_date_param_should_return_distinct_values() = runBlocking { + val e1 = + Encounter().apply { + id = "encounter-multiple-locations-1" + + subject = Reference("Group/group") + + addLocation().apply { + location = Reference("Location/3") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T09:00:00-05:30") + endElement = DateTimeType("2024-03-13T09:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/2") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T11:00:00-05:30") + endElement = DateTimeType("2024-03-13T11:30:00-05:30") + } + } + } + + val e2 = + Encounter().apply { + id = "encounter-multiple-locations-2" + + subject = Reference("Group/group") + + addLocation().apply { + location = Reference("Location/1") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T10:00:00-05:30") + endElement = DateTimeType("2024-03-13T10:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/2") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T11:30:00-05:30") + endElement = DateTimeType("2024-03-13T12:00:00-05:30") + } + } + } + + database.insert(e1, e2) + + val ascendingResult = + Search(ResourceType.Encounter) + .apply { sort(Encounter.LOCATION_PERIOD, Order.ASCENDING) } + .execute(database) + + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(e1, null, null), + SearchResult(e2, null, null), + ) + .inOrder() + } + + @Test + fun sort_descending_repeated_values_with_date_param_should_return_distinct_values() = + runBlocking { + val e1 = + Encounter().apply { + id = "encounter-multiple-locations-1" + + subject = Reference("Group/group") + + addLocation().apply { + location = Reference("Location/3") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T09:00:00-05:30") + endElement = DateTimeType("2024-03-13T09:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/2") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T11:00:00-05:30") + endElement = DateTimeType("2024-03-13T11:30:00-05:30") + } + } + } + + val e2 = + Encounter().apply { + id = "encounter-multiple-locations-2" + + subject = Reference("Group/group") + + addLocation().apply { + location = Reference("Location/1") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T10:00:00-05:30") + endElement = DateTimeType("2024-03-13T10:30:00-05:30") + } + } + + addLocation().apply { + location = Reference("Location/2") + period = + Period().apply { + startElement = DateTimeType("2024-03-13T11:30:00-05:30") + endElement = DateTimeType("2024-03-13T12:00:00-05:30") + } + } + } + + database.insert(e1, e2) + + val descendingResult = + Search(ResourceType.Encounter) + .apply { sort(Encounter.LOCATION_PERIOD, Order.DESCENDING) } + .execute(database) + + assertThat(descendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(e2, null, null), + SearchResult(e1, null, null), + ) + .inOrder() + } + + @Test + fun sort_ascending_repeated_values_with_numeric_param_should_return_distinct_values() = + runBlocking { + val r1 = + RiskAssessment().apply { + id = "ris-01" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.8) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.5) + }, + ) + } + + val r2 = + RiskAssessment().apply { + id = "ris-02" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.6) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.9) + }, + ) + } + + val r3 = + RiskAssessment().apply { + id = "ris-03" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.2) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.4) + }, + ) + } + database.insert(r1, r2, r3) + val ascendingResult = + Search(ResourceType.RiskAssessment) + .apply { + filter( + RiskAssessment.PROBABILITY, + { + value = BigDecimal.valueOf(0.4) + prefix = ParamPrefixEnum.GREATERTHAN + }, + ) + sort(RiskAssessment.PROBABILITY, Order.ASCENDING) + } + .execute(database) + + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(r1, null, null), + SearchResult(r2, null, null), + ) + .inOrder() + } + + @Test + fun sort_descending_repeated_values_with_numeric_param_should_return_distinct_values() = + runBlocking { + val r1 = + RiskAssessment().apply { + id = "ris-01" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.8) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.5) + }, + ) + } + + val r2 = + RiskAssessment().apply { + id = "ris-02" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.6) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.9) + }, + ) + } + + val r3 = + RiskAssessment().apply { + id = "ris-03" + subject = Reference("Patient/risk-patient") + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.2) + }, + ) + + addPrediction( + RiskAssessment.RiskAssessmentPredictionComponent().apply { + probability = DecimalType(0.4) + }, + ) + } + database.insert(r1, r2, r3) + + val descendingResult = + Search(ResourceType.RiskAssessment) + .apply { + filter( + RiskAssessment.PROBABILITY, + { + value = BigDecimal.valueOf(0.4) + prefix = ParamPrefixEnum.GREATERTHAN + }, + ) + sort(RiskAssessment.PROBABILITY, Order.DESCENDING) + } + .execute(database) + + assertThat(descendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(r2, null, null), + SearchResult(r1, null, null), + ) + .inOrder() + } + + @Test + fun sort_resource_with_null_sort_index_value_but_matching_filter_should_be_included() = + runBlocking { + val p1 = + Patient().apply { + id = "p1" + addName( + HumanName().apply { + family = "Cooper" + addGiven("4") + addGiven("1") + }, + ) + this.birthDateElement = DateType(2020, 4, 2) + } + + val p2 = + Patient().apply { + id = "p2" + addName( + HumanName().apply { + family = "Cooper" + addGiven("2") + addGiven("5") + }, + ) + this.birthDateElement = DateType(2010, 4, 2) + } + + val p3 = + Patient().apply { + id = "p3" + addName( + HumanName().apply { + family = "Cooper" + addGiven("3") + addGiven("6") + }, + ) + } + database.insert(p1, p2, p3) + + val ascendingResult = + Search(ResourceType.Patient) + .apply { + filter( + Patient.FAMILY, + { + value = "Cooper" + modifier = StringFilterModifier.MATCHES_EXACTLY + }, + ) + sort(Patient.BIRTHDATE, Order.ASCENDING) + } + .execute(database) + assertThat(ascendingResult) + .comparingElementsUsing(SearchResultCorrespondence) + .displayingDiffsPairedBy { it.resource.logicalId } + .containsExactly( + SearchResult(p2, null, null), + SearchResult(p1, null, null), + SearchResult(p3, null, null), + ) + .inOrder() + } + private companion object { const val mockEpochTimeStamp = 1628516301000 const val TEST_PATIENT_1_ID = "test_patient_1" diff --git a/engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt b/engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt index b43f64480e..0e652d9f4c 100644 --- a/engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt +++ b/engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt @@ -204,10 +204,9 @@ internal class DatabaseImpl( query: SearchQuery, ): List> { return db.withTransaction { - resourceDao - .getResources(SimpleSQLiteQuery(query.query, query.args.toTypedArray())) - .map { ResourceWithUUID(it.uuid, iParser.parseResource(it.serializedResource) as R) } - .distinctBy { it.uuid } + resourceDao.getResources(SimpleSQLiteQuery(query.query, query.args.toTypedArray())).map { + ResourceWithUUID(it.uuid, iParser.parseResource(it.serializedResource) as R) + } } } diff --git a/engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt b/engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt index 84458cf7a1..449277f31d 100644 --- a/engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt +++ b/engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt @@ -71,6 +71,7 @@ internal abstract class ResourceDao { it.copy( serializedResource = iParser.encodeResourceToString(resource), lastUpdatedLocal = timeOfLocalChange, + lastUpdatedRemote = resource.meta.lastUpdated?.toInstant() ?: it.lastUpdatedRemote, ) updateChanges(entity, resource) } @@ -129,11 +130,6 @@ internal abstract class ResourceDao { createLocalLastUpdatedIndex(resource.resourceType, InstantType(Date.from(instant))), ) } - entity.lastUpdatedRemote?.let { instant -> - addDateTimeIndex( - createLastUpdatedIndex(resource.resourceType, InstantType(Date.from(instant))), - ) - } } .build() updateIndicesForResource(index, resource.resourceType, entity.resourceUuid) diff --git a/engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt b/engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt index 79be44b747..9bfd11a594 100644 --- a/engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt +++ b/engine/src/main/java/com/google/android/fhir/search/BaseSearch.kt @@ -1,5 +1,5 @@ /* - * Copyright 2022-2023 Google LLC + * Copyright 2022-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. @@ -30,6 +30,8 @@ import com.google.android.fhir.search.filter.ReferenceParamFilterCriterion import com.google.android.fhir.search.filter.StringParamFilterCriterion import com.google.android.fhir.search.filter.TokenParamFilterCriterion import com.google.android.fhir.search.filter.UriParamFilterCriterion +import org.hl7.fhir.r4.model.HumanName +import org.hl7.fhir.r4.model.Patient @DslMarker @Target(AnnotationTarget.CLASS, AnnotationTarget.TYPE) annotation class BaseSearchDsl @@ -90,9 +92,65 @@ interface BaseSearch { operation: Operation = Operation.OR, ) + /** + * When sorting is applied on a field with repeated values (e.g. [Patient.GIVEN] ), the order is + * defined by the `value` of the repeated values in the resource (e.g. [HumanName.given] for + * [Patient]). + * + * If there are two Patients p1 and p2 as follows + * + * ``` + * { + * "resourceType": "Patient", + * "id": "p1", + * "name": [ + * { + * "family": "Cooper", + * "given": [ + * "3", + * "1" + * ] + * } + * ] + * } + * ``` + * + * AND + * + * ``` + * { + * "resourceType": "Patient", + * "id": "p2", + * "name": [ + * { + * "family": "Cooper", + * "given": [ + * "2", + * "4" + * ] + * } + * ] + * } + * ``` + * + * Then sorting the patients in ascending or descending order with their given i.e [Patient.GIVEN] + * depends on the smallest (`1`, `3`) or largest (`2`, `4`) given in the first name respectively . + */ fun sort(parameter: StringClientParam, order: Order) + /** + * When sorting is applied on a field with repeated values, defined by the `value` of the repeated + * values in the resource. + * + * @see sort(parameter: StringClientParam, order: Order) for more details. + */ fun sort(parameter: NumberClientParam, order: Order) + /** + * When sorting is applied on a field with repeated values, defined by the `value` of the repeated + * values in the resource. + * + * @see sort(parameter: StringClientParam, order: Order) for more details. + */ fun sort(parameter: DateClientParam, order: Order) } diff --git a/engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt b/engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt index 44c342200a..2d1f4044f5 100644 --- a/engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt +++ b/engine/src/main/java/com/google/android/fhir/search/MoreSearch.kt @@ -20,6 +20,7 @@ import android.annotation.SuppressLint import androidx.annotation.VisibleForTesting import androidx.room.util.convertUUIDToByte import ca.uhn.fhir.rest.gclient.DateClientParam +import ca.uhn.fhir.rest.gclient.IParam import ca.uhn.fhir.rest.gclient.NumberClientParam import ca.uhn.fhir.rest.gclient.StringClientParam import ca.uhn.fhir.rest.param.ParamPrefixEnum @@ -48,6 +49,14 @@ import timber.log.Timber */ private const val APPROXIMATION_COEFFICIENT = 0.1 +/** + * SQLite supports signed and unsigned integers with a maximum length of 8 bytes. The signed + * integers can range from `-9223372036854775808` to `+9223372036854775807`. See + * [Storage Classes and Datatypes](https://www.sqlite.org/datatype3.html) + */ +private const val MIN_VALUE = "-9223372036854775808" +private const val MAX_VALUE = "9223372036854775808" + internal suspend fun Search.execute(database: Database): List> { val baseResources = database.search(getQuery()) val includedResources = @@ -132,11 +141,12 @@ internal fun Search.getRevIncludeQuery(includeIds: List): SearchQuery { return revIncludes .map { - val (join, order) = it.search.getSortOrder(otherTable = "re") + val (join, order) = + it.search.getSortOrder(otherTable = "re", groupByColumn = "rie.index_value") args.addAll(join.args) val filterQuery = generateFilterQuery(it) """ - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -194,11 +204,12 @@ internal fun Search.getIncludeQuery(includeIds: List): SearchQuery { return forwardIncludes .map { - val (join, order) = it.search.getSortOrder(otherTable = "re") + val (join, order) = + it.search.getSortOrder(otherTable = "re", groupByColumn = "rie.resourceuuid") args.addAll(join.args) val filterQuery = generateFilterQuery(it) """ - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -221,6 +232,7 @@ internal fun Search.getIncludeQuery(includeIds: List): SearchQuery { private fun Search.getSortOrder( otherTable: String, isReferencedSearch: Boolean = false, + groupByColumn: String = "", ): Pair { var sortJoinStatement = "" var sortOrderStatement = "" @@ -255,22 +267,83 @@ private fun Search.getSortOrder( .joinToString(separator = "\n") sortTableNames.forEach { _ -> args.add(sort.paramName) } - sortTableNames.forEachIndexed { index, sortTableName -> - val tableAlias = 'b' + index - sortOrderStatement += - if (index == 0) { - """ - ORDER BY $tableAlias.${sortTableName.columnName} ${order.sqlString} - """ - .trimIndent() - } else { - ", $tableAlias.${SortTableInfo.DATE_TIME_SORT_TABLE_INFO.columnName} ${order.sqlString}" - } - } + sortOrderStatement += + generateGroupAndOrderQuery(sort, order!!, otherTable, groupByColumn, sortTableNames) } return Pair(SearchQuery(sortJoinStatement, args), sortOrderStatement) } +/** + * Sorting by a field that has multiple indexed values may result in duplicated resources. So, we + * use `GROUP BY` + `HAVING` clause to find distinct values in specified order. + * + * To make the sorting order a bit predictable, we use MIN and MAX functions with `HAVING` to use + * the corresponding values for GROUPING to find the distinct results. + * + * e.g. If there are Two Patients resources with multiple first names P1 ( first names =`3`, `1`) + * and P2 (first names = `2`, `4`), when sorting them in + * + * *ASCENDING order*: MIN function is used so that the smallest names of both the patients are + * considered for Grouping `[P1(`1`), P2(`2`)]`. + * + * *DESCENDING order*: MAX function is used so that the largest names of both the patients are + * considered for Grouping `[P2(`4`), P1(`3`)]`. + * + * For the special case where the index value is NULL, we use the default 0 value and to complete + * the expression, we check that the value is greater than [MIN_VALUE], the minimum value an INTEGER + * type can store in SQLITE. The reason to check against [MIN_VALUE] rather that 0, since string is + * always greater than integer (StringIndexEntity) and Date/DateTimeIndexEntity will always have + * positive integer values, is because the NumberIndexEntity table may contain negative values in + * it. + * + * Without the `>= MIN_VALUE` check, NULL values are not included in the results if the default + * is 0. + * + * The default values provided in GROUP BY stage are not carried forward during the ORDER BY, so we + * provide [MAX_VALUE] and [MIN_VALUE] as default in ORDER BY respectively for ASCENDING and + * DESCENDING to make sure that results with null index values are always at the bottom. + */ +private fun generateGroupAndOrderQuery( + sort: IParam, + order: Order, + otherTable: String, + groupByColumn: String, + sortTableNames: List, +): String { + var sortOrderStatement = "" + val havingColumn = + when (sort) { + is StringClientParam, + is NumberClientParam, -> "IFNULL(b.index_value,0)" + /* The DateClientParam is used for both Date and DateTime values and the value is present exclusively in either one of the DateIndexEntity or DateTimeIndexEntity tables. + To find the MIN or MAX values, we add the values from both the tables. It results in the exact value as the other table would have null and hence default 0 would be added to actual date/datetime value.*/ + is DateClientParam -> "IFNULL(b.index_from,0) + IFNULL(c.index_from,0)" + else -> throw NotImplementedError("Unhandled sort parameter of type ${sort::class}: $sort") + } + + sortOrderStatement += + """ + GROUP BY $otherTable.resourceUuid ${if (groupByColumn.isNotEmpty()) ", $groupByColumn" else ""} + HAVING ${if (order == Order.ASCENDING) "MIN($havingColumn) >= $MIN_VALUE" else "MAX($havingColumn) >= $MIN_VALUE"} + + """ + .trimIndent() + val defaultValue = if (order == Order.ASCENDING) MAX_VALUE else MIN_VALUE + sortTableNames.forEachIndexed { index, sortTableName -> + val tableAlias = 'b' + index + sortOrderStatement += + if (index == 0) { + """ + ORDER BY IFNULL($tableAlias.${sortTableName.columnName}, $defaultValue) ${order.sqlString} + """ + .trimIndent() + } else { + ", IFNULL($tableAlias.${SortTableInfo.DATE_TIME_SORT_TABLE_INFO.columnName}, $defaultValue) ${order.sqlString}" + } + } + return sortOrderStatement +} + private fun Search.getFilterQueries() = (stringFilterCriteria + quantityFilterCriteria + diff --git a/engine/src/test/java/com/google/android/fhir/search/SearchTest.kt b/engine/src/test/java/com/google/android/fhir/search/SearchTest.kt index d46c110664..938dd64bc6 100644 --- a/engine/src/test/java/com/google/android/fhir/search/SearchTest.kt +++ b/engine/src/test/java/com/google/android/fhir/search/SearchTest.kt @@ -1719,7 +1719,9 @@ class SearchTest { LEFT JOIN StringIndexEntity b ON a.resourceUuid = b.resourceUuid AND b.index_name = ? WHERE a.resourceType = ? - ORDER BY b.index_value ASC + GROUP BY a.resourceUuid + HAVING MIN(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, 9223372036854775808) ASC """ .trimIndent(), ) @@ -1739,7 +1741,9 @@ class SearchTest { LEFT JOIN StringIndexEntity b ON a.resourceUuid = b.resourceUuid AND b.index_name = ? WHERE a.resourceType = ? - ORDER BY b.index_value DESC + GROUP BY a.resourceUuid + HAVING MAX(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, -9223372036854775808) DESC """ .trimIndent(), ) @@ -1761,7 +1765,9 @@ class SearchTest { LEFT JOIN NumberIndexEntity b ON a.resourceUuid = b.resourceUuid AND b.index_name = ? WHERE a.resourceType = ? - ORDER BY b.index_value ASC + GROUP BY a.resourceUuid + HAVING MIN(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, 9223372036854775808) ASC """ .trimIndent(), ) @@ -1791,7 +1797,9 @@ class SearchTest { SELECT resourceUuid FROM StringIndexEntity WHERE resourceType = ? AND index_name = ? AND index_value LIKE ? || '%' COLLATE NOCASE ) - ORDER BY b.index_value ASC + GROUP BY a.resourceUuid + HAVING MIN(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, 9223372036854775808) ASC LIMIT ? OFFSET ? """ .trimIndent(), @@ -2045,8 +2053,10 @@ class SearchTest { LEFT JOIN DateTimeIndexEntity c ON a.resourceUuid = c.resourceUuid AND c.index_name = ? WHERE a.resourceType = ? - ORDER BY b.index_from ASC, c.index_from ASC - """ + GROUP BY a.resourceUuid + HAVING MIN(IFNULL(b.index_from,0) + IFNULL(c.index_from,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_from, 9223372036854775808) ASC, IFNULL(c.index_from, 9223372036854775808) ASC + """ .trimIndent(), ) } @@ -2066,7 +2076,9 @@ class SearchTest { LEFT JOIN DateTimeIndexEntity c ON a.resourceUuid = c.resourceUuid AND c.index_name = ? WHERE a.resourceType = ? - ORDER BY b.index_from DESC, c.index_from DESC + GROUP BY a.resourceUuid + HAVING MAX(IFNULL(b.index_from,0) + IFNULL(c.index_from,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_from, -9223372036854775808) DESC, IFNULL(c.index_from, -9223372036854775808) DESC """ .trimIndent(), ) @@ -2277,7 +2289,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -2319,7 +2331,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -2369,7 +2381,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -2380,7 +2392,9 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND index_value = ? ) - ORDER BY b.index_value DESC + GROUP BY re.resourceUuid , rie.resourceuuid + HAVING MAX(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, -9223372036854775808) DESC ) """ .trimIndent(), @@ -2428,7 +2442,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -2439,11 +2453,13 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND index_value = ? ) - ORDER BY b.index_value DESC + GROUP BY re.resourceUuid , rie.resourceuuid + HAVING MAX(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, -9223372036854775808) DESC ) UNION ALL SELECT * FROM ( - SELECT rie.index_name, rie.resourceUuid, re.serializedResource + SELECT rie.index_name, rie.resourceUuid, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceType||"/"||re.resourceId = rie.index_value @@ -2454,7 +2470,9 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND index_value = ? ) - ORDER BY b.index_value DESC + GROUP BY re.resourceUuid , rie.resourceuuid + HAVING MAX(IFNULL(b.index_value,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_value, -9223372036854775808) DESC ) """ .trimIndent(), @@ -2496,7 +2514,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -2534,7 +2552,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -2587,7 +2605,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -2600,7 +2618,9 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND (index_value = ? AND IFNULL(index_system,'') = ?) ) - ORDER BY b.index_from DESC, c.index_from DESC + GROUP BY re.resourceUuid , rie.index_value + HAVING MAX(IFNULL(b.index_from,0) + IFNULL(c.index_from,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_from, -9223372036854775808) DESC, IFNULL(c.index_from, -9223372036854775808) DESC ) """ .trimIndent(), @@ -2664,7 +2684,7 @@ class SearchTest { .isEqualTo( """ SELECT * FROM ( - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -2677,11 +2697,13 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND (index_value = ? AND IFNULL(index_system,'') = ?) ) - ORDER BY b.index_from DESC, c.index_from DESC + GROUP BY re.resourceUuid , rie.index_value + HAVING MAX(IFNULL(b.index_from,0) + IFNULL(c.index_from,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_from, -9223372036854775808) DESC, IFNULL(c.index_from, -9223372036854775808) DESC ) UNION ALL SELECT * FROM ( - SELECT rie.index_name, rie.index_value, re.serializedResource + SELECT rie.index_name, rie.index_value, re.serializedResource FROM ResourceEntity re JOIN ReferenceIndexEntity rie ON re.resourceUuid = rie.resourceUuid @@ -2694,7 +2716,9 @@ class SearchTest { SELECT resourceUuid FROM TokenIndexEntity WHERE resourceType = ? AND index_name = ? AND (index_value = ? AND IFNULL(index_system,'') = ?) ) - ORDER BY b.index_from DESC, c.index_from DESC + GROUP BY re.resourceUuid , rie.index_value + HAVING MAX(IFNULL(b.index_from,0) + IFNULL(c.index_from,0)) >= -9223372036854775808 + ORDER BY IFNULL(b.index_from, -9223372036854775808) DESC, IFNULL(c.index_from, -9223372036854775808) DESC ) """ .trimIndent(),