diff --git a/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/PagingPatientProvider.java b/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/PagingPatientProvider.java index 2622ad526d1..f0a015b206d 100644 --- a/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/PagingPatientProvider.java +++ b/hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/PagingPatientProvider.java @@ -24,6 +24,7 @@ import ca.uhn.fhir.rest.annotation.Search; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.server.IResourceProvider; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.model.InstantType; import org.hl7.fhir.r4.model.Patient; @@ -63,7 +64,7 @@ public class PagingPatientProvider implements IResourceProvider { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { int end = Math.max(theToIndex, matchingResourceIds.size() - 1); List idsToReturn = matchingResourceIds.subList(theFromIndex, end); return loadResourcesByIds(idsToReturn); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java index 7e442bda264..00d8f474c98 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java @@ -55,23 +55,25 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.api.server.SimplePreResourceAccessDetails; import ca.uhn.fhir.rest.api.server.SimplePreResourceShowDetails; import ca.uhn.fhir.rest.server.interceptor.ServerInterceptorUtil; +import ca.uhn.fhir.rest.server.method.ResponsePage; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.rest.server.util.CompositeInterceptorBroadcaster; import com.google.common.annotations.VisibleForTesting; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import javax.annotation.Nonnull; +import javax.persistence.EntityManager; +import javax.persistence.PersistenceContext; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.Set; import java.util.function.Function; -import javax.annotation.Nonnull; -import javax.persistence.EntityManager; -import javax.persistence.PersistenceContext; public class PersistedJpaBundleProvider implements IBundleProvider { @@ -128,6 +130,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider { private String myUuid; private SearchCacheStatusEnum myCacheStatus; private RequestPartitionId myRequestPartitionId; + /** * Constructor */ @@ -150,6 +153,13 @@ public class PersistedJpaBundleProvider implements IBundleProvider { myRequestPartitionHelperSvc = theRequestPartitionHelperSvc; } + @Override + public ResponsePage.ResponsePageBuilder getResponsePageBuilder() { + ResponsePage.ResponsePageBuilder builder = new ResponsePage.ResponsePageBuilder(); + + return builder; + } + protected Search getSearchEntity() { return mySearchEntity; } @@ -163,7 +173,9 @@ public class PersistedJpaBundleProvider implements IBundleProvider { /** * Perform a history search */ - private List doHistoryInTransaction(Integer theOffset, int theFromIndex, int theToIndex) { + private List doHistoryInTransaction( + Integer theOffset, int theFromIndex, int theToIndex + ) { HistoryBuilder historyBuilder = myHistoryBuilderFactory.newHistoryBuilder( mySearchEntity.getResourceType(), @@ -180,7 +192,6 @@ public class PersistedJpaBundleProvider implements IBundleProvider { BaseHasResource resource; resource = next; - IFhirResourceDao dao = myDaoRegistry.getResourceDao(next.getResourceType()); retVal.add(myJpaStorageResourceParser.toResource(resource, true)); } @@ -238,7 +249,9 @@ public class PersistedJpaBundleProvider implements IBundleProvider { myRequestPartitionId = theRequestPartitionId; } - protected List doSearchOrEverything(final int theFromIndex, final int theToIndex) { + protected List doSearchOrEverything( + final int theFromIndex, final int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder + ) { if (mySearchEntity.getTotalCount() != null && mySearchEntity.getNumFound() <= 0) { // No resources to fetch (e.g. we did a _summary=count search) return Collections.emptyList(); @@ -253,12 +266,14 @@ public class PersistedJpaBundleProvider implements IBundleProvider { RequestPartitionId requestPartitionId = getRequestPartitionId(); final List pidsSubList = mySearchCoordinatorSvc.getResources(myUuid, theFromIndex, theToIndex, myRequest, requestPartitionId); - return myTxService + List resources = myTxService .withRequest(myRequest) .withRequestPartitionId(requestPartitionId) .execute(() -> { - return toResourceList(sb, pidsSubList); + return toResourceList(sb, pidsSubList, theResponsePageBuilder); }); + + return resources; } /** @@ -349,9 +364,16 @@ public class PersistedJpaBundleProvider implements IBundleProvider { return new InstantDt(mySearchEntity.getCreated()); } - @Nonnull + @NotNull @Override - public List getResources(final int theFromIndex, final int theToIndex) { + public List getResources(int theFromIndex, int theToIndex) { + return getResources(theFromIndex, theToIndex, getResponsePageBuilder()); + } + + @Override + public List getResources( + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder + ) { boolean entityLoaded = ensureSearchEntityLoaded(); assert entityLoaded; assert mySearchEntity != null; @@ -360,13 +382,13 @@ public class PersistedJpaBundleProvider implements IBundleProvider { switch (mySearchEntity.getSearchType()) { case HISTORY: return myTxService - .withRequest(myRequest) - .withRequestPartitionId(getRequestPartitionId()) - .execute(() -> doHistoryInTransaction(mySearchEntity.getOffset(), theFromIndex, theToIndex)); + .withRequest(myRequest) + .withRequestPartitionId(getRequestPartitionId()) + .execute(() -> doHistoryInTransaction(mySearchEntity.getOffset(), theFromIndex, theToIndex)); case SEARCH: case EVERYTHING: default: - List retVal = doSearchOrEverything(theFromIndex, theToIndex); + List retVal = doSearchOrEverything(theFromIndex, theToIndex, theResponsePageBuilder); /* * If we got fewer resources back than we asked for, it's possible that the search * completed. If that's the case, the cached version of the search entity is probably @@ -443,8 +465,11 @@ public class PersistedJpaBundleProvider implements IBundleProvider { // Note: Leave as protected, HSPC depends on this @SuppressWarnings("WeakerAccess") - protected List toResourceList(ISearchBuilder theSearchBuilder, List thePids) { - + protected List toResourceList( + ISearchBuilder theSearchBuilder, + List thePids, + ResponsePage.ResponsePageBuilder theResponsePageBuilder + ) { List includedPidList = new ArrayList<>(); if (mySearchEntity.getSearchType() == SearchTypeEnum.SEARCH) { Integer maxIncludes = myStorageSettings.getMaximumIncludesToLoadPerPage(); @@ -522,6 +547,8 @@ public class PersistedJpaBundleProvider implements IBundleProvider { theSearchBuilder.loadResourcesByPid(thePids, includedPidList, resources, false, myRequest); resources = ServerInterceptorUtil.fireStoragePreshowResource(resources, myRequest, myInterceptorBroadcaster); + theResponsePageBuilder.setResources(resources); + theResponsePageBuilder.setIncludedResourceCount(includedPidList.size()); return resources; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaSearchFirstPageBundleProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaSearchFirstPageBundleProvider.java index 28dbe622acb..cea1c339cc3 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaSearchFirstPageBundleProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaSearchFirstPageBundleProvider.java @@ -30,23 +30,26 @@ import ca.uhn.fhir.jpa.util.QueryParameterUtils; import ca.uhn.fhir.model.api.ResourceMetadataKeyEnum; import ca.uhn.fhir.model.valueset.BundleEntrySearchModeEnum; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.hl7.fhir.instance.model.api.IBaseResource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; import java.util.List; import java.util.Set; import java.util.stream.Collectors; -import javax.annotation.Nonnull; public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundleProvider { private static final Logger ourLog = LoggerFactory.getLogger(PersistedJpaSearchFirstPageBundleProvider.class); private final SearchTask mySearchTask; + @SuppressWarnings("rawtypes") private final ISearchBuilder mySearchBuilder; /** * Constructor */ + @SuppressWarnings("rawtypes") public PersistedJpaSearchFirstPageBundleProvider( Search theSearch, SearchTask theSearchTask, @@ -65,7 +68,7 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder thePageBuilder) { ensureSearchEntityLoaded(); QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(getSearchEntity()); @@ -80,7 +83,7 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl List retVal = myTxService .withRequest(myRequest) .withRequestPartitionId(requestPartitionId) - .execute(() -> toResourceList(mySearchBuilder, pids)); + .execute(() -> toResourceList(mySearchBuilder, pids, thePageBuilder)); long totalCountWanted = theToIndex - theFromIndex; long totalCountMatch = (int) retVal.stream().filter(t -> !isInclude(t)).count(); @@ -101,7 +104,7 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl long remainingWanted = totalCountWanted - totalCountMatch; long fromIndex = theToIndex - remainingWanted; - List remaining = super.getResources((int) fromIndex, theToIndex); + List remaining = super.getResources((int) fromIndex, theToIndex, thePageBuilder); remaining.forEach(t -> { if (!existingIds.contains(t.getIdElement().getValue())) { retVal.add(t); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index 27312cda007..e11330b2c81 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java @@ -146,6 +146,7 @@ import org.hl7.fhir.r4.model.StructureDefinition; import org.hl7.fhir.r4.model.Subscription; import org.hl7.fhir.r4.model.Subscription.SubscriptionChannelType; import org.hl7.fhir.r4.model.Subscription.SubscriptionStatus; +import org.hl7.fhir.r4.model.Task; import org.hl7.fhir.r4.model.UriType; import org.hl7.fhir.r4.model.ValueSet; import org.hl7.fhir.utilities.xhtml.NodeType; @@ -2654,6 +2655,92 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { } + + + private void printResources(Bundle theBundle) { + IParser parser = myFhirContext.newJsonParser(); + for (BundleEntryComponent entry : theBundle.getEntry()) { + ourLog.info(entry.getResource().getId()); +// ourLog.info( +// parser.encodeResourceToString(entry.getResource()) +// ); + } + } + + // TODO - we should not be including the + // _include count in the _count + // so current behaviour is correct + // but we should also not be including a next + // list if there are no more resources + @Test + public void testPagingWithIncludesReturnsConsistentValues() { + // setup + int total = 19; + int orgs = 10; + // create resources + { + Coding tagCode = new Coding(); + tagCode.setCode("test"); + tagCode.setSystem("http://example.com"); + int orgCount = orgs; + for (int i = 0; i < total; i++) { + Task t = new Task(); + t.getMeta() + .addTag(tagCode); + t.setStatus(Task.TaskStatus.REQUESTED); + if (orgCount > 0) { + Organization org = new Organization(); + org.setName("ORG"); + IIdType orgId = myOrganizationDao.create(org).getId().toUnqualifiedVersionless(); + + orgCount--; + t.getOwner().setReference(orgId.getValue()); + } + myTaskDao.create(t); + } + } + + int requestedAmount = 10; + Bundle bundle = myClient + .search() + .byUrl("Task?_count=10&_tag=test&status=requested&_include=Task%3Aowner&_sort=status") + .returnBundle(Bundle.class) + .execute(); + int count = bundle.getEntry().size(); + assertTrue(bundle.getEntry().size() > 0); + System.out.println("Received " + bundle.getEntry().size()); +// assertEquals(requestedAmount, count); + printResources(bundle); + String nextUrl = null; + do { + Bundle.BundleLinkComponent nextLink = bundle.getLink("next"); + if (nextLink != null) { + nextUrl = nextLink.getUrl(); + + // make sure we're always requesting 10 + assertTrue(nextUrl.contains(String.format("_count=%d", requestedAmount))); + + // get next batch + bundle = myClient.fetchResourceFromUrl(Bundle.class, nextUrl); + int received = bundle.getEntry().size(); + + // verify it's the same amount as we requested + System.out.println("Recieved " + received); + printResources(bundle); + + assertTrue(bundle.getEntry().size() > 0); +// assertEquals(requestedAmount, received); + count += received; + } else { + nextUrl = null; + } + } while (nextUrl != null); + + // verify + assertEquals(total + orgs, count); + } + + /** * See #793 */ diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/IBundleProvider.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/IBundleProvider.java index 829453dc08b..621f9d3b20a 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/IBundleProvider.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/IBundleProvider.java @@ -21,6 +21,7 @@ package ca.uhn.fhir.rest.api.server; import ca.uhn.fhir.context.ConfigurationException; import ca.uhn.fhir.i18n.Msg; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.apache.commons.lang3.Validate; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IPrimitiveType; @@ -34,6 +35,14 @@ import javax.annotation.Nullable; public interface IBundleProvider { + /** + * Returns a ResponsePageBuilder for constructing + * pages that return results. + */ + default ResponsePage.ResponsePageBuilder getResponsePageBuilder() { + return new ResponsePage.ResponsePageBuilder(); + } + /** * If this method is implemented, provides an ID for the current * page of results. This ID should be unique (at least within @@ -113,6 +122,32 @@ public interface IBundleProvider { */ IPrimitiveType getPublished(); + /** + * Deprecated: Use the getResources(int from, int to, ResourcePageBuilder builder) instead. + *

+ * Load the given collection of resources by index, plus any additional resources per the + * server's processing rules (e.g. _include'd resources, OperationOutcome, etc.). For example, + * if the method is invoked with index 0,10 the method might return 10 search results, plus an + * additional 20 resources which matched a client's _include specification. + *

+ *

+ * Note that if this bundle provider was loaded using a + * page ID (i.e. via {@link ca.uhn.fhir.rest.server.IPagingProvider#retrieveResultList(RequestDetails, String, String)} + * because {@link #getNextPageId()} provided a value on the + * previous page, then the indexes should be ignored and the + * whole page returned. + *

+ * + * @param theFromIndex The low index (inclusive) to return + * @param theToIndex The high index (exclusive) to return + * @return A list of resources. The size of this list must be at least theToIndex - theFromIndex. + */ + @Nonnull + @Deprecated(forRemoval = true, since = "7.0.0") + default List getResources(int theFromIndex, int theToIndex) { + return getResources(theFromIndex, theToIndex, getResponsePageBuilder()); + } + /** * Load the given collection of resources by index, plus any additional resources per the * server's processing rules (e.g. _include'd resources, OperationOutcome, etc.). For example, @@ -128,10 +163,13 @@ public interface IBundleProvider { * * @param theFromIndex The low index (inclusive) to return * @param theToIndex The high index (exclusive) to return + * @param theResponsePageBuilder The ResponsePageBuilder. The builder will add values needed for the response page. * @return A list of resources. The size of this list must be at least theToIndex - theFromIndex. */ - @Nonnull - List getResources(int theFromIndex, int theToIndex); + default List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + // TODO - override and implement + return getResources(theFromIndex, theToIndex); + } /** * Get all resources diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviderWithNamedPages.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviderWithNamedPages.java index f313a8a0d5b..9f91f33a201 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviderWithNamedPages.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviderWithNamedPages.java @@ -19,6 +19,7 @@ */ package ca.uhn.fhir.rest.server; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.apache.commons.lang3.Validate; import org.hl7.fhir.instance.model.api.IBaseResource; @@ -85,9 +86,10 @@ public class BundleProviderWithNamedPages extends SimpleBundleProvider { return this; } + @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { return (List) getList(); // indexes are ignored for this provider type } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviders.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviders.java index afceb9c2497..010940ae2e0 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviders.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/BundleProviders.java @@ -21,6 +21,7 @@ package ca.uhn.fhir.rest.server; import ca.uhn.fhir.model.primitive.InstantDt; import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.server.method.ResponsePage; import ca.uhn.fhir.util.CoverageIgnore; import org.hl7.fhir.instance.model.api.IBaseResource; @@ -47,7 +48,7 @@ public class BundleProviders { return new IBundleProvider() { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder) { return Collections.emptyList(); } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/SimpleBundleProvider.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/SimpleBundleProvider.java index b9f0ee2cf8d..c0cf34d89c2 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/SimpleBundleProvider.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/SimpleBundleProvider.java @@ -21,15 +21,16 @@ package ca.uhn.fhir.rest.server; import ca.uhn.fhir.model.primitive.InstantDt; import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.apache.commons.lang3.builder.ToStringBuilder; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IPrimitiveType; +import javax.annotation.Nonnull; import java.util.Arrays; import java.util.Collections; import java.util.Date; import java.util.List; -import javax.annotation.Nonnull; public class SimpleBundleProvider implements IBundleProvider { @@ -40,6 +41,7 @@ public class SimpleBundleProvider implements IBundleProvider { private IPrimitiveType myPublished = InstantDt.withCurrentTime(); private Integer myCurrentPageOffset; private Integer myCurrentPageSize; + private ResponsePage.ResponsePageBuilder myPageBuilder; /** * Constructor @@ -137,9 +139,10 @@ public class SimpleBundleProvider implements IBundleProvider { myPublished = thePublished; } + @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { return (List) myList.subList(Math.min(theFromIndex, myList.size()), Math.min(theToIndex, myList.size())); } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/HistoryMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/HistoryMethodBinding.java index af8eba13de9..f6e5dd38742 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/HistoryMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/HistoryMethodBinding.java @@ -181,8 +181,8 @@ public class HistoryMethodBinding extends BaseResourceReturningMethodBinding { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { - List retVal = resources.getResources(theFromIndex, theToIndex); + public List getResources(int theFromIndex, int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + List retVal = resources.getResources(theFromIndex, theToIndex, theResponsePageBuilder); int index = theFromIndex; for (IBaseResource nextResource : retVal) { if (nextResource.getIdElement() == null diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilder.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilder.java index bbecf228c42..873fbe56c45 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilder.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilder.java @@ -91,6 +91,8 @@ public class ResponseBundleBuilder { final List resourceList; final int pageSize; + ResponsePage.ResponsePageBuilder responsePageBuilder = bundleProvider.getResponsePageBuilder(); + int numToReturn; String searchId = null; @@ -98,24 +100,35 @@ public class ResponseBundleBuilder { pageSize = offsetCalculatePageSize(server, requestedPage, bundleProvider.size()); numToReturn = pageSize; - resourceList = offsetBuildResourceList(bundleProvider, requestedPage, numToReturn); + resourceList = offsetBuildResourceList(bundleProvider, requestedPage, numToReturn, responsePageBuilder); RestfulServerUtils.validateResourceListNotNull(resourceList); } else { pageSize = pagingCalculatePageSize(requestedPage, server.getPagingProvider()); - if (bundleProvider.size() == null) { + Integer size = bundleProvider.size(); + if (size == null) { numToReturn = pageSize; } else { - numToReturn = Math.min(pageSize, bundleProvider.size() - theResponseBundleRequest.offset); + numToReturn = Math.min(pageSize, size.intValue() - theResponseBundleRequest.offset); } - resourceList = pagingBuildResourceList(theResponseBundleRequest, bundleProvider, numToReturn); + resourceList = pagingBuildResourceList(theResponseBundleRequest, bundleProvider, numToReturn, responsePageBuilder); RestfulServerUtils.validateResourceListNotNull(resourceList); searchId = pagingBuildSearchId(theResponseBundleRequest, numToReturn, bundleProvider.size()); } - return new ResponsePage(searchId, resourceList, pageSize, numToReturn, bundleProvider.size()); + // We should leave the IBundleProvider to populate these values (specifically resourceList). + // But since we haven't updated all such providers, we will + // build it here (this is at best 'duplicating' work). + responsePageBuilder + .setSearchId(searchId) + .setPageSize(pageSize) + .setNumToReturn(numToReturn) + .setNumTotalResults(bundleProvider.size()) + .setResources(resourceList); + + return responsePageBuilder.build(); } private static String pagingBuildSearchId( @@ -141,11 +154,18 @@ public class ResponseBundleBuilder { } private static List pagingBuildResourceList( - ResponseBundleRequest theResponseBundleRequest, IBundleProvider theBundleProvider, int theNumToReturn) { + ResponseBundleRequest theResponseBundleRequest, + IBundleProvider theBundleProvider, + int theNumToReturn, + ResponsePage.ResponsePageBuilder theResponsePageBuilder + ) { final List retval; if (theNumToReturn > 0 || theBundleProvider.getCurrentPageId() != null) { retval = theBundleProvider.getResources( - theResponseBundleRequest.offset, theNumToReturn + theResponseBundleRequest.offset); + theResponseBundleRequest.offset, + theNumToReturn + theResponseBundleRequest.offset, + theResponsePageBuilder + ); } else { retval = Collections.emptyList(); } @@ -161,15 +181,19 @@ public class ResponseBundleBuilder { } private List offsetBuildResourceList( - IBundleProvider theBundleProvider, RequestedPage theRequestedPage, int theNumToReturn) { + IBundleProvider theBundleProvider, + RequestedPage theRequestedPage, + int theNumToReturn, + ResponsePage.ResponsePageBuilder theResponsePageBuilder + ) { final List retval; if ((theRequestedPage.offset != null && !myIsOffsetModeHistory) || theBundleProvider.getCurrentPageOffset() != null) { // When offset query is done theResult already contains correct amount (+ their includes etc.) so return // everything - retval = theBundleProvider.getResources(0, Integer.MAX_VALUE); + retval = theBundleProvider.getResources(0, Integer.MAX_VALUE, theResponsePageBuilder); } else if (theNumToReturn > 0) { - retval = theBundleProvider.getResources(0, theNumToReturn); + retval = theBundleProvider.getResources(0, theNumToReturn, theResponsePageBuilder); } else { retval = Collections.emptyList(); } @@ -177,7 +201,10 @@ public class ResponseBundleBuilder { } private static int offsetCalculatePageSize( - IRestfulServer server, RequestedPage theRequestedPage, Integer theNumTotalResults) { + IRestfulServer server, + RequestedPage theRequestedPage, + Integer theNumTotalResults + ) { final int retval; if (theRequestedPage.limit != null) { retval = theRequestedPage.limit; @@ -271,7 +298,6 @@ public class ResponseBundleBuilder { } if (theResponsePage.numTotalResults == null || requestedToReturn < theResponsePage.numTotalResults) { - retval.setNext(RestfulServerUtils.createOffsetPagingLink( retval, theResponseBundleRequest.requestDetails.getRequestPath(), diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponsePage.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponsePage.java index 2f9a4742631..c33aaed19a2 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponsePage.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponsePage.java @@ -49,20 +49,83 @@ public class ResponsePage { */ public final int numToReturn; - public ResponsePage( - String theSearchId, - List theResourceList, - int thePageSize, - int theNumToReturn, - Integer theNumTotalResults) { + /** + * The count of resources included from the _include filter. + * These _include resources are otherwise included in the resourceList. + */ + private final int includedResourceCount; + + ResponsePage( + String theSearchId, + List theResourceList, + int thePageSize, + int theNumToReturn, + Integer theNumTotalResults, + int theIncludedResourceCount + ) { searchId = theSearchId; resourceList = theResourceList; pageSize = thePageSize; numToReturn = theNumToReturn; numTotalResults = theNumTotalResults; + includedResourceCount = theIncludedResourceCount; } public int size() { return resourceList.size(); } + + /** + * A builder for constructing ResponsePage objects. + */ + public static class ResponsePageBuilder { + + private String mySearchId; + private List myResources; + private Integer myNumTotalResults; + private int myPageSize; + private int myNumToReturn; + private int myIncludedResourceCount; + + public ResponsePageBuilder setIncludedResourceCount(int theIncludedResourceCount) { + myIncludedResourceCount = theIncludedResourceCount; + return this; + } + + public ResponsePageBuilder setNumToReturn(int theNumToReturn) { + myNumToReturn = theNumToReturn; + return this; + } + + public ResponsePageBuilder setPageSize(int thePageSize) { + myPageSize = thePageSize; + return this; + } + + public ResponsePageBuilder setNumTotalResults(Integer theNumTotalResults) { + myNumTotalResults = theNumTotalResults; + return this; + } + + public ResponsePageBuilder setResources(List theResources) { + myResources = theResources; + return this; + } + + public ResponsePageBuilder setSearchId(String theSearchId) { + mySearchId = theSearchId; + return this; + } + + public ResponsePage build() { + return new ResponsePage( + mySearchId, // search id + myResources, // resource list + myPageSize, // page size + myNumToReturn, // num to return + myNumTotalResults, // total results + myIncludedResourceCount // included count + ); + } + } } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/provider/HashMapResourceProvider.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/provider/HashMapResourceProvider.java index 5fe86adc4ea..4295900cc5a 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/provider/HashMapResourceProvider.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/provider/HashMapResourceProvider.java @@ -53,6 +53,7 @@ import ca.uhn.fhir.rest.server.IResourceProvider; import ca.uhn.fhir.rest.server.SimpleBundleProvider; import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import ca.uhn.fhir.rest.server.method.ResponsePage; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import ca.uhn.fhir.util.ValidateUtil; import com.google.common.collect.Lists; @@ -64,6 +65,7 @@ import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nonnull; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -76,7 +78,6 @@ import java.util.Map; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; -import javax.annotation.Nonnull; import static java.lang.Math.max; import static java.lang.Math.min; @@ -319,7 +320,7 @@ public class HashMapResourceProvider implements IResour @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { // Make sure that "from" isn't less than 0, "to" isn't more than the number available, // and "from" <= "to" diff --git a/hapi-fhir-storage/src/test/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilderTest.java b/hapi-fhir-storage/src/test/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilderTest.java index 3db1725a9db..cb1b7e3e05e 100644 --- a/hapi-fhir-storage/src/test/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilderTest.java +++ b/hapi-fhir-storage/src/test/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilderTest.java @@ -13,6 +13,7 @@ import ca.uhn.fhir.rest.server.SimpleBundleProvider; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.r4.model.Bundle; +import org.hl7.fhir.r4.model.Organization; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -150,6 +151,55 @@ class ResponseBundleBuilderTest { assertNextLink(bundle, DEFAULT_PAGE_SIZE); } + @Test + public void buildResponseBundle_withIncludeFilterAndFewerResultsThanPageSize_doesNotReturnNextLink() { + // setup + int includeResources = 4; + // we want the number of resources returned to be equal to the pagesize + List list = buildXPatientList(DEFAULT_PAGE_SIZE + 1 - includeResources); + + ResponsePage.ResponsePageBuilder builder = new ResponsePage.ResponsePageBuilder(); + builder.setIncludedResourceCount(includeResources); + + ResponseBundleBuilder svc = new ResponseBundleBuilder(true); + + SimpleBundleProvider provider = new SimpleBundleProvider() { + + @Override + public ResponsePage.ResponsePageBuilder getResponsePageBuilder() { + return builder; + } + + @Nonnull + @Override + public List getResources(int theFrom, int theTo, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + List retList = list.subList(theFrom, Math.min(theTo, list.size()-1)); + // our fake includes + for (int i = 0; i < includeResources; i++) { + retList.add(new Organization().setId("Organization/" + i)); + } + return retList; + } + }; + + // TODO - if null, it adds a next link + provider.setSize(DEFAULT_PAGE_SIZE); + + // mocking + when(myServer.canStoreSearchResults()).thenReturn(true); + when(myServer.getPagingProvider()).thenReturn(myPagingProvider); + when(myPagingProvider.getDefaultPageSize()).thenReturn(DEFAULT_PAGE_SIZE); + + ResponseBundleRequest req = buildResponseBundleRequest(provider); + + // test + Bundle bundle = (Bundle) svc.buildResponseBundle(req); + + // verify + assertEquals(1, bundle.getLink().size()); + verifyBundle(bundle, RESOURCE_COUNT, DEFAULT_PAGE_SIZE -1, "A0", "A14"); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void testFilterNulls(boolean theCanStoreSearchResults) { @@ -423,8 +473,12 @@ class ResponseBundleBuilderTest { } private List buildPatientList() { + return buildXPatientList(ResponseBundleBuilderTest.RESOURCE_COUNT); + } + + private List buildXPatientList(int theCount) { List retval = new ArrayList<>(); - for (int i = 0; i < ResponseBundleBuilderTest.RESOURCE_COUNT; ++i) { + for (int i = 0; i < theCount; ++i) { Patient p = new Patient(); p.setId("A" + i); p.setActive(true); @@ -499,10 +553,9 @@ class ResponseBundleBuilderTest { } @Nonnull - @Override - public List getResources(int theFromIndex, int theToIndex) { + public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponseBundleBuilder) { getResourcesCalled = true; - return super.getResources(theFromIndex, theToIndex); + return super.getResources(theFromIndex, theToIndex, theResponseBundleBuilder); } // Emulate the behaviour of PersistedJpaBundleProvider where size() is only set after getResources() has been called