From 89c45eebdc31c3dd9533570d9c709ecc7ed5a7a6 Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Thu, 10 Feb 2022 13:59:18 -0500 Subject: [PATCH] fix handling of common search parameters --- .../3374-chained-common-search-params.yaml | 4 + .../fhir/jpa/search/builder/QueryStack.java | 57 ++++-- .../fhir/jpa/dao/r4/ChainingR4SearchTest.java | 176 ++++++++++++++++++ 3 files changed, 217 insertions(+), 20 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_7_0/3374-chained-common-search-params.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_7_0/3374-chained-common-search-params.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_7_0/3374-chained-common-search-params.yaml new file mode 100644 index 00000000000..eaf6c29dde9 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_7_0/3374-chained-common-search-params.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 3374 +title: "Chained searches will handle common search parameters correctly when the `Index Contained Resources` configuration parameter is enabled." 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 66a6cb9452b..db04f9d7fa3 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 @@ -731,18 +731,35 @@ public class QueryStack { private class ChainElement { private final String myResourceType; private final RuntimeSearchParam mySearchParam; + private final String myPath; public ChainElement(String theResourceType, RuntimeSearchParam theSearchParam) { this.myResourceType = theResourceType; this.mySearchParam = theSearchParam; + this.myPath = extractPath(theResourceType, theSearchParam); } public String getResourceType() { return myResourceType; } - public RuntimeSearchParam getSearchParam() { - return mySearchParam; + public String getPath() { return myPath; } + + public String getSearchParameterName() { return mySearchParam.getName(); } + + private String extractPath(String theResourceType, RuntimeSearchParam theSearchParam) { + List pathsForType = theSearchParam.getPathsSplit().stream() + .map(String::trim) + .filter(t -> t.startsWith(theResourceType)) + .collect(Collectors.toList()); + if (pathsForType.isEmpty()) { + ourLog.warn("Search parameter {} does not have a path for resource type {}.", theSearchParam.getName(), theResourceType); + return ""; + } else if (pathsForType.size() > 1) { + ourLog.warn("Search parameter {} has multiple paths for resource type {}. Selecting {}", theSearchParam.getName(), theResourceType, pathsForType.get(0)); + } + + return pathsForType.get(0); } @Override @@ -988,65 +1005,65 @@ public class QueryStack { // Note: the first element in each chain is assumed to be discrete. This may need to change when we add proper support for `_contained` if (nextChain.size() == 1) { // discrete -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath()), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath()), leafNodes); // discrete -> contained updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParameterName())) .collect(Collectors.toSet())); } else if (nextChain.size() == 2) { // discrete -> discrete -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath()), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath(), nextChain.get(1).getPath()), leafNodes); // discrete -> discrete -> contained - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath()), + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath()), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(1).getResourceType(), nextChain.get(1).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(1).getResourceType(), nextChain.get(1).getSearchParameterName())) .collect(Collectors.toSet())); // discrete -> contained -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath())), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getPath(), nextChain.get(1).getPath())), leafNodes); if (myModelConfig.isIndexOnContainedResourcesRecursively()) { // discrete -> contained -> contained updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParam().getName() + "." + nextChain.get(1).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParameterName() + "." + nextChain.get(1).getSearchParameterName())) .collect(Collectors.toSet())); } } else if (nextChain.size() == 3) { // discrete -> discrete -> discrete -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath(), nextChain.get(2).getSearchParam().getPath()), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath(), nextChain.get(1).getPath(), nextChain.get(2).getPath()), leafNodes); // discrete -> discrete -> discrete -> contained - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath()), + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath(), nextChain.get(1).getPath()), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(2).getResourceType(), nextChain.get(2).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(2).getResourceType(), nextChain.get(2).getSearchParameterName())) .collect(Collectors.toSet())); // discrete -> discrete -> contained -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath(), mergePaths(nextChain.get(1).getSearchParam().getPath(), nextChain.get(2).getSearchParam().getPath())), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath(), mergePaths(nextChain.get(1).getPath(), nextChain.get(2).getPath())), leafNodes); // discrete -> contained -> discrete -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath()), nextChain.get(2).getSearchParam().getPath()), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getPath(), nextChain.get(1).getPath()), nextChain.get(2).getPath()), leafNodes); // discrete -> contained -> discrete -> contained - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath())), + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getPath(), nextChain.get(1).getPath())), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(2).getResourceType(), nextChain.get(2).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(2).getResourceType(), nextChain.get(2).getSearchParameterName())) .collect(Collectors.toSet())); if (myModelConfig.isIndexOnContainedResourcesRecursively()) { // discrete -> contained -> contained -> discrete - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getSearchParam().getPath(), nextChain.get(1).getSearchParam().getPath(), nextChain.get(2).getSearchParam().getPath())), leafNodes); + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(mergePaths(nextChain.get(0).getPath(), nextChain.get(1).getPath(), nextChain.get(2).getPath())), leafNodes); // discrete -> discrete -> contained -> contained - updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getSearchParam().getPath()), + updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(nextChain.get(0).getPath()), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(1).getResourceType(), nextChain.get(1).getSearchParam().getName() + "." + nextChain.get(2).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(1).getResourceType(), nextChain.get(1).getSearchParameterName() + "." + nextChain.get(2).getSearchParameterName())) .collect(Collectors.toSet())); // discrete -> contained -> contained -> contained updateMapOfReferenceLinks(referenceLinks, Lists.newArrayList(), leafNodes .stream() - .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParam().getName() + "." + nextChain.get(1).getSearchParam().getName() + "." + nextChain.get(2).getSearchParam().getName())) + .map(t -> t.withPathPrefix(nextChain.get(0).getResourceType(), nextChain.get(0).getSearchParameterName() + "." + nextChain.get(1).getSearchParameterName() + "." + nextChain.get(2).getSearchParameterName())) .collect(Collectors.toSet())); } } else { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java index 69a825ec837..51b363c1957 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java @@ -12,11 +12,14 @@ import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Device; +import org.hl7.fhir.r4.model.Encounter; import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Location; 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.Quantity; +import org.hl7.fhir.r4.model.Reference; import org.hl7.fhir.r4.model.StringType; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -130,6 +133,50 @@ public class ChainingR4SearchTest extends BaseJpaR4Test { assertThat(oids, contains(oid1.getIdPart())); } + @Test + public void testShouldResolveATwoLinkChainWithStandAloneResources_CommonReference() throws Exception { + + // setup + myModelConfig.setIndexOnContainedResources(true); + + IIdType oid1; + + { + Patient p = new Patient(); + p.setId(IdType.newRandomUuid()); + p.addName().setFamily("Smith").addGiven("John"); + myPatientDao.create(p, mySrd); + + Encounter encounter = new Encounter(); + encounter.setId(IdType.newRandomUuid()); + encounter.addIdentifier().setSystem("foo").setValue("bar"); + myEncounterDao.create(encounter, mySrd); + + Observation obs = new Observation(); + obs.getCode().setText("Body Weight"); + obs.getCode().addCoding().setCode("obs2").setSystem("Some System").setDisplay("Body weight as measured by me"); + obs.setStatus(Observation.ObservationStatus.FINAL); + obs.setValue(new Quantity(81)); + obs.setSubject(new Reference(p.getId())); + obs.setEncounter(new Reference(encounter.getId())); + oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + + // Create a dummy record so that an unconstrained query doesn't pass the test due to returning the only record + myObservationDao.create(new Observation(), mySrd); + } + + String url = "/Observation?encounter.identifier=foo|bar"; + + // execute + myCaptureQueriesListener.clear(); + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + myCaptureQueriesListener.logSelectQueries(); + + // validate + assertEquals(1L, oids.size()); + assertThat(oids, contains(oid1.getIdPart())); + } + @Test public void testShouldResolveATwoLinkChainWithAContainedResource() throws Exception { // setup @@ -330,6 +377,46 @@ public class ChainingR4SearchTest extends BaseJpaR4Test { assertThat(oids, contains(oid1.getIdPart())); } + @Test + public void testShouldResolveATwoLinkChainWithAContainedResource_CommonReference() throws Exception { + + // setup + myModelConfig.setIndexOnContainedResources(true); + + IIdType oid1; + + { + Encounter encounter = new Encounter(); + encounter.setId("enc"); + encounter.addIdentifier().setSystem("foo").setValue("bar"); + + Observation obs = new Observation(); + obs.getCode().setText("Body Weight"); + obs.getCode().addCoding().setCode("obs2").setSystem("Some System").setDisplay("Body weight as measured by me"); + obs.setStatus(Observation.ObservationStatus.FINAL); + obs.setValue(new Quantity(81)); + + obs.addContained(encounter); + obs.setEncounter(new Reference("#enc")); + + oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + + // Create a dummy record so that an unconstrained query doesn't pass the test due to returning the only record + myObservationDao.create(new Observation(), mySrd); + } + + String url = "/Observation?encounter.identifier=foo|bar"; + + // execute + myCaptureQueriesListener.clear(); + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + myCaptureQueriesListener.logSelectQueries(); + + // validate + assertEquals(1L, oids.size()); + assertThat(oids, contains(oid1.getIdPart())); + } + @Test public void testShouldResolveAThreeLinkChainWhereAllResourcesStandAloneWithoutContainedResourceIndexing() throws Exception { @@ -477,6 +564,51 @@ public class ChainingR4SearchTest extends BaseJpaR4Test { assertThat(oids, contains(oid1.getIdPart())); } + @Test + public void testShouldResolveAThreeLinkChainWithAContainedResourceAtTheEndOfTheChain_CommonReference() throws Exception { + + // setup + myModelConfig.setIndexOnContainedResources(true); + + IIdType oid1; + + { + Patient p = new Patient(); + p.setId("pat"); + p.addName().setFamily("Smith").addGiven("John"); + + Encounter encounter = new Encounter(); + encounter.addContained(p); + encounter.setId(IdType.newRandomUuid()); + encounter.addIdentifier().setSystem("foo").setValue("bar"); + encounter.setSubject(new Reference("#pat")); + myEncounterDao.create(encounter, mySrd); + + Observation obs = new Observation(); + obs.getCode().setText("Body Weight"); + obs.getCode().addCoding().setCode("obs2").setSystem("Some System").setDisplay("Body weight as measured by me"); + obs.setStatus(Observation.ObservationStatus.FINAL); + obs.setValue(new Quantity(81)); + obs.setSubject(new Reference(p.getId())); + obs.setEncounter(new Reference(encounter.getId())); + oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + + // Create a dummy record so that an unconstrained query doesn't pass the test due to returning the only record + myObservationDao.create(new Observation(), mySrd); + } + + String url = "/Observation?encounter.patient.name=Smith"; + + // execute + myCaptureQueriesListener.clear(); + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + myCaptureQueriesListener.logSelectQueries(); + + // validate + assertEquals(1L, oids.size()); + assertThat(oids, contains(oid1.getIdPart())); + } + @Test public void testShouldResolveAThreeLinkChainWithAContainedResourceAtTheBeginningOfTheChain() throws Exception { // Adding support for this case in SMILE-3151 @@ -518,6 +650,50 @@ public class ChainingR4SearchTest extends BaseJpaR4Test { assertThat(oids, contains(oid1.getIdPart())); } + @Test + public void testShouldResolveAThreeLinkChainWithAContainedResourceAtTheBeginningOfTheChain_CommonReference() throws Exception { + + // setup + myModelConfig.setIndexOnContainedResources(true); + + IIdType oid1; + + { + Patient p = new Patient(); + p.setId(IdType.newRandomUuid()); + p.addName().setFamily("Smith").addGiven("John"); + myPatientDao.create(p, mySrd); + + Encounter encounter = new Encounter(); + encounter.setId("enc"); + encounter.addIdentifier().setSystem("foo").setValue("bar"); + encounter.setSubject(new Reference(p.getId())); + + Observation obs = new Observation(); + obs.addContained(encounter); + obs.getCode().setText("Body Weight"); + obs.getCode().addCoding().setCode("obs2").setSystem("Some System").setDisplay("Body weight as measured by me"); + obs.setStatus(Observation.ObservationStatus.FINAL); + obs.setValue(new Quantity(81)); + obs.setEncounter(new Reference("#enc")); + oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + + // Create a dummy record so that an unconstrained query doesn't pass the test due to returning the only record + myObservationDao.create(new Observation(), mySrd); + } + + String url = "/Observation?encounter.identifier=foo|bar"; + + // execute + myCaptureQueriesListener.clear(); + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + myCaptureQueriesListener.logSelectQueries(); + + // validate + assertEquals(1L, oids.size()); + assertThat(oids, contains(oid1.getIdPart())); + } + @Test public void testShouldNotResolveAThreeLinkChainWithAllContainedResourcesWhenRecursiveContainedIndexesAreDisabled() throws Exception {