Fix for path parsing in STU3 SPs (#3503)

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."

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);
 	}
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java

@@ -174,9 +174,7 @@ public class FhirResourceDaoSearchParameterR4 extends BaseHapiFhirResourceDao<Se
 						}
 					}
 				}
-
 			} else {
-
 				if (!isUnique && theResource.getType() != Enumerations.SearchParamType.COMPOSITE && theResource.getType() != Enumerations.SearchParamType.SPECIAL && !REGEX_SP_EXPRESSION_HAS_PATH.matcher(expression).matches()) {
 					throw new UnprocessableEntityException(Msg.code(1120) + "SearchParameter.expression value \"" + expression + "\" is invalid");
 				}

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3SearchCustomSearchParamTest.java

@@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.dao.dstu3;
 
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.model.api.Include;
@@ -53,6 +54,8 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
@@ -87,6 +90,34 @@ public class FhirResourceDaoDstu3SearchCustomSearchParamTest extends BaseJpaDstu
 		}
 	}
 
+	@Test
+	public void testCreateSpWithMultiplePaths(){
+		SearchParameter sp = new SearchParameter();
+		sp.setCode("telephone-unformatted");
+		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
+		sp.setExpression("Patient.telecom.where(system='phone' or system='email')");
+		sp.getBase().add(new CodeType("Patient"));
+		sp.setType(Enumerations.SearchParamType.TOKEN);
+
+		DaoMethodOutcome daoMethodOutcome = mySearchParameterDao.create(sp);
+		assertThat(daoMethodOutcome.getId(), is(notNullValue()));
+
+		sp.setExpression("Patient.telecom.where(system='phone') or Patient.telecome.where(system='email')");
+		sp.setCode("telephone-unformatted-2");
+		daoMethodOutcome = mySearchParameterDao.create(sp);
+		assertThat(daoMethodOutcome.getId(), is(notNullValue()));
+
+		sp.setExpression("Patient.telecom.where(system='phone' or system='email') | Patient.telecome.where(system='email')");
+		sp.setCode("telephone-unformatted-3");
+		daoMethodOutcome = mySearchParameterDao.create(sp);
+		assertThat(daoMethodOutcome.getId(), is(notNullValue()));
+
+		sp.setExpression("Patient.telecom.where(system='phone' or system='email') | Patient.telecom.where(system='email') or Patient.telecom.where(system='mail' | system='phone')");
+		sp.setCode("telephone-unformatted-3");
+		daoMethodOutcome = mySearchParameterDao.create(sp);
+		assertThat(daoMethodOutcome.getId(), is(notNullValue()));
+	}
+
 	@Test
 	public void testCreateInvalidParamInvalidResourceName() {
 		SearchParameter fooSp = new SearchParameter();

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java

@@ -96,7 +96,6 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.Date;
 import java.util.List;
-import java.util.Set;
 
 import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.hamcrest.MatcherAssert.assertThat;

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/BaseSearchParamExtractor.java

@@ -85,7 +85,6 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
-import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 import static org.apache.commons.lang3.StringUtils.isBlank;
@@ -95,7 +94,6 @@ import static org.apache.commons.lang3.StringUtils.trim;
 public abstract class BaseSearchParamExtractor implements ISearchParamExtractor {
 
 	public static final Set<String> 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<String> topLevelOrExpressions = splitOutOfParensToken(thePaths, " or ");
+		List<String> retVal = topLevelOrExpressions.stream()
+			.flatMap(s -> splitOutOfParensToken(s, "|").stream())
+			.collect(Collectors.toList());
+		return retVal.toArray(new String[retVal.size()]);
+	}
+
+	private List<String> splitOutOfParensToken(String thePath, String theToken) {
+		int tokenLength = theToken.length();
+		int index = thePath.indexOf(theToken);
+		int rightIndex = 0;
+		List<String> 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)) {

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() {
 

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;