From 4570c28cb63c6b8ba74d6c15ef36c51e6947613c Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 4 Mar 2024 10:52:04 -0500 Subject: [PATCH] Fix _has then chain then _has searches (#5735) * Add SQL logging: Attempt to add test. * Small fix. * moar tests * Tweak unit tests to be parameterized. * Enhance tests and more logging. * Better tests. More logging. More TODOs. * Better tests. More logging. More TODOs. * Changes * Changes * Updates to tests. More logging and experimental code. * Introduce fix along with more logging and TODOs and tweaks to tests. * Cleanup solution. Get rid of logs and todos. Rename test class, test methods and clean up unnecessary code. * Add changelog. * Implement simpler chain algorithm without conditional logic and leave a long comment explaining why. * Implement simpler chain algorithm without conditional logic and leave a long comment explaining why. * Fix comment. --------- Co-authored-by: Ken Stevens --- ...s-then-chain-has-searches-not-working.yaml | 7 + .../fhir/jpa/search/builder/QueryStack.java | 20 +- ...eProviderR4SearchVariousScenariosTest.java | 366 ++++++++++++++++++ 3 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5735-has-then-chain-has-searches-not-working.yaml create mode 100644 hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchVariousScenariosTest.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5735-has-then-chain-has-searches-not-working.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5735-has-then-chain-has-searches-not-working.yaml new file mode 100644 index 00000000000..9db77df4cbe --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5735-has-then-chain-has-searches-not-working.yaml @@ -0,0 +1,7 @@ +--- +type: fix +issue: 5735 +jira: SMILE-7592 +title: "Previously, complex _has then chain then _has searches would fail to return results when expected + (ex: 'Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor._has:List:item:_id=list1') + This has been fixed." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java index 9cd22443e97..4f0b3e60778 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java @@ -119,6 +119,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static ca.uhn.fhir.jpa.search.builder.QueryStack.SearchForIdsParams.with; @@ -139,6 +140,7 @@ public class QueryStack { private static final Logger ourLog = LoggerFactory.getLogger(QueryStack.class); public static final String LOCATION_POSITION = "Location.position"; + private static final Pattern PATTERN_DOT_AND_ALL_AFTER = Pattern.compile("\\..*"); private final FhirContext myFhirContext; private final SearchQueryBuilder mySqlBuilder; @@ -1118,7 +1120,7 @@ public class QueryStack { targetResourceType = next.getTargetResourceType(); paramReference = next.getReferenceFieldName(); parameterName = next.getParameterName(); - paramName = parameterName.replaceAll("\\..*", ""); + paramName = PATTERN_DOT_AND_ALL_AFTER.matcher(parameterName).replaceAll(""); parameters.add(QualifiedParamList.singleton(null, next.getValueAsQueryToken(myFhirContext))); } @@ -1175,11 +1177,23 @@ public class QueryStack { // Handle internal chain inside the has. if (parameterName.contains(".")) { - String chainedPartOfParameter = getChainedPart(parameterName); + // Previously, for some unknown reason, we were calling getChainedPart() twice. This broke the _has + // then chain, then _has use case by effectively cutting off the second part of the chain and + // missing one iteration of the recursive call to build the query. + // So, for example, for + // Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor._has:List:item:_id=list1 + // instead of passing " payor._has:List:item:_id=list1" to the next recursion, the second call to + // getChainedPart() was wrongly removing "payor." and passing down "_has:List:item:_id=list1" instead. + // This resulted in running incorrect SQL with nonsensical join that resulted in 0 results. + // However, after running the pipeline, I've concluded there's no use case at all for the + // double call to "getChainedPart()", which is why there's no conditional logic at all to make a double + // call to getChainedPart(). + final String chainedPart = getChainedPart(parameterName); + orValues.stream() .filter(qp -> qp instanceof ReferenceParam) .map(qp -> (ReferenceParam) qp) - .forEach(rp -> rp.setChain(getChainedPart(chainedPartOfParameter))); + .forEach(rp -> rp.setChain(chainedPart)); parameterName = parameterName.substring(0, parameterName.indexOf('.')); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchVariousScenariosTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchVariousScenariosTest.java new file mode 100644 index 00000000000..95254668661 --- /dev/null +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchVariousScenariosTest.java @@ -0,0 +1,366 @@ +package ca.uhn.fhir.jpa.provider.r4; + +import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test; +import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.param.HasParam; +import org.hl7.fhir.instance.model.api.IIdType; +import org.hl7.fhir.r4.model.Bundle; +import org.hl7.fhir.r4.model.CarePlan; +import org.hl7.fhir.r4.model.Coverage; +import org.hl7.fhir.r4.model.Encounter; +import org.hl7.fhir.r4.model.Enumerations; +import org.hl7.fhir.r4.model.ExplanationOfBenefit; +import org.hl7.fhir.r4.model.Group; +import org.hl7.fhir.r4.model.ListResource; +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.Practitioner; +import org.hl7.fhir.r4.model.Reference; +import org.hl7.fhir.r4.model.SearchParameter; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.junit.jupiter.api.Assertions.assertFalse; + + +public class ResourceProviderR4SearchVariousScenariosTest extends BaseResourceProviderR4Test { + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ResourceProviderR4SearchVariousScenariosTest.class); + + @Nested + class HasMultipleNoChains { + private static final String PAT_ID = "pat1"; + private static final String OBSERVATION_ID = "obs1"; + private static final String ENCOUNTER_ID = "enc1"; + private static final String CARE_PLAN_ID = "cp1"; + + @BeforeEach + void beforeEach() { + final Patient patient = new Patient(); + patient.setId(PAT_ID); + + final IIdType patientId = myPatientDao.update(patient, mySrd).getId().toUnqualifiedVersionless(); + + final Observation observation = new Observation(); + observation.setId(OBSERVATION_ID); + observation.setSubject(new Reference(patientId.getValue())); + + final IIdType observationId = myObservationDao.update(observation, mySrd).getId().toUnqualifiedVersionless(); + + final Encounter encounter = new Encounter(); + encounter.setId(ENCOUNTER_ID); + encounter.addReasonReference(new Reference(observationId.getValue())); + + final IIdType encounterId = myEncounterDao.update(encounter, mySrd).getId().toUnqualifiedVersionless(); + + final CarePlan carePlan = new CarePlan(); + carePlan.setId(CARE_PLAN_ID); + carePlan.setEncounter(new Reference(encounterId.getValue())); + + myCarePlanDao.update(carePlan, mySrd); + } + + @ParameterizedTest + @ValueSource(strings = { + "Patient?_has:Observation:subject:_has:Encounter:reason-reference:_id="+ENCOUNTER_ID, + }) + void doubleHas(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Patient?_has:Observation:subject:_has:Encounter:reason-reference:_has:CarePlan:encounter:_id="+CARE_PLAN_ID + }) + void tripleHas(String theQueryString) { + runAndAssert(theQueryString); + } + } + + @Nested + class PractitionerEobCoveragePractitionerListNoCustomSearchParam { + private static final String PAT_ID = "pat1"; + private static final String ORG_ID = "org1"; + private static final String ORG_NAME = "myOrg"; + private static final String PRACTITIONER_ID = "pra1"; + private static final String COVERAGE_ID = "cov1"; + private static final String LIST_ID = "list1"; + private static final String EOB_ID = "eob1"; + + @BeforeEach + void beforeEach() { + myStorageSettings.setIndexOnContainedResources(false); + final Patient patient = new Patient(); + patient.setId(PAT_ID); + + final IIdType patientId = myPatientDao.update(patient, mySrd).getId().toUnqualifiedVersionless(); + + final Organization organization = new Organization(); + organization.setId(ORG_ID); + organization.setName(ORG_NAME); + + final IIdType orgId = myOrganizationDao.update(organization, mySrd).getId().toUnqualifiedVersionless(); + + final Practitioner practitioner = new Practitioner(); + practitioner.setId(PRACTITIONER_ID); + + final IIdType practitionerId = myPractitionerDao.update(practitioner, mySrd).getId().toUnqualifiedVersionless(); + + final Coverage coverage = new Coverage(); + coverage.setId(COVERAGE_ID); + coverage.addPayor().setReference(orgId.getValue()); + + final IIdType coverageId = myCoverageDao.update(coverage, mySrd).getId().toUnqualifiedVersionless(); + + final ListResource list = new ListResource(); + list.setId(LIST_ID); + list.addEntry().setItem(new Reference(orgId.getValue())); + + myListDao.update(list, mySrd).getId().toUnqualifiedVersionless(); + + final ExplanationOfBenefit explanationOfBenefit = new ExplanationOfBenefit(); + explanationOfBenefit.setId(EOB_ID); + explanationOfBenefit.setPatient(new Reference(patientId.getValue())); + explanationOfBenefit.setInsurer(new Reference(orgId.getValue())); + explanationOfBenefit.setProvider(new Reference(orgId.getValue())); + explanationOfBenefit.addCareTeam().setProvider(new Reference(practitionerId.getValue())); + explanationOfBenefit.addInsurance().setCoverage(new Reference(coverageId.getValue())); + + myExplanationOfBenefitDao.update(explanationOfBenefit, mySrd).getId().toUnqualifiedVersionless(); + } + + @ParameterizedTest + @ValueSource(strings = { + "ExplanationOfBenefit?coverage.payor.name="+ORG_NAME, + }) + void chainSimple(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor="+ORG_ID, // This is the first half + }) + void hasThenChainSimple(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "ExplanationOfBenefit?coverage.payor:Organization._has:List:item:_id="+LIST_ID + }) + void chainThenHasSimple(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Coverage?payor._has:List:item:_id="+LIST_ID, // dot _has + "Coverage?payor.name="+ORG_NAME, + "Coverage?payor="+ORG_ID, + "ExplanationOfBenefit?coverage="+COVERAGE_ID, + "ExplanationOfBenefit?coverage.payor.name="+ORG_NAME, + "ExplanationOfBenefit?coverage.payor="+ORG_ID, + "ExplanationOfBenefit?coverage.payor._has:List:item:_id="+LIST_ID, + "ExplanationOfBenefit?coverage.payor:Organization._has:List:item:_id="+LIST_ID, + "Organization?_has:List:item:_id="+LIST_ID, // This the second half of the buggy query + }) + void complexQueryFromList(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Organization?_has:Coverage:payor:_has:ExplanationOfBenefit:coverage:_id="+EOB_ID, // THIS WORKS!!!!!!! + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor="+ORG_ID, // this works +// "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor.name="+ORG_NAME, // this does not work + "Practitioner?_has:ExplanationOfBenefit:care-team:_id="+EOB_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage="+COVERAGE_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor="+ORG_ID, // This is the first half +// "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor._has:List:item:_id="+LIST_ID, // this doesn't work ... reomoves Organzation + }) + void complexQueryFromPractitioner(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor._has:List:item:_id="+LIST_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor:Organization._has:List:item:_id="+LIST_ID // same thing + }) + void hasThenChainThenHas(String theQueryString) { + runAndAssert(theQueryString); + } + + @Test + void searchWithSearchParameterHasThenChainThenChain() { + final SearchParameterMap searchParameterMap = new SearchParameterMap(); + + final HasParam hasParam = new HasParam("ExplanationOfBenefit", "care-team", "coverage.payor:Organization._has:List:item:_id", LIST_ID); + + searchParameterMap.add("_has", hasParam); + + final IBundleProvider search = myPractitionerDao.search(searchParameterMap, mySrd); + + assertFalse(search.isEmpty()); + } + + @Test + void searchWithSearchParameterFirstHalf() { + final SearchParameterMap searchParameterMap = new SearchParameterMap(); + +// "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor:Organization="+ORG_ID // This the first half of the buggy query + final HasParam hasParam = new HasParam("ExplanationOfBenefit", "care-team", "coverage.payor", ORG_ID); + + searchParameterMap.add("_has", hasParam); + + final IBundleProvider search = myPractitionerDao.search(searchParameterMap, mySrd); + + assertFalse(search.isEmpty()); + } + + @Test + void searchWithSearchParameterSecondHalf() { + final SearchParameterMap searchParameterMap = new SearchParameterMap(); + +// "Organization?_has:List:item:_id="+LIST_ID, // This the second half of the buggy query + final HasParam hasParam = new HasParam("List", "item", "_id", LIST_ID); + + searchParameterMap.add("_has", hasParam); + + final IBundleProvider search = myOrganizationDao.search(searchParameterMap, mySrd); + + assertFalse(search.isEmpty()); + } + } + + @Nested + class PractitionerEobCoveragePractitionerGroupYesCustomSearchParam { + private static final String PAT_ID = "pat1"; + private static final String ORG_ID = "org1"; + private static final String ORG_NAME = "myOrg"; + private static final String PRACTITIONER_ID = "pra1"; + private static final String COVERAGE_ID = "cov1"; + private static final String GROUP_ID = "grp1"; + private static final String EOB_ID = "eob1"; + + @BeforeEach + void beforeEach() { + myStorageSettings.setMarkResourcesForReindexingUponSearchParameterChange(false); + myStorageSettings.setIndexOnContainedResources(false); + + // This is not exactly lik the production scenario but create SearchParameter first to create the RES_LINK + // and avoid a call to reindex + final SearchParameter searchParameterGroupValueReference = new SearchParameter(); + searchParameterGroupValueReference.setId("group-value-reference"); + searchParameterGroupValueReference.setName("group-value-reference"); + searchParameterGroupValueReference.addBase("Group"); + searchParameterGroupValueReference.setStatus(Enumerations.PublicationStatus.ACTIVE); + searchParameterGroupValueReference.setCode("value-reference"); + searchParameterGroupValueReference.setType(Enumerations.SearchParamType.REFERENCE); + searchParameterGroupValueReference.setExpression("Group.characteristic.value.as(Reference)"); + searchParameterGroupValueReference.addTarget("Organization"); + searchParameterGroupValueReference.setXpathUsage(SearchParameter.XPathUsageType.NORMAL); + + mySearchParameterDao.update(searchParameterGroupValueReference, mySrd); + + final Patient patient = new Patient(); + patient.setId(PAT_ID); + + final IIdType patientId = myPatientDao.update(patient, mySrd).getId().toUnqualifiedVersionless(); + + final Organization organization = new Organization(); + organization.setId(ORG_ID); + organization.setName(ORG_NAME); + + final IIdType orgId = myOrganizationDao.update(organization, mySrd).getId().toUnqualifiedVersionless(); + + final Practitioner practitioner = new Practitioner(); + practitioner.setId(PRACTITIONER_ID); + + final IIdType practitionerId = myPractitionerDao.update(practitioner, mySrd).getId().toUnqualifiedVersionless(); + + final Coverage coverage = new Coverage(); + coverage.setId(COVERAGE_ID); + coverage.addPayor().setReference(orgId.getValue()); + + final IIdType coverageId = myCoverageDao.update(coverage, mySrd).getId().toUnqualifiedVersionless(); + + final Group group = new Group(); + group.setId(GROUP_ID); + group.addCharacteristic().getValueReference().setReference(orgId.getValue()); + + myGroupDao.update(group, mySrd); + + final ExplanationOfBenefit explanationOfBenefit = new ExplanationOfBenefit(); + explanationOfBenefit.setId(EOB_ID); + explanationOfBenefit.setPatient(new Reference(patientId.getValue())); + explanationOfBenefit.setInsurer(new Reference(orgId.getValue())); + explanationOfBenefit.setProvider(new Reference(orgId.getValue())); + explanationOfBenefit.addCareTeam().setProvider(new Reference(practitionerId.getValue())); + explanationOfBenefit.addInsurance().setCoverage(new Reference(coverageId.getValue())); + + myExplanationOfBenefitDao.update(explanationOfBenefit, mySrd).getId().toUnqualifiedVersionless(); + } + + @ParameterizedTest + @ValueSource(strings = { + "Coverage?payor.name="+ORG_NAME, + "Coverage?payor="+ORG_ID, + "ExplanationOfBenefit?coverage.payor.name="+ORG_NAME, + "ExplanationOfBenefit?coverage.payor="+ORG_ID, + "ExplanationOfBenefit?care-team="+PRACTITIONER_ID, + "ExplanationOfBenefit?coverage="+COVERAGE_ID, + "ExplanationOfBenefit?provider="+ORG_ID, + }) + void complexQueryFromGroup(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Group?value-reference="+ORG_ID, + "Organization?_has:Group:value-reference:_id="+GROUP_ID, // This the second half of the buggy query + }) + void useCustomSearchParam(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor:Organization._has:Group:value-reference:_id="+GROUP_ID, + }) + void hasThenChainThenHas(String theQueryString) { + runAndAssert(theQueryString); + } + + @ParameterizedTest + @ValueSource(strings = { + "Practitioner?_has:ExplanationOfBenefit:care-team:_id="+EOB_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage="+COVERAGE_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor="+ORG_ID, + "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor:Organization="+ORG_ID, // This the first half of the buggy query +// "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor._has:Group:value-reference:_id="+GROUP_ID, // this doesn't work +// "Practitioner?_has:ExplanationOfBenefit:care-team:coverage.payor:Organization.name="+ORG_NAME // This doesn't work + }) + void complexQueryFromPractitioner(String theQueryString) { + runAndAssert(theQueryString); + } + } + + private void runAndAssert(String theQueryString) { + ourLog.info("queryString:\n{}", theQueryString); + + final Bundle outcome = myClient.search() + .byUrl(theQueryString) + .returnBundle(Bundle.class) + .execute(); + + assertFalse(outcome.getEntry().isEmpty()); + ourLog.info("result:\n{}", theQueryString); + } +}