diff --git a/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 b/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 new file mode 100644 index 00000000000..817b9b64960 --- /dev/null +++ b/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 @@ -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" diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java index a60983656ae..59e9a790e1b 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java @@ -148,16 +148,19 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi List 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())); } } } diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/PagingTest.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/PagingTest.java new file mode 100644 index 00000000000..6f4f1ee87b2 --- /dev/null +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/PagingTest.java @@ -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 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 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 theIncludes) { + return ourBundleProvider; + } + + @Override + public Class getResourceType() { + return Patient.class; + } + + } + + +}