honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Use pageSize variable to hold page size (#2719) 

* Use pageSize variable to hold page size as previously used variable has other function, so not always hols intended value required for previous link

* Adjust test to standards and use RestfulServerExtension instead of own server

Co-authored-by: juan.marchionatto <juan.marchionatto@smilecdr.com>
This commit is contained in:
jmarchionatto 2021-06-11 17:29:14 -04:00 committed by GitHub
parent 8e807c1436
commit 39d7d4ad03
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 207 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2509-fix-pagination-incorrect-offset-and-count-previous-link-of-last-page.yaml Normal file
View File

@ -0,0 +1,5 @@
---
type: add
issue: 2509
title: "Pagination returned incorrect offset and count in the previous link of the last page
when total element count was one more than multiple of page size. Problem is now fixed"

22
hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java
View File

@ -148,16 +148,19 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi
List<IBaseResource> resourceList;
Integer numTotalResults = theResult.size();
int pageSize;
if (requestOffset != null || !theServer.canStoreSearchResults()) {
if (theLimit != null) {
numToReturn = theLimit;
pageSize = theLimit;
} else {
if (theServer.getDefaultPageSize() != null) {
numToReturn = theServer.getDefaultPageSize();
pageSize = theServer.getDefaultPageSize();
} else {
numToReturn = numTotalResults != null ? numTotalResults : Integer.MAX_VALUE;
pageSize = numTotalResults != null ? numTotalResults : Integer.MAX_VALUE;
}
}
numToReturn = pageSize;
if (requestOffset != null) {
// When offset query is done theResult already contains correct amount (+ their includes etc.) so return everything
resourceList = theResult.getResources(0, Integer.MAX_VALUE);
@ -171,10 +174,11 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi
} else {
IPagingProvider pagingProvider = theServer.getPagingProvider();
if (theLimit == null || theLimit.equals(0)) {
numToReturn = pagingProvider.getDefaultPageSize();
pageSize = pagingProvider.getDefaultPageSize();
} else {
numToReturn = Math.min(pagingProvider.getMaximumPageSize(), theLimit);
pageSize = Math.min(pagingProvider.getMaximumPageSize(), theLimit);
}
numToReturn = pageSize;
if (numTotalResults != null) {
numToReturn = Math.min(numToReturn, numTotalResults - theOffset);
@ -247,8 +251,8 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi
links.setNext(RestfulServerUtils.createOffsetPagingLink(links, theRequest.getRequestPath(), theRequest.getTenantId(), offset + numToReturn, numToReturn, theRequest.getParameters()));
}
if (offset > 0) {
int start = Math.max(0, offset - numToReturn);
links.setPrev(RestfulServerUtils.createOffsetPagingLink(links, theRequest.getRequestPath(), theRequest.getTenantId(), start, numToReturn, theRequest.getParameters()));
int start = Math.max(0, theOffset - pageSize);
links.setPrev(RestfulServerUtils.createOffsetPagingLink(links, theRequest.getRequestPath(), theRequest.getTenantId(), start, pageSize, theRequest.getParameters()));
}
} else if (isNotBlank(theResult.getCurrentPageId())) {
// We're doing named pages
@ -271,8 +275,8 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi
links.setNext((RestfulServerUtils.createPagingLink(links, theRequest, searchId, theOffset + numToReturn, numToReturn, theRequest.getParameters())));
}
if (theOffset > 0) {
int start = Math.max(0, theOffset - numToReturn);
links.setPrev(RestfulServerUtils.createPagingLink(links, theRequest, searchId, start, numToReturn, theRequest.getParameters()));
int start = Math.max(0, theOffset - pageSize);
links.setPrev(RestfulServerUtils.createPagingLink(links, theRequest, searchId, start, pageSize, theRequest.getParameters()));
}
}
}

189
hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/PagingTest.java Normal file
View File

@ -0,0 +1,189 @@
package ca.uhn.fhir.rest.server;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.model.api.Include;
import ca.uhn.fhir.rest.annotation.IncludeParam;
import ca.uhn.fhir.rest.annotation.Search;
import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.test.utilities.server.RestfulServerExtension;
import com.google.common.base.Charsets;
import org.apache.commons.io.IOUtils;
import org.apache.http.NameValuePair;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.utils.URLEncodedUtils;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.hl7.fhir.instance.model.api.IBaseBundle;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.r4.model.Bundle;
import org.hl7.fhir.r4.model.Patient;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import static ca.uhn.fhir.rest.api.Constants.CHARSET_UTF8;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
/**
* Created by jmarchionatto based on old test from: Created by dsotnikov on 2/25/2014.
*/
public class PagingTest {
private FhirContext ourContext = FhirContext.forR4();
@RegisterExtension
public RestfulServerExtension myServerExtension = new RestfulServerExtension(ourContext);
private static SimpleBundleProvider ourBundleProvider;
private static CloseableHttpClient ourClient;
private final IPagingProvider pagingProvider = mock(IPagingProvider.class);
@BeforeAll
public static void beforeClass() throws Exception {
PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(5000, TimeUnit.MILLISECONDS);
HttpClientBuilder builder = HttpClientBuilder.create();
builder.setConnectionManager(connectionManager);
ourClient = builder.build();
}
/**
* Reproduced: https://github.com/hapifhir/hapi-fhir/issues/2509
*
* A bundle size of 21 is used to make last page the third one to validate that the previous link of the
* last page has the correct offset
*/
@Test()
public void testPreviousLinkLastPageWhenBundleSizeEqualsPageSizePlusOne() throws Exception {
initBundleProvider(21);
myServerExtension.getRestfulServer().registerProvider(new DummyPatientResourceProvider());
myServerExtension.getRestfulServer().setPagingProvider(pagingProvider);
when(pagingProvider.canStoreSearchResults()).thenReturn(true);
when(pagingProvider.getDefaultPageSize()).thenReturn(10);
when(pagingProvider.getMaximumPageSize()).thenReturn(50);
when(pagingProvider.storeResultList(any(RequestDetails.class), any(IBundleProvider.class))).thenReturn("ABCD");
when(pagingProvider.retrieveResultList(any(RequestDetails.class), anyString())).thenReturn(ourBundleProvider);
String nextLink;
String base = "http://localhost:" + myServerExtension.getPort();
HttpGet get = new HttpGet(base + "/Patient?");
String responseContent;
try (CloseableHttpResponse resp = ourClient.execute(get)) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(10, bundle.getEntry().size());
assertNull(bundle.getLink(IBaseBundle.LINK_PREV));
String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");
nextLink = bundle.getLink(IBaseBundle.LINK_NEXT).getUrl();
assertNotNull(nextLink, "'next' link is not present");
checkParam(nextLink, Constants.PARAM_PAGINGOFFSET, "10");
checkParam(nextLink, Constants.PARAM_COUNT, "10");
}
try (CloseableHttpResponse resp = ourClient.execute(new HttpGet(nextLink))) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(10, bundle.getEntry().size());
String linkPrev = bundle.getLink(IBaseBundle.LINK_PREV).getUrl();
assertNotNull(linkPrev, "'previous' link is not present");
checkParam(linkPrev, Constants.PARAM_PAGINGOFFSET, "0");
checkParam(linkPrev, Constants.PARAM_COUNT, "10");
String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");
checkParam(linkSelf, Constants.PARAM_PAGINGOFFSET, "10");
checkParam(linkSelf, Constants.PARAM_COUNT, "10");
nextLink = bundle.getLink(IBaseBundle.LINK_NEXT).getUrl();
assertNotNull(nextLink, "'next' link is not present");
checkParam(nextLink, Constants.PARAM_PAGINGOFFSET, "20");
checkParam(nextLink, Constants.PARAM_COUNT, "10");
}
try (CloseableHttpResponse resp = ourClient.execute(new HttpGet(nextLink))) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(1, bundle.getEntry().size());
String linkPrev = bundle.getLink(IBaseBundle.LINK_PREV).getUrl();
assertNotNull(linkPrev, "'previous' link is not present");
checkParam(linkPrev, Constants.PARAM_PAGINGOFFSET, "10");
checkParam(linkPrev, Constants.PARAM_COUNT, "10");
String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");
checkParam(linkSelf, Constants.PARAM_PAGINGOFFSET, "20");
// assertTrue(linkSelf.contains(Constants.PARAM_COUNT + "=1"));
assertNull(bundle.getLink(IBaseBundle.LINK_NEXT));
}
}
private void checkParam(String theUri, String theCheckedParam, String theExpectedValue) {
Optional<String> paramValue = URLEncodedUtils.parse(theUri, CHARSET_UTF8).stream()
.filter(nameValuePair -> nameValuePair.getName().equals(theCheckedParam))
.map(NameValuePair::getValue)
.findAny();
assertTrue(paramValue.isPresent(), "No parameter '" + theCheckedParam + "' present in response");
assertEquals(theExpectedValue, paramValue.get());
}
private void initBundleProvider(int theResourceQty) {
List<IBaseResource> retVal = new ArrayList<>();
for (int i = 0; i < theResourceQty; i++) {
Patient patient = new Patient();
patient.setId("" + i);
patient.addName().setFamily("" + i);
retVal.add(patient);
}
ourBundleProvider = new SimpleBundleProvider(retVal);
}
/**
* Created by dsotnikov on 2/25/2014.
*/
public static class DummyPatientResourceProvider implements IResourceProvider {
@Search
public IBundleProvider findPatient(@IncludeParam Set<Include> theIncludes) {
return ourBundleProvider;
}
@Override
public Class<? extends IBaseResource> getResourceType() {
return Patient.class;
}
}
}