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 f0a015b206d..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 @@ -64,7 +64,10 @@ public class PagingPatientProvider implements IResourceProvider { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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..b6b774b9998 --- /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,7 @@ +--- +type: fix +issue: 5192 +title: "Fixed a bug where searches with an _include filter would + break paging resulting in 'next' links to blank pages when + no more results 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 f9b953898ce..00e892163d5 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 @@ -65,15 +65,15 @@ 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 { @@ -173,9 +173,7 @@ 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(), @@ -250,8 +248,9 @@ public class PersistedJpaBundleProvider implements IBundleProvider { } protected List doSearchOrEverything( - final int theFromIndex, final int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder - ) { + 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(); @@ -372,8 +371,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider { @Override public List getResources( - int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder - ) { + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { boolean entityLoaded = ensureSearchEntityLoaded(); assert entityLoaded; assert mySearchEntity != null; @@ -382,9 +380,9 @@ 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: @@ -466,10 +464,9 @@ public class PersistedJpaBundleProvider implements IBundleProvider { // Note: Leave as protected, HSPC depends on this @SuppressWarnings("WeakerAccess") protected List toResourceList( - ISearchBuilder theSearchBuilder, - List thePids, - ResponsePage.ResponsePageBuilder theResponsePageBuilder - ) { + ISearchBuilder theSearchBuilder, + List thePids, + ResponsePage.ResponsePageBuilder theResponsePageBuilder) { List includedPidList = new ArrayList<>(); if (mySearchEntity.getSearchType() == SearchTypeEnum.SEARCH) { Integer maxIncludes = myStorageSettings.getMaximumIncludesToLoadPerPage(); @@ -546,7 +543,11 @@ 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); + theResponsePageBuilder.setOmittedResourceCount(precount - resources.size()); theResponsePageBuilder.setResources(resources); theResponsePageBuilder.setIncludedResourceCount(includedPidList.size()); 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 cea1c339cc3..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 @@ -35,14 +35,15 @@ 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; @@ -68,7 +69,8 @@ public class PersistedJpaSearchFirstPageBundleProvider extends PersistedJpaBundl @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder thePageBuilder) { + public List getResources( + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder thePageBuilder) { ensureSearchEntityLoaded(); QueryParameterUtils.verifySearchHasntFailedOrThrowInternalErrorException(getSearchEntity()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 98ea88db0b9..13c636d585f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java @@ -108,15 +108,6 @@ import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.SingleColumnRowMapper; import org.springframework.transaction.support.TransactionSynchronizationManager; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import javax.persistence.EntityManager; -import javax.persistence.PersistenceContext; -import javax.persistence.PersistenceContextType; -import javax.persistence.Query; -import javax.persistence.Tuple; -import javax.persistence.TypedQuery; -import javax.persistence.criteria.CriteriaBuilder; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -128,6 +119,15 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import javax.persistence.EntityManager; +import javax.persistence.PersistenceContext; +import javax.persistence.PersistenceContextType; +import javax.persistence.Query; +import javax.persistence.Tuple; +import javax.persistence.TypedQuery; +import javax.persistence.criteria.CriteriaBuilder; import static ca.uhn.fhir.jpa.search.builder.QueryStack.LOCATION_POSITION; import static org.apache.commons.lang3.StringUtils.defaultString; 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 e96ed66867f..a94a1b8f689 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 @@ -595,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); @@ -605,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 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 621f9d3b20a..bf2bc4680c0 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 @@ -166,7 +166,8 @@ public interface IBundleProvider { * @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) { + default List getResources( + int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { // TODO - override and implement return getResources(theFromIndex, theToIndex); } 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 9f91f33a201..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 @@ -89,7 +89,8 @@ public class BundleProviderWithNamedPages extends SimpleBundleProvider { @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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 010940ae2e0..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 @@ -48,7 +48,8 @@ public class BundleProviders { return new IBundleProvider() { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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 c0cf34d89c2..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 @@ -26,11 +26,11 @@ 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 { @@ -142,7 +142,8 @@ public class SimpleBundleProvider implements IBundleProvider { @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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 f6e5dd38742..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,7 +181,8 @@ public class HistoryMethodBinding extends BaseResourceReturningMethodBinding { @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + public List getResources( + int theFromIndex, int theToIndex, ResponsePage.ResponsePageBuilder theResponsePageBuilder) { List retVal = resources.getResources(theFromIndex, theToIndex, theResponsePageBuilder); int index = theFromIndex; for (IBaseResource nextResource : retVal) { 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 bc1a32ea59c..9bd24eb7009 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 @@ -111,7 +111,8 @@ public class ResponseBundleBuilder { numToReturn = Math.min(pageSize, size.intValue() - theResponseBundleRequest.offset); } - resourceList = pagingBuildResourceList(theResponseBundleRequest, bundleProvider, numToReturn, responsePageBuilder); + resourceList = + pagingBuildResourceList(theResponseBundleRequest, bundleProvider, numToReturn, responsePageBuilder); RestfulServerUtils.validateResourceListNotNull(resourceList); searchId = pagingBuildSearchId(theResponseBundleRequest, numToReturn, bundleProvider.size()); @@ -121,11 +122,11 @@ public class ResponseBundleBuilder { // 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); + .setSearchId(searchId) + .setPageSize(pageSize) + .setNumToReturn(numToReturn) + .setNumTotalResults(bundleProvider.size()) + .setResources(resourceList); return responsePageBuilder.build(); } @@ -156,15 +157,13 @@ public class ResponseBundleBuilder { ResponseBundleRequest theResponseBundleRequest, IBundleProvider theBundleProvider, int theNumToReturn, - ResponsePage.ResponsePageBuilder theResponsePageBuilder - ) { + ResponsePage.ResponsePageBuilder theResponsePageBuilder) { final List retval; if (theNumToReturn > 0 || theBundleProvider.getCurrentPageId() != null) { retval = theBundleProvider.getResources( - theResponseBundleRequest.offset, - theNumToReturn + theResponseBundleRequest.offset, - theResponsePageBuilder - ); + theResponseBundleRequest.offset, + theNumToReturn + theResponseBundleRequest.offset, + theResponsePageBuilder); } else { retval = Collections.emptyList(); } @@ -180,11 +179,10 @@ public class ResponseBundleBuilder { } private List offsetBuildResourceList( - IBundleProvider theBundleProvider, - RequestedPage theRequestedPage, - int theNumToReturn, - ResponsePage.ResponsePageBuilder theResponsePageBuilder - ) { + IBundleProvider theBundleProvider, + RequestedPage theRequestedPage, + int theNumToReturn, + ResponsePage.ResponsePageBuilder theResponsePageBuilder) { final List retval; if ((theRequestedPage.offset != null && !myIsOffsetModeHistory) || theBundleProvider.getCurrentPageOffset() != null) { @@ -200,10 +198,7 @@ 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; @@ -266,8 +261,8 @@ public class ResponseBundleBuilder { // determine if we are using offset / uncached pages theResponsePage.setUseOffsetPaging(pageRequest.offset != null - || (!server.canStoreSearchResults() && !isEverythingOperation(theResponseBundleRequest.requestDetails)) - || myIsOffsetModeHistory); + || (!server.canStoreSearchResults() && !isEverythingOperation(theResponseBundleRequest.requestDetails)) + || myIsOffsetModeHistory); theResponsePage.setResponseBundleRequest(theResponseBundleRequest); theResponsePage.setRequestedPage(pageRequest); theResponsePage.setBundleProvider(bundleProvider); 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 46a0c71a304..065631927b0 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 @@ -25,6 +25,8 @@ 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; @@ -32,6 +34,9 @@ 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 */ @@ -59,6 +64,13 @@ public class ResponsePage { * 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; // Properties below here are set for calculation of pages; // not part of the response pages in and of themselves @@ -91,19 +103,20 @@ public class ResponsePage { private PagingStyle myPagingStyle; ResponsePage( - String theSearchId, - List theResourceList, - int thePageSize, - int theNumToReturn, - Integer theNumTotalResults, - int theIncludedResourceCount - ) { + String theSearchId, + List theResourceList, + int thePageSize, + int theNumToReturn, + Integer theNumTotalResults, + int theIncludedResourceCount, + int theOmittedResourceCount) { mySearchId = theSearchId; myResourceList = theResourceList; myPageSize = thePageSize; myNumToReturn = theNumToReturn; myNumTotalResults = theNumTotalResults; myIncludedResourceCount = theIncludedResourceCount; + myOmittedResourceCount = theOmittedResourceCount; } public int size() { @@ -120,7 +133,9 @@ public class ResponsePage { return; } - if (myBundleProvider != null && myBundleProvider.getCurrentPageOffset() != null) { + if (myBundleProvider != null && (myBundleProvider.getCurrentPageOffset() != null + || (StringUtils.isNotBlank(myBundleProvider.getNextPageId()) || StringUtils.isNotBlank(myBundleProvider.getPreviousPageId()))) + ) { myPagingStyle = PagingStyle.BUNDLE_PROVIDER_OFFSETS; } else if (myIsUsingOffsetPages) { myPagingStyle = PagingStyle.NONCACHED_OFFSET; @@ -128,9 +143,12 @@ public class ResponsePage { myPagingStyle = PagingStyle.BUNDLE_PROVIDER_PAGE_IDS; } else if (StringUtils.isNotBlank(mySearchId)) { myPagingStyle = PagingStyle.SAVED_SEARCH; + } else { + myPagingStyle = PagingStyle.UNKNOWN; + // only our unit tests end up here + ourLog.warn("Response page requires more information to determine paging style. " + + "Did you remember to mock out all proper values?"); } - - assert myPagingStyle != null : "Response page requires more information to determine paging style"; } public void setRequestedPage(RequestedPage theRequestedPage) { @@ -170,22 +188,18 @@ public class ResponsePage { * we'll also have a myNumTotalResults value. */ return true; - } else if (myNumTotalResults > myNumToReturn + myRequestedPage.offset) { + } else if (myNumTotalResults > myNumToReturn + ObjectUtils.defaultIfNull(myRequestedPage.offset, 0)) { return true; } break; case SAVED_SEARCH: if (myNumTotalResults == null) { - if (myPageSize == myResourceList.size() - myIncludedResourceCount) { - // if the size of the resource list - included resources == pagesize + 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 - myIncludedResourceCount < myNumTotalResults) { - /* - * if we have an accurate total (myNumTotalResults), - * this value is supposed to exclude _include's resource count - */ + } else if (myResponseBundleRequest.offset + myNumToReturn < myNumTotalResults) { return true; } break; @@ -201,42 +215,38 @@ public class ResponsePage { switch (myPagingStyle) { case BUNDLE_PROVIDER_OFFSETS: next = RestfulServerUtils.createOffsetPagingLink( - theLinks, - myResponseBundleRequest.requestDetails.getRequestPath(), - myResponseBundleRequest.requestDetails.getTenantId(), - myRequestedPage.offset + myRequestedPage.limit, - myRequestedPage.limit, - myResponseBundleRequest.getRequestParameters() - ); + 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() - ); + 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() - ); + 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() - ); + theLinks, + myResponseBundleRequest.requestDetails, + mySearchId, + myResponseBundleRequest.offset + myNumToReturn, + myNumToReturn, + myResponseBundleRequest.getRequestParameters()); break; default: next = null; @@ -274,47 +284,45 @@ public class ResponsePage { 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() - ); + 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 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() - ); + 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; - 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; } @@ -336,6 +344,12 @@ public class ResponsePage { private int myPageSize; private int myNumToReturn; private int myIncludedResourceCount; + private int myOmittedResourceCount; + + public ResponsePageBuilder setOmittedResourceCount(int theOmittedResourceCount) { + myOmittedResourceCount = theOmittedResourceCount; + return this; + } public ResponsePageBuilder setIncludedResourceCount(int theIncludedResourceCount) { myIncludedResourceCount = theIncludedResourceCount; @@ -369,33 +383,60 @@ public class ResponsePage { 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 + mySearchId, // search id + myResources, // resource list + myPageSize, // page size + myNumToReturn, // num to return + myNumTotalResults, // total results + myIncludedResourceCount, // included count + myOmittedResourceCount // omitted resources ); } } + /** + * 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, /** - * The bundle provider provides the offsets + * Paging is done by offsets, but + * the bundle provider provides the offsets */ BUNDLE_PROVIDER_OFFSETS, /** - * Paging by page ids, but provided by the bundle provider + * Paging is done by page ids, + * but bundle provider provides the page ids */ BUNDLE_PROVIDER_PAGE_IDS, - /** - * A saved search (search exists in the database). + * The server has a saved search object with an id + * that is used to page through results. */ - SAVED_SEARCH + SAVED_SEARCH, + /** + * Our unit tests have a tendency to not include all the mocked out + * values, resulting in invalid search pages being returned. + * + * These tests also do not check links, so correctness was never verified. + * This search status should never appear in production. + */ + UNKNOWN; } } 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 4295900cc5a..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 @@ -65,7 +65,6 @@ 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; @@ -78,6 +77,7 @@ 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; @@ -320,7 +320,10 @@ public class HashMapResourceProvider implements IResour @SuppressWarnings("unchecked") @Nonnull @Override - public List getResources(int theFromIndex, int theToIndex, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) { + 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"