From 86ea0d60ef14616e8baa9482ed46a09b25c5cc83 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Mon, 25 Jan 2021 08:19:29 -0500 Subject: [PATCH] reduce _count to the maximum page size rather than throwing an error. (#2319) * reduce _count to the maximum page size rather than throwing an error. Also when setting the paging provider on a restful server, automatically set the default page size and max page sized of the restful server from the paging provider. * reword log message * fixing this paging issue has uncovered all sorts of interesting things, including this long hidden bug! * fix tests in sub-optimal way * fix NPE * fix test * fixed a couple of bugs in SearchCoordinatorSvcImpl that surfaced when i added a default page size to RestfulServer --- .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 7 +++--- .../jpa/search/SearchCoordinatorSvcImpl.java | 4 ++-- .../jpa/dao/r4/ConsentEventsDaoR4Test.java | 2 +- ...sentInterceptorResourceProviderR4Test.java | 3 +++ .../r4/ResourceProviderInterceptorR4Test.java | 22 ------------------- .../uhn/fhir/rest/server/RestfulServer.java | 8 ++++++- 6 files changed, 17 insertions(+), 29 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index 5b55793ed12..7cbdd76c2a7 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -1315,11 +1315,12 @@ public abstract class BaseHapiFhirResourceDao extends B theParams.setOffset(offset); } - final Integer count = RestfulServerUtils.extractCountParameter(theRequest); + Integer count = RestfulServerUtils.extractCountParameter(theRequest); if (count != null) { Integer maxPageSize = theRequest.getServer().getMaximumPageSize(); - if (maxPageSize != null) { - Validate.inclusiveBetween(1, theRequest.getServer().getMaximumPageSize(), count, "Count must be positive integer and less than " + maxPageSize); + if (maxPageSize != null && count > maxPageSize) { + ourLog.info("Reducing {} from {} to {} which is the maximum allowable page size.", Constants.PARAM_COUNT, count, maxPageSize); + count = maxPageSize; } theParams.setCount(count); } else if (theRequest.getServer().getDefaultPageSize() != null) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 80328cf2f73..6786f9ff002 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -251,7 +251,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { ourLog.trace("Search entity marked as finished with {} results", search.getNumFound()); break; } - if (search.getNumFound() >= theTo) { + if ((search.getNumFound() - search.getNumBlocked()) >= theTo) { ourLog.trace("Search entity has {} results so far", search.getNumFound()); break; } @@ -1091,7 +1091,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { int minWanted = 0; if (myParams.getCount() != null) { minWanted = myParams.getCount(); - minWanted = Math.max(minWanted, myPagingProvider.getMaximumPageSize()); + minWanted = Math.min(minWanted, myPagingProvider.getMaximumPageSize()); minWanted += currentlyLoaded; } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ConsentEventsDaoR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ConsentEventsDaoR4Test.java index 0db292467f8..8cf3c06258e 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ConsentEventsDaoR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ConsentEventsDaoR4Test.java @@ -140,7 +140,7 @@ public class ConsentEventsDaoR4Test extends BaseJpaR4SystemTest { } } assertEquals(myObservationIdsEvenOnly.subList(10, 25), returnedIdValues, "Wrong response from " + outcome.getClass()); - assertEquals(2, hitCount.get()); + assertEquals(3, hitCount.get()); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4Test.java index 3fbd8e3e15a..2c92497b1a1 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4Test.java @@ -65,6 +65,7 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.blankOrNullString; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.matchesPattern; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -126,6 +127,7 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid .execute(); List resources = BundleUtil.toListOfResources(myFhirCtx, result); List returnedIdValues = toUnqualifiedVersionlessIdValues(resources); + assertThat(returnedIdValues, hasSize(15)); assertEquals(myObservationIdsEvenOnly.subList(0, 15), returnedIdValues); // Fetch the next page @@ -135,6 +137,7 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid .execute(); resources = BundleUtil.toListOfResources(myFhirCtx, result); returnedIdValues = toUnqualifiedVersionlessIdValues(resources); + assertThat(returnedIdValues, hasSize(10)); assertEquals(myObservationIdsEvenOnly.subList(15, 25), returnedIdValues); } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java index 907d9baf064..8d02586a564 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java @@ -13,8 +13,6 @@ import ca.uhn.fhir.parser.IParser; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.server.RequestDetails; -import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor; -import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails; import ca.uhn.fhir.rest.server.interceptor.ServerOperationInterceptorAdapter; import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import com.google.common.collect.Lists; @@ -52,7 +50,6 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; @@ -428,23 +425,4 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes } } - public static void verifyDaoInterceptor(IServerInterceptor theDaoInterceptor) { - ArgumentCaptor ardCaptor; - ArgumentCaptor opTypeCaptor; - ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class); - opTypeCaptor = ArgumentCaptor.forClass(RestOperationTypeEnum.class); - verify(theDaoInterceptor, atLeast(1)).incomingRequestPreHandled(opTypeCaptor.capture(), ardCaptor.capture()); -// boolean good = false; -// for (int i = 0; i < opTypeCaptor.getAllValues().size(); i++) { -// if (RestOperationTypeEnum.CREATE.equals(opTypeCaptor.getAllValues().get(i))) { -// if ("Patient".equals(ardCaptor.getValue().getResourceType())) { -// if (ardCaptor.getValue().getResource() != null) { -// good = true; -// } -// } -// } -// } -// assertTrue(good); - } - } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java index e1e44a4b1c4..ee9b1cb1da8 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServer.java @@ -675,10 +675,16 @@ public class RestfulServer extends HttpServlet implements IRestfulServernull to use no paging (which is the default) + * Sets the paging provider to use, or null to use no paging (which is the default). + * This will set defaultPageSize and maximumPageSize from the paging provider. */ public void setPagingProvider(IPagingProvider thePagingProvider) { myPagingProvider = thePagingProvider; + if (myPagingProvider != null) { + setDefaultPageSize(myPagingProvider.getDefaultPageSize()); + setMaximumPageSize(myPagingProvider.getMaximumPageSize()); + } + } @Override