Fix queries with chained sort with Lucene by checking supported SortSpecs (#5958)

* First commit with very rough solution.

* Solidify solutions for both requirements.  Add new tests.  Enhance others.

* Spotless.

* Add new chained sort spec algorithm.  Add new Msg.codes.  Finalize tests.  Update docs.  Add changelog.

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5960-lucene-chain-sort-silently-not-working.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 5960
+title: "Previously, queries with chained would fail to sort correctly with lucene and full text searches enabled.
+        This has been fixed."

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa/elastic.md

@@ -115,3 +115,10 @@ This can cause issues, particularly in unit tests where data is being examined s
 
 You can force synchronous writing to them in HAPI FHIR JPA by setting the Hibernate Search [synchronization strategy](https://docs.jboss.org/hibernate/stable/search/reference/en-US/html_single/#mapper-orm-indexing-automatic-synchronization). This setting is internally setting the ElasticSearch [refresh=wait_for](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-refresh.html) option. Be warned that this will have a negative impact on overall performance. THE HAPI FHIR TEAM has not tried to quantify this impact but the ElasticSearch docs seem to make a fairly big deal about it.
 
+# Sorting
+
+It is possible to sort with Lucene indexing and full text searching enabled.  For example, this will work: `Practitioner?_sort=family`.
+
+Also, chained sorts will work: `PractitionerRole?_sort=practitioner.family`.
+
+However, chained sorting _combined_ with full text searches will fail.  For example, this query will fail with an error:  `PractitionerRole?_text=blah&_sort=practitioner.family`

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

@@ -497,6 +497,11 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc {
 		return myAdvancedIndexQueryBuilder.isSupportsAllOf(theParams);
 	}
 
+	@Override
+	public boolean supportsAllSortTerms(String theResourceType, SearchParameterMap theParams) {
+		return myExtendedFulltextSortHelper.supportsAllSortTerms(theResourceType, theParams);
+	}
+
 	private void dispatchEvent(IHSearchEventListener.HSearchEventType theEventType) {
 		if (myHSearchEventListener != null) {
 			myHSearchEventListener.hsearchEvent(theEventType);

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

@@ -120,4 +120,13 @@ public interface IFulltextSearchSvc {
 	 * @param theGivenIds The list of IDs for the given document type. Note that while this is a List<Object>, the type must match the type of the `@Id` field on the given class.
 	 */
 	void deleteIndexedDocumentsByTypeAndId(Class theClazz, List<Object> theGivenIds);
+
+	/**
+	 * Given a resource type and a {@link SearchParameterMap}, return true only if all sort terms are supported.
+	 *
+	 * @param theResourceName The resource type for the query.
+	 * @param theParams The {@link SearchParameterMap} being searched with.
+	 * @return true if all sort terms are supported, false otherwise.
+	 */
+	boolean supportsAllSortTerms(String theResourceName, SearchParameterMap theParams);
 }

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

@@ -20,6 +20,8 @@
 package ca.uhn.fhir.jpa.dao.search;
 
 import ca.uhn.fhir.context.RuntimeSearchParam;
+import ca.uhn.fhir.i18n.Msg;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.RestSearchParameterTypeEnum;
 import ca.uhn.fhir.rest.api.SortOrderEnum;
 import ca.uhn.fhir.rest.api.SortSpec;
@@ -94,6 +96,19 @@ public class HSearchSortHelperImpl implements IHSearchSortHelper {
 		return sortStep;
 	}
 
+	@Override
+	public boolean supportsAllSortTerms(String theResourceType, SearchParameterMap theParams) {
+		for (SortSpec sortSpec : theParams.getAllChainsInOrder()) {
+			final Optional<RestSearchParameterTypeEnum> paramTypeOpt =
+					getParamType(theResourceType, sortSpec.getParamName());
+			if (paramTypeOpt.isEmpty()) {
+				return false;
+			}
+		}
+
+		return true;
+	}
+
 	/**
 	 * Builds sort clauses for the received SortSpec by
 	 *  _ finding out the corresponding RestSearchParameterTypeEnum for the parameter
@@ -104,13 +119,12 @@ public class HSearchSortHelperImpl implements IHSearchSortHelper {
 	Optional<SortFinalStep> getSortClause(SearchSortFactory theF, SortSpec theSortSpec, String theResourceType) {
 		Optional<RestSearchParameterTypeEnum> paramTypeOpt = getParamType(theResourceType, theSortSpec.getParamName());
 		if (paramTypeOpt.isEmpty()) {
-			ourLog.warn("Sprt parameter type couldn't be determined for parameter: " + theSortSpec.getParamName()
+			throw new IllegalArgumentException(
-					+ ". Result will not be properly sorted");
+					Msg.code(2523) + "Invalid sort specification: " + theSortSpec.getParamName());
-			return Optional.empty();
 		}
 		List<String> paramFieldNameList = getSortPropertyList(paramTypeOpt.get(), theSortSpec.getParamName());
 		if (paramFieldNameList.isEmpty()) {
-			ourLog.warn("Unable to sort by parameter '" + theSortSpec.getParamName() + "'. Sort parameter ignored.");
+			ourLog.warn("Unable to sort by parameter '{}' . Sort parameter ignored.", theSortSpec.getParamName());
 			return Optional.empty();
 		}
 
@@ -128,6 +142,7 @@ public class HSearchSortHelperImpl implements IHSearchSortHelper {
 			sortFinalStep.add(sortStep.missing().last());
 		}
 
+		// regular sorting is supported
 		return Optional.of(sortFinalStep);
 	}
 

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

@@ -19,6 +19,7 @@
  */
 package ca.uhn.fhir.jpa.dao.search;
 
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.SortSpec;
 import org.hibernate.search.engine.search.sort.dsl.SearchSortFactory;
 import org.hibernate.search.engine.search.sort.dsl.SortFinalStep;
@@ -29,4 +30,13 @@ import org.hibernate.search.engine.search.sort.dsl.SortFinalStep;
 public interface IHSearchSortHelper {
 
 	SortFinalStep getSortClauses(SearchSortFactory theSortFactory, SortSpec theSort, String theResourceType);
+
+	/**
+	 * Given a resource type and a {@link SearchParameterMap}, return true only if all sort terms are supported.
+	 *
+	 * @param theResourceType The resource type for the query.
+	 * @param theParams The {@link SearchParameterMap} being searched with.
+	 * @return true if all sort terms are supported, false otherwise.
+	 */
+	boolean supportsAllSortTerms(String theResourceType, SearchParameterMap theParams);
 }

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

@@ -466,6 +466,14 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		if (!fulltextEnabled) {
 			failIfUsed(Constants.PARAM_TEXT);
 			failIfUsed(Constants.PARAM_CONTENT);
+		} else {
+			for (SortSpec sortSpec : myParams.getAllChainsInOrder()) {
+				final String paramName = sortSpec.getParamName();
+				if (paramName.contains(".")) {
+					failIfUsedWithChainedSort(Constants.PARAM_TEXT);
+					failIfUsedWithChainedSort(Constants.PARAM_CONTENT);
+				}
+			}
 		}
 
 		// someday we'll want a query planner to figure out if we _should_ or _must_ use the ft index, not just if we
@@ -473,7 +481,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		return fulltextEnabled
 				&& myParams != null
 				&& myParams.getSearchContainedMode() == SearchContainedModeEnum.FALSE
-				&& myFulltextSearchSvc.supportsSomeOf(myParams);
+				&& myFulltextSearchSvc.supportsSomeOf(myParams)
+				&& myFulltextSearchSvc.supportsAllSortTerms(myResourceName, myParams);
 	}
 
 	private void failIfUsed(String theParamName) {
@@ -483,6 +492,14 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		}
 	}
 
+	private void failIfUsedWithChainedSort(String theParamName) {
+		if (myParams.containsKey(theParamName)) {
+			throw new InvalidRequestException(Msg.code(2524)
+					+ "Fulltext search combined with chained sorts are not supported, can not process parameter: "
+					+ theParamName);
+		}
+	}
+
 	private List<JpaPid> executeLastNAgainstIndex(Integer theMaximumResults) {
 		// Can we use our hibernate search generated index on resource to support lastN?:
 		if (myStorageSettings.isAdvancedHSearchIndexing()) {

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

@@ -894,4 +894,13 @@ public class SearchParameterMap implements Serializable {
 			return SearchParameterMap.compare(myCtx, theO1, theO2);
 		}
 	}
+
+	public List<SortSpec> getAllChainsInOrder() {
+		final List<SortSpec> allChainsInOrder = new ArrayList<>();
+		for (SortSpec sortSpec = getSort(); sortSpec != null; sortSpec = sortSpec.getChain()) {
+			allChainsInOrder.add(sortSpec);
+		}
+
+		return Collections.unmodifiableList(allChainsInOrder);
+	}
 }

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchVariousScenariosTest.java

@@ -3,7 +3,11 @@ package ca.uhn.fhir.jpa.provider.r4;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ca.uhn.fhir.rest.client.api.IGenericClient;
 import ca.uhn.fhir.rest.param.HasParam;
+import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
+import org.hamcrest.MatcherAssert;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
 import org.hl7.fhir.r4.model.CarePlan;
@@ -12,12 +16,16 @@ import org.hl7.fhir.r4.model.Encounter;
 import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.ExplanationOfBenefit;
 import org.hl7.fhir.r4.model.Group;
+import org.hl7.fhir.r4.model.HumanName;
+import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.ListResource;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.Organization;
 import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.Practitioner;
+import org.hl7.fhir.r4.model.PractitionerRole;
 import org.hl7.fhir.r4.model.Reference;
+import org.hl7.fhir.r4.model.Resource;
 import org.hl7.fhir.r4.model.SearchParameter;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
@@ -25,7 +33,11 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
+import java.util.List;
+
+import static org.hamcrest.Matchers.contains;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 
 public class ResourceProviderR4SearchVariousScenariosTest extends BaseResourceProviderR4Test {
@@ -352,15 +364,104 @@ public class ResourceProviderR4SearchVariousScenariosTest extends BaseResourcePr
 		}
 	}
 
-	private void runAndAssert(String theQueryString) {
+	@Nested
-		ourLog.info("queryString:\n{}", theQueryString);
+	class Sorting {
+		private IdType myPraId1;
+		private IdType myPraId2;
+		private IdType myPraId3;
+		private IdType myPraRoleId1;
+		private IdType myPraRoleId2;
+		private IdType myPraRoleId3;
 
-		final Bundle outcome = myClient.search()
+		@BeforeEach
-			.byUrl(theQueryString)
+		void beforeEach() {
-			.returnBundle(Bundle.class)
+			myPraId1 = createPractitioner("pra1", "C_Family");
-			.execute();
+			myPraId2 = createPractitioner("pra2", "A_Family");
+			myPraId3 = createPractitioner("pra3", "B_Family");
+
+			myPraRoleId1 = createPractitionerRole("praRole1", myPraId1);
+			myPraRoleId2 = createPractitionerRole("praRole2", myPraId2);
+			myPraRoleId3 = createPractitionerRole("praRole3", myPraId3);
+		}
+
+		@Test
+		void testRegularSortAscendingWorks() {
+			runAndAssert("regular sort ascending works", "Practitioner?_sort=family", myPraId2.getIdPart(), myPraId3.getIdPart(), myPraId1.getIdPart());
+		}
+
+		@Test
+		void testRegularSortDescendingWorks() {
+			runAndAssert("regular sort descending works", "Practitioner?_sort=-family", myPraId1.getIdPart(), myPraId3.getIdPart(), myPraId2.getIdPart());
+		}
+
+		@Test
+		void testChainedSortWorks() {
+			runAndAssert("chain sort works", "PractitionerRole?_sort=practitioner.family", myPraRoleId2.getIdPart(), myPraRoleId3.getIdPart(), myPraRoleId1.getIdPart());
+		}
+
+		@ParameterizedTest
+		@ValueSource(strings = {
+			"PractitionerRole?_text=blahblah&_sort=practitioner.family",
+			"PractitionerRole?_content=blahblah&_sort=practitioner.family"
+		})
+		void unsupportedSearchesWithChainedSorts(String theQueryString) {
+			runAndAssertThrows(InvalidRequestException.class, theQueryString);
+		}
+
+		private IdType createPractitioner(String theId, String theFamilyName) {
+			final Practitioner practitioner = (Practitioner) new Practitioner()
+				.setActive(true)
+				.setName(List.of(new HumanName().setFamily(theFamilyName)))
+				.setId(theId);
+
+			myPractitionerDao.update(practitioner, new SystemRequestDetails());
+
+			return practitioner.getIdElement().toUnqualifiedVersionless();
+		}
+
+		private IdType createPractitionerRole(String theId, IdType thePractitionerId) {
+			final PractitionerRole practitionerRole = (PractitionerRole) new PractitionerRole()
+				.setActive(true)
+				.setPractitioner(new Reference(thePractitionerId.asStringValue()))
+				.setId(theId);
+
+			myPractitionerRoleDao.update(practitionerRole, new SystemRequestDetails());
+
+			return practitionerRole.getIdElement().toUnqualifiedVersionless();
+		}
+	}
+
+	private void runAndAssert(String theReason, String theQueryString, String... theExpectedIdsInOrder) {
+		final Bundle outcome = runQueryAndGetBundle(theQueryString, myClient);
 
 		assertFalse(outcome.getEntry().isEmpty());
-		ourLog.info("result:\n{}", theQueryString);
+
+		final List<String> actualIdsInOrder = outcome.getEntry()
+			.stream()
+			.map(Bundle.BundleEntryComponent::getResource)
+			.map(Resource::getIdPart)
+			.toList();
+
+		MatcherAssert.assertThat(theReason, actualIdsInOrder, contains(theExpectedIdsInOrder));
+	}
+
+	private void runAndAssert(String theQueryString) {
+		ourLog.debug("queryString:\n{}", theQueryString);
+
+		final Bundle outcome = runQueryAndGetBundle(theQueryString, myClient);
+
+		assertFalse(outcome.getEntry().isEmpty());
+		ourLog.debug("result:\n{}", theQueryString);
+	}
+
+	private void runAndAssertThrows(Class<? extends Exception> theExceptedException, String theQueryString) {
+		assertThrows(theExceptedException, () -> runQueryAndGetBundle(theQueryString, myClient));
+	}
+
+	private static Bundle runQueryAndGetBundle(String theTheQueryString, IGenericClient theClient) {
+		return theClient.search()
+			.byUrl(theTheQueryString)
+			.returnBundle(Bundle.class)
+			.execute();
 	}
 }

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4StandardQueriesLuceneTest.java

@@ -9,12 +9,19 @@ import ca.uhn.fhir.jpa.search.QuantitySearchParameterTestCases;
 import ca.uhn.fhir.jpa.search.BaseSourceSearchParameterTestCases;
 import ca.uhn.fhir.jpa.test.BaseJpaTest;
 import ca.uhn.fhir.jpa.test.config.TestR4Config;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.storage.test.BaseDateSearchDaoTests;
 import ca.uhn.fhir.storage.test.DaoTestDataBuilder;
+import org.hl7.fhir.r4.model.HumanName;
+import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Observation;
+import org.hl7.fhir.r4.model.Practitioner;
+import org.hl7.fhir.r4.model.PractitionerRole;
+import org.hl7.fhir.r4.model.Reference;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -23,6 +30,10 @@ import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
 import org.springframework.transaction.PlatformTransactionManager;
 
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
 @ExtendWith(SpringExtension.class)
 @ContextConfiguration(classes = {
 	TestR4Config.class,
@@ -41,6 +52,10 @@ public class FhirResourceDaoR4StandardQueriesLuceneTest extends BaseJpaTest {
 	@Autowired
 	@Qualifier("myObservationDaoR4")
 	IFhirResourceDao<Observation> myObservationDao;
+	@Autowired
+	IFhirResourceDao<Practitioner> myPractitionerDao;
+	@Autowired
+	IFhirResourceDao<PractitionerRole> myPractitionerRoleDao;
 
 	// todo mb create an extension to restore via clone or xstream + BeanUtils.copyProperties().
 	@BeforeEach
@@ -104,4 +119,63 @@ public class FhirResourceDaoR4StandardQueriesLuceneTest extends BaseJpaTest {
 		}
 	}
 
+	@Nested
+	class ChainedSort {
+		private IdType myPraId1;
+		private IdType myPraId2;
+		private IdType myPraId3;
+		private IdType myPraRoleId1;
+		private IdType myPraRoleId2;
+		private IdType myPraRoleId3;
+
+		@BeforeEach
+		void beforeEach() {
+			myPraId1 = createPractitioner("pra1", "C_Family");
+			myPraId2 = createPractitioner("pra2", "A_Family");
+			myPraId3 = createPractitioner("pra3", "B_Family");
+
+			myPraRoleId1 = createPractitionerRole("praRole1", myPraId1);
+			myPraRoleId2 = createPractitionerRole("praRole2", myPraId2);
+			myPraRoleId3 = createPractitionerRole("praRole3", myPraId3);
+		}
+
+		@Test
+		void testRegularSortAscendingWorks() {
+			myTestDaoSearch.assertSearchFindsInOrder("direct ascending sort works", "Practitioner?_sort=family", myPraId2.getIdPart(), myPraId3.getIdPart(), myPraId1.getIdPart());
+		}
+
+		@Test
+		void testRegularSortDescendingWorks() {
+			myTestDaoSearch.assertSearchFindsInOrder("direct descending sort works", "Practitioner?_sort=-family", myPraId1.getIdPart(), myPraId3.getIdPart(), myPraId2.getIdPart());
+		}
+
+		@Test
+		void testChainedSortWorks() {
+			myTestDaoSearch.assertSearchFindsInOrder("chain works", "PractitionerRole?_sort=practitioner.family", myPraRoleId2.getIdPart(), myPraRoleId3.getIdPart(), myPraRoleId1.getIdPart());
+		}
+
+		// TestDaoSearch doesn't seem to work when using "_text:
+
+		private IdType createPractitioner(String theId, String theFamilyName) {
+			final Practitioner practitioner = (Practitioner) new Practitioner()
+				.setActive(true)
+				.setName(List.of(new HumanName().setFamily(theFamilyName)))
+				.setId(theId);
+
+			myPractitionerDao.update(practitioner, new SystemRequestDetails());
+
+			return practitioner.getIdElement().toUnqualifiedVersionless();
+		}
+
+		private IdType createPractitionerRole(String theId, IdType thePractitionerId) {
+			final PractitionerRole practitionerRole = (PractitionerRole) new PractitionerRole()
+				.setActive(true)
+				.setPractitioner(new Reference(thePractitionerId.asStringValue()))
+				.setId(theId);
+
+			myPractitionerRoleDao.update(practitionerRole, new SystemRequestDetails());
+
+			return practitionerRole.getIdElement().toUnqualifiedVersionless();
+		}
+	}
 }