From 26ef32c1ddbe4cd5da9c0f6f4e7239fab74a4219 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 14 Oct 2022 16:46:18 -0400 Subject: [PATCH] Fix 2 issues with has reverse chaining (#4139) * Fix 2 issues with chains * Add changelog * Cleanup * Add comment * Cleanup --- .../6_2_0/4139-reverse-chaining-fixes.yaml | 6 ++ .../fhir/jpa/search/builder/QueryStack.java | 45 ++++++---- .../provider/r4/ResourceProviderR4Test.java | 82 +++++++++++++++++++ .../test/utilities/RestServerR4Helper.java | 10 ++- 4 files changed, 122 insertions(+), 21 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4139-reverse-chaining-fixes.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4139-reverse-chaining-fixes.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4139-reverse-chaining-fixes.yaml new file mode 100644 index 00000000000..8b3c2a1f694 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4139-reverse-chaining-fixes.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 4139 +title: "Two issues with reverse chaining (i.e. the `_has` search parameter) have been addressed: + * Searching for a reverse chain with a target search parameter of `_id` did not work correctly, e.g. `Patient?_has:Observation:subject:_id=Patient/123` + * Searching with a combination of a forward chain and a reverse chain did not work correctly if indexing contained resources was enabled, e.g. `Observation?subject._has:Group:member:_id=Group/123`" 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 f53bb7f50a8..2578e507033 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 @@ -128,6 +128,8 @@ import static ca.uhn.fhir.jpa.util.QueryParameterUtils.toAndPredicate; import static ca.uhn.fhir.jpa.util.QueryParameterUtils.toEqualToOrInPredicate; import static ca.uhn.fhir.jpa.util.QueryParameterUtils.toOperation; import static ca.uhn.fhir.jpa.util.QueryParameterUtils.toOrPredicate; +import static ca.uhn.fhir.rest.api.Constants.PARAM_HAS; +import static ca.uhn.fhir.rest.api.Constants.PARAM_ID; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.apache.commons.lang3.StringUtils.split; @@ -256,7 +258,7 @@ public class QueryStack { TokenPredicateBuilder tokenPredicateBuilder = mySqlBuilder.createTokenPredicateBuilder(); Condition hashIdentityPredicate = tokenPredicateBuilder.createHashIdentityPredicate(theResourceName, theParamName); - + addSortCustomJoin(firstPredicateBuilder, tokenPredicateBuilder, hashIdentityPredicate); mySqlBuilder.addSortString(tokenPredicateBuilder.getColumnSystem(), theAscending); @@ -727,10 +729,16 @@ public class QueryStack { String qualifier = paramName.substring(4); for (IQueryParameterType next : nextOrList) { HasParam nextHasParam = new HasParam(); - nextHasParam.setValueAsQueryToken(myFhirContext, Constants.PARAM_HAS, qualifier, next.getValueAsQueryToken(myFhirContext)); + nextHasParam.setValueAsQueryToken(myFhirContext, PARAM_HAS, qualifier, next.getValueAsQueryToken(myFhirContext)); orValues.add(nextHasParam); } + } else if (paramName.equals(PARAM_ID)) { + + for (IQueryParameterType next : nextOrList) { + orValues.add(new TokenParam(next.getValueAsQueryToken(myFhirContext))); + } + } else { //Ensure that the name of the search param @@ -959,7 +967,7 @@ public class QueryStack { } public String getPath() { return myPath; } - + public String getSearchParameterName() { return mySearchParameterName; } @Override @@ -1006,7 +1014,6 @@ public class QueryStack { String targetValue = nextOr.getValueAsQueryToken(myFhirContext); if (nextOr instanceof ReferenceParam) { ReferenceParam referenceParam = (ReferenceParam) nextOr; - if (!isReferenceParamValid(referenceParam)) { throw new InvalidRequestException(Msg.code(2007) + "The search chain " + theSearchParam.getName() + "." + referenceParam.getChain() + @@ -1538,11 +1545,11 @@ public class QueryStack { String theSpnamePrefix, RuntimeSearchParam theSearchParam, List theList, SearchFilterParser.CompareOperation theOperation, RequestPartitionId theRequestPartitionId, SearchQueryBuilder theSqlBuilder) { - List tokens = new ArrayList<>(); - + List tokens = new ArrayList<>(); + boolean paramInverted = false; TokenParamModifier modifier; - + for (IQueryParameterType nextOr : theList) { if (nextOr instanceof TokenParam) { if (!((TokenParam) nextOr).isEmpty()) { @@ -1561,8 +1568,8 @@ public class QueryStack { throw new MethodNotAllowedException(Msg.code(1219) + msg); } return createPredicateString(theSourceJoinColumn, theResourceName, theSpnamePrefix, theSearchParam, theList, null, theRequestPartitionId, theSqlBuilder); - } - + } + modifier = id.getModifier(); // for :not modifier, create a token and remove the :not modifier if (modifier == TokenParamModifier.NOT) { @@ -1584,23 +1591,23 @@ public class QueryStack { String paramName = getParamNameWithPrefix(theSpnamePrefix, theSearchParam.getName()); Condition predicate; BaseJoiningPredicateBuilder join; - + if (paramInverted) { SearchQueryBuilder sqlBuilder = theSqlBuilder.newChildSqlBuilder(); TokenPredicateBuilder tokenSelector = sqlBuilder.addTokenPredicateBuilder(null); sqlBuilder.addPredicate(tokenSelector.createPredicateToken(tokens, theResourceName, theSpnamePrefix, theSearchParam, theRequestPartitionId)); SelectQuery sql = sqlBuilder.getSelect(); Expression subSelect = new Subquery(sql); - + join = theSqlBuilder.getOrCreateFirstPredicateBuilder(); - + if (theSourceJoinColumn == null) { predicate = new InCondition(join.getResourceIdColumn(), subSelect).setNegate(true); } else { //-- for the resource link, need join with target_resource_id predicate = new InCondition(theSourceJoinColumn, subSelect).setNegate(true); } - + } else { Boolean isMissing = theList.get(0).getMissing(); if (isMissing != null) { @@ -1620,9 +1627,9 @@ public class QueryStack { TokenPredicateBuilder tokenJoin = createOrReusePredicateBuilder(PredicateBuilderTypeEnum.TOKEN, theSourceJoinColumn, paramName, () -> theSqlBuilder.addTokenPredicateBuilder(theSourceJoinColumn)).getResult(); predicate = tokenJoin.createPredicateToken(tokens, theResourceName, theSpnamePrefix, theSearchParam, theOperation, theRequestPartitionId); - join = tokenJoin; - } - + join = tokenJoin; + } + return join.combineWithRequestPartitionIdPredicate(theRequestPartitionId, predicate); } @@ -1676,7 +1683,7 @@ public class QueryStack { case IAnyResource.SP_RES_ID: return createPredicateResourceId(theSourceJoinColumn, theAndOrParams, theResourceName, null, theRequestPartitionId); - case Constants.PARAM_HAS: + case PARAM_HAS: return createPredicateHas(theSourceJoinColumn, theResourceName, theAndOrParams, theRequest, theRequestPartitionId); case Constants.PARAM_TAG: @@ -1820,7 +1827,9 @@ public class QueryStack { nextAnd.stream() .filter(t -> t instanceof ReferenceParam) .map(t -> ((ReferenceParam) t).getChain()) - .anyMatch(StringUtils::isNotBlank); + .filter(StringUtils::isNotBlank) + // Chains on _has can't be indexed for contained searches - At least not yet. It's not clear to me if we ever want to support this, it would be really hard to do. + .anyMatch(t->!t.startsWith(PARAM_HAS + ":")); } public void addPredicateCompositeUnique(String theIndexString, RequestPartitionId theRequestPartitionId) { diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index 3075d945d8c..b8e7a32b8e0 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.jpa.api.config.DaoConfig; import ca.uhn.fhir.jpa.dao.data.ISearchDao; import ca.uhn.fhir.jpa.entity.Search; +import ca.uhn.fhir.jpa.model.entity.ModelConfig; import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel; import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; import ca.uhn.fhir.jpa.model.util.JpaConstants; @@ -70,6 +71,7 @@ import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; import org.apache.http.message.BasicNameValuePair; import org.apache.http.util.EntityUtils; +import org.apache.jena.rdf.model.ModelCon; import org.hamcrest.Matchers; import org.hl7.fhir.common.hapi.validation.validator.FhirInstanceValidator; import org.hl7.fhir.instance.model.api.IAnyResource; @@ -246,6 +248,8 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { myDaoConfig.setIndexMissingFields(new DaoConfig().getIndexMissingFields()); myDaoConfig.setAdvancedHSearchIndexing(new DaoConfig().isAdvancedHSearchIndexing()); + myModelConfig.setIndexOnContainedResources(new ModelConfig().isIndexOnContainedResources()); + mySearchCoordinatorSvcRaw.setLoadingThrottleForUnitTests(null); mySearchCoordinatorSvcRaw.setSyncSizeForUnitTests(QueryParameterUtils.DEFAULT_SYNC_SIZE); mySearchCoordinatorSvcRaw.setNeverUseLocalSearchForUnitTests(false); @@ -3136,6 +3140,84 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testHasParameterOnChain(boolean theWithIndexOnContainedResources) throws Exception { + myModelConfig.setIndexOnContainedResources(theWithIndexOnContainedResources); + + IIdType pid0; + IIdType pid1; + IIdType groupId; + IIdType obsId; + { + Patient patient = new Patient(); + patient.addIdentifier().setSystem("urn:system").setValue("001"); + patient.addName().setFamily("Tester").addGiven("Joe"); + pid0 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); + } + { + Patient patient = new Patient(); + patient.addIdentifier().setSystem("urn:system").setValue("001"); + patient.addName().setFamily("Tester").addGiven("Joe"); + pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); + } + { + Group group = new Group(); + group.addMember().setEntity(new Reference(pid0.getValue())); + group.addMember().setEntity(new Reference(pid1.getValue())); + groupId = myGroupDao.create(group, mySrd).getId().toUnqualifiedVersionless(); + } + { + Observation obs = new Observation(); + obs.getCode().addCoding().setSystem("urn:system").setCode("FOO"); + obs.getSubject().setReferenceElement(pid0); + obsId = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + } + + String uri; + List ids; + + logAllTokenIndexes(); + + uri = ourServerBase + "/Observation?code=urn:system%7CFOO&subject._has:Group:member:_id=" + groupId.getValue(); + myCaptureQueriesListener.clear(); + ids = searchAndReturnUnqualifiedVersionlessIdValues(uri); + myCaptureQueriesListener.logAllQueries(); + assertThat(ids, contains(obsId.getValue())); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testHasParameterWithIdTarget(boolean theWithIndexOnContainedResources) throws Exception { + myModelConfig.setIndexOnContainedResources(theWithIndexOnContainedResources); + + IIdType pid0; + IIdType obsId; + { + Patient patient = new Patient(); + patient.addIdentifier().setSystem("urn:system").setValue("001"); + patient.addName().setFamily("Tester").addGiven("Joe"); + pid0 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); + } + { + Observation obs = new Observation(); + obs.addIdentifier().setSystem("urn:system").setValue("FOO"); + obs.getSubject().setReferenceElement(pid0); + obsId = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); + } + + String uri; + List ids; + + logAllResourceLinks(); + + uri = ourServerBase + "/Patient?_has:Observation:subject:_id=" + obsId.getValue(); + myCaptureQueriesListener.clear(); + ids = searchAndReturnUnqualifiedVersionlessIdValues(uri); + myCaptureQueriesListener.logAllQueries(); + assertThat(ids, contains(pid0.getValue())); + } + @Test public void testHistoryWithAtParameter() throws Exception { String methodName = "testHistoryWithFromAndTo"; diff --git a/hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/RestServerR4Helper.java b/hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/RestServerR4Helper.java index 7064272c998..fa6facd64b4 100644 --- a/hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/RestServerR4Helper.java +++ b/hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/RestServerR4Helper.java @@ -33,6 +33,7 @@ import ca.uhn.fhir.rest.server.RestfulServer; import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException; import ca.uhn.fhir.rest.server.provider.HashMapResourceProvider; import org.eclipse.jetty.server.Request; +import org.hl7.fhir.instance.model.api.IBaseBundle; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Bundle; @@ -89,9 +90,12 @@ public class RestServerR4Helper extends BaseRestServerHelper implements BeforeEa } public List getTransactions() { - return myRestServer - .getPlainProvider() - .getTransactions() + List transactions = myRestServer.getPlainProvider().getTransactions(); + + // Make a copy to avoid synchronization issues + transactions = new ArrayList<>(transactions); + + return transactions .stream() .map(t -> (Bundle) t) .collect(Collectors.toList());