5237 fixing empty last page if even results (#5506)

paging will not return empty pages if no results left

hapi-fhir-cli/hapi-fhir-cli-api/src/test/java/ca/uhn/fhir/cli/BulkImportCommandIT.java

@@ -8,6 +8,8 @@
 import ca.uhn.fhir.batch2.model.StatusEnum;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.jpa.batch.models.Batch2JobStartResponse;
+import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc;
+import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
 import ca.uhn.fhir.rest.server.interceptor.LoggingInterceptor;
 import ca.uhn.fhir.system.HapiSystemProperties;
@@ -66,6 +68,8 @@ public class BulkImportCommandIT {
  private IJobCoordinator myJobCoordinator;
  private final BulkDataImportProvider myProvider = new BulkDataImportProvider();
  private final FhirContext myCtx = FhirContext.forR4Cached();
+ @Mock
+ private IRequestPartitionHelperSvc myRequestPartitionHelperSvc;
  @RegisterExtension
  public RestfulServerExtension myRestfulServerExtension = new RestfulServerExtension(myCtx, myProvider)
  .registerInterceptor(new LoggingInterceptor());
@@ -77,6 +81,7 @@ public class BulkImportCommandIT {
  public void beforeEach() throws IOException {
  myProvider.setFhirContext(myCtx);
  myProvider.setJobCoordinator(myJobCoordinator);
+ myProvider.setRequestPartitionHelperService(myRequestPartitionHelperSvc);
  myTempDir = Files.createTempDirectory("hapifhir");
  ourLog.info("Created temp directory: {}", myTempDir);
  }
@@ -123,7 +128,7 @@ public void testBulkImport() throws IOException {
  await().until(() -> myRestfulServerExtension.getRequestContentTypes().size(), equalTo(2));
  ourLog.info("Initiation requests complete");
 
- verify(myJobCoordinator, timeout(10000).times(1)).startInstance(myStartCaptor.capture());
+ verify(myJobCoordinator, timeout(10000).times(1)).startInstance(any(RequestDetails.class), myStartCaptor.capture());
 
  JobInstanceStartRequest startRequest = myStartCaptor.getValue();
  BulkImportJobParameters jobParameters = startRequest.getParameters(BulkImportJobParameters.class);
@@ -165,7 +170,7 @@ public void testBulkImport_GzippedFile() throws IOException {
  await().until(() -> myRestfulServerExtension.getRequestContentTypes().size(), equalTo(2));
  ourLog.info("Initiation requests complete");
 
- verify(myJobCoordinator, timeout(10000).times(1)).startInstance(myStartCaptor.capture());
+ verify(myJobCoordinator, timeout(10000).times(1)).startInstance(any(RequestDetails.class), myStartCaptor.capture());
 
  JobInstanceStartRequest startRequest = myStartCaptor.getValue();
  BulkImportJobParameters jobParameters = startRequest.getParameters(BulkImportJobParameters.class);
@@ -206,7 +211,7 @@ public void testBulkImport_FAILED() throws IOException {
  await().until(() -> myRestfulServerExtension.getRequestContentTypes().size(), equalTo(2));
  ourLog.info("Initiation requests complete");
 
- verify(myJobCoordinator, timeout(10000).times(1)).startInstance(myStartCaptor.capture());
+ verify(myJobCoordinator, timeout(10000).times(1)).startInstance(any(RequestDetails.class), myStartCaptor.capture());
 
  try{
  JobInstanceStartRequest startRequest = myStartCaptor.getValue();

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5192-include-in-search-paging-fix.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 5192
+title: "Fixed a bug where search Bundles with `include` entries from an _include query parameter might
+ trigger a 'next' link to blank pages when
+ no more results `match` results are available.
+"

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

@@ -125,7 +125,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
  * of this class, since it's a prototype
  */
  private Search mySearchEntity;
- private String myUuid;
+ private final String myUuid;
  private SearchCacheStatusEnum myCacheStatus;
  private RequestPartitionId myRequestPartitionId;
 
@@ -259,13 +259,21 @@ protected List<IBaseResource> doSearchOrEverything(
  final ISearchBuilder sb = mySearchBuilderFactory.newSearchBuilder(dao, resourceName, resourceType);
 
  RequestPartitionId requestPartitionId = getRequestPartitionId();
- final List<JpaPid> pidsSubList =
- mySearchCoordinatorSvc.getResources(myUuid, theFromIndex, theToIndex, myRequest, requestPartitionId);
+ // we request 1 more resource than we need
+ // this is so we can be sure of when we hit the last page
+ // (when doing offset searches)
+ final List<JpaPid> pidsSubList = mySearchCoordinatorSvc.getResources(
+ myUuid, theFromIndex, theToIndex + 1, myRequest, requestPartitionId);
+ // max list size should be either the entire list, or from - to length
+ int maxSize = Math.min(theToIndex - theFromIndex, pidsSubList.size());
+ theResponsePageBuilder.setTotalRequestedResourcesFetched(pidsSubList.size());
+
+ List<JpaPid> firstBatchOfPids = pidsSubList.subList(0, maxSize);
  List<IBaseResource> resources = myTxService
  .withRequest(myRequest)
  .withRequestPartitionId(requestPartitionId)
  .execute(() -> {
- return toResourceList(sb, pidsSubList, theResponsePageBuilder);
+ return toResourceList(sb, firstBatchOfPids, theResponsePageBuilder);
  });
 
  return resources;
@@ -541,8 +549,8 @@ protected List<IBaseResource> toResourceList(
  // this can (potentially) change the results being returned.
  int precount = resources.size();
  resources = ServerInterceptorUtil.fireStoragePreshowResource(resources, myRequest, myInterceptorBroadcaster);
- // we only care about omitted results from *this* page
- theResponsePageBuilder.setToOmittedResourceCount(precount - resources.size());
+ // we only care about omitted results from this page
+ theResponsePageBuilder.setOmittedResourceCount(precount - resources.size());
  theResponsePageBuilder.setResources(resources);
  theResponsePageBuilder.setIncludedResourceCount(includedPidList.size());
 

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

@@ -73,16 +73,23 @@ public List<IBaseResource> getResources(
 
  mySearchTask.awaitInitialSync();
 
+ // request 1 more than we need to, in order to know if there are extra values
  ourLog.trace("Fetching search resource PIDs from task: {}", mySearchTask.getClass());
- final List<JpaPid> pids = mySearchTask.getResourcePids(theFromIndex, theToIndex);
+ final List<JpaPid> pids = mySearchTask.getResourcePids(theFromIndex, theToIndex + 1);
  ourLog.trace("Done fetching search resource PIDs");
 
+ int countOfPids = pids.size();
+ ;
+ int maxSize = Math.min(theToIndex - theFromIndex, countOfPids);
+ thePageBuilder.setTotalRequestedResourcesFetched(countOfPids);
+
  RequestPartitionId requestPartitionId = getRequestPartitionId();
 
+ List<JpaPid> firstBatch = pids.subList(0, maxSize);
  List<IBaseResource> retVal = myTxService
  .withRequest(myRequest)
  .withRequestPartitionId(requestPartitionId)
- .execute(() -> toResourceList(mySearchBuilder, pids, thePageBuilder));
+ .execute(() -> toResourceList(mySearchBuilder, firstBatch, thePageBuilder));
 
  long totalCountWanted = theToIndex - theFromIndex;
  long totalCountMatch = (int) retVal.stream().filter(t -> !isInclude(t)).count();
@@ -103,12 +110,15 @@ && getSearchEntity().getNumFound() >= theToIndex))) {
 
  long remainingWanted = totalCountWanted - totalCountMatch;
  long fromIndex = theToIndex - remainingWanted;
- List<IBaseResource> remaining = super.getResources((int) fromIndex, theToIndex, thePageBuilder);
+ ResponsePage.ResponsePageBuilder pageBuilder = new ResponsePage.ResponsePageBuilder();
+ pageBuilder.setBundleProvider(this);
+ List<IBaseResource> remaining = super.getResources((int) fromIndex, theToIndex, pageBuilder);
  remaining.forEach(t -> {
  if (!existingIds.contains(t.getIdElement().getValue())) {
  retVal.add(t);
  }
  });
+ thePageBuilder.combineWith(pageBuilder);
  }
  }
  ourLog.trace("Loaded resources to return");

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

@@ -115,7 +115,7 @@ public IBundleProvider executeQuery(
  .execute(() -> {
 
  // Load the results synchronously
- final List<JpaPid> pids = new ArrayList<>();
+ List<JpaPid> pids = new ArrayList<>();
 
  Long count = 0L;
  if (wantCount) {
@@ -145,8 +145,17 @@ public IBundleProvider executeQuery(
  return bundleProvider;
  }
 
+ // if we have a count, we'll want to request
+ // additional resources
+ SearchParameterMap clonedParams = theParams.clone();
+ Integer requestedCount = clonedParams.getCount();
+ boolean hasACount = requestedCount != null;
+ if (hasACount) {
+ clonedParams.setCount(requestedCount.intValue() + 1);
+ }
+
  try (IResultIterator<JpaPid> resultIter = theSb.createQuery(
- theParams, searchRuntimeDetails, theRequestDetails, theRequestPartitionId)) {
+ clonedParams, searchRuntimeDetails, theRequestDetails, theRequestPartitionId)) {
  while (resultIter.hasNext()) {
  pids.add(resultIter.next());
  if (theLoadSynchronousUpTo != null && pids.size() >= theLoadSynchronousUpTo) {
@@ -162,6 +171,15 @@ public IBundleProvider executeQuery(
  throw new InternalErrorException(Msg.code(1164) + e);
  }
 
+ // truncate the list we retrieved - if needed
+ int receivedResourceCount = -1;
+ if (hasACount) {
+ // we want the accurate received resource count
+ receivedResourceCount = pids.size();
+ int resourcesToReturn = Math.min(theParams.getCount(), pids.size());
+ pids = pids.subList(0, resourcesToReturn);
+ }
+
  JpaPreResourceAccessDetails accessDetails = new JpaPreResourceAccessDetails(pids, () -> theSb);
  HookParams params = new HookParams()
  .add(IPreResourceAccessDetails.class, accessDetails)
@@ -228,6 +246,9 @@ public IBundleProvider executeQuery(
  resources, theRequestDetails, myInterceptorBroadcaster);
 
  SimpleBundleProvider bundleProvider = new SimpleBundleProvider(resources);
+ if (hasACount) {
+ bundleProvider.setTotalResourcesRequestedReturned(receivedResourceCount);
+ }
  if (theParams.isOffsetQuery()) {
  bundleProvider.setCurrentPageOffset(theParams.getOffset());
  bundleProvider.setCurrentPageSize(theParams.getCount());

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

@@ -42,14 +42,17 @@ public void before() {
 
  @Test
  public void testSynchronousSearch() {
- when(mySearchBuilderFactory.newSearchBuilder(any(), any(), any())).thenReturn(mySearchBuilder);
+ when(mySearchBuilderFactory.newSearchBuilder(any(), any(), any()))
+ .thenReturn(mySearchBuilder);
 
  SearchParameterMap params = new SearchParameterMap();
 
  List<JpaPid> pids = createPidSequence(800);
- when(mySearchBuilder.createQuery(same(params), any(), any(), nullable(RequestPartitionId.class))).thenReturn(new BaseSearchSvc.ResultIterator(pids.iterator()));
+ when(mySearchBuilder.createQuery(any(SearchParameterMap.class), any(), any(), nullable(RequestPartitionId.class)))
+ .thenReturn(new BaseSearchSvc.ResultIterator(pids.iterator()));
 
- doAnswer(loadPids()).when(mySearchBuilder).loadResourcesByPid(any(Collection.class), any(Collection.class), any(List.class), anyBoolean(), any());
+ doAnswer(loadPids()).when(mySearchBuilder)
+ .loadResourcesByPid(any(Collection.class), any(Collection.class), any(List.class), anyBoolean(), any());
 
  IBundleProvider result = mySynchronousSearchSvc.executeQuery( "Patient", params, RequestPartitionId.allPartitions());
  assertNull(result.getUuid());
@@ -71,8 +74,8 @@ public void testSynchronousSearchWithOffset() {
  params.setSearchTotalMode(SearchTotalModeEnum.ACCURATE);
 
  List<JpaPid> pids = createPidSequence(30);
- when(mySearchBuilder.createCountQuery(same(params), any(String.class),nullable(RequestDetails.class), nullable(RequestPartitionId.class))).thenReturn(20L);
- when(mySearchBuilder.createQuery(same(params), any(), nullable(RequestDetails.class), nullable(RequestPartitionId.class))).thenReturn(new BaseSearchSvc.ResultIterator(pids.subList(10, 20).iterator()));
+ when(mySearchBuilder.createCountQuery(any(SearchParameterMap.class), any(String.class),nullable(RequestDetails.class), nullable(RequestPartitionId.class))).thenReturn(20L);
+ when(mySearchBuilder.createQuery(any(SearchParameterMap.class), any(), nullable(RequestDetails.class), nullable(RequestPartitionId.class))).thenReturn(new BaseSearchSvc.ResultIterator(pids.subList(10, 20).iterator()));
 
  doAnswer(loadPids()).when(mySearchBuilder).loadResourcesByPid(any(Collection.class), any(Collection.class), any(List.class), anyBoolean(), any());
 
@@ -92,7 +95,8 @@ public void testSynchronousSearchUpTo() {
  params.setLoadSynchronousUpTo(100);
 
  List<JpaPid> pids = createPidSequence(800);
- when(mySearchBuilder.createQuery(same(params), any(), nullable(RequestDetails.class), nullable(RequestPartitionId.class))).thenReturn(new BaseSearchSvc.ResultIterator(pids.iterator()));
+ when(mySearchBuilder.createQuery(any(SearchParameterMap.class), any(), nullable(RequestDetails.class), nullable(RequestPartitionId.class)))
+ .thenReturn(new BaseSearchSvc.ResultIterator(pids.iterator()));
 
  pids = createPidSequence(110);
  List<JpaPid> finalPids = pids;

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

@@ -29,11 +29,11 @@
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
 
-import javax.servlet.ServletException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import static org.apache.commons.lang3.StringUtils.leftPad;
@@ -54,7 +54,7 @@ public class ConsentEventsDaoR4Test extends BaseJpaR4SystemTest {
  private List<String> myPatientIds;
  private List<String> myObservationIdsOddOnly;
  private List<String> myObservationIdsEvenOnly;
- private List<String> myObservationIdsWithVersions;
+ private List<String> myObservationIdsWithoutVersions;
  private List<String> myPatientIdsEvenOnly;
 
  @AfterEach
@@ -64,13 +64,16 @@ public void after() {
  }
 
  @BeforeEach
- public void before() throws ServletException {
+ @Override
+ public void beforeInitMocks() throws Exception {
+ super.beforeInitMocks();
  RestfulServer restfulServer = new RestfulServer();
  restfulServer.setPagingProvider(myPagingProvider);
 
  when(mySrd.getServer()).thenReturn(restfulServer);
 
  myStorageSettings.setSearchPreFetchThresholds(Arrays.asList(20, 50, 190));
+ restfulServer.setDefaultPageSize(null);
  }
 
  @Test
@@ -147,6 +150,7 @@ public void testSearchAndBlockSome() {
 
  @Test
  public void testSearchAndBlockSome_LoadSynchronous() {
+ // setup
  create50Observations();
 
  AtomicInteger hitCount = new AtomicInteger(0);
@@ -281,6 +285,7 @@ public void testSearchAndBlockNoneOnIncludes() {
 
  @Test
  public void testSearchAndBlockSomeOnIncludes_LoadSynchronous() {
+ // setup
  create50Observations();
 
  AtomicInteger hitCount = new AtomicInteger(0);
@@ -328,9 +333,8 @@ public void testHistoryAndBlockSome() {
  * returned results because we create it then update it in create50Observations()
  */
  assertEquals(1, hitCount.get());
- assertEquals(myObservationIdsWithVersions.subList(90, myObservationIdsWithVersions.size()), sort(interceptedResourceIds));
+ assertEquals(sort(myObservationIdsWithoutVersions.subList(90, myObservationIdsWithoutVersions.size())), sort(interceptedResourceIds));
  returnedIdValues.forEach(t -> assertTrue(new IdType(t).getIdPartAsLong() % 2 == 0));
-
  }
 
  @Test
@@ -363,7 +367,7 @@ public void testReadAndBlockSome() {
  private void create50Observations() {
  myPatientIds = new ArrayList<>();
  myObservationIds = new ArrayList<>();
- myObservationIdsWithVersions = new ArrayList<>();
+ myObservationIdsWithoutVersions = new ArrayList<>();
 
  Patient p = new Patient();
  p.setActive(true);
@@ -383,9 +387,9 @@ private void create50Observations() {
  final Observation obs1 = new Observation();
  obs1.setStatus(Observation.ObservationStatus.FINAL);
  obs1.addIdentifier().setSystem("urn:system").setValue("I" + leftPad("" + i, 5, '0'));
- IIdType obs1id = myObservationDao.create(obs1).getId().toUnqualifiedVersionless();
+ IIdType obs1id = myObservationDao.create(obs1).getId();
  myObservationIds.add(obs1id.toUnqualifiedVersionless().getValue());
- myObservationIdsWithVersions.add(obs1id.toUnqualifiedVersionless().getValue());
+ myObservationIdsWithoutVersions.add(obs1id.toUnqualifiedVersionless().getValue());
 
  obs1.setId(obs1id);
  if (obs1id.getIdPartAsLong() % 2 == 0) {
@@ -394,7 +398,7 @@ private void create50Observations() {
  obs1.getSubject().setReference(oddPid);
  }
  myObservationDao.update(obs1);
- myObservationIdsWithVersions.add(obs1id.toUnqualifiedVersionless().getValue());
+ myObservationIdsWithoutVersions.add(obs1id.toUnqualifiedVersionless().getValue());
 
  }
 
@@ -483,14 +487,24 @@ public void invoke(IPointcut thePointcut, HookParams theArgs) {
  }
  }
 
- private static List<String> sort(List<String>... theLists) {
+ private List<String> sort(List<String>... theLists) {
+ return sort(id -> {
+ String idParsed = id.substring(id.indexOf("/") + 1);
+ if (idParsed.contains("/_history")) {
+ idParsed = idParsed.substring(0, idParsed.indexOf("/"));
+ }
+ return Long.parseLong(idParsed);
+ }, theLists);
+ }
+
+ private List<String> sort(Function<String, Long> theParser, List<String>... theLists) {
  ArrayList<String> retVal = new ArrayList<>();
  for (List<String> next : theLists) {
  retVal.addAll(next);
  }
  retVal.sort((o0, o1) -> {
- long i0 = Long.parseLong(o0.substring(o0.indexOf('/') + 1));
- long i1 = Long.parseLong(o1.substring(o1.indexOf('/') + 1));
+ long i0 = theParser.apply(o0);
+ long i1 = theParser.apply(o1);
  return (int) (i0 - i1);
  });
  return retVal;