From 2e1f4c25f518c883b116200bec5c12156da9797d Mon Sep 17 00:00:00 2001 From: jmarchionatto <60409882+jmarchionatto@users.noreply.github.com> Date: Thu, 4 Aug 2022 18:19:06 -0400 Subject: [PATCH] =?UTF-8?q?Make=20sure=20that=20direct=20resource=20load?= =?UTF-8?q?=20path=20is=20only=20executed=20for=20synchro=E2=80=A6=20(#383?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make sure that direct resource load path is only executed for synchronous searches * Remove unneeded mockito extension * Only allow for resource loading from fulltext index when advanced search (indexed parameters) is also enabled Co-authored-by: juan.marchionatto --- .../jpa/search/SearchCoordinatorSvcImpl.java | 12 +++--- .../jpa/search/builder/SearchBuilder.java | 2 +- ...esourceDaoR4SearchWithElasticSearchIT.java | 43 +++++++++++++++++++ .../registry/SearchParamRegistryImpl.java | 2 +- .../ca/uhn/fhir/jpa/dao/TestDaoSearch.java | 37 +++++++++++++--- 5 files changed, 82 insertions(+), 14 deletions(-) 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 fc937ab3b3a..d6c7b78299b 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 @@ -361,13 +361,13 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { // SearchStrategyFactory.ISearchStrategy searchStrategy = mySearchStrategyFactory.pickStrategy(theResourceType, theParams, theRequestDetails); // 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(); - } - if (theParams.isLoadSynchronous() || loadSynchronousUpTo != null || isOffsetQuery) { + 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(); + } + 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/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 98f4fdebd0f..0ce619cdf37 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 @@ -969,9 +969,9 @@ public class SearchBuilder implements ISearchBuilder { * @return can we fetch from Hibernate Search? */ private boolean isLoadingFromElasticSearchSupported(Collection thePids) { - // is storage enabled? return myDaoConfig.isStoreResourceInHSearchIndex() && + myDaoConfig.isAdvancedHSearchIndexing() && // we don't support history thePids.stream().noneMatch(p->p.getVersion()!=null) && // skip the complexity for metadata in dstu2 diff --git a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java index c3c33256104..067bc1a84ab 100644 --- a/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java +++ b/hapi-fhir-jpaserver-elastic-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchWithElasticSearchIT.java @@ -35,12 +35,14 @@ 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.RequestDetails; import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId; import ca.uhn.fhir.rest.param.ReferenceParam; import ca.uhn.fhir.rest.param.StringOrListParam; import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.param.TokenParam; import ca.uhn.fhir.rest.param.TokenParamModifier; +import ca.uhn.fhir.rest.server.IPagingProvider; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.rest.server.util.ISearchParamRegistry; import ca.uhn.fhir.storage.test.BaseDateSearchDaoTests; @@ -127,6 +129,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; @ExtendWith(SpringExtension.class) @ExtendWith(MockitoExtension.class) @@ -1053,6 +1058,43 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl // assertThat(newMeta.getTag(), hasSize(2)); } + @Test + public void noDirectSearchWhenNotSynchronousOrOffsetQuery() { + myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-1"))); + myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-2"))); + myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-3"))); + myCaptureQueriesListener.clear(); + myHSearchEventDispatcher.register(mySearchEventListener); + + myTestDaoSearch.searchForBundleProvider("Observation?code=code-1,code-2,code-3", false); + + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); + + assertEquals(2, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size(), "we build the bundle with no sql"); + + // index only queried once for count + Mockito.verify(mySearchEventListener, Mockito.times(1)).hsearchEvent(IHSearchEventListener.HSearchEventType.SEARCH); + } + + + @Test + public void directSearchForOffsetQuery() { + myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-1"))); + IIdType idCode2 = myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-2"))); + IIdType idCode3 = myTestDataBuilder.createObservation(asArray(myTestDataBuilder.withObservationCode("http://example.com/", "code-3"))); + myCaptureQueriesListener.clear(); + myHSearchEventDispatcher.register(mySearchEventListener); + + List resultIds = myTestDaoSearch.searchForIds("Observation?code=code-1,code-2,code-3&_offset=1"); + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); + + assertThat(resultIds, containsInAnyOrder(idCode2.getIdPart(), idCode3.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); + } + } @@ -2622,4 +2664,5 @@ public class FhirResourceDaoR4SearchWithElasticSearchIT extends BaseJpaTest impl } + } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java index 94908af6d4a..ee58dc40d91 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/SearchParamRegistryImpl.java @@ -277,7 +277,7 @@ public class SearchParamRegistryImpl implements ISearchParamRegistry, IResourceC */ @PostConstruct public void registerListener() { - myResourceChangeListenerCache = myResourceChangeListenerRegistry.registerResourceResourceChangeListener("SearchParameter", SearchParameterMap.newSynchronous(), this, REFRESH_INTERVAL); + myResourceChangeListenerCache = myResourceChangeListenerRegistry.registerResourceResourceChangeListener("SearchParameter", SearchParameterMap.newSynchronous(), this, REFRESH_INTERVAL); } @PreDestroy diff --git a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java index ed5119bc44b..d43989f4a25 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java +++ b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java @@ -29,6 +29,8 @@ import ca.uhn.fhir.jpa.searchparam.ResourceSearch; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.server.IPagingProvider; +import ca.uhn.fhir.rest.server.IRestfulServerDefaults; import ca.uhn.fhir.rest.server.method.SortParameter; import org.hamcrest.Matcher; import org.hamcrest.MatcherAssert; @@ -48,6 +50,9 @@ import static org.hamcrest.Matchers.everyItem; import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.not; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; /** * Simplistic implementation of FHIR queries. @@ -138,24 +143,28 @@ public class TestDaoSearch { IBundleProvider result = searchForBundleProvider(theQueryUrl); // getAllResources is not safe as size is not always set - List resourceIds = result.getResources(0, Integer.MAX_VALUE) + return result.getResources(0, Integer.MAX_VALUE) .stream().map(resource -> resource.getIdElement().getIdPart()).collect(Collectors.toList()); - return resourceIds; } - public IBundleProvider searchForBundleProvider(String theQueryUrl) { + public IBundleProvider searchForBundleProvider(String theQueryUrl, boolean theSynchronousMode) { ResourceSearch search = myMatchUrlService.getResourceSearch(theQueryUrl); IFhirResourceDao dao = myDaoRegistry.getResourceDao(search.getResourceName()); SearchParameterMap map = search.getSearchParameterMap(); - map.setLoadSynchronous(true); + map.setLoadSynchronous(theSynchronousMode); SortSpec sort = (SortSpec) new SortParameter(myFhirCtx).translateQueryParametersIntoServerArgument(fakeRequestDetailsFromUrl(theQueryUrl), null); if (sort != null) { map.setSort(sort); } - IBundleProvider result = dao.search(map, fakeRequestDetailsFromUrl(theQueryUrl)); - return result; + // for asynchronous mode, we also need to make the request paginated ar synchronous is forced + SystemRequestDetails reqDetails = theSynchronousMode ? fakeRequestDetailsFromUrl(theQueryUrl) : fakePaginatedRequestDetailsFromUrl(theQueryUrl); + return dao.search(map, reqDetails); + } + + public IBundleProvider searchForBundleProvider(String theQueryUrl) { + return searchForBundleProvider(theQueryUrl, true); } public SearchParameterMap toSearchParameters(String theQueryUrl) { @@ -178,4 +187,20 @@ public class TestDaoSearch { .forEach((key, value) -> request.addParameter(key, value.toArray(new String[0]))); return request; } + + @Nonnull + private SystemRequestDetails fakePaginatedRequestDetailsFromUrl(String theQueryUrl) { + SystemRequestDetails spiedReqDetails = spy(SystemRequestDetails.class); + UriComponents uriComponents = UriComponentsBuilder.fromUriString(theQueryUrl).build(); + uriComponents.getQueryParams() + .forEach((key, value) -> spiedReqDetails.addParameter(key, value.toArray(new String[0]))); + + IPagingProvider mockPagingProvider = mock(IPagingProvider.class); + IRestfulServerDefaults mockServerDfts = mock(IRestfulServerDefaults.class); + doReturn(mockServerDfts).when(spiedReqDetails).getServer(); + doReturn(mockPagingProvider).when(mockServerDfts).getPagingProvider(); + return spiedReqDetails; + } + + }