Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6673-search-with-fulltextsearch-enabled-should-not-return-nulls.yaml

@@ -0,0 +1,8 @@
+---
+type: fix
+issue: 6673
+title: "When attempting to search for resources while both `AdvancedHSearchIndexing`
+        and `StoreResourcesInHibernateSearchIndex` are enabled, returned lists
+        could sometimes contain null entries.
+        This has been fixed.
+"

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/SearchConfig.java

@@ -27,8 +27,10 @@
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
 import ca.uhn.fhir.jpa.api.svc.ISearchCoordinatorSvc;
+import ca.uhn.fhir.jpa.dao.IJpaStorageResourceParser;
 import ca.uhn.fhir.jpa.dao.ISearchBuilder;
 import ca.uhn.fhir.jpa.dao.SearchBuilderFactory;
+import ca.uhn.fhir.jpa.dao.data.IResourceHistoryTableDao;
 import ca.uhn.fhir.jpa.dao.data.IResourceTagDao;
 import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
@@ -127,6 +129,12 @@ public class SearchConfig {
 	@Autowired
 	private HapiTransactionService myHapiTransactionService;
 
+	@Autowired
+	private IResourceHistoryTableDao myResourceHistoryTableDao;
+
+	@Autowired
+	private IJpaStorageResourceParser myJpaStorageResourceParser;
+
 	@Bean
 	public ISearchCoordinatorSvc searchCoordinatorSvc() {
 		return new SearchCoordinatorSvcImpl(
@@ -167,6 +175,8 @@ public ISearchBuilder newSearchBuilder(String theResourceName, Class<? extends I
 				myDaoRegistry,
 				myContext,
 				myIdHelperService,
+				myResourceHistoryTableDao,
+				myJpaStorageResourceParser,
 				theResourceType);
 	}
 

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

@@ -2218,7 +2218,9 @@ private Stream<T> pidsToResource(
 				.flatMap(pidChunk -> searchBuilder.loadResourcesByPid(pidChunk, theRequest).stream());
 		// apply interceptors
 		return resourceStream
-				.flatMap(resource -> invokeStoragePreAccessResources(theRequest, resource).stream())
+				.flatMap(resource -> resource == null
+						? Stream.empty()
+						: invokeStoragePreAccessResources(theRequest, resource).stream())
 				.flatMap(resource -> Optional.ofNullable(invokeStoragePreShowResources(theRequest, resource)).stream());
 	}
 

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

@@ -224,17 +224,19 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 
 	private SearchQueryProperties mySearchProperties;
 
-	@Autowired(required = false)
 	private IFulltextSearchSvc myFulltextSearchSvc;
 
 	@Autowired(required = false)
-	private IElasticsearchSvc myIElasticsearchSvc;
+	public void setFullTextSearch(IFulltextSearchSvc theFulltextSearchSvc) {
+		myFulltextSearchSvc = theFulltextSearchSvc;
+	}
+
+	private IResourceHistoryTableDao myResourceHistoryTableDao;
 
-	@Autowired
 	private IJpaStorageResourceParser myJpaStorageResourceParser;
 
-	@Autowired
-	private IResourceHistoryTableDao myResourceHistoryTableDao;
+	@Autowired(required = false)
+	private IElasticsearchSvc myIElasticsearchSvc;
 
 	@Autowired
 	private IResourceHistoryTagDao myResourceHistoryTagDao;
@@ -259,6 +261,8 @@ public SearchBuilder(
 			DaoRegistry theDaoRegistry,
 			FhirContext theContext,
 			IIdHelperService theIdHelperService,
+			IResourceHistoryTableDao theResourceHistoryTagDao,
+			IJpaStorageResourceParser theIJpaStorageResourceParser,
 			Class<? extends IBaseResource> theResourceType) {
 		myResourceName = theResourceName;
 		myResourceType = theResourceType;
@@ -274,6 +278,8 @@ public SearchBuilder(
 		myDaoRegistry = theDaoRegistry;
 		myContext = theContext;
 		myIdHelperService = theIdHelperService;
+		myResourceHistoryTableDao = theResourceHistoryTagDao;
+		myJpaStorageResourceParser = theIJpaStorageResourceParser;
 
 		mySearchProperties = new SearchQueryProperties();
 	}
@@ -1266,20 +1272,14 @@ private void doLoadPids(
 			}
 
 			IBaseResource resource = null;
-			if (next != null) {
-				resource = myJpaStorageResourceParser.toResource(
-						resourceType, next, tagMap.get(next.getResourceId()), theForHistoryOperation);
-			}
+			resource = myJpaStorageResourceParser.toResource(
+					resourceType, next, tagMap.get(next.getResourceId()), theForHistoryOperation);
 			if (resource == null) {
-				if (next != null) {
-					ourLog.warn(
-							"Unable to find resource {}/{}/_history/{} in database",
-							next.getResourceType(),
-							next.getIdDt().getIdPart(),
-							next.getVersion());
-				} else {
-					ourLog.warn("Unable to find resource in database.");
-				}
+				ourLog.warn(
+						"Unable to find resource {}/{}/_history/{} in database",
+						next.getResourceType(),
+						next.getIdDt().getIdPart(),
+						next.getVersion());
 				continue;
 			}
 
@@ -1295,12 +1295,15 @@ private void doLoadPids(
 				ResourceMetadataKeyEnum.ENTRY_SEARCH_MODE.put(resource, BundleEntrySearchModeEnum.MATCH);
 			}
 
+			// ensure there's enough space; "<=" because of 0-indexing
+			while (theResourceListToPopulate.size() <= index) {
+				theResourceListToPopulate.add(null);
+			}
 			theResourceListToPopulate.set(index, resource);
 		}
 	}
 
 	private Map<JpaPid, Collection<BaseTag>> getResourceTagMap(Collection<ResourceHistoryTable> theHistoryTables) {
-
 		switch (myStorageSettings.getTagStorageMode()) {
 			case VERSIONED:
 				return getPidToTagMapVersioned(theHistoryTables);
@@ -1410,13 +1413,14 @@ public void loadResourcesByPid(
 		assert new HashSet<>(thePids).size() == thePids.size() : "PID list contains duplicates: " + thePids;
 
 		Map<Long, Integer> position = new HashMap<>();
+		int index = 0;
 		for (JpaPid next : thePids) {
-			position.put(next.getId(), theResourceListToPopulate.size());
-			theResourceListToPopulate.add(null);
+			position.put(next.getId(), index++);
 		}
 
 		// Can we fast track this loading by checking elastic search?
-		if (isLoadingFromElasticSearchSupported(thePids)) {
+		boolean isUsingElasticSearch = isLoadingFromElasticSearchSupported(thePids);
+		if (isUsingElasticSearch) {
 			try {
 				theResourceListToPopulate.addAll(loadResourcesFromElasticSearch(thePids));
 				return;

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/builder/SearchBuilderTest.java

@@ -1,32 +1,58 @@
 package ca.uhn.fhir.jpa.search.builder;
 
 import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
+import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
+import ca.uhn.fhir.jpa.dao.IJpaStorageResourceParser;
+import ca.uhn.fhir.jpa.dao.data.IResourceHistoryTableDao;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
+import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
+import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.server.util.FhirContextSearchParamRegistry;
 import ca.uhn.fhir.rest.server.util.ISearchParamRegistry;
+import org.hl7.fhir.instance.model.api.IBaseResource;
+import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Spy;
 import org.mockito.junit.jupiter.MockitoExtension;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(MockitoExtension.class)
 class SearchBuilderTest {
 
 	public static final FhirContext ourCtx = FhirContext.forR4Cached();
+
+	@Mock
+	private IResourceHistoryTableDao myResourceHistoryTableDao;
+
+	@Mock
+	private IJpaStorageResourceParser myJpaStorageResourceParser;
+
 	@Spy
 	private FhirContext myFhirContext = ourCtx;
 
@@ -36,9 +62,19 @@ class SearchBuilderTest {
 	@Spy
 	private PartitionSettings myPartitionSettings = new PartitionSettings();
 
+	@Spy
+	private JpaStorageSettings myStorageSettings = new JpaStorageSettings();
+
+	@Mock
+	private IFulltextSearchSvc myFulltextSearchSvc;
+
 	@Mock(strictness = Mock.Strictness.LENIENT)
 	private DaoRegistry myDaoRegistry;
 
+	/**
+	 * NB: only the fields that are injected in the constructor will be injected by
+	 * mockito
+	 */
 	@InjectMocks
 	private SearchBuilder mySearchBuilder;
 
@@ -82,6 +118,72 @@ void testPartitionBySizeAndPartitionId_ReuseIfSmallEnoughAndAllSamePartition() {
 		assertSame(input, actual.iterator().next());
 	}
 
+	@ParameterizedTest
+	@ValueSource(booleans = { true, false })
+	@SuppressWarnings("unchecked")
+	public void loadResourcesByPid_containsNoNullElements(boolean theUseElasticSearch) {
+		// setup
+		List<JpaPid> pids = new ArrayList<>();
+		List<JpaPid> includedPids = new ArrayList<>();
+		List<IBaseResource> resources = new ArrayList<>();
+		RequestDetails requestDetails = new SystemRequestDetails();
+
+		pids.add(JpaPid.fromId(1L));
+		Patient patient = new Patient();
+		patient.setId("Patient/1");
+
+		if (theUseElasticSearch) {
+			mySearchBuilder.setFullTextSearch(myFulltextSearchSvc);
+
+			myStorageSettings.setStoreResourceInHSearchIndex(true);
+			myStorageSettings.setHibernateSearchIndexSearchParams(true);
+			myStorageSettings.setHibernateSearchIndexFullText(true);
+		}
+
+		// when
+		// (these are just for output values)
+		if (!theUseElasticSearch) {
+			ResourceHistoryTable ht = new ResourceHistoryTable();
+			ht.setResourceId(1L);
+			ht.setResourceType("Patient");
+
+			when(myResourceHistoryTableDao.findCurrentVersionsByResourcePidsAndFetchResourceTable(any()))
+				.thenReturn(List.of(ht));
+			when(myJpaStorageResourceParser.toResource(any(Class.class), any(ResourceHistoryTable.class), any(), anyBoolean()))
+				.thenReturn(patient);
+		} else {
+			when(myFulltextSearchSvc.getResources(any(List.class)))
+				.thenReturn(List.of(patient));
+		}
+
+		// test
+		mySearchBuilder.loadResourcesByPid(
+			pids,
+			includedPids,
+			resources,
+			false,
+			requestDetails
+		);
+
+		// verify
+		assertFalse(resources.contains(null));
+
+		// validating the returns for completion's sake
+		assertEquals(1, resources.size());
+		if (theUseElasticSearch) {
+			// if using elastisearch, we want to know the getResources was invoked
+			// with the pid list we sent in
+			ArgumentCaptor<Collection<Long>> pidCapture = ArgumentCaptor.forClass(Collection.class);
+			verify(myFulltextSearchSvc).getResources(pidCapture.capture());
+			assertNotNull(pidCapture.getValue());
+			assertEquals(pids.size(), pidCapture.getValue().size());
+			assertTrue(pidCapture.getValue().contains(1L)); // the only element
+		}
+
+		// reset
+		myStorageSettings = new JpaStorageSettings();
+	}
+
 	@Test
 	void testPartitionBySizeAndPartitionId_Partitioned() {
 		List<JpaPid> input = List.of(