3664 fast elastic load (#3769)

* Start direct HSearch path

* Support no HSearch

* Spike out the direct resource query

* Implement hsearch fast load

* Fix last master merge in issues

* Implement revision requests

* Test direct resources (no IDs query) sorting

* Use mock to count freetext searches to avoid implementing interface in test

* Remove fixme

* Make listener optional as it is used only for tests

* Provide new dependency

* Widen fast path test scope and fix previously untested configurations

* Make method transactional as it can be called from outside a TX (at least testObservationLastNAllParamsPopulated does)

* Update test validation

Co-authored-by: Michael Buckley <michael.buckley@smilecdr.com>
Co-authored-by: juan.marchionatto <juan.marchionatto@smilecdr.com>

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

@@ -359,7 +359,7 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc {
 		dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
 		List<ExtendedHSearchResourceProjection> rawResourceDataList = session.search(ResourceTable.class)
 			.select(
-				f -> buildResourceSelectClause(f)
+				this::buildResourceSelectClause
 			)
 			.where(
 				f -> f.id().matchingAny(thePids) // matches '_id' from resource index
@@ -403,11 +403,12 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc {
 
 
 	@Override
+	@Transactional(readOnly = true)
 	public List<IBaseResource> searchForResources(String theResourceType, SearchParameterMap theParams) {
 		int offset = 0; int limit = DEFAULT_MAX_PAGE_SIZE;
 		if (theParams.getOffset() != null && theParams.getOffset() != 0) {
 			offset = theParams.getOffset();
-			limit = theParams.getCount();
+			limit = theParams.getCount() == null ? DEFAULT_MAX_PAGE_SIZE : theParams.getCount();
 			// indicate param was already processed, otherwise queries DB to process it
 			theParams.setOffset(null);
 		}

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

@@ -24,6 +24,7 @@ import ca.uhn.fhir.context.RuntimeSearchParam;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.rest.api.Constants;
+import ca.uhn.fhir.rest.api.SearchContainedModeEnum;
 import ca.uhn.fhir.rest.param.DateParam;
 import ca.uhn.fhir.rest.param.NumberParam;
 import ca.uhn.fhir.rest.param.QuantityParam;
@@ -34,6 +35,8 @@ import ca.uhn.fhir.rest.param.UriParam;
 import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
 
 import java.util.ArrayList;
@@ -72,16 +75,16 @@ public class ExtendedHSearchSearchBuilder {
 	 */
 	public boolean isSupportsAllOf(SearchParameterMap myParams) {
 		return
-			myParams.getRevIncludes() == null && // ???
+			CollectionUtils.isEmpty( myParams.getRevIncludes() ) && // ???
-			myParams.getIncludes() == null && // ???
+			CollectionUtils.isEmpty( myParams.getIncludes() ) && // ???
 			myParams.getEverythingMode() == null && // ???
-			! myParams.isDeleteExpunge() && // ???
+			BooleanUtils.isFalse( myParams.isDeleteExpunge() ) && // ???
 
 			// not yet supported in HSearch
 			myParams.getNearDistanceParam() == null && // ???
 
 			// not yet supported in HSearch
-			myParams.getSearchContainedMode() == null && // ???
+			myParams.getSearchContainedMode() == SearchContainedModeEnum.FALSE && // ???
 
 			myParams.entrySet().stream()
 				.filter(e -> !ourUnsafeSearchParmeters.contains(e.getKey()))
@@ -135,10 +138,8 @@ public class ExtendedHSearchSearchBuilder {
 					return false;
 			}
 		} else if (param instanceof DateParam) {
-			if (EMPTY_MODIFIER.equals(modifier)) {
+			return modifier.equals(EMPTY_MODIFIER);
-				return true;
+
-			}
-			return false;
 		} else if (param instanceof UriParam) {
 			return modifier.equals(EMPTY_MODIFIER);
 

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

@@ -362,6 +362,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 //		return searchStrategy.get();
 
 		if (mySearchStrategyFactory.isSupportsHSearchDirect(theResourceType, theParams, theRequestDetails)) {
+			ourLog.info("Search {} is using direct load strategy", searchUuid);
 			SearchStrategyFactory.ISearchStrategy direct =  mySearchStrategyFactory.makeDirectStrategy(searchUuid, theResourceType, theParams, theRequestDetails);
 			return direct.get();
 		}

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

@@ -29,6 +29,7 @@ import ca.uhn.fhir.rest.server.SimpleBundleProvider;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 
 import javax.annotation.Nullable;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Supplier;
 
@@ -68,10 +69,13 @@ public class SearchStrategyFactory {
 
 	public ISearchStrategy makeDirectStrategy(String theSearchUUID, String theResourceType, SearchParameterMap theParams, RequestDetails theRequestDetails) {
 		return () -> {
+			if (myFulltextSearchSvc == null) {
+				return new SimpleBundleProvider(Collections.emptyList(), theSearchUUID);
+			}
+
 			List<IBaseResource> resources = myFulltextSearchSvc.searchForResources(theResourceType, theParams);
 			SimpleBundleProvider result = new SimpleBundleProvider(resources, theSearchUUID);
-			// we don't know the size
+			result.setSize(resources.size());
-			result.setSize(null);
 			return result;
 		};
 	}

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

@@ -226,12 +226,8 @@ abstract public class BaseR4SearchLastN extends BaseJpaTest {
 	private void executeTestCase(SearchParameterMap params, List<String> sortedPatients, List<String> sortedObservationCodes, List<String> theCategories, int expectedObservationCount) {
 		List<String> actual;
 		params.setLastN(true);
-
-		Map<String, String[]> requestParameters = new HashMap<>();
 		params.setLastNMax(100);
 
-		when(mySrd.getParameters()).thenReturn(requestParameters);
-
 		actual = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null));
 
 		assertEquals(expectedObservationCount, actual.size());

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

@@ -125,7 +125,6 @@ public class FhirResourceDaoR4SearchLastNIT extends BaseR4SearchLastN {
 		when(mySrd.getParameters()).thenReturn(requestParameters);
 
 		List<String> results = toUnqualifiedVersionlessIdValues(myObservationDao.observationsLastN(params, mockSrd(), null));
-		verifyResourcesLoadedFromElastic(observationIds, results);
 
 	}
 

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

@@ -1,24 +1,18 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
-import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
+import ca.uhn.fhir.jpa.dao.IHSearchEventListener;
+import ca.uhn.fhir.jpa.test.util.TestHSearchEventDispatcher;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.extension.ExtendWith;
-import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.boot.test.mock.mockito.SpyBean;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
 
 import java.util.List;
-import java.util.stream.Collectors;
-
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.is;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
 
 /**
  * Run entire @see {@link FhirResourceDaoR4SearchLastNIT} test suite this time
@@ -28,14 +22,18 @@ import static org.mockito.Mockito.verify;
  */
 @ExtendWith(SpringExtension.class)
 public class FhirResourceDaoR4SearchLastNUsingExtendedHSearchIndexIT extends FhirResourceDaoR4SearchLastNIT {
-	// awkward override so we can spy
+
-	@SpyBean
+	@Autowired
-	@Autowired(required = false)
+	private TestHSearchEventDispatcher myHSearchEventDispatcher;
-	IFulltextSearchSvc myFulltestSearchSvc;
+
+	@Mock
+	private IHSearchEventListener mySearchEventListener;
+
 
 	@BeforeEach
 	public void enableAdvancedHSearchIndexing() {
 		myDaoConfig.setAdvancedHSearchIndexing(true);
+		myHSearchEventDispatcher.register(mySearchEventListener);
 	}
 
 	@AfterEach
@@ -44,24 +42,13 @@ public class FhirResourceDaoR4SearchLastNUsingExtendedHSearchIndexIT extends Fhi
 	}
 
 	/**
-	 * We pull the resources from Hibernate Search when LastN uses Hibernate Search.
+	 * We pull the resources from Hibernate Search when LastN uses Hibernate Search
-	 * Override the test verification
+	 * Override the test verification to validate only one search was performed
 	 */
 	@Override
 	void verifyResourcesLoadedFromElastic(List<IIdType> theObservationIds, List<String> theResults) {
-		List<Long> expectedArgumentPids =
+		Mockito.verify(mySearchEventListener, Mockito.times(1))
-			theObservationIds.stream().map(IIdType::getIdPartAsLong).collect(Collectors.toList());
+			.hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
-
-		ArgumentCaptor<List<Long>> actualPids = ArgumentCaptor.forClass(List.class);
-
-		verify(myFulltestSearchSvc, times(1)).getResources(actualPids.capture());
-		assertThat(actualPids.getValue(), is(expectedArgumentPids));
-
-		// we don't include the type in the id returned from Hibernate Search for now.
-		List<String> expectedObservationList = theObservationIds.stream()
-			.map(id -> id.toUnqualifiedVersionless().getIdPart()).collect(Collectors.toList());
-		assertEquals(expectedObservationList, theResults);
-
 	}
 
 }

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

@@ -10,7 +10,6 @@ import ca.uhn.fhir.jpa.api.dao.IFhirSystemDao;
 import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.api.svc.ISearchCoordinatorSvc;
 import ca.uhn.fhir.jpa.bulk.export.api.IBulkDataExportJobSchedulingHelper;
-import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
 import ca.uhn.fhir.jpa.dao.IHSearchEventListener;
 import ca.uhn.fhir.jpa.dao.TestDaoSearch;
 import ca.uhn.fhir.jpa.dao.data.IResourceTableDao;
@@ -22,8 +21,6 @@ import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.partition.SystemRequestDetails;
 import ca.uhn.fhir.jpa.search.reindex.IResourceReindexingSvc;
-import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
-import ca.uhn.fhir.jpa.searchparam.ResourceSearch;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.sp.ISearchParamPresenceSvc;
 import ca.uhn.fhir.jpa.term.api.ITermCodeSystemStorageSvc;
@@ -211,14 +208,9 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
 	@RegisterExtension
 	LogbackLevelOverrideExtension myLogbackLevelOverrideExtension = new LogbackLevelOverrideExtension();
 
-	@Autowired
-	private IFulltextSearchSvc myIFulltextSearchSvc;
-
 	@Autowired
 	private TestHSearchEventDispatcher myHSearchEventDispatcher;
 
-	@Autowired
-	private MatchUrlService myMatchUrlService;
 
 	@Mock private IHSearchEventListener mySearchEventListener;
 
@@ -2282,32 +2274,27 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
 
 		}
 
-		@Nested
+		@Test
-		public class NoIdsQuery {
+		public void directResourceLoadWhenSorting() {
+			String idA = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-a"))).getIdPart();
+			String idC = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-c"))).getIdPart();
+			String idB = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-b"))).getIdPart();
+			myCaptureQueriesListener.clear();
+			myHSearchEventDispatcher.register(mySearchEventListener);
 
-			@Test
+			List<IBaseResource> result = searchForFastResources("Observation?_sort=-code");
-			public void simpleTokenSkipsSql() {
+			myCaptureQueriesListener.logSelectQueriesForCurrentThread();
-				String idA = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-a"))).getIdPart();
-				String idC = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-c"))).getIdPart();
-				String idB = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "code-b"))).getIdPart();
-				myCaptureQueriesListener.clear();
-				myHSearchEventDispatcher.register(mySearchEventListener);
 
-				List<IBaseResource> result = searchForFastResources("Observation?_sort=-code");
+			assertThat( result.stream().map(r -> r.getIdElement().getIdPart()).collect(Collectors.toList()), contains(idC, idB, idA) );
-				myCaptureQueriesListener.logSelectQueriesForCurrentThread();
+			assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size(), "we build the bundle with no sql");
-
-				assertThat( result.stream().map(r -> r.getIdElement().getIdPart()).collect(Collectors.toList()), contains(idC, idB, idA) );
-				assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size(), "we build the bundle with no sql");
-
-				// only one hibernate search took place
-				Mockito.verify(mySearchEventListener, Mockito.times(1)).hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
-			}
 
+			// only one hibernate search took place
+			Mockito.verify(mySearchEventListener, Mockito.times(1)).hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH);
 		}
 
-
 	}
 
+
 	@Nested
 	public class NumberParameter {
 
@@ -2540,14 +2527,14 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl
 	public List<IBaseResource> searchForFastResources(String theQueryUrl) {
 		SearchParameterMap map = myTestDaoSearch.toSearchParameters(theQueryUrl);
 		map.setLoadSynchronous(true);
+
 		SortSpec sort = (SortSpec) new ca.uhn.fhir.rest.server.method.SortParameter(myFhirCtx)
 			.translateQueryParametersIntoServerArgument(fakeRequestDetailsFromUrl(theQueryUrl), null);
 		if (sort != null) {
 			map.setSort(sort);
 		}
 
-		ResourceSearch search = myMatchUrlService.getResourceSearch(theQueryUrl);
+		return myTestDaoSearch.searchForResources(theQueryUrl);
-		return runInTransaction( () ->  myIFulltextSearchSvc.searchForResources(search.getResourceName(), map) );
 	}