Remove duplicate field in JPA bundle provider (#4435)

* Remove duplicate field in JPA bundle provider

* Add changelog

* Work on tests

* Test fixes

* Test fix

* Add logging to detect an intermittent

* Test fix

* Address review comment

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4435-jpa-bundle-provider-api-cleanup.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 4435
+title: "Java API callers of the JPA IFhirResourceDao#search() method could experience a NPE
+   if they invoked getResources() multiple times on the same response object. This is an edge case
+   that is very unlikely to occur, but is now corrected."

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

@@ -129,6 +129,17 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
 	public PersistedJpaBundleProvider(RequestDetails theRequest, Search theSearch) {
 		myRequest = theRequest;
 		mySearchEntity = theSearch;
+		myUuid = theSearch.getUuid();
+	}
+
+	protected Search getSearchEntity() {
+		return mySearchEntity;
+	}
+
+	// Note: Leave as protected, HSPC depends on this
+	@SuppressWarnings("WeakerAccess")
+	protected void setSearchEntity(Search theSearchEntity) {
+		mySearchEntity = theSearchEntity;
 	}
 
 	/**
@@ -292,11 +303,8 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
 	@Nonnull
 	@Override
 	public List<IBaseResource> getResources(final int theFromIndex, final int theToIndex) {
-		myTxService.withRequest(myRequest).execute(() -> {
+		boolean entityLoaded = ensureSearchEntityLoaded();
-			boolean entityLoaded = ensureSearchEntityLoaded();
+		assert entityLoaded;
-			assert entityLoaded;
-		});
-
 		assert mySearchEntity != null;
 		assert mySearchEntity.getSearchType() != null;
 
@@ -357,12 +365,6 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
 		myTxService = theTxManager;
 	}
 
-	// Note: Leave as protected, HSPC depends on this
-	@SuppressWarnings("WeakerAccess")
-	protected void setSearchEntity(Search theSearchEntity) {
-		mySearchEntity = theSearchEntity;
-	}
-
 	@Override
 	public Integer size() {
 		ensureSearchEntityLoaded();

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

@@ -44,7 +44,6 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl
 	private static final Logger ourLog = LoggerFactory.getLogger(PersistedJpaSearchFirstPageBundleProvider.class);
 	private final SearchTask mySearchTask;
 	private final ISearchBuilder mySearchBuilder;
-	private final Search mySearch;
 
 	/**
 	 * Constructor
@@ -54,13 +53,13 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl
 		setSearchEntity(theSearch);
 		mySearchTask = theSearchTask;
 		mySearchBuilder = theSearchBuilder;
-		mySearch = theSearch;
 	}
 
 	@Nonnull
 	@Override
 	public List<IBaseResource> getResources(int theFromIndex, int theToIndex) {
-		QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(mySearch);
+		ensureSearchEntityLoaded();
+		QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(getSearchEntity());
 
 		mySearchTask.awaitInitialSync();
 
@@ -68,7 +67,9 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl
 		final List<JpaPid> pids = mySearchTask.getResourcePids(theFromIndex, theToIndex);
 		ourLog.trace("Done fetching search resource PIDs");
 
-		List<IBaseResource> retVal = myTxService.withRequest(myRequest).execute(() -> toResourceList(mySearchBuilder, pids));
+		List<IBaseResource> retVal = myTxService.withRequest(myRequest).execute(() -> {
+			return toResourceList(mySearchBuilder, pids);
+		});
 
 		long totalCountWanted = theToIndex - theFromIndex;
 		long totalCountMatch = (int) retVal
@@ -77,7 +78,8 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl
 			.count();
 
 		if (totalCountMatch < totalCountWanted) {
-			if (mySearch.getStatus() == SearchStatusEnum.PASSCMPLET) {
+			if (getSearchEntity().getStatus() == SearchStatusEnum.PASSCMPLET
+				|| ((getSearchEntity().getStatus() == SearchStatusEnum.FINISHED && getSearchEntity().getNumFound() >= theToIndex))) {
 
 				/*
 				 * This is a bit of complexity to account for the possibility that
@@ -115,7 +117,8 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl
 		Integer size = mySearchTask.awaitInitialSync();
 		ourLog.trace("size() - Finished waiting for local sync");
 
-		QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(mySearch);
+		ensureSearchEntityLoaded();
+		QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(getSearchEntity());
 		if (size != null) {
 			return size;
 		}

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

@@ -122,7 +122,7 @@ public class DatabaseSearchCacheSvcImpl implements ISearchCacheSvc {
 				Search search = mySearchDao.findById(theSearch.getId()).orElse(theSearch);
 
 				if (search.getStatus() != SearchStatusEnum.PASSCMPLET) {
-					throw new IllegalStateException(Msg.code(1167) + "Can't change to LOADING because state is " + theSearch.getStatus());
+					throw new IllegalStateException(Msg.code(1167) + "Can't change to LOADING because state is " + search.getStatus());
 				}
 				search.setStatus(SearchStatusEnum.LOADING);
 				Search newSearch = mySearchDao.save(search);

hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java

@@ -104,7 +104,7 @@ public class SearchCoordinatorSvcImplTest extends BaseSearchSvc{
 	@Mock
 	private ISynchronousSearchSvc mySynchronousSearchSvc;
 	@Spy
-	protected FhirContext myContext = FhirContext.forR4();
+	protected FhirContext myContext = FhirContext.forDstu2Cached();
 
 	@Spy
 	private ExceptionService myExceptionSvc = new ExceptionService(myContext);
@@ -307,6 +307,7 @@ public class SearchCoordinatorSvcImplTest extends BaseSearchSvc{
 			retVal.setDaoConfigForUnitTest(new DaoConfig());
 			retVal.setTxServiceForUnitTest(myTransactionService);
 			retVal.setSearchCoordinatorSvcForUnitTest(mySvc);
+			retVal.setContext(myContext);
 			return retVal;
 		});
 	}

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

@@ -19,6 +19,7 @@ import ca.uhn.fhir.jpa.util.SqlQuery;
 import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
 import ca.uhn.fhir.rest.api.SortSpec;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
+import ca.uhn.fhir.rest.param.HasParam;
 import ca.uhn.fhir.rest.param.ReferenceParam;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.server.SimpleBundleProvider;
@@ -57,13 +58,16 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestMethodOrder;
 import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Slice;
+import org.springframework.util.comparator.ComparableComparator;
 
 import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Date;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -784,6 +788,89 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 	}
 
 
+	@Test
+	public void testSearchAndPageThroughResults_SmallChunksOnSameBundleProvider() {
+		List<String> ids = create150Patients();
+
+		myCaptureQueriesListener.clear();
+		IBundleProvider search = myPatientDao.search(new SearchParameterMap(), mySrd);
+		List<String> foundIds = new ArrayList<>();
+		for (int i = 0; i < 170; i += 10) {
+			List<IBaseResource> nextChunk = search.getResources(i, i + 10);
+			nextChunk.forEach(t->foundIds.add(t.getIdElement().toUnqualifiedVersionless().getValue()));
+		}
+
+		ids.sort(new ComparableComparator<>());
+		foundIds.sort(new ComparableComparator<>());
+		assertEquals(ids, foundIds);
+
+		// This really generates a surprising number of selects and commits. We
+		// could stand to reduce this!
+		myCaptureQueriesListener.logSelectQueries();
+		assertEquals(56, myCaptureQueriesListener.countSelectQueries());
+		assertEquals(71, myCaptureQueriesListener.getCommitCount());
+		assertEquals(0, myCaptureQueriesListener.getRollbackCount());
+	}
+
+	@Test
+	public void testSearchAndPageThroughResults_LargeChunksOnIndependentBundleProvider() {
+		List<String> ids = create150Patients();
+
+		myCaptureQueriesListener.clear();
+		IBundleProvider search = myPatientDao.search(new SearchParameterMap(), mySrd);
+		List<String> foundIds = new ArrayList<>();
+		for (int i = 0; i < 170; i += 60) {
+			List<IBaseResource> nextChunk = search.getResources(i, i + 60);
+			nextChunk.forEach(t->foundIds.add(t.getIdElement().toUnqualifiedVersionless().getValue()));
+			search = myPagingProvider.retrieveResultList(mySrd, search.getUuid());
+		}
+
+		ids.sort(new ComparableComparator<>());
+		foundIds.sort(new ComparableComparator<>());
+		assertEquals(ids, foundIds);
+
+		assertEquals(22, myCaptureQueriesListener.countSelectQueries());
+		assertEquals(21, myCaptureQueriesListener.getCommitCount());
+		assertEquals(0, myCaptureQueriesListener.getRollbackCount());
+	}
+
+	@Test
+	public void testSearchAndPageThroughResults_LargeChunksOnSameBundleProvider_Synchronous() {
+		List<String> ids = create150Patients();
+
+		myCaptureQueriesListener.clear();
+		IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd);
+		List<String> foundIds = new ArrayList<>();
+		for (int i = 0; i < 170; i += 60) {
+			List<IBaseResource> nextChunk = search.getResources(i, i + 60);
+			nextChunk.forEach(t->foundIds.add(t.getIdElement().toUnqualifiedVersionless().getValue()));
+		}
+
+		ids.sort(new ComparableComparator<>());
+		foundIds.sort(new ComparableComparator<>());
+		assertEquals(ids, foundIds);
+
+		assertEquals(2, myCaptureQueriesListener.countSelectQueries());
+		assertEquals(1, myCaptureQueriesListener.getCommitCount());
+		assertEquals(0, myCaptureQueriesListener.getRollbackCount());
+	}
+
+	@Nonnull
+	private List<String> create150Patients() {
+		BundleBuilder b = new BundleBuilder(myFhirContext);
+		List<String> ids = new ArrayList<>();
+		for (int i = 0; i < 150; i++) {
+			Patient p = new Patient();
+			String nextId = "Patient/A" + i;
+			ids.add(nextId);
+			p.setId(nextId);
+			b.addTransactionUpdateEntry(p);
+		}
+		mySystemDao.transaction(mySrd, b.getBundleTyped());
+		return ids;
+	}
+
+
 	@Test
 	public void testSearchUsingOffsetMode_Explicit() {
 		for (int i = 0; i < 10; i++) {

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/interceptor/ConsentInterceptorTest.java

@@ -507,8 +507,12 @@ public class ConsentInterceptorTest {
 
 	@Test
 	public void testPage_SeeResourceReplacesInnerResource() throws IOException {
-		ourPatientProvider.store((Patient) new Patient().setActive(true).setId("PTA"));
+		Patient pta = (Patient) new Patient().setActive(true).setId("PTA");
-		ourPatientProvider.store((Patient) new Patient().setActive(false).setId("PTB"));
+		pta.addIdentifier().setSystem("OldSystemA");
+		ourPatientProvider.store(pta);
+		Patient ptb = (Patient) new Patient().setActive(false).setId("PTB");
+		ptb.addIdentifier().setSystem("OldSystemB");
+		ourPatientProvider.store(ptb);
 
 		when(myConsentSvc.startOperation(any(), any())).thenReturn(ConsentOutcome.PROCEED);
 		when(myConsentSvc.canSeeResource(any(), any(), any())).thenReturn(ConsentOutcome.PROCEED);