Improve ValueSet filtering (#2162)

* Improve filter search

* Filter improvements

* Tests passing

* Test fixes

* Fix transaction filter

* Add changelog

* Test fix

* Update hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties

Co-authored-by: Diederik Muylwyk <[email protected]>

* Update hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/BaseTermReadSvcImpl.java

Co-authored-by: Diederik Muylwyk <[email protected]>

* Resolve FIXME

* Test fix

* Update hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionR4Test.java

Co-authored-by: Diederik Muylwyk <[email protected]>

Co-authored-by: Diederik Muylwyk <[email protected]>

Author: jamesagnew

hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties

@@ -1,5 +1,8 @@
 
 ca.uhn.fhir.jpa.term.BaseTermReadSvcImpl.expansionRefersToUnknownCs=Unknown CodeSystem URI "{0}" referenced from ValueSet
+ca.uhn.fhir.jpa.term.BaseTermReadSvcImpl.valueSetNotYetExpanded=ValueSet "{0}" has not yet been pre-expanded. Performing in-memory expansion without parameters. Current status: {1} | {2}
+ca.uhn.fhir.jpa.term.BaseTermReadSvcImpl.valueSetNotYetExpanded_OffsetNotAllowed=ValueSet expansion can not combine "offset" with "ValueSet.compose.exclude" unless the ValueSet has been pre-expanded. ValueSet "{0}" must be pre-expanded for this operation to work.
+
 
 # Core Library Messages
 ca.uhn.fhir.context.FhirContext.unknownResourceName=Unknown resource name "{0}" (this name is not known in FHIR version "{1}")

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2162-improve-vs-expansion-filtering.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 2162
+title: "When expanding a pre-expanded ValueSet using a filter, the filter was ignored and the pre-expansion was not used
+   resulting in an inefficient and potentially incorrect expansion. This has been corrected."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/ITermValueSetConceptViewDao.java

@@ -1,6 +1,8 @@
 package ca.uhn.fhir.jpa.dao.data;
 
 import ca.uhn.fhir.jpa.entity.TermValueSetConceptView;
+import org.springframework.data.domain.PageRequest;
+import org.springframework.data.domain.Pageable;
 import org.springframework.data.jpa.repository.JpaRepository;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
@@ -32,4 +34,7 @@ public interface ITermValueSetConceptViewDao extends JpaRepository<TermValueSetC
 	@Query("SELECT v FROM TermValueSetConceptView v WHERE v.myConceptValueSetPid = :pid AND v.myConceptOrder >= :from AND v.myConceptOrder < :to ORDER BY v.myConceptOrder")
 	List<TermValueSetConceptView> findByTermValueSetId(@Param("from") int theFrom, @Param("to") int theTo, @Param("pid") Long theValueSetId);
 
+	@Query("SELECT v FROM TermValueSetConceptView v WHERE v.myConceptValueSetPid = :pid AND v.myConceptDisplay LIKE :display ORDER BY v.myConceptOrder")
+	List<TermValueSetConceptView> findByTermValueSetId(@Param("pid") Long theValueSetId, @Param("display") String theDisplay);
+
 }

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

@@ -27,6 +27,7 @@
 import ca.uhn.fhir.jpa.dao.BaseHapiFhirResourceDao;
 import ca.uhn.fhir.jpa.model.cross.IBasePersistedResource;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
+import ca.uhn.fhir.jpa.model.util.JpaConstants;
 import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
@@ -221,7 +222,7 @@ private void applyFilter(IntegerType theTotalElement, List<ValueSetExpansionCont
 	private void addFilterIfPresent(String theFilter, ConceptSetComponent include) {
 		if (ElementUtil.isEmpty(include.getConcept())) {
 			if (isNotBlank(theFilter)) {
-				include.addFilter().setProperty("display").setOp(FilterOperator.EQUAL).setValue(theFilter);
+				include.addFilter().setProperty(JpaConstants.VALUESET_FILTER_DISPLAY).setOp(FilterOperator.EQUAL).setValue(theFilter);
 			}
 		}
 	}

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/tx/HapiTransactionService.java

@@ -94,7 +94,6 @@ public <T> T execute(RequestDetails theRequestDetails, TransactionCallback<T> th
 			}
 		}
 
-
 	}
 
 	/**

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java

@@ -37,8 +37,6 @@
 import ca.uhn.fhir.jpa.entity.SearchInclude;
 import ca.uhn.fhir.jpa.entity.SearchTypeEnum;
 import ca.uhn.fhir.jpa.interceptor.JpaPreResourceAccessDetails;
-import ca.uhn.fhir.model.api.IQueryParameterType;
-import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
 import ca.uhn.fhir.jpa.model.search.SearchRuntimeDetails;
 import ca.uhn.fhir.jpa.model.search.SearchStatusEnum;
 import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
@@ -47,6 +45,7 @@
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.util.InterceptorUtil;
 import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster;
+import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.model.api.Include;
 import ca.uhn.fhir.rest.api.CacheControlDirective;
 import ca.uhn.fhir.rest.api.Constants;
@@ -468,7 +467,7 @@ private IBundleProvider executeQuery(String theResourceType, SearchParameterMap
 		TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager);
 		txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRED);
 		return txTemplate.execute(t -> {
-				
+
 			// Load the results synchronously
 			final List<ResourcePersistentId> pids = new ArrayList<>();
 
@@ -668,6 +667,12 @@ public void setRequestPartitionHelperService(IRequestPartitionHelperSvc theReque
 		myRequestPartitionHelperService = theRequestPartitionHelperService;
 	}
 
+	private boolean isWantCount(SearchParameterMap myParams, boolean wantOnlyCount) {
+		return wantOnlyCount ||
+			SearchTotalModeEnum.ACCURATE.equals(myParams.getSearchTotalMode()) ||
+			(myParams.getSearchTotalMode() == null && SearchTotalModeEnum.ACCURATE.equals(myDaoConfig.getDefaultTotalMode()));
+	}
+
 	/**
 	 * A search task is a Callable task that runs in
 	 * a thread pool to handle an individual search. One instance
@@ -691,6 +696,8 @@ public class SearchTask implements Callable<Void> {
 		private final ArrayList<ResourcePersistentId> myUnsyncedPids = new ArrayList<>();
 		private final RequestDetails myRequest;
 		private final RequestPartitionId myRequestPartitionId;
+		private final SearchRuntimeDetails mySearchRuntimeDetails;
+		private final Transaction myParentTransaction;
 		private Search mySearch;
 		private boolean myAbortRequested;
 		private int myCountSavedTotal = 0;
@@ -699,8 +706,6 @@ public class SearchTask implements Callable<Void> {
 		private boolean myAdditionalPrefetchThresholdsRemaining;
 		private List<ResourcePersistentId> myPreviouslyAddedResourcePids;
 		private Integer myMaxResultsToFetch;
-		private final SearchRuntimeDetails mySearchRuntimeDetails;
-		private final Transaction myParentTransaction;
 
 		/**
 		 * Constructor
@@ -1193,17 +1198,6 @@ protected void doInTransactionWithoutResult(@Nonnull TransactionStatus theArg0)
 		}
 	}
 
-	private boolean isWantCount(SearchParameterMap myParams, boolean wantOnlyCount) {
-		return wantOnlyCount ||
-			SearchTotalModeEnum.ACCURATE.equals(myParams.getSearchTotalMode()) ||
-			(myParams.getSearchTotalMode() == null && SearchTotalModeEnum.ACCURATE.equals(myDaoConfig.getDefaultTotalMode()));
-	}
-
-	private static boolean isWantOnlyCount(SearchParameterMap myParams) {
-		return SummaryEnum.COUNT.equals(myParams.getSummaryMode())
-			| INTEGER_0.equals(myParams.getCount());
-	}
-
 	public class SearchContinuationTask extends SearchTask {
 
 		public SearchContinuationTask(Search theSearch, IDao theCallingDao, SearchParameterMap theParams, String theResourceType, RequestDetails theRequest, RequestPartitionId theRequestPartitionId) {
@@ -1242,6 +1236,10 @@ public Void call() {
 
 	}
 
+	private static boolean isWantOnlyCount(SearchParameterMap myParams) {
+		return SummaryEnum.COUNT.equals(myParams.getSummaryMode())
+			| INTEGER_0.equals(myParams.getCount());
+	}
 
 	public static void populateSearchEntity(SearchParameterMap theParams, String theResourceType, String theSearchUuid, String theQueryString, Search theSearch) {
 		theSearch.setDeleted(false);
@@ -1270,8 +1268,8 @@ public static void populateSearchEntity(SearchParameterMap theParams, String the
 	 * Creates a {@link Pageable} using a start and end index
 	 */
 	@SuppressWarnings("WeakerAccess")
-	public static @Nullable
-	Pageable toPage(final int theFromIndex, int theToIndex) {
+	@Nullable
+	public static Pageable toPage(final int theFromIndex, int theToIndex) {
 		int pageSize = theToIndex - theFromIndex;
 		if (pageSize < 1) {
 			return null;

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/UriPredicateBuilder.java

@@ -77,6 +77,7 @@ public UriPredicateBuilder(SearchQueryBuilder theSearchSqlBuilder) {
 	public Condition addPredicate(List<? extends IQueryParameterType> theUriOrParameterList, String theParamName, SearchFilterParser.CompareOperation theOperation, RequestDetails theRequestDetails) {
 
 		List<Condition> codePredicates = new ArrayList<>();
+		boolean predicateIsHash = false;
 		for (IQueryParameterType nextOr : theUriOrParameterList) {
 
 			if (nextOr instanceof UriParam) {
@@ -141,8 +142,8 @@ public Condition addPredicate(List<? extends IQueryParameterType> theUriOrParame
 					Condition uriPredicate = null;
 					if (theOperation == null || theOperation == SearchFilterParser.CompareOperation.eq) {
 						long hashUri = ResourceIndexedSearchParamUri.calculateHashUri(getPartitionSettings(), getRequestPartitionId(), getResourceType(), theParamName, value);
-						Condition hashPredicate = BinaryCondition.equalTo(myColumnHashUri, generatePlaceholder(hashUri));
-						codePredicates.add(hashPredicate);
+						uriPredicate = BinaryCondition.equalTo(myColumnHashUri, generatePlaceholder(hashUri));
+						predicateIsHash = true;
 					} else if (theOperation == SearchFilterParser.CompareOperation.ne) {
 						uriPredicate = BinaryCondition.notEqualTo(myColumnUri, generatePlaceholder(value));
 					} else if (theOperation == SearchFilterParser.CompareOperation.co) {
@@ -164,11 +165,7 @@ public Condition addPredicate(List<? extends IQueryParameterType> theUriOrParame
 							theOperation.toString()));
 					}
 
-					if (uriPredicate != null) {
-						long hashIdentity = BaseResourceIndexedSearchParam.calculateHashIdentity(getPartitionSettings(), getRequestPartitionId(), getResourceType(), theParamName);
-						BinaryCondition hashIdentityPredicate = BinaryCondition.equalTo(getColumnHashIdentity(), generatePlaceholder(hashIdentity));
-						codePredicates.add(ComboCondition.and(hashIdentityPredicate, uriPredicate));
-					}
+					codePredicates.add(uriPredicate);
 				}
 
 			} else {
@@ -186,8 +183,11 @@ public Condition addPredicate(List<? extends IQueryParameterType> theUriOrParame
 		}
 
 		ComboCondition orPredicate = ComboCondition.or(codePredicates.toArray(new Condition[0]));
-		Condition outerPredicate = combineWithHashIdentityPredicate(getResourceType(), theParamName, orPredicate);
-		return outerPredicate;
+		if (predicateIsHash) {
+			return orPredicate;
+		} else {
+			return combineWithHashIdentityPredicate(getResourceType(), theParamName, orPredicate);
+		}
 
 	}