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
This commit is contained in:
parent
5d2e830b15
commit
86ea0d60ef
|
@ -1315,11 +1315,12 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
|
||||||
theParams.setOffset(offset);
|
theParams.setOffset(offset);
|
||||||
}
|
}
|
||||||
|
|
||||||
final Integer count = RestfulServerUtils.extractCountParameter(theRequest);
|
Integer count = RestfulServerUtils.extractCountParameter(theRequest);
|
||||||
if (count != null) {
|
if (count != null) {
|
||||||
Integer maxPageSize = theRequest.getServer().getMaximumPageSize();
|
Integer maxPageSize = theRequest.getServer().getMaximumPageSize();
|
||||||
if (maxPageSize != null) {
|
if (maxPageSize != null && count > maxPageSize) {
|
||||||
Validate.inclusiveBetween(1, theRequest.getServer().getMaximumPageSize(), count, "Count must be positive integer and less than " + maxPageSize);
|
ourLog.info("Reducing {} from {} to {} which is the maximum allowable page size.", Constants.PARAM_COUNT, count, maxPageSize);
|
||||||
|
count = maxPageSize;
|
||||||
}
|
}
|
||||||
theParams.setCount(count);
|
theParams.setCount(count);
|
||||||
} else if (theRequest.getServer().getDefaultPageSize() != null) {
|
} else if (theRequest.getServer().getDefaultPageSize() != null) {
|
||||||
|
|
|
@ -251,7 +251,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
|
||||||
ourLog.trace("Search entity marked as finished with {} results", search.getNumFound());
|
ourLog.trace("Search entity marked as finished with {} results", search.getNumFound());
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
if (search.getNumFound() >= theTo) {
|
if ((search.getNumFound() - search.getNumBlocked()) >= theTo) {
|
||||||
ourLog.trace("Search entity has {} results so far", search.getNumFound());
|
ourLog.trace("Search entity has {} results so far", search.getNumFound());
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
|
@ -1091,7 +1091,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
|
||||||
int minWanted = 0;
|
int minWanted = 0;
|
||||||
if (myParams.getCount() != null) {
|
if (myParams.getCount() != null) {
|
||||||
minWanted = myParams.getCount();
|
minWanted = myParams.getCount();
|
||||||
minWanted = Math.max(minWanted, myPagingProvider.getMaximumPageSize());
|
minWanted = Math.min(minWanted, myPagingProvider.getMaximumPageSize());
|
||||||
minWanted += currentlyLoaded;
|
minWanted += currentlyLoaded;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -140,7 +140,7 @@ public class ConsentEventsDaoR4Test extends BaseJpaR4SystemTest {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
assertEquals(myObservationIdsEvenOnly.subList(10, 25), returnedIdValues, "Wrong response from " + outcome.getClass());
|
assertEquals(myObservationIdsEvenOnly.subList(10, 25), returnedIdValues, "Wrong response from " + outcome.getClass());
|
||||||
assertEquals(2, hitCount.get());
|
assertEquals(3, hitCount.get());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|
|
@ -65,6 +65,7 @@ import static org.hamcrest.CoreMatchers.containsString;
|
||||||
import static org.hamcrest.CoreMatchers.not;
|
import static org.hamcrest.CoreMatchers.not;
|
||||||
import static org.hamcrest.MatcherAssert.assertThat;
|
import static org.hamcrest.MatcherAssert.assertThat;
|
||||||
import static org.hamcrest.Matchers.blankOrNullString;
|
import static org.hamcrest.Matchers.blankOrNullString;
|
||||||
|
import static org.hamcrest.Matchers.hasSize;
|
||||||
import static org.hamcrest.Matchers.matchesPattern;
|
import static org.hamcrest.Matchers.matchesPattern;
|
||||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||||
|
@ -126,6 +127,7 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
|
||||||
.execute();
|
.execute();
|
||||||
List<IBaseResource> resources = BundleUtil.toListOfResources(myFhirCtx, result);
|
List<IBaseResource> resources = BundleUtil.toListOfResources(myFhirCtx, result);
|
||||||
List<String> returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
|
List<String> returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
|
||||||
|
assertThat(returnedIdValues, hasSize(15));
|
||||||
assertEquals(myObservationIdsEvenOnly.subList(0, 15), returnedIdValues);
|
assertEquals(myObservationIdsEvenOnly.subList(0, 15), returnedIdValues);
|
||||||
|
|
||||||
// Fetch the next page
|
// Fetch the next page
|
||||||
|
@ -135,6 +137,7 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
|
||||||
.execute();
|
.execute();
|
||||||
resources = BundleUtil.toListOfResources(myFhirCtx, result);
|
resources = BundleUtil.toListOfResources(myFhirCtx, result);
|
||||||
returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
|
returnedIdValues = toUnqualifiedVersionlessIdValues(resources);
|
||||||
|
assertThat(returnedIdValues, hasSize(10));
|
||||||
assertEquals(myObservationIdsEvenOnly.subList(15, 25), returnedIdValues);
|
assertEquals(myObservationIdsEvenOnly.subList(15, 25), returnedIdValues);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -13,8 +13,6 @@ import ca.uhn.fhir.parser.IParser;
|
||||||
import ca.uhn.fhir.rest.api.Constants;
|
import ca.uhn.fhir.rest.api.Constants;
|
||||||
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
|
import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
|
||||||
import ca.uhn.fhir.rest.api.server.RequestDetails;
|
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.interceptor.ServerOperationInterceptorAdapter;
|
||||||
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
|
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
|
||||||
import com.google.common.collect.Lists;
|
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.assertEquals;
|
||||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||||
import static org.mockito.ArgumentMatchers.any;
|
import static org.mockito.ArgumentMatchers.any;
|
||||||
import static org.mockito.Mockito.atLeast;
|
|
||||||
import static org.mockito.Mockito.eq;
|
import static org.mockito.Mockito.eq;
|
||||||
import static org.mockito.Mockito.mock;
|
import static org.mockito.Mockito.mock;
|
||||||
import static org.mockito.Mockito.reset;
|
import static org.mockito.Mockito.reset;
|
||||||
|
@ -428,23 +425,4 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
public static void verifyDaoInterceptor(IServerInterceptor theDaoInterceptor) {
|
|
||||||
ArgumentCaptor<ActionRequestDetails> ardCaptor;
|
|
||||||
ArgumentCaptor<RestOperationTypeEnum> 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);
|
|
||||||
}
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -675,10 +675,16 @@ public class RestfulServer extends HttpServlet implements IRestfulServer<Servlet
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Sets the paging provider to use, or <code>null</code> to use no paging (which is the default)
|
* Sets the paging provider to use, or <code>null</code> to use no paging (which is the default).
|
||||||
|
* This will set defaultPageSize and maximumPageSize from the paging provider.
|
||||||
*/
|
*/
|
||||||
public void setPagingProvider(IPagingProvider thePagingProvider) {
|
public void setPagingProvider(IPagingProvider thePagingProvider) {
|
||||||
myPagingProvider = thePagingProvider;
|
myPagingProvider = thePagingProvider;
|
||||||
|
if (myPagingProvider != null) {
|
||||||
|
setDefaultPageSize(myPagingProvider.getDefaultPageSize());
|
||||||
|
setMaximumPageSize(myPagingProvider.getMaximumPageSize());
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
|
|
Loading…
Reference in New Issue