fixing tests

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

@@ -107,18 +107,6 @@ import org.springframework.jdbc.core.JdbcTemplate;
 import org.springframework.jdbc.core.SingleColumnRowMapper;
 import org.springframework.transaction.support.TransactionSynchronizationManager;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.Set;
-import java.util.stream.Collectors;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.persistence.EntityManager;
@@ -128,6 +116,17 @@ import javax.persistence.Query;
 import javax.persistence.Tuple;
 import javax.persistence.TypedQuery;
 import javax.persistence.criteria.CriteriaBuilder;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 import static ca.uhn.fhir.jpa.search.builder.QueryStack.LOCATION_POSITION;
 import static org.apache.commons.lang3.StringUtils.defaultString;
@@ -459,9 +458,11 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 			}
 		} else {
 			// do everything in the database.
-			Optional<SearchQueryExecutor> query = createChunkedQuery(
+			createChunkedQuery(
-					theParams, sort, theOffset, theMaximumResults, theCountOnlyFlag, theRequest, null);
+					theParams, sort, theOffset, theMaximumResults, theCountOnlyFlag, theRequest, null,
-			query.ifPresent(queries::add);
+				queries
+			);
+//			query.ifPresent(queries::add);
 		}
 
 		return queries;
@@ -551,9 +552,10 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		if (thePids.size() < getMaximumPageSize()) {
 			normalizeIdListForLastNInClause(thePids);
 		}
-		Optional<SearchQueryExecutor> query =
+//		Optional<SearchQueryExecutor> query =
-				createChunkedQuery(theParams, sort, theOffset, thePids.size(), theCount, theRequest, thePids);
+				createChunkedQuery(theParams, sort, theOffset, thePids.size(), theCount, theRequest, thePids,
-		query.ifPresent(t -> theQueries.add(t));
+					theQueries);
+//		query.ifPresent(t -> theQueries.add(t));
 	}
 
 	/**
@@ -596,14 +598,16 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		}
 	}
 
-	private Optional<SearchQueryExecutor> createChunkedQuery(
+	private void createChunkedQuery(
 			SearchParameterMap theParams,
 			SortSpec sort,
 			Integer theOffset,
 			Integer theMaximumResults,
 			boolean theCountOnlyFlag,
 			RequestDetails theRequest,
-			List<Long> thePidList) {
+			List<Long> thePidList,
+			List<ISearchQueryExecutor> theSearchQueryExecutors
+	) {
 		String sqlBuilderResourceName = myParams.getEverythingMode() == null ? myResourceName : null;
 		SearchQueryBuilder sqlBuilder = new SearchQueryBuilder(
 				myContext,
@@ -655,11 +659,35 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 				GeneratedSql allTargetsSql = fetchPidsSqlBuilder.generate(theOffset, myMaxResultsToFetch);
 				String sql = allTargetsSql.getSql();
 				Object[] args = allTargetsSql.getBindVariables().toArray(new Object[0]);
+
 				List<Long> output = jdbcTemplate.query(sql, args, new SingleColumnRowMapper<>(Long.class));
-				if (myAlsoIncludePids == null) {
+//				if (myAlsoIncludePids == null) {
-					myAlsoIncludePids = new ArrayList<>(output.size());
+//					myAlsoIncludePids = new ArrayList<>(output.size());
-				}
+//				}
-				myAlsoIncludePids.addAll(JpaPid.fromLongList(output));
+//				myAlsoIncludePids.addAll(JpaPid.fromLongList(output));
+
+				theSearchQueryExecutors.add(new ISearchQueryExecutor() {
+					private final Iterator<Long> myIterator = output.iterator();
+
+					@Override
+					public void close() {
+
+					}
+
+					@Override
+					public boolean hasNext() {
+						return myIterator.hasNext();
+					}
+
+					@Override
+					public Long next() {
+						return myIterator.next();
+					}
+				});
+
+//				theSearchQueryExecutors.add(
+//					mySqlBuilderFactory.newSearchQueryExecutor(allTargetsSql, myMaxResultsToFetch)
+//				);
 			}
 
 			List<String> typeSourceResources = new ArrayList<>();
@@ -758,11 +786,13 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		 */
 		GeneratedSql generatedSql = sqlBuilder.generate(theOffset, myMaxResultsToFetch);
 		if (generatedSql.isMatchNothing()) {
-			return Optional.empty();
+//			return Optional.empty();
+			return;
 		}
 
 		SearchQueryExecutor executor = mySqlBuilderFactory.newSearchQueryExecutor(generatedSql, myMaxResultsToFetch);
-		return Optional.of(executor);
+		theSearchQueryExecutors.add(executor);
+//		return Optional.of(executor);
 	}
 
 	private Collection<String> extractTypeSourceResourcesFromParams() {
@@ -2024,6 +2054,12 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 							// Update iterator with next chunk if necessary.
 							if (!myResultsIterator.hasNext()) {
 								retrieveNextIteratorQuery();
+
+								// if our new results iterator is also empty
+								// we're done here
+								if (!myResultsIterator.hasNext()) {
+									break;
+								}
 							}
 
 							Long nextLong = myResultsIterator.next();

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

@@ -334,7 +334,7 @@ public class SearchTask implements Callable<Void> {
 
 						if (theResultIter.hasNext() == false) {
 							int skippedCount = theResultIter.getSkippedCount();
-							ourLog.trace(
+							ourLog.info(
 									"MaxToFetch[{}] SkippedCount[{}] CountSavedThisPass[{}] CountSavedThisTotal[{}] AdditionalPrefetchRemaining[{}]",
 									myMaxResultsToFetch,
 									skippedCount,
@@ -578,13 +578,7 @@ public class SearchTask implements Callable<Void> {
 		// we only use the values in SearchPreFetchThresholds
 		// but if there is a count...
 		if (myParams.getCount() != null) {
-			// we want either the max page size or the requested count size
-			// (+1 iff count == max page size)
 			minWanted = Math.min(myParams.getCount(), myPagingProvider.getMaximumPageSize());
-			// Always fetch one past this page size, so we know if there is a next page.
-			if (minWanted == myParams.getCount()) {
-				minWanted += 1;
-			}
 			minWanted += currentlyLoaded;
 		}
 
@@ -600,8 +594,11 @@ public class SearchTask implements Callable<Void> {
 			if (next == -1) {
 				sb.setMaxResultsToFetch(null);
 			} else {
+				// we want at least 1 more than our requested amount
+				// so we know that there are other results
+				// (in case we get the exact amount back)
 				myMaxResultsToFetch = Math.max(next, minWanted);
-				sb.setMaxResultsToFetch(myMaxResultsToFetch);
+				sb.setMaxResultsToFetch(myMaxResultsToFetch + 1);
 			}
 
 			if (iter.hasNext()) {
@@ -692,9 +689,9 @@ public class SearchTask implements Callable<Void> {
 
 	/**
 	 * Does the query but only for the count.
-	 * @param myParamWantOnlyCount - if count query is wanted only
+	 * @param theParamWantOnlyCount - if count query is wanted only
 	 */
-	private void doCountOnlyQuery(boolean myParamWantOnlyCount) {
+	private void doCountOnlyQuery(boolean theParamWantOnlyCount) {
 		ourLog.trace("Performing count");
 		ISearchBuilder sb = newSearchBuilder();
 
@@ -716,7 +713,7 @@ public class SearchTask implements Callable<Void> {
 				.withRequestPartitionId(myRequestPartitionId)
 				.execute(() -> {
 					mySearch.setTotalCount(count.intValue());
-					if (myParamWantOnlyCount) {
+					if (theParamWantOnlyCount) {
 						mySearch.setStatus(SearchStatusEnum.FINISHED);
 					}
 					doSaveSearch();

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

@@ -93,7 +93,7 @@ public class ConsentEventsDaoR4Test extends BaseJpaR4SystemTest {
 		List<String> returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
 		assertEquals(myObservationIds.subList(0, 10), returnedIdValues);
 		assertEquals(1, hitCount.get());
-		assertEquals(myObservationIds.subList(0, 20), interceptedResourceIds);
+		assertEquals(myObservationIds.subList(0, 21), interceptedResourceIds);
 
 		// Fetch the next 30 (do cross a fetch boundary)
 		outcome = myPagingProvider.retrieveResultList(mySrd, outcome.getUuid());
@@ -125,7 +125,7 @@ public class ConsentEventsDaoR4Test extends BaseJpaR4SystemTest {
 		List<String> returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
 		assertEquals(myObservationIdsEvenOnly.subList(0, 10), returnedIdValues);
 		assertEquals(1, hitCount.get());
-		assertEquals(myObservationIds.subList(0, 20), interceptedResourceIds, "Wrong response from " + outcome.getClass());
+		assertEquals(myObservationIds.subList(0, 21), interceptedResourceIds, "Wrong response from " + outcome.getClass());
 
 		// Fetch the next 30 (do cross a fetch boundary)
 		String searchId = outcome.getUuid();

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

@@ -10,6 +10,7 @@ import ca.uhn.fhir.rest.param.DateParam;
 import ca.uhn.fhir.rest.param.StringParam;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.util.HapiExtensions;
+import joptsimple.internal.Strings;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.BooleanType;
 import org.hl7.fhir.r4.model.DateType;
@@ -26,6 +27,7 @@ import org.springframework.beans.factory.annotation.Autowired;
 
 import java.util.Comparator;
 import java.util.List;
+import java.util.stream.Collectors;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
@@ -178,7 +180,9 @@ public class FhirResourceDaoR4ComboNonUniqueParamTest extends BaseComboParamsR4T
 		IIdType id1 = createPatient1();
 		assertNotNull(id1);
 
-		assertEquals(0, myCaptureQueriesListener.countSelectQueries());
+		assertEquals(0, myCaptureQueriesListener.countSelectQueries(),
+			String.join(",", "\n" + myCaptureQueriesListener.getSelectQueries().stream().map(q -> q.getThreadName()).collect(Collectors.toList()))
+		);
 		assertEquals(12, myCaptureQueriesListener.countInsertQueries());
 		assertEquals(0, myCaptureQueriesListener.countUpdateQueries());
 		assertEquals(0, myCaptureQueriesListener.countDeleteQueries());

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

@@ -421,7 +421,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 
 		runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
-			assertEquals(20, search.getNumFound());
+			assertEquals(21, search.getNumFound());
 			assertEquals(search.getNumFound(), mySearchResultDao.count());
 			assertNull(search.getTotalCount());
 			assertEquals(1, search.getVersion().intValue());
@@ -462,7 +462,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 		runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
 			assertEquals(SearchStatusEnum.PASSCMPLET, search.getStatus());
-			assertEquals(50, search.getNumFound());
+			assertEquals(51, search.getNumFound());
 			assertEquals(search.getNumFound(), mySearchResultDao.count());
 			assertNull(search.getTotalCount());
 			assertEquals(3, search.getVersion().intValue());
@@ -501,9 +501,9 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 		 */
 		runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
-			assertEquals(190, search.getNumFound());
+			assertEquals(191, search.getNumFound());
 			assertEquals(search.getNumFound(), mySearchResultDao.count());
-			assertEquals(190, search.getTotalCount().intValue());
+			assertEquals(191, search.getTotalCount().intValue());
 			assertEquals(5, search.getVersion().intValue());
 			assertEquals(SearchStatusEnum.FINISHED, search.getStatus());
 		});
@@ -513,10 +513,10 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 		 */
 
 		ids = toUnqualifiedVersionlessIdValues(results, 180, 200, false);
-		assertEquals(10, ids.size());
+		assertEquals(11, ids.size());
 		assertEquals("Patient/PT00180", ids.get(0));
 		assertEquals("Patient/PT00189", ids.get(9));
-		assertEquals(190, myDatabaseBackedPagingProvider.retrieveResultList(null, uuid).size().intValue());
+		assertEquals(191, myDatabaseBackedPagingProvider.retrieveResultList(null, uuid).size().intValue());
 
 
 	}
@@ -589,10 +589,10 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 		await().until(() -> runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
 			return search.getNumFound();
-		}), equalTo(20));
+		}), equalTo(21));
 		runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
-			assertEquals(20, search.getNumFound());
+			assertEquals(21, search.getNumFound());
 			assertEquals(search.getNumFound(), mySearchResultDao.count());
 			assertNull(search.getTotalCount());
 			assertEquals(1, search.getVersion().intValue());
@@ -649,14 +649,14 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test {
 		 */
 
 		waitForSize(
-			20,
+			21,
 			10000,
 			() -> runInTransaction(() -> mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException("")).getNumFound()),
 			() -> "Wanted 20: " + runInTransaction(() -> mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException("")).toString()));
 
 		runInTransaction(() -> {
 			Search search = mySearchEntityDao.findByUuidAndFetchIncludes(uuid).orElseThrow(() -> new InternalErrorException(""));
-			assertEquals(20, search.getNumFound());
+			assertEquals(21, search.getNumFound());
 			assertEquals(search.getNumFound(), mySearchResultDao.count());
 			assertNull(search.getTotalCount());
 			assertEquals(1, search.getVersion().intValue());

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/PatientEverythingPaginationR4Test.java

@@ -17,19 +17,30 @@ import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.springframework.beans.factory.annotation.Autowired;
 
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import static org.hl7.fhir.instance.model.api.IBaseBundle.LINK_NEXT;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @SuppressWarnings("Duplicates")
 public class PatientEverythingPaginationR4Test extends BaseResourceProviderR4Test {
 
 	private int myOriginalServerDefaultPageSize;
 
+	@Autowired
+	JpaStorageSettings myStorageSettings;
+
 	@BeforeEach
 	public void beforeDisableResultReuse() {
 		myStorageSettings.setReuseCachedSearchResultsForMillis(null);
@@ -65,21 +76,19 @@ public class PatientEverythingPaginationR4Test extends BaseResourceProviderR4Tes
 	public void testEverythingPaginatesThroughAllPatients_whenCountIsEqualToMaxPageSize() throws IOException {
 		// setup
 		int totalPatients = 54;
-		for (int i = 0; i < totalPatients; i++) {
+		createPatients(totalPatients);
-			Patient patient = new Patient();
+
-			patient.addName().setFamily("lastn").addGiven("name");
+		String url = myServerBase + "/Patient/$everything?_format=json&_count=" + BasePagingProvider.DEFAULT_MAX_PAGE_SIZE;
-			myPatientDao.create(patient, new SystemRequestDetails()).getId().toUnqualifiedVersionless();
-		}
 
 		// test
-		Bundle bundle = fetchBundle(myServerBase + "/Patient/$everything?_format=json&_count=" + BasePagingProvider.DEFAULT_MAX_PAGE_SIZE);
+		Bundle bundle = fetchBundle(url);
 
 		// verify
 		List<Patient> patientsFirstPage = BundleUtil.toListOfResourcesOfType(myFhirContext, bundle, Patient.class);
 		assertEquals(50, patientsFirstPage.size());
 
 		String nextUrl = BundleUtil.getLinkUrlOfType(myFhirContext, bundle, LINK_NEXT);
-		System.out.println(nextUrl);
+
 		assertNotNull(nextUrl);
 		Bundle page2 = fetchBundle(nextUrl);
 		assertNotNull(page2);
@@ -88,6 +97,72 @@ public class PatientEverythingPaginationR4Test extends BaseResourceProviderR4Tes
 		assertEquals(4, patientsPage2.size());
 	}
 
+	@ParameterizedTest
+	@ValueSource(booleans = { true, false })
+	public void testEverythingPagination_LastPage(boolean theProvideCountBool) throws IOException {
+		// setup
+		myStorageSettings.setSearchPreFetchThresholds(Arrays.asList(10, 50,-1));
+		// 3 pages
+		int total = 154;
+		createPatients(total);
+		Set<String> ids = new HashSet<>();
+
+		String url = myServerBase + "/Patient/$everything?_format=json";
+		if (theProvideCountBool) {
+			url += "&_count=" + BasePagingProvider.DEFAULT_MAX_PAGE_SIZE;
+		}
+
+		String nextUrl;
+
+		// test
+		Bundle bundle = fetchBundle(url);
+
+		// first page
+		List<Patient> patientsPage = BundleUtil.toListOfResourcesOfType(myFhirContext, bundle, Patient.class);
+//		assertEquals(50, patientsPage.size());
+		for (Patient p : patientsPage) {
+			assertTrue(ids.add(p.getId()));
+		}
+		nextUrl = BundleUtil.getLinkUrlOfType(myFhirContext, bundle, LINK_NEXT);
+		assertNotNull(nextUrl);
+
+		// all future pages
+		do {
+			bundle = fetchBundle(nextUrl);
+			assertNotNull(bundle);
+			patientsPage = BundleUtil.toListOfResourcesOfType(myFhirContext, bundle, Patient.class);
+			for (Patient p : patientsPage) {
+				assertTrue(ids.add(p.getId()));
+			}
+//			assertEquals(50, patientsPage.size());
+			nextUrl = BundleUtil.getLinkUrlOfType(myFhirContext, bundle, LINK_NEXT);
+		} while (nextUrl != null);
+		assertNull(nextUrl);
+
+		// last page
+//		nextUrl = BundleUtil.getLinkUrlOfType(myFhirContext, bundle, LINK_NEXT);
+//		assertNotNull(nextUrl);
+//		bundle = fetchBundle(nextUrl);
+//		nextUrl = BundleUtil.getLinkUrlOfType(myFhirContext, bundle, LINK_NEXT);
+//		assertNull(nextUrl);
+//		patientsPage = BundleUtil.toListOfResourcesOfType(myFhirContext, bundle, Patient.class);
+//
+//		assertEquals(4, patientsPage.size());
+//		for (Patient p : patientsPage) {
+//			assertTrue(ids.add(p.getId()));
+//		}
+
+		assertEquals(total, ids.size());
+	}
+
+
+	private void createPatients(int theCount) {
+		for (int i = 0; i < theCount; i++) {
+			Patient patient = new Patient();
+			patient.addName().setFamily("lastn").addGiven("name");
+			myPatientDao.create(patient, new SystemRequestDetails()).getId().toUnqualifiedVersionless();
+		}
+	}
 
 	private Bundle fetchBundle(String theUrl) throws IOException {
 		Bundle bundle;