From 05bc20819242b064ae29b55fcde44179a7a4fe9b Mon Sep 17 00:00:00 2001 From: Tadgh Date: Wed, 30 Mar 2022 08:40:17 -0700 Subject: [PATCH] Fix for path parsing in STU3 SPs (#3503) --- ...3469-stu3-fhirpath-validation-failure.yaml | 4 ++ .../FhirResourceDaoSearchParameterDstu3.java | 4 +- .../r4/FhirResourceDaoSearchParameterR4.java | 2 - ...ceDaoDstu3SearchCustomSearchParamTest.java | 31 ++++++++++++ .../dao/dstu3/FhirResourceDaoDstu3Test.java | 1 - .../extractor/BaseSearchParamExtractor.java | 47 +++++++++++++++++-- .../SearchParamExtractorDstu3Test.java | 21 +++++++++ .../fhir/dstu3/utils/FhirPathEngineTest.java | 1 + 8 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_0_0/3469-stu3-fhirpath-validation-failure.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_0_0/3469-stu3-fhirpath-validation-failure.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_0_0/3469-stu3-fhirpath-validation-failure.yaml new file mode 100644 index 00000000000..3e024941e65 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_0_0/3469-stu3-fhirpath-validation-failure.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 3469 +title: "Previously, `or` expressions were not being properly validated in FHIRPath in STU3 due to a bug with expression path splitting. This has been corrected." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSearchParameterDstu3.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSearchParameterDstu3.java index 505d8519496..2bf9c02e1fd 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSearchParameterDstu3.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoSearchParameterDstu3.java @@ -64,8 +64,10 @@ public class FhirResourceDaoSearchParameterDstu3 extends BaseHapiFhirResourceDao protected void validateResourceForStorage(SearchParameter theResource, ResourceTable theEntityToSave) { super.validateResourceForStorage(theResource, theEntityToSave); + org.hl7.fhir.r4.model.SearchParameter resource = (org.hl7.fhir.r4.model.SearchParameter) VersionConvertorFactory_30_40.convertResource(theResource, new BaseAdvisor_30_40(false)); + FhirResourceDaoSearchParameterR4.validateSearchParam( - (org.hl7.fhir.r4.model.SearchParameter) VersionConvertorFactory_30_40.convertResource(theResource, new BaseAdvisor_30_40(false)), + resource, getContext(), getConfig(), mySearchParamRegistry, mySearchParamExtractor); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java index 1c18af9ba26..3ea864dbab1 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java @@ -174,9 +174,7 @@ public class FhirResourceDaoSearchParameterR4 extends BaseHapiFhirResourceDao COORDS_INDEX_PATHS; - private static final Pattern SPLIT = Pattern.compile("\\||( or )"); private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(BaseSearchParamExtractor.class); static { @@ -1119,10 +1117,53 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor if (!thePaths.contains("|") && !thePaths.contains(" or ")) { return new String[]{thePaths}; } - return SPLIT.split(thePaths); + return splitOutOfParensOrs(thePaths); } } + /** + * Iteratively splits a string on any ` or ` or | that is ** not** contained inside a set of parentheses. e.g. + * + * "Patient.select(a or b)" --> ["Patient.select(a or b)"] + * "Patient.select(a or b) or Patient.select(c or d )" --> ["Patient.select(a or b)", "Patient.select(c or d)"] + * "Patient.select(a|b) or Patient.select(c or d )" --> ["Patient.select(a|b)", "Patient.select(c or d)"] + * "Patient.select(b) | Patient.select(c)" --> ["Patient.select(b)", "Patient.select(c)"] + * + * @param thePaths The string to split + * @return The split string + + */ + private String[] splitOutOfParensOrs(String thePaths) { + List topLevelOrExpressions = splitOutOfParensToken(thePaths, " or "); + List retVal = topLevelOrExpressions.stream() + .flatMap(s -> splitOutOfParensToken(s, "|").stream()) + .collect(Collectors.toList()); + return retVal.toArray(new String[retVal.size()]); + } + + private List splitOutOfParensToken(String thePath, String theToken) { + int tokenLength = theToken.length(); + int index = thePath.indexOf(theToken); + int rightIndex = 0; + List retVal = new ArrayList<>(); + while (index > -1 ) { + String left = thePath.substring(rightIndex, index); + if (allParensHaveBeenClosed(left)) { + retVal.add(left); + rightIndex = index + tokenLength; + } + index = thePath.indexOf(theToken, index + tokenLength); + } + retVal.add(thePath.substring(rightIndex)); + return retVal; + } + + private boolean allParensHaveBeenClosed(String thePaths) { + int open = StringUtils.countMatches(thePaths, "("); + int close = StringUtils.countMatches(thePaths, ")"); + return open == close; + } + private BigDecimal normalizeQuantityContainingTimeUnitsIntoDaysForNumberParam(String theSystem, String theCode, BigDecimal theValue) { if (SearchParamConstants.UCUM_NS.equals(theSystem)) { if (isNotBlank(theCode)) { diff --git a/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/extractor/SearchParamExtractorDstu3Test.java b/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/extractor/SearchParamExtractorDstu3Test.java index 2465d6c90d1..d971ab91d24 100644 --- a/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/extractor/SearchParamExtractorDstu3Test.java +++ b/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/extractor/SearchParamExtractorDstu3Test.java @@ -43,6 +43,9 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; public class SearchParamExtractorDstu3Test { @@ -105,6 +108,24 @@ public class SearchParamExtractorDstu3Test { assertEquals("2", params.iterator().next().getValue().toPlainString()); } + @Test + public void testPathSplitOnSpsWorks() { + ISearchParamRegistry searchParamRegistry = new MySearchParamRegistry(); + SearchParamExtractorDstu3 extractor = new SearchParamExtractorDstu3(new ModelConfig(), new PartitionSettings(), ourCtx, searchParamRegistry); + String threeSegmentPath = "Patient.telecom.where(system='phone' or system='email') | Patient.telecom.where(system='email') or Patient.telecom.where(system='mail' | system='phone')"; + + String[] expressions = extractor.split(threeSegmentPath); + assertThat(expressions.length, is(equalTo(3))); + assertThat(expressions[0], containsString("Patient.telecom.where(system='phone' or system='email')")); + assertThat(expressions[1], containsString("Patient.telecom.where(system='email')")); + assertThat(expressions[2], containsString("Patient.telecom.where(system='mail' | system='phone')")); + + String zeroPathSplit = "Patient.telecom.where(system='phone' or system='email')"; + String[] singularExpression = extractor.split(zeroPathSplit); + assertThat(singularExpression.length, is(equalTo(1))); + assertThat(singularExpression[0], containsString("Patient.telecom.where(system='phone' or system='email')")); + } + @Test public void testEncounterDuration_NotNormalized() { diff --git a/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/utils/FhirPathEngineTest.java b/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/utils/FhirPathEngineTest.java index 402ba30c6df..d524e0387b3 100644 --- a/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/utils/FhirPathEngineTest.java +++ b/hapi-fhir-validation/src/test/java/org/hl7/fhir/dstu3/utils/FhirPathEngineTest.java @@ -6,6 +6,7 @@ import ca.uhn.fhir.util.TestUtil; import org.hl7.fhir.dstu3.hapi.ctx.HapiWorkerContext; import org.hl7.fhir.dstu3.model.Base; import org.hl7.fhir.dstu3.model.BooleanType; +import org.hl7.fhir.dstu3.model.ContactPoint; import org.hl7.fhir.dstu3.model.DateTimeType; import org.hl7.fhir.dstu3.model.Observation; import org.hl7.fhir.dstu3.model.Patient;