From a13f2411a1f946cc7ce02598e4ffba4dfad34ab4 Mon Sep 17 00:00:00 2001 From: michaelabuckley Date: Mon, 4 Jul 2022 18:18:29 -0400 Subject: [PATCH] Implement direct HSearch search path (#3727) * 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 Co-authored-by: juan.marchionatto --- .../ca/uhn/fhir/jpa/config/JpaConfig.java | 11 ++ .../fhir/jpa/dao/FulltextSearchSvcImpl.java | 182 ++++++++++++------ .../uhn/fhir/jpa/dao/IFulltextSearchSvc.java | 6 + .../fhir/jpa/dao/IHSearchEventListener.java | 10 + .../search/ExtendedHSearchSearchBuilder.java | 24 +++ .../jpa/search/SearchCoordinatorSvcImpl.java | 42 +++- .../jpa/search/SearchStrategyFactory.java | 59 ++++++ .../jpa/search/builder/SearchBuilder.java | 2 + .../test/config/TestHSearchAddInConfig.java | 19 +- .../test/util/TestHSearchEventDispatcher.java | 33 ++++ .../ca/uhn/fhir/jpa/dao/TestDaoSearch.java | 5 + ...esourceDaoR4SearchWithElasticSearchIT.java | 87 ++++++++- .../search/SearchCoordinatorSvcImplTest.java | 12 +- 13 files changed, 416 insertions(+), 76 deletions(-) create mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IHSearchEventListener.java create mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchStrategyFactory.java create mode 100644 hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/util/TestHSearchEventDispatcher.java diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/JpaConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/JpaConfig.java index 081d2ea3fde..02fb03a691a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/JpaConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/JpaConfig.java @@ -30,6 +30,7 @@ import ca.uhn.fhir.jpa.cache.ResourceVersionSvcDaoImpl; import ca.uhn.fhir.jpa.dao.DaoSearchParamProvider; import ca.uhn.fhir.jpa.dao.HistoryBuilder; import ca.uhn.fhir.jpa.dao.HistoryBuilderFactory; +import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc; import ca.uhn.fhir.jpa.dao.ISearchBuilder; import ca.uhn.fhir.jpa.dao.LegacySearchBuilder; import ca.uhn.fhir.jpa.dao.MatchResourceUrlService; @@ -89,6 +90,7 @@ import ca.uhn.fhir.jpa.provider.r4.MemberMatcherR4Helper; import ca.uhn.fhir.jpa.reindex.ResourceReindexSvcImpl; import ca.uhn.fhir.jpa.sched.AutowiringSpringBeanJobFactory; import ca.uhn.fhir.jpa.sched.HapiSchedulerServiceImpl; +import ca.uhn.fhir.jpa.search.SearchStrategyFactory; import ca.uhn.fhir.jpa.search.ISynchronousSearchSvc; import ca.uhn.fhir.jpa.search.PersistedJpaBundleProvider; import ca.uhn.fhir.jpa.search.PersistedJpaBundleProviderFactory; @@ -152,6 +154,7 @@ import org.hl7.fhir.common.hapi.validation.support.UnknownCodeSystemWarningValid import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.utilities.graphql.IGraphQLStorageServices; import org.hl7.fhir.utilities.npm.PackageClient; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -210,6 +213,9 @@ public class JpaConfig { public static final String HISTORY_BUILDER = "HistoryBuilder"; private static final String HAPI_DEFAULT_SCHEDULER_GROUP = "HAPI"; + @Autowired + public DaoConfig myDaoConfig; + @Bean("myDaoRegistry") public DaoRegistry daoRegistry() { return new DaoRegistry(); @@ -745,6 +751,11 @@ public class JpaConfig { return new SearchCoordinatorSvcImpl(); } + @Bean + public SearchStrategyFactory searchStrategyFactory(@Autowired(required = false) IFulltextSearchSvc theFulltextSvc) { + return new SearchStrategyFactory(myDaoConfig, theFulltextSvc); + } + @Bean public DeleteConflictService deleteConflictService() { return new DeleteConflictService(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java index 95b86cb97c5..b314fc37507 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java @@ -44,17 +44,19 @@ import ca.uhn.fhir.jpa.searchparam.extractor.ResourceIndexedSearchParams; import ca.uhn.fhir.model.api.IQueryParameterType; import ca.uhn.fhir.parser.IParser; import ca.uhn.fhir.rest.api.Constants; -import ca.uhn.fhir.rest.api.SortOrderEnum; import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId; import ca.uhn.fhir.rest.server.util.ISearchParamRegistry; import ca.uhn.fhir.rest.server.util.ResourceSearchParams; import com.google.common.collect.Ordering; import org.hibernate.search.backend.elasticsearch.ElasticsearchExtension; +import org.hibernate.search.engine.search.predicate.dsl.PredicateFinalStep; +import org.hibernate.search.engine.search.predicate.dsl.SearchPredicateFactory; +import org.hibernate.search.engine.search.projection.dsl.CompositeProjectionOptionsStep; +import org.hibernate.search.engine.search.projection.dsl.SearchProjectionFactory; import org.hibernate.search.engine.search.query.SearchScroll; import org.hibernate.search.engine.search.query.dsl.SearchQueryOptionsStep; -import org.hibernate.search.engine.search.sort.dsl.SearchSortFactory; -import org.hibernate.search.engine.search.sort.dsl.SortFinalStep; import org.hibernate.search.mapper.orm.Search; +import org.hibernate.search.mapper.orm.common.EntityReference; import org.hibernate.search.mapper.orm.search.loading.dsl.SearchLoadingOptionsStep; import org.hibernate.search.mapper.orm.session.SearchSession; import org.hibernate.search.mapper.orm.work.SearchIndexingPlan; @@ -77,10 +79,12 @@ import java.util.Spliterators; import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import static ca.uhn.fhir.rest.server.BasePagingProvider.DEFAULT_MAX_PAGE_SIZE; import static org.apache.commons.lang3.StringUtils.isNotBlank; public class FulltextSearchSvcImpl implements IFulltextSearchSvc { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FulltextSearchSvcImpl.class); + @PersistenceContext(type = PersistenceContextType.TRANSACTION) private EntityManager myEntityManager; @Autowired @@ -104,6 +108,9 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { final private ExtendedHSearchSearchBuilder myAdvancedIndexQueryBuilder = new ExtendedHSearchSearchBuilder(); + @Autowired(required = false) + private IHSearchEventListener myHSearchEventListener; + private Boolean ourDisabled; /** @@ -172,6 +179,7 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { private SearchQueryOptionsStep getSearchQueryOptionsStep( String theResourceType, SearchParameterMap theParams, ResourcePersistentId theReferencingPid) { + dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH); var query= getSearchSession().search(ResourceTable.class) // The document id is the PK which is pid. We use this instead of _myId to avoid fetching the doc body. .select( @@ -181,51 +189,12 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { f.documentReference()) ) .where( - f -> f.bool(b -> { - ExtendedHSearchClauseBuilder builder = new ExtendedHSearchClauseBuilder(myFhirContext, myModelConfig, b, f); - - /* - * Handle _content parameter (resource body content) - * - * Posterity: - * We do not want the HAPI-FHIR dao's to process the - * _content parameter, so we remove it from the map here - */ - List> contentAndTerms = theParams.remove(Constants.PARAM_CONTENT); - builder.addStringTextSearch(Constants.PARAM_CONTENT, contentAndTerms); - - /* - * Handle _text parameter (resource narrative content) - * - * Posterity: - * We do not want the HAPI-FHIR dao's to process the - * _text parameter, so we remove it from the map here - */ - List> textAndTerms = theParams.remove(Constants.PARAM_TEXT); - builder.addStringTextSearch(Constants.PARAM_TEXT, textAndTerms); - - if (theReferencingPid != null) { - b.must(f.match().field("myResourceLinksField").matching(theReferencingPid.toString())); - } - - if (isNotBlank(theResourceType)) { - builder.addResourceTypeClause(theResourceType); - } - - /* - * Handle other supported parameters - */ - if (myDaoConfig.isAdvancedHSearchIndexing() && theParams.getEverythingMode() == null) { - myAdvancedIndexQueryBuilder.addAndConsumeAdvancedQueryClauses(builder, theResourceType, theParams, mySearchParamRegistry); - } - - //DROP EARLY HERE IF BOOL IS EMPTY? - - }) + f -> buildWhereClause(f, theResourceType, theParams, theReferencingPid) ); if (theParams.getSort() != null) { - query.sort( f -> myExtendedFulltextSortHelper.getSortClauses(f, theParams.getSort(), theResourceType) ); + query.sort( + f -> myExtendedFulltextSortHelper.getSortClauses(f, theParams.getSort(), theResourceType) ); // indicate parameter was processed theParams.setSort(null); @@ -235,6 +204,50 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { } + private PredicateFinalStep buildWhereClause(SearchPredicateFactory f, String theResourceType, + SearchParameterMap theParams, ResourcePersistentId theReferencingPid) { + return f.bool(b -> { + ExtendedHSearchClauseBuilder builder = new ExtendedHSearchClauseBuilder(myFhirContext, myModelConfig, b, f); + + /* + * Handle _content parameter (resource body content) + * + * Posterity: + * We do not want the HAPI-FHIR dao's to process the + * _content parameter, so we remove it from the map here + */ + List> contentAndTerms = theParams.remove(Constants.PARAM_CONTENT); + builder.addStringTextSearch(Constants.PARAM_CONTENT, contentAndTerms); + + /* + * Handle _text parameter (resource narrative content) + * + * Posterity: + * We do not want the HAPI-FHIR dao's to process the + * _text parameter, so we remove it from the map here + */ + List> textAndTerms = theParams.remove(Constants.PARAM_TEXT); + builder.addStringTextSearch(Constants.PARAM_TEXT, textAndTerms); + + if (theReferencingPid != null) { + b.must(f.match().field("myResourceLinksField").matching(theReferencingPid.toString())); + } + + if (isNotBlank(theResourceType)) { + builder.addResourceTypeClause(theResourceType); + } + + /* + * Handle other supported parameters + */ + if (myDaoConfig.isAdvancedHSearchIndexing() && theParams.getEverythingMode() == null) { + myAdvancedIndexQueryBuilder.addAndConsumeAdvancedQueryClauses(builder, theResourceType, theParams, mySearchParamRegistry); + } + //DROP EARLY HERE IF BOOL IS EMPTY? + }); + } + + @Nonnull private SearchSession getSearchSession() { return Search.session(myEntityManager); @@ -304,6 +317,7 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { ValueSetAutocompleteSearch autocomplete = new ValueSetAutocompleteSearch(myFhirContext, myModelConfig, getSearchSession()); + dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH); return autocomplete.search(theOptions); } @@ -329,6 +343,7 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { @Override public List lastN(SearchParameterMap theParams, Integer theMaximumResults) { ensureElastic(); + dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH); List pidList = new LastNOperation(getSearchSession(), myFhirContext, myModelConfig, mySearchParamRegistry) .executeLastN(theParams, theMaximumResults); return convertLongsToResourcePersistentIds(pidList); @@ -341,37 +356,40 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { } SearchSession session = getSearchSession(); + dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH); List rawResourceDataList = session.search(ResourceTable.class) .select( - f -> f.composite( - ExtendedHSearchResourceProjection::new, - f.field("myId", Long.class), - f.field("myForcedId", String.class), - f.field("myRawResource", String.class)) + f -> buildResourceSelectClause(f) ) .where( - f -> f.id().matchingAny(thePids)) - .fetchAllHits(); // matches '_id' from resource index + f -> f.id().matchingAny(thePids) // matches '_id' from resource index + ).fetchAllHits(); + // order resource projections as per thePids ArrayList pidList = new ArrayList<>(thePids); List orderedAsPidsResourceDataList = rawResourceDataList.stream() .sorted( Ordering.explicit(pidList).onResultOf(ExtendedHSearchResourceProjection::getPid) ).collect( Collectors.toList() ); + return resourceProjectionsToResources(orderedAsPidsResourceDataList); + } + + + @Nonnull + private List resourceProjectionsToResources(List theResourceDataList) { IParser parser = myFhirContext.newJsonParser(); - return orderedAsPidsResourceDataList.stream() + return theResourceDataList.stream() .map(p -> p.toResource(parser)) .collect(Collectors.toList()); } - private SortFinalStep getSortOrder(SearchSortFactory theF, SortOrderEnum theOrder) { - var finalSortStep = theF.field("myId"); - if (theOrder == SortOrderEnum.DESC) { - finalSortStep.desc(); - } else { - finalSortStep.asc(); - } - return finalSortStep; + private CompositeProjectionOptionsStep buildResourceSelectClause( + SearchProjectionFactory f) { + return f.composite( + ExtendedHSearchResourceProjection::new, + f.field("myId", Long.class), + f.field("myForcedId", String.class), + f.field("myRawResource", String.class)); } @@ -382,4 +400,46 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc { return queryOptionsStep.fetchTotalHitCount(); } + + + @Override + public List 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(); + // indicate param was already processed, otherwise queries DB to process it + theParams.setOffset(null); + } + + dispatchEvent(IHSearchEventListener.HSearchEventType.SEARCH); + + var query = getSearchSession().search(ResourceTable.class) + .select(this::buildResourceSelectClause) + .where(f -> buildWhereClause(f, theResourceType, theParams, null)); + + if (theParams.getSort() != null && offset == 0) { + query.sort( + f -> myExtendedFulltextSortHelper.getSortClauses(f, theParams.getSort(), theResourceType) ); + } + + List extendedLuceneResourceProjections = query.fetchHits(offset, limit); + + return resourceProjectionsToResources(extendedLuceneResourceProjections); + } + + + @Override + public boolean supportsAllOf(SearchParameterMap theParams) { + return myAdvancedIndexQueryBuilder.isSupportsAllOf(theParams); + } + + + private void dispatchEvent(IHSearchEventListener.HSearchEventType theEventType) { + if (myHSearchEventListener != null) { + myHSearchEventListener.hsearchEvent(theEventType); + } + } + + } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IFulltextSearchSvc.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IFulltextSearchSvc.java index 23a028ab3fd..9d08043ded2 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IFulltextSearchSvc.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IFulltextSearchSvc.java @@ -95,4 +95,10 @@ public interface IFulltextSearchSvc { * Returns accurate hit count */ long count(String theResourceName, SearchParameterMap theParams); + + List searchForResources(String theResourceType, SearchParameterMap theParams); + + boolean supportsAllOf(SearchParameterMap theParams); + + } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IHSearchEventListener.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IHSearchEventListener.java new file mode 100644 index 00000000000..067299de2f6 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IHSearchEventListener.java @@ -0,0 +1,10 @@ +package ca.uhn.fhir.jpa.dao; + +public interface IHSearchEventListener { + + enum HSearchEventType { + SEARCH + } + + void hsearchEvent(HSearchEventType theEventType); +} diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java index bd0d7e8d2bf..997ead915fb 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/search/ExtendedHSearchSearchBuilder.java @@ -67,6 +67,30 @@ public class ExtendedHSearchSearchBuilder { .anyMatch(this::isParamTypeSupported); } + /** + * Are all the queries supported by our indexing? + */ + public boolean isSupportsAllOf(SearchParameterMap myParams) { + return + myParams.getRevIncludes() == null && // ??? + myParams.getIncludes() == null && // ??? + myParams.getEverythingMode() == null && // ??? + ! myParams.isDeleteExpunge() && // ??? + + // not yet supported in HSearch + myParams.getNearDistanceParam() == null && // ??? + + // not yet supported in HSearch + myParams.getSearchContainedMode() == 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) + .allMatch(this::isParamTypeSupported); + } + /** * Do we support this query param type+modifier? *

diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index a126c6a2468..2d1bd71602b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -163,6 +163,8 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { private IRequestPartitionHelperSvc myRequestPartitionHelperService; @Autowired private ISearchParamRegistry mySearchParamRegistry; + @Autowired + private SearchStrategyFactory mySearchStrategyFactory; /** * Constructor @@ -211,6 +213,29 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { myMaxMillisToWaitForRemoteResults = theMaxMillisToWaitForRemoteResults; } + /** + * facade over raw hook intererface + */ + public class StorageInterceptorHooks { + /** + * Interceptor call: STORAGE_PRESEARCH_REGISTERED + * + * @param theRequestDetails + * @param theParams + * @param search + */ + private void callStoragePresearchRegistered(RequestDetails theRequestDetails, SearchParameterMap theParams, Search search) { + HookParams params = new HookParams() + .add(ICachedSearchDetails.class, search) + .add(RequestDetails.class, theRequestDetails) + .addIfMatchesType(ServletRequestDetails.class, theRequestDetails) + .add(SearchParameterMap.class, theParams); + CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_PRESEARCH_REGISTERED, params); + } + //private IInterceptorBroadcaster myInterceptorBroadcaster; + } + private StorageInterceptorHooks myStorageInterceptorHooks = new StorageInterceptorHooks(); + /** * This method is called by the HTTP client processing thread in order to * fetch resources. @@ -321,13 +346,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { Search search = new Search(); populateSearchEntity(theParams, theResourceType, searchUuid, queryString, search, theRequestPartitionId); - // Interceptor call: STORAGE_PRESEARCH_REGISTERED - HookParams params = new HookParams() - .add(ICachedSearchDetails.class, search) - .add(RequestDetails.class, theRequestDetails) - .addIfMatchesType(ServletRequestDetails.class, theRequestDetails) - .add(SearchParameterMap.class, theParams); - CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_PRESEARCH_REGISTERED, params); + myStorageInterceptorHooks.callStoragePresearchRegistered(theRequestDetails, theParams, search); validateSearch(theParams); @@ -338,6 +357,15 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { final Integer loadSynchronousUpTo = getLoadSynchronousUpToOrNull(theCacheControlDirective); boolean isOffsetQuery = theParams.isOffsetQuery(); + // todo someday - not today. +// SearchStrategyFactory.ISearchStrategy searchStrategy = mySearchStrategyFactory.pickStrategy(theResourceType, theParams, theRequestDetails); +// return searchStrategy.get(); + + if (mySearchStrategyFactory.isSupportsHSearchDirect(theResourceType, theParams, theRequestDetails)) { + SearchStrategyFactory.ISearchStrategy direct = mySearchStrategyFactory.makeDirectStrategy(searchUuid, theResourceType, theParams, theRequestDetails); + return direct.get(); + } + if (theParams.isLoadSynchronous() || loadSynchronousUpTo != null || isOffsetQuery) { ourLog.debug("Search {} is loading in synchronous mode", searchUuid); return mySynchronousSearchSvc.executeQuery(theParams, theRequestDetails, searchUuid, sb, loadSynchronousUpTo, theRequestPartitionId); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchStrategyFactory.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchStrategyFactory.java new file mode 100644 index 00000000000..e603eb1aac4 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchStrategyFactory.java @@ -0,0 +1,59 @@ +package ca.uhn.fhir.jpa.search; + +import ca.uhn.fhir.jpa.api.config.DaoConfig; +import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc; +import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.server.SimpleBundleProvider; +import org.hl7.fhir.instance.model.api.IBaseResource; + +import javax.annotation.Nullable; +import java.util.List; +import java.util.function.Supplier; + +/** + * Figure out how we're going to run the query up front, and build a branchless strategy object. + */ +public class SearchStrategyFactory { + private final DaoConfig myDaoConfig; + @Nullable + private final IFulltextSearchSvc myFulltextSearchSvc; + + public interface ISearchStrategy extends Supplier { + + } + + // someday +// public class DirectHSearch implements ISearchStrategy {}; +// public class JPAOffsetSearch implements ISearchStrategy {}; +// public class JPASavedSearch implements ISearchStrategy {}; +// public class JPAHybridHSearchSavedSearch implements ISearchStrategy {}; +// public class SavedSearchAdaptorStrategy implements ISearchStrategy {}; + + public SearchStrategyFactory(DaoConfig theDaoConfig, @Nullable IFulltextSearchSvc theFulltextSearchSvc) { + myDaoConfig = theDaoConfig; + myFulltextSearchSvc = theFulltextSearchSvc; + } + + public boolean isSupportsHSearchDirect(String theResourceType, SearchParameterMap theParams, RequestDetails theRequestDetails) { + return + myFulltextSearchSvc != null && + myDaoConfig.isStoreResourceInHSearchIndex() && + myDaoConfig.isAdvancedHSearchIndexing() && + myFulltextSearchSvc.supportsAllOf(theParams) && + theParams.getSummaryMode() == null && + theParams.getSearchTotalMode() == null; + } + + public ISearchStrategy makeDirectStrategy(String theSearchUUID, String theResourceType, SearchParameterMap theParams, RequestDetails theRequestDetails) { + return () -> { + List resources = myFulltextSearchSvc.searchForResources(theResourceType, theParams); + SimpleBundleProvider result = new SimpleBundleProvider(resources, theSearchUUID); + // we don't know the size + result.setSize(null); + return result; + }; + } + +} diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 81cd8e175db..61465d2adac 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java @@ -378,11 +378,13 @@ public class SearchBuilder implements ISearchBuilder { ); if (canSkipDatabase) { + ourLog.trace("Query finished after HSearch. Skip db query phase"); if (theMaximumResults != null) { fulltextExecutor = SearchQueryExecutors.limited(fulltextExecutor, theMaximumResults); } queries.add(fulltextExecutor); } else { + ourLog.trace("Query needs db after HSearch. Chunking."); // Finish the query in the database for the rest of the search parameters, sorting, partitioning, etc. // We break the pids into chunks that fit in the 1k limit for jdbc bind params. // wipmb change chunk to take iterator diff --git a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/config/TestHSearchAddInConfig.java b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/config/TestHSearchAddInConfig.java index 70acacf8ce2..fd8a8b2c1e0 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/config/TestHSearchAddInConfig.java +++ b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/config/TestHSearchAddInConfig.java @@ -22,10 +22,12 @@ package ca.uhn.fhir.jpa.test.config; import ca.uhn.fhir.jpa.dao.FulltextSearchSvcImpl; import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc; +import ca.uhn.fhir.jpa.dao.IHSearchEventListener; import ca.uhn.fhir.jpa.model.config.PartitionSettings; import ca.uhn.fhir.jpa.search.HapiHSearchAnalysisConfigurers; import ca.uhn.fhir.jpa.search.elastic.ElasticsearchHibernatePropertiesBuilder; import ca.uhn.fhir.jpa.search.lastn.ElasticsearchSvcImpl; +import ca.uhn.fhir.jpa.test.util.TestHSearchEventDispatcher; import ca.uhn.fhir.test.utilities.docker.RequiresDocker; import org.hibernate.search.backend.elasticsearch.index.IndexStatus; import org.hibernate.search.backend.lucene.cfg.LuceneBackendSettings; @@ -103,6 +105,12 @@ public class TestHSearchAddInConfig { ourLog.info("Hibernate Search: FulltextSearchSvcImpl present"); return new FulltextSearchSvcImpl(); } + + @Bean + public IHSearchEventListener testHSearchEventDispatcher() { + return new TestHSearchEventDispatcher(); + } + } @@ -134,6 +142,12 @@ public class TestHSearchAddInConfig { ourLog.info("Hibernate Search: FulltextSearchSvcImpl present"); return new FulltextSearchSvcImpl(); } + + @Bean + public IHSearchEventListener testHSearchEventDispatcher() { + return new TestHSearchEventDispatcher(); + } + } /** @@ -189,7 +203,10 @@ public class TestHSearchAddInConfig { }; } - + @Bean + public IHSearchEventListener testHSearchEventDispatcher() { + return new TestHSearchEventDispatcher(); + } @Bean public ElasticsearchContainer elasticContainer() { diff --git a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/util/TestHSearchEventDispatcher.java b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/util/TestHSearchEventDispatcher.java new file mode 100644 index 00000000000..e983f5eed59 --- /dev/null +++ b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/util/TestHSearchEventDispatcher.java @@ -0,0 +1,33 @@ +package ca.uhn.fhir.jpa.test.util; + +import ca.uhn.fhir.jpa.dao.IHSearchEventListener; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.List; + +public class TestHSearchEventDispatcher implements IHSearchEventListener { + private final Logger ourLog = LoggerFactory.getLogger(TestHSearchEventDispatcher.class); + + private final List listeners = new ArrayList<>(); + + + public void register(IHSearchEventListener theListener) { + if ( theListener.equals(this) ) { + ourLog.error("Dispatcher is not supposed to register itself as a listener. Ignored."); + return; + } + + listeners.add(theListener); + } + + + /** + * Dispatch event to registered listeners + */ + @Override + public void hsearchEvent(HSearchEventType theEventType) { + listeners.forEach( l -> l.hsearchEvent(theEventType) ); + } +} diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java index 908751bf8b6..4ea046cd14b 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java @@ -7,6 +7,7 @@ import ca.uhn.fhir.jpa.partition.SystemRequestDetails; 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.rest.annotation.Transaction; import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.server.method.SortParameter; @@ -17,6 +18,7 @@ import org.hl7.fhir.instance.model.api.IIdType; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.transaction.annotation.Transactional; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; @@ -46,6 +48,9 @@ public class TestDaoSearch { } } + @Autowired + private IFulltextSearchSvc myFulltextSearchSvc; + final MatchUrlService myMatchUrlService; final DaoRegistry myDaoRegistry; final FhirContext myFhirCtx; diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java index fd3b71c9d26..f5d007a2bf5 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java @@ -10,6 +10,8 @@ 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; import ca.uhn.fhir.jpa.entity.TermCodeSystemVersion; @@ -20,6 +22,8 @@ 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; @@ -27,10 +31,12 @@ import ca.uhn.fhir.jpa.term.api.ITermReadSvcR4; import ca.uhn.fhir.jpa.test.BaseJpaTest; import ca.uhn.fhir.jpa.test.config.TestHSearchAddInConfig; import ca.uhn.fhir.jpa.test.config.TestR4Config; +import ca.uhn.fhir.jpa.test.util.TestHSearchEventDispatcher; import ca.uhn.fhir.parser.DataFormatException; import ca.uhn.fhir.parser.IParser; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.SearchTotalModeEnum; +import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId; import ca.uhn.fhir.rest.param.ReferenceParam; @@ -79,6 +85,9 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.test.annotation.DirtiesContext; @@ -89,7 +98,10 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.test.context.support.DependencyInjectionTestExecutionListener; import org.springframework.test.context.support.DirtiesContextTestExecutionListener; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.web.util.UriComponents; +import org.springframework.web.util.UriComponentsBuilder; +import javax.annotation.Nonnull; import javax.persistence.EntityManager; import java.io.IOException; import java.net.URLEncoder; @@ -120,6 +132,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @ExtendWith(SpringExtension.class) +@ExtendWith(MockitoExtension.class) @RequiresDocker @ContextConfiguration(classes = { TestR4Config.class, @@ -198,6 +211,17 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest { @RegisterExtension LogbackLevelOverrideExtension myLogbackLevelOverrideExtension = new LogbackLevelOverrideExtension(); + @Autowired + private IFulltextSearchSvc myIFulltextSearchSvc; + + @Autowired + private TestHSearchEventDispatcher myHSearchEventDispatcher; + + @Autowired + private MatchUrlService myMatchUrlService; + + @Mock private IHSearchEventListener mySearchEventListener; + @BeforeEach public void beforePurgeDatabase() { @@ -925,18 +949,22 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest { @Test public void simpleTokenSkipsSql() { - IIdType id = myTestDataBuilder.createObservation(List.of(myTestDataBuilder.withObservationCode("http://example.com/", "theCode"))); myCaptureQueriesListener.clear(); + myHSearchEventDispatcher.register(mySearchEventListener); - List ids = myTestDaoSearch.searchForIds("Observation?code=theCode"); + List result = searchForFastResources("Observation?code=theCode"); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); - assertThat(ids, hasSize(1)); - assertThat(ids, contains(id.getIdPart())); + assertThat(result, hasSize(1)); + assertEquals( ((Observation) result.get(0)).getId(), id.getIdPart() ); 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); } + @Test public void sortDoesntRequireSqlAnymore() { @@ -2244,6 +2272,29 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest { } + @Nested + public class NoIdsQuery { + + @Test + public void simpleTokenSkipsSql() { + 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 result = searchForFastResources("Observation?_sort=-code"); + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); + + 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); + } + + } + } @@ -2472,4 +2523,32 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest { } + + /** + * Search for resources in the first query, instead of searching for IDs first + */ + public List 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 runInTransaction( () -> myIFulltextSearchSvc.searchForResources(search.getResourceName(), map) ); + } + + + @Nonnull + private SystemRequestDetails fakeRequestDetailsFromUrl(String theQueryUrl) { + SystemRequestDetails request = new SystemRequestDetails(); + UriComponents uriComponents = UriComponentsBuilder.fromUriString(theQueryUrl).build(); + uriComponents.getQueryParams() + .forEach((key, value) -> request.addParameter(key, value.toArray(new String[0]))); + return request; + } + + } diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java index c405db594af..6dacadc4e5f 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.slf4j.Logger; @@ -72,13 +73,17 @@ import static org.mockito.Mockito.when; @SuppressWarnings({"unchecked"}) @ExtendWith(MockitoExtension.class) public class SearchCoordinatorSvcImplTest extends BaseSearchSvc{ - private static final Logger ourLog = LoggerFactory.getLogger(SearchCoordinatorSvcImplTest.class); + + @InjectMocks + private SearchCoordinatorSvcImpl mySvc; + + @Mock private SearchStrategyFactory mySearchStrategyFactory; + @Mock private ISearchCacheSvc mySearchCacheSvc; @Mock private ISearchResultCacheSvc mySearchResultCacheSvc; - private SearchCoordinatorSvcImpl mySvc; private Search myCurrentSearch; @Mock private IInterceptorBroadcaster myInterceptorBroadcaster; @@ -91,6 +96,8 @@ public class SearchCoordinatorSvcImplTest extends BaseSearchSvc{ @Mock private ISynchronousSearchSvc mySynchronousSearchSvc; + + @AfterEach public void after() { System.clearProperty(SearchCoordinatorSvcImpl.UNIT_TEST_CAPTURE_STACK); @@ -103,7 +110,6 @@ public class SearchCoordinatorSvcImplTest extends BaseSearchSvc{ myCurrentSearch = null; - mySvc = new SearchCoordinatorSvcImpl(); mySvc.setTransactionManagerForUnitTest(myTxManager); mySvc.setContextForUnitTest(ourCtx); mySvc.setSearchCacheServicesForUnitTest(mySearchCacheSvc, mySearchResultCacheSvc);