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..7712ad307d2 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,10 @@ 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-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5192-include-in-search-paging-fix.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5192-include-in-search-paging-fix.yaml new file mode 100644 index 00000000000..7a7acb851a1 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5192-include-in-search-paging-fix.yaml @@ -0,0 +1,9 @@ +--- +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. + Specifically, if _include'd resources + requested resources were greater than (or equal to) + requested page size, a 'next' link would be generated, even though no additional + resources are available. +" 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..1738b02f120 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,6 +55,7 @@ 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; @@ -128,6 +129,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider { private String myUuid; private SearchCacheStatusEnum myCacheStatus; private RequestPartitionId myRequestPartitionId; + /** * Constructor */ @@ -180,7 +182,6 @@ public class PersistedJpaBundleProvider implements IBundleProvider { BaseHasResource resource; resource = next; - IFhirResourceDao dao = myDaoRegistry.getResourceDao(next.getResourceType()); retVal.add(myJpaStorageResourceParser.toResource(resource, true)); } @@ -238,7 +239,10 @@ 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, + @Nonnull 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 +257,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; } /** @@ -351,7 +357,13 @@ public class PersistedJpaBundleProvider implements IBundleProvider { @Nonnull @Override - public List getResources(final int theFromIndex, final int theToIndex) { + public List getResources(int theFromIndex, int theToIndex) { + return getResources(theFromIndex, theToIndex, new ResponsePage.ResponsePageBuilder()); + } + + @Override + public List getResources( + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { boolean entityLoaded = ensureSearchEntityLoaded(); assert entityLoaded; assert mySearchEntity != null; @@ -366,7 +378,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider { 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 +455,10 @@ 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(); @@ -521,7 +535,14 @@ public class PersistedJpaBundleProvider implements IBundleProvider { List resources = new ArrayList<>(); theSearchBuilder.loadResourcesByPid(thePids, includedPidList, resources, false, myRequest); + // we will send the resource list to our interceptors + // 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()); + 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..7c61be7d556 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,6 +30,7 @@ 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; @@ -42,11 +43,14 @@ 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 +69,8 @@ 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 +85,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 +106,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/PatientEverythingPaginationR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/PatientEverythingPaginationR4Test.java index aa9a751a9fc..92b9aedb3a8 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/PatientEverythingPaginationR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/PatientEverythingPaginationR4Test.java @@ -7,8 +7,6 @@ import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.EncodingEnum; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.server.BasePagingProvider; -import ca.uhn.fhir.rest.server.IPagingProvider; -import ca.uhn.fhir.rest.server.RestfulServer; import ca.uhn.fhir.util.BundleUtil; import com.google.common.base.Charsets; import org.apache.commons.io.IOUtils; 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..f2cb94da312 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; @@ -594,7 +595,6 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { @Test public void testSearchLinksWorkWithIncludes() { for (int i = 0; i < 5; i++) { - Organization o = new Organization(); o.setId("O" + i); o.setName("O" + i); @@ -604,7 +604,6 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { p.setId("P" + i); p.getManagingOrganization().setReference(oid.getValue()); myClient.update().resource(p).execute(); - } Bundle output = myClient @@ -2651,9 +2650,138 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { int newSize = client.search().forResource(ImagingStudy.class).returnBundle(Bundle.class).execute().getEntry().size(); assertEquals(1, newSize - initialSize); - } + @Test + public void testPagingWithIncludesOnEachResource() { + // setup + int total = 20; + Organization org = new Organization(); + org.setName("ORG"); + IIdType orgId = myOrganizationDao.create(org).getId().toUnqualifiedVersionless(); + + Coding tagCode = new Coding(); + tagCode.setCode("test"); + tagCode.setSystem("http://example.com"); + for (int i = 0; i < total; i++) { + Task t = new Task(); + t.getMeta() + .addTag(tagCode); + t.setStatus(Task.TaskStatus.REQUESTED); + t.getOwner().setReference(orgId.getValue()); + myTaskDao.create(t); + } + HashSet ids = new HashSet<>(); + + // test + int requestedAmount = 10; + Bundle bundle = myClient + .search() + .byUrl("Task?_count=10&_tag=test&status=requested&_include=Task%3Aowner&_sort=status") + .returnBundle(Bundle.class) + .execute(); + assertFalse(bundle.getEntry().isEmpty()); + assertEquals(11, bundle.getEntry().size()); + for (BundleEntryComponent resource : bundle.getEntry()) { + ids.add(resource.getResource().getId()); + } + + 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(); + + // currently, last page could be empty... so we'll + // short circuit out here + if (received != 0) { + // every batch should include the 10 tasks + 1 orgranization + assertEquals(11, received); + for (BundleEntryComponent resource : bundle.getEntry()) { + ids.add(resource.getResource().getId()); + } + } + } else { + nextUrl = null; + } + } while (nextUrl != null); + + // verify + // we should receive all resources and the single organization (repeatedly) + assertEquals(total + 1, ids.size()); + } + + @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(); + assertFalse(bundle.getEntry().isEmpty()); + + 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(); + + // every next result should produce results + assertFalse(bundle.getEntry().isEmpty()); + count += received; + } else { + nextUrl = null; + } + } while (nextUrl != null); + + // verify + // we should receive all resources and linked resources + assertEquals(total + orgs, count); + } + + /** * See #793 */ diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProviderTest.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProviderTest.java index cfdd9dd852c..b68e6d6b221 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProviderTest.java +++ b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProviderTest.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.jpa.api.dao.IDao; import ca.uhn.fhir.jpa.dao.SearchBuilderFactory; import ca.uhn.fhir.jpa.entity.Search; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.server.method.ResponsePage; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -32,7 +33,7 @@ public class PersistedJpaBundleProviderTest { Search searchEntity = new Search(); searchEntity.setTotalCount(1); myPersistedJpaBundleProvider.setSearchEntity(searchEntity); - myPersistedJpaBundleProvider.doSearchOrEverything(0, 1); + myPersistedJpaBundleProvider.doSearchOrEverything(0, 1, new ResponsePage.ResponsePageBuilder()); verifyNoInteractions(myDao); verifyNoInteractions(mySearchBuilderFactory); } 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..5e1b96d63c7 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; @@ -125,13 +126,41 @@ public interface IBundleProvider { * previous page, then the indexes should be ignored and the * whole page returned. *

+ * Note that this implementation should not be used if accurate paging is required, + * as page calculation depends on _include'd resource counts. + * For accurate paging, use {@link IBundleProvider#getResources(int, int, ResponsePage.ResponsePageBuilder)} * * @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 - List getResources(int theFromIndex, int theToIndex); + default List getResources(int theFromIndex, int theToIndex) { + return getResources(theFromIndex, theToIndex, new ResponsePage.ResponsePageBuilder()); + } + + /** + * 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 + * @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. + */ + default List getResources( + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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..7f7adc1ab62 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,11 @@ 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..f8933f50640 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,8 @@ 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..e9479ccd7ad 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,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 org.apache.commons.lang3.builder.ToStringBuilder; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IPrimitiveType; @@ -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,11 @@ 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..2a7ea30cd82 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,9 @@ 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..0d99af7ea91 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 @@ -29,7 +29,6 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.server.IPagingProvider; import ca.uhn.fhir.rest.server.RestfulServerUtils; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; -import org.apache.commons.lang3.ObjectUtils; import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.IBaseBundle; import org.hl7.fhir.instance.model.api.IBaseOperationOutcome; @@ -57,8 +56,8 @@ public class ResponseBundleBuilder { IBaseBundle buildResponseBundle(ResponseBundleRequest theResponseBundleRequest) { final ResponsePage responsePage = buildResponsePage(theResponseBundleRequest); - removeNulls(responsePage.resourceList); - validateIds(responsePage.resourceList); + removeNulls(responsePage.getResourceList()); + validateIds(responsePage.getResourceList()); BundleLinks links = buildLinks(theResponseBundleRequest, responsePage); @@ -75,7 +74,7 @@ public class ResponseBundleBuilder { bundleFactory.addRootPropertiesToBundle( bundleProvider.getUuid(), links, bundleProvider.size(), bundleProvider.getPublished()); bundleFactory.addResourcesToBundle( - new ArrayList<>(pageResponse.resourceList), + new ArrayList<>(pageResponse.getResourceList()), theResponseBundleRequest.bundleType, links.serverBase, server.getBundleInclusionRule(), @@ -91,6 +90,8 @@ public class ResponseBundleBuilder { final List resourceList; final int pageSize; + ResponsePage.ResponsePageBuilder responsePageBuilder = new ResponsePage.ResponsePageBuilder(); + int numToReturn; String searchId = null; @@ -98,24 +99,33 @@ 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) { - numToReturn = pageSize; - } else { - numToReturn = Math.min(pageSize, bundleProvider.size() - theResponseBundleRequest.offset); - } + Integer size = bundleProvider.size(); + numToReturn = + (size == null) ? pageSize : 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) + .setBundleProvider(bundleProvider) + .setResources(resourceList); + + return responsePageBuilder.build(); } private static String pagingBuildSearchId( @@ -141,11 +151,16 @@ 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 +176,18 @@ 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(); } @@ -226,7 +244,6 @@ public class ResponseBundleBuilder { private BundleLinks buildLinks(ResponseBundleRequest theResponseBundleRequest, ResponsePage theResponsePage) { final IRestfulServer server = theResponseBundleRequest.server; - final IBundleProvider bundleProvider = theResponseBundleRequest.bundleProvider; final RequestedPage pageRequest = theResponseBundleRequest.requestedPage; BundleLinks retval = new BundleLinks( @@ -237,107 +254,16 @@ public class ResponseBundleBuilder { retval.setSelf(theResponseBundleRequest.linkSelf); - if (bundleProvider.getCurrentPageOffset() != null) { - - if (StringUtils.isNotBlank(bundleProvider.getNextPageId())) { - retval.setNext(RestfulServerUtils.createOffsetPagingLink( - retval, - theResponseBundleRequest.requestDetails.getRequestPath(), - theResponseBundleRequest.requestDetails.getTenantId(), - pageRequest.offset + pageRequest.limit, - pageRequest.limit, - theResponseBundleRequest.getRequestParameters())); - } - if (StringUtils.isNotBlank(bundleProvider.getPreviousPageId())) { - retval.setNext(RestfulServerUtils.createOffsetPagingLink( - retval, - theResponseBundleRequest.requestDetails.getRequestPath(), - theResponseBundleRequest.requestDetails.getTenantId(), - Math.max(pageRequest.offset - pageRequest.limit, 0), - pageRequest.limit, - theResponseBundleRequest.getRequestParameters())); - } - } - - if (pageRequest.offset != null + // determine if we are using offset / uncached pages + theResponsePage.setUseOffsetPaging(pageRequest.offset != null || (!server.canStoreSearchResults() && !isEverythingOperation(theResponseBundleRequest.requestDetails)) - || myIsOffsetModeHistory) { - // Paging without caching - // We're doing offset pages - int requestedToReturn = theResponsePage.numToReturn; + || myIsOffsetModeHistory); + theResponsePage.setResponseBundleRequest(theResponseBundleRequest); + theResponsePage.setRequestedPage(pageRequest); - if (pageRequest.offset != null) { - requestedToReturn += pageRequest.offset; - } - - if (theResponsePage.numTotalResults == null || requestedToReturn < theResponsePage.numTotalResults) { - - retval.setNext(RestfulServerUtils.createOffsetPagingLink( - retval, - theResponseBundleRequest.requestDetails.getRequestPath(), - theResponseBundleRequest.requestDetails.getTenantId(), - ObjectUtils.defaultIfNull(pageRequest.offset, 0) + theResponsePage.numToReturn, - theResponsePage.numToReturn, - theResponseBundleRequest.getRequestParameters())); - } - - if (pageRequest.offset != null && pageRequest.offset > 0) { - int start = Math.max(0, pageRequest.offset - theResponsePage.pageSize); - retval.setPrev(RestfulServerUtils.createOffsetPagingLink( - retval, - theResponseBundleRequest.requestDetails.getRequestPath(), - theResponseBundleRequest.requestDetails.getTenantId(), - start, - theResponsePage.pageSize, - theResponseBundleRequest.getRequestParameters())); - } - - } else if (StringUtils.isNotBlank(bundleProvider.getCurrentPageId())) { - // We're doing named pages - final String uuid = bundleProvider.getUuid(); - if (StringUtils.isNotBlank(bundleProvider.getNextPageId())) { - retval.setNext(RestfulServerUtils.createPagingLink( - retval, - theResponseBundleRequest.requestDetails, - uuid, - bundleProvider.getNextPageId(), - theResponseBundleRequest.getRequestParameters())); - } - - if (StringUtils.isNotBlank(bundleProvider.getPreviousPageId())) { - retval.setPrev(RestfulServerUtils.createPagingLink( - retval, - theResponseBundleRequest.requestDetails, - uuid, - bundleProvider.getPreviousPageId(), - theResponseBundleRequest.getRequestParameters())); - } - - } else if (theResponsePage.searchId != null) { - - if (theResponsePage.numTotalResults == null - || theResponseBundleRequest.offset + theResponsePage.numToReturn - < theResponsePage.numTotalResults) { - retval.setNext((RestfulServerUtils.createPagingLink( - retval, - theResponseBundleRequest.requestDetails, - theResponsePage.searchId, - theResponseBundleRequest.offset + theResponsePage.numToReturn, - theResponsePage.numToReturn, - theResponseBundleRequest.getRequestParameters()))); - } - - if (theResponseBundleRequest.offset > 0) { - int start = Math.max(0, theResponseBundleRequest.offset - theResponsePage.pageSize); - retval.setPrev(RestfulServerUtils.createPagingLink( - retval, - theResponseBundleRequest.requestDetails, - theResponsePage.searchId, - start, - theResponsePage.pageSize, - theResponseBundleRequest.getRequestParameters())); - } - } + // generate our links + theResponsePage.setNextPageIfNecessary(retval); + theResponsePage.setPreviousPageIfNecessary(retval); return retval; } 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..5f797ffd435 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 @@ -19,7 +19,14 @@ */ package ca.uhn.fhir.rest.server.method; +import ca.uhn.fhir.rest.api.BundleLinks; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.server.RestfulServerUtils; +import org.apache.commons.lang3.ObjectUtils; +import org.apache.commons.lang3.StringUtils; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.List; @@ -27,42 +34,419 @@ import java.util.List; * This is an intermediate record object that holds all the fields required to make the final bundle that will be returned to the client. */ public class ResponsePage { + private static final Logger ourLog = LoggerFactory.getLogger(ResponsePage.class); + /** * The id of the search used to page through search results */ - public final String searchId; + private final String mySearchId; /** * The list of resources that will be used to create the bundle */ - public final List resourceList; + private final List myResourceList; /** * The total number of results that matched the search */ - public final Integer numTotalResults; + private final Integer myNumTotalResults; /** * The number of resources that should be returned in each page */ - public final int pageSize; + private final int myPageSize; /** - * The number of resources that should be returned in the bundle. Can be smaller than pageSize when the bundleProvider + * The number of resources that should be returned in the bundle. + * Can be smaller than pageSize when the bundleProvider * has fewer results than the page size. */ - public final int numToReturn; + private final int myNumToReturn; - public ResponsePage( + /** + * The count of resources included from the _include filter. + * These _include resources are otherwise included in the resourceList. + */ + private final int myIncludedResourceCount; + /** + * This is the count of resources that have been omitted from results + * (typically because of consent interceptors). + * We track these because they shouldn't change paging results, + * even though it will change number of resources returned. + */ + private final int myOmittedResourceCount; + + /** + * The bundle provider. + */ + private final IBundleProvider myBundleProvider; + + // Properties below here are set for calculation of pages; + // not part of the response pages in and of themselves + + /** + * The response bundle request object + */ + private ResponseBundleRequest myResponseBundleRequest; + + /** + * Whether or not this page uses (non-cached) offset paging + */ + private boolean myIsUsingOffsetPages = false; + + /** + * The requested page object (should not be null for proper calculations) + */ + private RequestedPage myRequestedPage; + + /** + * The paging style being used. + * This is determined by a number of conditions, + * including what the bundleprovider provides. + */ + private PagingStyle myPagingStyle; + + ResponsePage( String theSearchId, List theResourceList, int thePageSize, int theNumToReturn, - Integer theNumTotalResults) { - searchId = theSearchId; - resourceList = theResourceList; - pageSize = thePageSize; - numToReturn = theNumToReturn; - numTotalResults = theNumTotalResults; + int theIncludedResourceCount, + int theOmittedResourceCount, + IBundleProvider theBundleProvider) { + mySearchId = theSearchId; + myResourceList = theResourceList; + myPageSize = thePageSize; + myNumToReturn = theNumToReturn; + myIncludedResourceCount = theIncludedResourceCount; + myOmittedResourceCount = theOmittedResourceCount; + myBundleProvider = theBundleProvider; + + myNumTotalResults = myBundleProvider.size(); } public int size() { - return resourceList.size(); + return myResourceList.size(); + } + + public List getResourceList() { + return myResourceList; + } + + private boolean isBundleProviderOffsetPaging() { + if (myBundleProvider != null) { + if (myBundleProvider.getCurrentPageOffset() != null) { + // it's not enough that currentpageoffset is not null + // (sometimes it's 0, even if it's not a currentpageoffset search) + // so we have to make sure either next or prev links are not null + return (StringUtils.isNotBlank(myBundleProvider.getNextPageId()) + || StringUtils.isNotBlank(myBundleProvider.getPreviousPageId())); + } + } + + return false; + } + + private void determinePagingStyle() { + if (myPagingStyle != null) { + // already assigned + return; + } + + if (isBundleProviderOffsetPaging()) { + myPagingStyle = PagingStyle.BUNDLE_PROVIDER_OFFSETS; + } else if (myIsUsingOffsetPages) { + myPagingStyle = PagingStyle.NONCACHED_OFFSET; + } else if (myBundleProvider != null && StringUtils.isNotBlank(myBundleProvider.getCurrentPageId())) { + myPagingStyle = PagingStyle.BUNDLE_PROVIDER_PAGE_IDS; + } else if (StringUtils.isNotBlank(mySearchId)) { + myPagingStyle = PagingStyle.SAVED_SEARCH; + } else { + myPagingStyle = PagingStyle.NONE; + // only end up here if no paging is desired + ourLog.debug( + "No accurate paging will be generated." + + " If accurate paging is desired, ResponsePageBuilder must be provided with additioanl information."); + } + } + + public void setRequestedPage(RequestedPage theRequestedPage) { + myRequestedPage = theRequestedPage; + } + + public IBundleProvider getBundleProvider() { + return myBundleProvider; + } + + public void setUseOffsetPaging(boolean theIsUsingOffsetPaging) { + myIsUsingOffsetPages = theIsUsingOffsetPaging; + } + + public void setResponseBundleRequest(ResponseBundleRequest theRequest) { + myResponseBundleRequest = theRequest; + } + + private boolean hasNextPage() { + determinePagingStyle(); + switch (myPagingStyle) { + case BUNDLE_PROVIDER_OFFSETS: + case BUNDLE_PROVIDER_PAGE_IDS: + return StringUtils.isNotBlank(myBundleProvider.getNextPageId()); + case NONCACHED_OFFSET: + if (myNumTotalResults == null) { + /* + * Having a null total results is synonymous with + * having a next link. Once our results are exhausted, + * we will always have a myNumTotalResults value. + * + * Alternatively, if _total=accurate is provided, + * we'll also have a myNumTotalResults value. + */ + return true; + } else if (myNumTotalResults > myNumToReturn + ObjectUtils.defaultIfNull(myRequestedPage.offset, 0)) { + return true; + } + break; + case SAVED_SEARCH: + if (myNumTotalResults == null) { + if (myPageSize == myResourceList.size() + myOmittedResourceCount - myIncludedResourceCount) { + // if the size of the resource list - included resources + omitted resources == pagesize + // we have more pages + return true; + } + } else if (myResponseBundleRequest.offset + myNumToReturn < myNumTotalResults) { + return true; + } + break; + } + + // fallthrough + return false; + } + + public void setNextPageIfNecessary(BundleLinks theLinks) { + if (hasNextPage()) { + String next; + switch (myPagingStyle) { + case BUNDLE_PROVIDER_OFFSETS: + next = RestfulServerUtils.createOffsetPagingLink( + theLinks, + myResponseBundleRequest.requestDetails.getRequestPath(), + myResponseBundleRequest.requestDetails.getTenantId(), + myRequestedPage.offset + myRequestedPage.limit, + myRequestedPage.limit, + myResponseBundleRequest.getRequestParameters()); + break; + case NONCACHED_OFFSET: + next = RestfulServerUtils.createOffsetPagingLink( + theLinks, + myResponseBundleRequest.requestDetails.getRequestPath(), + myResponseBundleRequest.requestDetails.getTenantId(), + ObjectUtils.defaultIfNull(myRequestedPage.offset, 0) + myNumToReturn, + myNumToReturn, + myResponseBundleRequest.getRequestParameters()); + break; + case BUNDLE_PROVIDER_PAGE_IDS: + next = RestfulServerUtils.createPagingLink( + theLinks, + myResponseBundleRequest.requestDetails, + myBundleProvider.getUuid(), + myBundleProvider.getNextPageId(), + myResponseBundleRequest.getRequestParameters()); + break; + case SAVED_SEARCH: + next = RestfulServerUtils.createPagingLink( + theLinks, + myResponseBundleRequest.requestDetails, + mySearchId, + myResponseBundleRequest.offset + myNumToReturn, + myNumToReturn, + myResponseBundleRequest.getRequestParameters()); + break; + default: + next = null; + break; + } + + if (StringUtils.isNotBlank(next)) { + theLinks.setNext(next); + } + } + } + + private boolean hasPreviousPage() { + determinePagingStyle(); + switch (myPagingStyle) { + case BUNDLE_PROVIDER_OFFSETS: + case BUNDLE_PROVIDER_PAGE_IDS: + return StringUtils.isNotBlank(myBundleProvider.getPreviousPageId()); + case NONCACHED_OFFSET: + if (myRequestedPage != null && myRequestedPage.offset != null && myRequestedPage.offset > 0) { + return true; + } + break; + case SAVED_SEARCH: + return myResponseBundleRequest.offset > 0; + } + + // fallthrough + return false; + } + + public void setPreviousPageIfNecessary(BundleLinks theLinks) { + if (hasPreviousPage()) { + String prev; + switch (myPagingStyle) { + case BUNDLE_PROVIDER_OFFSETS: + prev = RestfulServerUtils.createOffsetPagingLink( + theLinks, + myResponseBundleRequest.requestDetails.getRequestPath(), + myResponseBundleRequest.requestDetails.getTenantId(), + Math.max(ObjectUtils.defaultIfNull(myRequestedPage.offset, 0) - myRequestedPage.limit, 0), + myRequestedPage.limit, + myResponseBundleRequest.getRequestParameters()); + break; + case NONCACHED_OFFSET: + { + int start = Math.max(0, ObjectUtils.defaultIfNull(myRequestedPage.offset, 0) - myPageSize); + prev = RestfulServerUtils.createOffsetPagingLink( + theLinks, + myResponseBundleRequest.requestDetails.getRequestPath(), + myResponseBundleRequest.requestDetails.getTenantId(), + start, + myPageSize, + myResponseBundleRequest.getRequestParameters()); + } + break; + case BUNDLE_PROVIDER_PAGE_IDS: + prev = RestfulServerUtils.createPagingLink( + theLinks, + myResponseBundleRequest.requestDetails, + myBundleProvider.getUuid(), + myBundleProvider.getPreviousPageId(), + myResponseBundleRequest.getRequestParameters()); + break; + case SAVED_SEARCH: + { + int start = Math.max(0, myResponseBundleRequest.offset - myPageSize); + prev = RestfulServerUtils.createPagingLink( + theLinks, + myResponseBundleRequest.requestDetails, + mySearchId, + start, + myPageSize, + myResponseBundleRequest.getRequestParameters()); + } + break; + default: + prev = null; + } + + if (StringUtils.isNotBlank(prev)) { + theLinks.setPrev(prev); + } + } + } + + /** + * A builder for constructing ResponsePage objects. + */ + public static class ResponsePageBuilder { + + private String mySearchId; + private List myResources; + private int myPageSize; + private int myNumToReturn; + private int myIncludedResourceCount; + private int myOmittedResourceCount; + private IBundleProvider myBundleProvider; + + public ResponsePageBuilder setToOmittedResourceCount(int theOmittedResourcesCountToAdd) { + myOmittedResourceCount = theOmittedResourcesCountToAdd; + return this; + } + + 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 setBundleProvider(IBundleProvider theBundleProvider) { + myBundleProvider = theBundleProvider; + 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 + myIncludedResourceCount, // included count + myOmittedResourceCount, // omitted resources + myBundleProvider // the bundle provider + ); + } + } + + /** + * First we determine what kind of paging we use: + * * Bundle Provider Offsets - the bundle provider has offset counts that it uses + * to determine the page. For legacy reasons, it's not enough + * that the bundle provider has a currentOffsetPage. Sometimes + * this value is provided (often as a 0), but no nextPageId nor previousPageId + * is available. Typically this is the case in UnitTests. + * * non-cached offsets - if the server is not storing the search results (and it's not + * an everything operator) OR the Requested Page has an initial offset + * OR it is explicitly set to use non-cached offset + * (ResponseBundleBuilder.myIsOffsetModeHistory) + * * Bundle Provider Page Ids - the bundle provider knows the page ids and will + * provide them. bundle provider will have a currentPageId + * * Saved Search - the server has a saved search object with an id that it + * uses to page through results. + */ + private enum PagingStyle { + /** + * Paging is done by offsets; pages are not cached + */ + NONCACHED_OFFSET, + /** + * Paging is done by offsets, but + * the bundle provider provides the offsets + */ + BUNDLE_PROVIDER_OFFSETS, + /** + * Paging is done by page ids, + * but bundle provider provides the page ids + */ + BUNDLE_PROVIDER_PAGE_IDS, + /** + * The server has a saved search object with an id + * that is used to page through results. + */ + SAVED_SEARCH, + /** + * No paging is done at all. + * No previous nor next links will be available, even if previous or next + * links exist. + * If paging is required, a different paging method must be specified. + */ + NONE; } } 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..71e2b8d8be3 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; @@ -319,7 +320,10 @@ 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-server/src/test/java/ca/uhn/fhir/rest/api/server/method/ResponsePageTest.java b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/api/server/method/ResponsePageTest.java new file mode 100644 index 00000000000..0546cdd8cd5 --- /dev/null +++ b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/api/server/method/ResponsePageTest.java @@ -0,0 +1,320 @@ +package ca.uhn.fhir.rest.api.server.method; + +import ca.uhn.fhir.model.valueset.BundleTypeEnum; +import ca.uhn.fhir.rest.api.BundleLinks; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.api.server.SystemRequestDetails; +import ca.uhn.fhir.rest.server.method.RequestedPage; +import ca.uhn.fhir.rest.server.method.ResponseBundleRequest; +import ca.uhn.fhir.rest.server.method.ResponsePage; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +public class ResponsePageTest { + + private ResponsePage.ResponsePageBuilder myBundleBuilder; + + private BundleLinks myLinks; + + private List myList; + + @Mock + private IBundleProvider myBundleProvider; + + private ResponseBundleRequest myRequest; + + @BeforeEach + public void before() { + myBundleBuilder = new ResponsePage.ResponsePageBuilder(); + + myLinks = new BundleLinks( + "http://localhost", // server base + new HashSet<>(), // includes set + false, // pretty print + BundleTypeEnum.SEARCHSET // links type + ); + + myList = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + // does not matter what these are + myList.add(mock(IBaseResource.class)); + } + + myRequest = createBundleRequest(0); + } + + @ParameterizedTest + @CsvSource({ + "false,false", + "true,false", + "false,true", + "true,true" + }) + public void bundleProviderOffsets_setsNextPreviousLinks_test( + boolean theHasPreviousBoolean, + boolean theHasNextBoolean + ) { + // setup + myBundleBuilder + .setBundleProvider(myBundleProvider) + .setResources(myList); + RequestedPage requestedPage = new RequestedPage( + 0, // offset + 10 // limit + ); + ResponsePage page = myBundleBuilder.build(); + + page.setResponseBundleRequest(myRequest); + page.setRequestedPage(requestedPage); + + // when + if (theHasNextBoolean) { + when(myBundleProvider.getNextPageId()) + .thenReturn("next"); + } + if (theHasPreviousBoolean) { + when(myBundleProvider.getPreviousPageId()) + .thenReturn("previous"); + } + when(myBundleProvider.getCurrentPageOffset()) + .thenReturn(1); + + // test + page.setNextPageIfNecessary(myLinks); + page.setPreviousPageIfNecessary(myLinks); + + // verify + verifyNextAndPreviousLinks(theHasPreviousBoolean, theHasNextBoolean); + } + + @ParameterizedTest + @CsvSource({ + "false,false", + "true,false", + "false,true", + "true,true" + }) + public void bundleProviderPageIds_setsNextPreviousLinks_test( + boolean theHasPreviousBoolean, + boolean theHasNextBoolean + ) { + // setup + // setup + myBundleBuilder + .setBundleProvider(myBundleProvider) + .setResources(myList) + ; + RequestedPage requestedPage = new RequestedPage( + 0, // offset + 10 // limit + ); + ResponsePage page = myBundleBuilder.build(); + + page.setResponseBundleRequest(myRequest); + page.setRequestedPage(requestedPage); + + // when + if (theHasNextBoolean) { + when(myBundleProvider.getNextPageId()) + .thenReturn("next"); + } + if (theHasPreviousBoolean) { + when(myBundleProvider.getPreviousPageId()) + .thenReturn("previous"); + } + + // test + page.setNextPageIfNecessary(myLinks); + page.setPreviousPageIfNecessary(myLinks); + + // verify + verifyNextAndPreviousLinks(theHasPreviousBoolean, theHasNextBoolean); + } + + /** + * Tests for next and previous links + * when doing non-cached offsets. + * + * NB: In a non-cached search, having a null + * myNumTotalResult is synonymous with having + * a next link. + * As such, we do not test for + * null myNumTotalResults and expect no + * next. + * These test cases are omitted as a result. + */ + @ParameterizedTest + @CsvSource({ + "true,false,true", + "true,true,true", + "false,false,false", + "false,true,false", + "false,false,true", + "false,true,true" + }) + public void nonCachedOffsetPaging_setsNextPreviousLinks_test( + boolean theNumTotalResultsIsNull, + boolean theHasPreviousBoolean, + boolean theHasNextBoolean + ) { + // setup + myBundleBuilder + .setBundleProvider(myBundleProvider) + .setResources(myList); + + int offset = theHasPreviousBoolean ? 10 : 0; + + if (!theHasNextBoolean) { + myBundleBuilder.setNumToReturn(10); + } + + // when + when(myBundleProvider.getCurrentPageOffset()) + .thenReturn(null); + if (!theNumTotalResultsIsNull) { + when(myBundleProvider.size()) + .thenReturn(10 + offset); + } else { + when(myBundleProvider.size()) + .thenReturn(null); + } + + RequestedPage requestedPage = new RequestedPage( + offset, // offset + 10 // limit + ); + ResponsePage page = myBundleBuilder.build(); + + page.setResponseBundleRequest(myRequest); + page.setRequestedPage(requestedPage); + page.setUseOffsetPaging(true); + + // test + page.setNextPageIfNecessary(myLinks); + page.setPreviousPageIfNecessary(myLinks); + + // verify + verifyNextAndPreviousLinks(theHasPreviousBoolean, theHasNextBoolean); + } + + @ParameterizedTest + @CsvSource({ + "true,false,false", + "true,true,false", + "true,false,true", + "true,true,true", + "false,false,false", + "false,true,false", + "false,false,true", + "false,true,true" + }) + public void savedSearch_setsNextPreviousLinks_test( + boolean theNumTotalResultsIsNull, + boolean theHasPreviousBoolean, + boolean theHasNextBoolean + ) { + // setup + int pageSize = myList.size(); + myBundleBuilder + .setResources(myList) + .setSearchId("search-id") + .setBundleProvider(myBundleProvider) + .setPageSize(pageSize); + + int offset = 0; + int includeResourceCount = 0; + if (theHasPreviousBoolean) { + offset = 10; + myRequest = createBundleRequest(offset); + } + + if (!theHasNextBoolean) { + // add some includes to reach up to pagesize + includeResourceCount = 1; + } + + myBundleBuilder.setIncludedResourceCount(includeResourceCount); + + if (!theNumTotalResultsIsNull) { + if (!theHasNextBoolean) { + myBundleBuilder.setNumToReturn(pageSize + offset + includeResourceCount); + } + } + + // when + when(myBundleProvider.getCurrentPageOffset()) + .thenReturn(null); + if (!theNumTotalResultsIsNull) { + // accurate total (myNumTotalResults has a value) + when(myBundleProvider.size()) + .thenReturn(offset + pageSize); + } else { + when(myBundleProvider.size()) + .thenReturn(null); + } + + RequestedPage requestedPage = new RequestedPage( + 0, // offset + 10 // limit + ); + ResponsePage page = myBundleBuilder.build(); + + page.setResponseBundleRequest(myRequest); + page.setRequestedPage(requestedPage); + + // test + page.setNextPageIfNecessary(myLinks); + page.setPreviousPageIfNecessary(myLinks); + + // verify + verifyNextAndPreviousLinks(theHasPreviousBoolean, theHasNextBoolean); + } + + private ResponseBundleRequest createBundleRequest(int theOffset) { + RequestDetails details = new SystemRequestDetails(); + details.setFhirServerBase("http://serverbase.com"); + return new ResponseBundleRequest( + null, // server + myBundleProvider, + details, + theOffset, // offset + null, // limit + "self", // self link + new HashSet<>(), // includes + BundleTypeEnum.SEARCHSET, + "search-id" + ); + } + + private void verifyNextAndPreviousLinks( + boolean theHasPreviousBoolean, + boolean theHasNextBoolean + ) { + if (theHasNextBoolean) { + assertNotNull(myLinks.getNext(), "Next link expected but not found"); + } else { + assertNull(myLinks.getNext(), "Found unexpected next link"); + } + if (theHasPreviousBoolean) { + assertNotNull(myLinks.getPrev(), "Previous link expected but not found"); + } else { + assertNull(myLinks.getPrev(), "Found unexpected previous link"); + } + } +} 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..9c2ecc222d6 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,47 @@ class ResponseBundleBuilderTest { assertNextLink(bundle, DEFAULT_PAGE_SIZE); } + @Test + public void buildResponseBundle_withIncludeParamAndFewerResultsThanPageSize_doesNotReturnNextLink() { + // setup + int includeResources = 4; + // we want the number of resources returned to be equal to the pagesize + List list = buildXPatientList(DEFAULT_PAGE_SIZE - includeResources); + + ResponseBundleBuilder svc = new ResponseBundleBuilder(false); + + SimpleBundleProvider provider = new SimpleBundleProvider() { + @Nonnull + @Override + public List getResources(int theFrom, int theTo, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + List retList = new ArrayList<>(list); + // our fake includes + for (int i = 0; i < includeResources; i++) { + retList.add(new Organization().setId("Organization/" + i)); + } + theResponsePageBuilder.setIncludedResourceCount(includeResources); + return retList; + } + }; + + provider.setSize(null); + + // mocking + when(myServer.canStoreSearchResults()).thenReturn(true); + when(myServer.getPagingProvider()).thenReturn(myPagingProvider); + when(myPagingProvider.getDefaultPageSize()).thenReturn(DEFAULT_PAGE_SIZE); + + ResponseBundleRequest req = buildResponseBundleRequest(provider, "search-id"); + + // test + Bundle bundle = (Bundle) svc.buildResponseBundle(req); + + // verify + // no next link + assertEquals(1, bundle.getLink().size()); + assertEquals(DEFAULT_PAGE_SIZE, bundle.getEntry().size()); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) void testFilterNulls(boolean theCanStoreSearchResults) { @@ -423,8 +465,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 +545,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 diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SearchBundleProviderWithNoSizeR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SearchBundleProviderWithNoSizeR4Test.java index ce16156d178..9034759aa55 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SearchBundleProviderWithNoSizeR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SearchBundleProviderWithNoSizeR4Test.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.rest.annotation.Search; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.param.TokenAndListParam; +import ca.uhn.fhir.rest.server.method.ResponsePage; import ca.uhn.fhir.test.utilities.JettyUtil; import ca.uhn.fhir.util.TestUtil; import com.google.common.collect.Lists; @@ -60,28 +61,30 @@ public class SearchBundleProviderWithNoSizeR4Test { @Test public void testBundleProviderReturnsNoSize() throws Exception { Bundle respBundle; - + ourLastBundleProvider = mock(IBundleProvider.class); when(ourLastBundleProvider.getCurrentPageOffset()).thenReturn(null); when(ourLastBundleProvider.size()).thenReturn(null); - when(ourLastBundleProvider.getResources(any(int.class), any(int.class))).then(new Answer>() { - @Override - public List answer(InvocationOnMock theInvocation) { - int from =(Integer)theInvocation.getArguments()[0]; - int to =(Integer)theInvocation.getArguments()[1]; - ArrayList retVal = Lists.newArrayList(); - for (int i = from; i < to; i++) { - Patient p = new Patient(); - p.setId(Integer.toString(i)); - retVal.add(p); + when(ourLastBundleProvider.getResources(any(int.class), any(int.class), any(ResponsePage.ResponsePageBuilder.class))) + .then(new Answer>() { + @Override + public List answer(InvocationOnMock theInvocation) { + int from = (Integer) theInvocation.getArguments()[0]; + int to = (Integer) theInvocation.getArguments()[1]; + ArrayList retVal = Lists.newArrayList(); + for (int i = from; i < to; i++) { + Patient p = new Patient(); + p.setId(Integer.toString(i)); + retVal.add(p); + } + return retVal; } - return retVal; - }}); - + }); + HttpGet httpGet; CloseableHttpResponse status = null; BundleLinkComponent linkNext; - + try { httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_format=json"); status = ourClient.execute(httpGet); @@ -90,17 +93,17 @@ public class SearchBundleProviderWithNoSizeR4Test { assertEquals(200, status.getStatusLine().getStatusCode()); assertEquals("searchAll", ourLastMethod); respBundle = ourCtx.newJsonParser().parseResource(Bundle.class, responseContent); - + assertEquals(10, respBundle.getEntry().size()); assertEquals("Patient/0", respBundle.getEntry().get(0).getResource().getIdElement().toUnqualifiedVersionless().getValue()); linkNext = respBundle.getLink("next"); assertNotNull(linkNext); - + } finally { IOUtils.closeQuietly(status.getEntity().getContent()); } - + when(ourLastBundleProvider.size()).thenReturn(25); try { @@ -111,7 +114,7 @@ public class SearchBundleProviderWithNoSizeR4Test { assertEquals(200, status.getStatusLine().getStatusCode()); assertEquals("searchAll", ourLastMethod); respBundle = ourCtx.newJsonParser().parseResource(Bundle.class, responseContent); - + assertEquals(10, respBundle.getEntry().size()); assertEquals("Patient/10", respBundle.getEntry().get(0).getResource().getIdElement().toUnqualifiedVersionless().getValue()); linkNext = respBundle.getLink("next"); @@ -129,7 +132,7 @@ public class SearchBundleProviderWithNoSizeR4Test { assertEquals(200, status.getStatusLine().getStatusCode()); assertEquals("searchAll", ourLastMethod); respBundle = ourCtx.newJsonParser().parseResource(Bundle.class, responseContent); - + assertEquals(5, respBundle.getEntry().size()); assertEquals("Patient/20", respBundle.getEntry().get(0).getResource().getIdElement().toUnqualifiedVersionless().getValue()); linkNext = respBundle.getLink("next");