hibernate search cannot be used for count query if non-active paramet…

…ers are present (hapifhir#6027)

 fixing count query with fulltextsearch bug

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6024-full-index-search-with-filter-parameters.yaml

@@ -0,0 +1,10 @@
+---
+type: fix
+issue: 6024
+title: "Fixed a bug in search where requesting a count with HSearch indexing
+        and FilterParameter enabled and using the _filter parameter would result
+        in inaccurate results being returned.
+        This happened because the count query would use an incorrect set of parameters
+        to find the count, and the regular search when then try and ensure its results
+        matched the count query (which it couldn't because it had different parameters).
+"

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

@@ -34,6 +34,7 @@
 import ca.uhn.fhir.jpa.dao.search.LastNOperation;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
+import ca.uhn.fhir.jpa.model.search.ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams;
 import ca.uhn.fhir.jpa.model.search.ExtendedHSearchIndexData;
 import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage;
 import ca.uhn.fhir.jpa.search.autocomplete.ValueSetAutocompleteOptions;
@@ -141,17 +142,17 @@ public ExtendedHSearchIndexData extractLuceneIndexData(
 	}
 
 	@Override
-	public boolean supportsSomeOf(SearchParameterMap myParams) {
-
-		// keep this in sync with the guts of doSearch
+	public boolean canUseHibernateSearch(String theResourceType, SearchParameterMap myParams) {
 		boolean requiresHibernateSearchAccess = myParams.containsKey(Constants.PARAM_CONTENT)
 				|| myParams.containsKey(Constants.PARAM_TEXT)
 				|| myParams.isLastN();
+		// we have to use it - _text and _content searches only use hibernate
+		if (requiresHibernateSearchAccess) {
+			return true;
+		}
 
-		requiresHibernateSearchAccess |=
-				myStorageSettings.isAdvancedHSearchIndexing() && myAdvancedIndexQueryBuilder.isSupportsSomeOf(myParams);
-
-		return requiresHibernateSearchAccess;
+		return myStorageSettings.isAdvancedHSearchIndexing()
+				&& myAdvancedIndexQueryBuilder.canUseHibernateSearch(theResourceType, myParams, mySearchParamRegistry);
 	}
 
 	@Override
@@ -174,6 +175,7 @@ public ISearchQueryExecutor searchNotScrolled(
 	}
 
 	// keep this in sync with supportsSomeOf();
+	@SuppressWarnings("rawtypes")
 	private ISearchQueryExecutor doSearch(
 			String theResourceType,
 			SearchParameterMap theParams,
@@ -208,6 +210,7 @@ private int getMaxFetchSize(SearchParameterMap theParams, Integer theMax) {
 		return DEFAULT_MAX_NON_PAGED_SIZE;
 	}
 
+	@SuppressWarnings("rawtypes")
 	private SearchQueryOptionsStep<?, Long, SearchLoadingOptionsStep, ?, ?> getSearchQueryOptionsStep(
 			String theResourceType, SearchParameterMap theParams, IResourcePersistentId theReferencingPid) {
 
@@ -230,6 +233,7 @@ private int getMaxFetchSize(SearchParameterMap theParams, Integer theMax) {
 		return query;
 	}
 
+	@SuppressWarnings("rawtypes")
 	private PredicateFinalStep buildWhereClause(
 			SearchPredicateFactory f,
 			String theResourceType,
@@ -271,8 +275,12 @@ private PredicateFinalStep buildWhereClause(
 			 * Handle other supported parameters
 			 */
 			if (myStorageSettings.isAdvancedHSearchIndexing() && theParams.getEverythingMode() == null) {
-				myAdvancedIndexQueryBuilder.addAndConsumeAdvancedQueryClauses(
-						builder, theResourceType, theParams, mySearchParamRegistry);
+				ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams params =
+						new ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams();
+				params.setSearchParamRegistry(mySearchParamRegistry)
+						.setResourceType(theResourceType)
+						.setSearchParameterMap(theParams);
+				myAdvancedIndexQueryBuilder.addAndConsumeAdvancedQueryClauses(builder, params);
 			}
 			// DROP EARLY HERE IF BOOL IS EMPTY?
 		});
@@ -283,11 +291,13 @@ private SearchSession getSearchSession() {
 		return Search.session(myEntityManager);
 	}
 
+	@SuppressWarnings("rawtypes")
 	private List<IResourcePersistentId> convertLongsToResourcePersistentIds(List<Long> theLongPids) {
 		return theLongPids.stream().map(JpaPid::fromId).collect(Collectors.toList());
 	}
 
 	@Override
+	@SuppressWarnings({"rawtypes", "unchecked"})
 	public List<IResourcePersistentId> everything(
 			String theResourceName,
 			SearchParameterMap theParams,
@@ -336,6 +346,7 @@ public boolean isDisabled() {
 
 	@Transactional()
 	@Override
+	@SuppressWarnings("unchecked")
 	public List<IResourcePersistentId> search(
 			String theResourceName, SearchParameterMap theParams, RequestDetails theRequestDetails) {
 		validateHibernateSearchIsEnabled();
@@ -347,6 +358,7 @@ public List<IResourcePersistentId> search(
 	/**
 	 * Adapt our async interface to the legacy concrete List
 	 */
+	@SuppressWarnings("rawtypes")
 	private List<IResourcePersistentId> toList(ISearchQueryExecutor theSearchResultStream, long theMaxSize) {
 		return StreamSupport.stream(Spliterators.spliteratorUnknownSize(theSearchResultStream, 0), false)
 				.map(JpaPid::fromId)
@@ -384,6 +396,7 @@ private void ensureElastic() {
 	}
 
 	@Override
+	@SuppressWarnings("rawtypes")
 	public List<IResourcePersistentId> lastN(SearchParameterMap theParams, Integer theMaximumResults) {
 		ensureElastic();
 		dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH);

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

@@ -32,6 +32,7 @@
 import java.util.Collection;
 import java.util.List;
 
+@SuppressWarnings({"rawtypes"})
 public interface IFulltextSearchSvc {
 
 	/**
@@ -79,11 +80,18 @@ <T extends IResourcePersistentId> List<T> everything(
 	ExtendedHSearchIndexData extractLuceneIndexData(
 			IBaseResource theResource, ResourceIndexedSearchParams theNewParams);
 
-	boolean supportsSomeOf(SearchParameterMap myParams);
+	/**
+	 * Returns true if the parameter map can be handled for hibernate search.
+	 * We have to filter out any queries that might use search params
+	 * we only know how to handle in JPA.
+	 * -
+	 * See {@link ca.uhn.fhir.jpa.dao.search.ExtendedHSearchSearchBuilder#addAndConsumeAdvancedQueryClauses}
+	 */
+	boolean canUseHibernateSearch(String theResourceType, SearchParameterMap theParameterMap);
 
 	/**
 	 * Re-publish the resource to the full-text index.
-	 *
+	 * -
 	 * During update, hibernate search only republishes the entity if it has changed.
 	 * During $reindex, we want to force the re-index.
 	 *

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

@@ -20,6 +20,7 @@
 package ca.uhn.fhir.jpa.dao.search;
 
 import ca.uhn.fhir.context.RuntimeSearchParam;
+import ca.uhn.fhir.jpa.model.search.ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.util.JpaParamUtil;
 import ca.uhn.fhir.model.api.IQueryParameterType;
@@ -34,6 +35,7 @@
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.param.UriParam;
 import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
+import ca.uhn.fhir.rest.server.util.ResourceSearchParams;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import org.apache.commons.collections4.CollectionUtils;
@@ -44,6 +46,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import static ca.uhn.fhir.rest.api.Constants.PARAMQUALIFIER_MISSING;
 
@@ -58,18 +61,56 @@ public class ExtendedHSearchSearchBuilder {
 	 */
 	public static final Set<String> ourUnsafeSearchParmeters = Sets.newHashSet("_id", "_meta");
 
+	/**
+	 * Determine if ExtendedHibernateSearchBuilder can support this parameter
+	 * @param theParamName param name
+	 * @param theActiveParamsForResourceType active search parameters for the desired resource type
+	 * @return whether or not this search parameter is supported in hibernate
+	 */
+	public boolean supportsSearchParameter(String theParamName, ResourceSearchParams theActiveParamsForResourceType) {
+		if (theActiveParamsForResourceType == null) {
+			return false;
+		}
+		if (ourUnsafeSearchParmeters.contains(theParamName)) {
+			return false;
+		}
+		if (!theActiveParamsForResourceType.containsParamName(theParamName)) {
+			return false;
+		}
+		return true;
+	}
+
 	/**
 	 * Are any of the queries supported by our indexing?
+	 * -
+	 * If not, do not use hibernate, because the results will
+	 * be inaccurate and wrong.
 	 */
-	public boolean isSupportsSomeOf(SearchParameterMap myParams) {
-		return myParams.getSort() != null
-				|| myParams.getLastUpdated() != null
-				|| myParams.entrySet().stream()
-						.filter(e -> !ourUnsafeSearchParmeters.contains(e.getKey()))
-						// each and clause may have a different modifier, so split down to the ORs
-						.flatMap(andList -> andList.getValue().stream())
-						.flatMap(Collection::stream)
-						.anyMatch(this::isParamTypeSupported);
+	public boolean canUseHibernateSearch(
+			String theResourceType, SearchParameterMap myParams, ISearchParamRegistry theSearchParamRegistry) {
+		boolean canUseHibernate = true;
+		ResourceSearchParams resourceActiveSearchParams = theSearchParamRegistry.getActiveSearchParams(theResourceType);
+		for (String paramName : myParams.keySet()) {
+			// is this parameter supported?
+			if (!supportsSearchParameter(paramName, resourceActiveSearchParams)) {
+				canUseHibernate = false;
+			} else {
+				// are the parameter values supported?
+				canUseHibernate =
+						myParams.get(paramName).stream()
+								.flatMap(Collection::stream)
+								.collect(Collectors.toList())
+								.stream()
+								.anyMatch(this::isParamTypeSupported);
+			}
+
+			// if not supported, don't use
+			if (!canUseHibernate) {
+				return false;
+			}
+		}
+
+		return canUseHibernate;
 	}
 
 	/**
@@ -166,86 +207,91 @@ private boolean isParamTypeSupported(IQueryParameterType param) {
 	}
 
 	public void addAndConsumeAdvancedQueryClauses(
-			ExtendedHSearchClauseBuilder builder,
-			String theResourceType,
-			SearchParameterMap theParams,
-			ISearchParamRegistry theSearchParamRegistry) {
+			ExtendedHSearchClauseBuilder theBuilder,
+			ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams theMethodParams) {
+		SearchParameterMap searchParameterMap = theMethodParams.getSearchParameterMap();
+		String resourceType = theMethodParams.getResourceType();
+		ISearchParamRegistry searchParamRegistry = theMethodParams.getSearchParamRegistry();
+
 		// copy the keys to avoid concurrent modification error
-		ArrayList<String> paramNames = compileParamNames(theParams);
+		ArrayList<String> paramNames = compileParamNames(searchParameterMap);
+		ResourceSearchParams activeSearchParams = searchParamRegistry.getActiveSearchParams(resourceType);
 		for (String nextParam : paramNames) {
-			if (ourUnsafeSearchParmeters.contains(nextParam)) {
-				continue;
-			}
-			RuntimeSearchParam activeParam = theSearchParamRegistry.getActiveSearchParam(theResourceType, nextParam);
-			if (activeParam == null) {
+			if (!supportsSearchParameter(nextParam, activeSearchParams)) {
 				// ignore magic params handled in JPA
 				continue;
 			}
+			RuntimeSearchParam activeParam = activeSearchParams.get(nextParam);
 
 			// NOTE - keep this in sync with isParamSupported() above.
 			switch (activeParam.getParamType()) {
 				case TOKEN:
 					List<List<IQueryParameterType>> tokenTextAndOrTerms =
-							theParams.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_TOKEN_TEXT);
-					builder.addStringTextSearch(nextParam, tokenTextAndOrTerms);
+							searchParameterMap.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_TOKEN_TEXT);
+					theBuilder.addStringTextSearch(nextParam, tokenTextAndOrTerms);
 
 					List<List<IQueryParameterType>> tokenUnmodifiedAndOrTerms =
-							theParams.removeByNameUnmodified(nextParam);
-					builder.addTokenUnmodifiedSearch(nextParam, tokenUnmodifiedAndOrTerms);
+							searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addTokenUnmodifiedSearch(nextParam, tokenUnmodifiedAndOrTerms);
 					break;
 
 				case STRING:
 					List<List<IQueryParameterType>> stringTextAndOrTerms =
-							theParams.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_TOKEN_TEXT);
-					builder.addStringTextSearch(nextParam, stringTextAndOrTerms);
+							searchParameterMap.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_TOKEN_TEXT);
+					theBuilder.addStringTextSearch(nextParam, stringTextAndOrTerms);
 
-					List<List<IQueryParameterType>> stringExactAndOrTerms =
-							theParams.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_STRING_EXACT);
-					builder.addStringExactSearch(nextParam, stringExactAndOrTerms);
+					List<List<IQueryParameterType>> stringExactAndOrTerms = searchParameterMap.removeByNameAndModifier(
+							nextParam, Constants.PARAMQUALIFIER_STRING_EXACT);
+					theBuilder.addStringExactSearch(nextParam, stringExactAndOrTerms);
 
 					List<List<IQueryParameterType>> stringContainsAndOrTerms =
-							theParams.removeByNameAndModifier(nextParam, Constants.PARAMQUALIFIER_STRING_CONTAINS);
-					builder.addStringContainsSearch(nextParam, stringContainsAndOrTerms);
+							searchParameterMap.removeByNameAndModifier(
+									nextParam, Constants.PARAMQUALIFIER_STRING_CONTAINS);
+					theBuilder.addStringContainsSearch(nextParam, stringContainsAndOrTerms);
 
-					List<List<IQueryParameterType>> stringAndOrTerms = theParams.removeByNameUnmodified(nextParam);
-					builder.addStringUnmodifiedSearch(nextParam, stringAndOrTerms);
+					List<List<IQueryParameterType>> stringAndOrTerms =
+							searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addStringUnmodifiedSearch(nextParam, stringAndOrTerms);
 					break;
 
 				case QUANTITY:
-					List<List<IQueryParameterType>> quantityAndOrTerms = theParams.removeByNameUnmodified(nextParam);
-					builder.addQuantityUnmodifiedSearch(nextParam, quantityAndOrTerms);
+					List<List<IQueryParameterType>> quantityAndOrTerms =
+							searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addQuantityUnmodifiedSearch(nextParam, quantityAndOrTerms);
 					break;
 
 				case REFERENCE:
-					List<List<IQueryParameterType>> referenceAndOrTerms = theParams.removeByNameUnmodified(nextParam);
-					builder.addReferenceUnchainedSearch(nextParam, referenceAndOrTerms);
+					List<List<IQueryParameterType>> referenceAndOrTerms =
+							searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addReferenceUnchainedSearch(nextParam, referenceAndOrTerms);
 					break;
 
 				case DATE:
 					List<List<IQueryParameterType>> dateAndOrTerms = nextParam.equalsIgnoreCase("_lastupdated")
-							? getLastUpdatedAndOrList(theParams)
-							: theParams.removeByNameUnmodified(nextParam);
-					builder.addDateUnmodifiedSearch(nextParam, dateAndOrTerms);
+							? getLastUpdatedAndOrList(searchParameterMap)
+							: searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addDateUnmodifiedSearch(nextParam, dateAndOrTerms);
 					break;
 
 				case COMPOSITE:
-					List<List<IQueryParameterType>> compositeAndOrTerms = theParams.removeByNameUnmodified(nextParam);
+					List<List<IQueryParameterType>> compositeAndOrTerms =
+							searchParameterMap.removeByNameUnmodified(nextParam);
 					// RuntimeSearchParam only points to the subs by reference.  Resolve here while we have
 					// ISearchParamRegistry
 					List<RuntimeSearchParam> subSearchParams =
-							JpaParamUtil.resolveCompositeComponentsDeclaredOrder(theSearchParamRegistry, activeParam);
-					builder.addCompositeUnmodifiedSearch(activeParam, subSearchParams, compositeAndOrTerms);
+							JpaParamUtil.resolveCompositeComponentsDeclaredOrder(searchParamRegistry, activeParam);
+					theBuilder.addCompositeUnmodifiedSearch(activeParam, subSearchParams, compositeAndOrTerms);
 					break;
 
 				case URI:
 					List<List<IQueryParameterType>> uriUnmodifiedAndOrTerms =
-							theParams.removeByNameUnmodified(nextParam);
-					builder.addUriUnmodifiedSearch(nextParam, uriUnmodifiedAndOrTerms);
+							searchParameterMap.removeByNameUnmodified(nextParam);
+					theBuilder.addUriUnmodifiedSearch(nextParam, uriUnmodifiedAndOrTerms);
 					break;
 
 				case NUMBER:
-					List<List<IQueryParameterType>> numberUnmodifiedAndOrTerms = theParams.remove(nextParam);
-					builder.addNumberUnmodifiedSearch(nextParam, numberUnmodifiedAndOrTerms);
+					List<List<IQueryParameterType>> numberUnmodifiedAndOrTerms = searchParameterMap.remove(nextParam);
+					theBuilder.addNumberUnmodifiedSearch(nextParam, numberUnmodifiedAndOrTerms);
 					break;
 
 				default:

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

@@ -22,6 +22,7 @@
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.StorageSettings;
+import ca.uhn.fhir.jpa.model.search.ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.util.LastNParameterHelper;
 import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
@@ -67,8 +68,12 @@ public List<Long> executeLastN(SearchParameterMap theParams, Integer theMaximumR
 					b.must(f.match().field("myResourceType").matching(OBSERVATION_RES_TYPE));
 					ExtendedHSearchClauseBuilder builder =
 							new ExtendedHSearchClauseBuilder(myFhirContext, myStorageSettings, b, f);
-					myExtendedHSearchSearchBuilder.addAndConsumeAdvancedQueryClauses(
-							builder, OBSERVATION_RES_TYPE, theParams.clone(), mySearchParamRegistry);
+					ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams params =
+							new ExtendedHSearchBuilderConsumeAdvancedQueryClausesParams();
+					params.setResourceType(OBSERVATION_RES_TYPE)
+							.setSearchParameterMap(theParams.clone())
+							.setSearchParamRegistry(mySearchParamRegistry);
+					myExtendedHSearchSearchBuilder.addAndConsumeAdvancedQueryClauses(builder, params);
 				}))
 				.aggregation(observationsByCodeKey, f -> f.fromJson(lastNAggregation.toAggregation()))
 				.fetch(0);