Merge branch 'rel_5_4_2'

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"

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<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);
+				int start = Math.max(0, theOffset - pageSize);
-				links.setPrev(RestfulServerUtils.createOffsetPagingLink(links, theRequest.getRequestPath(), theRequest.getTenantId(), start, numToReturn, theRequest.getParameters()));
+				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);
+					int start = Math.max(0, theOffset - pageSize);
-					links.setPrev(RestfulServerUtils.createPagingLink(links, theRequest, searchId, start, numToReturn, theRequest.getParameters()));
+					links.setPrev(RestfulServerUtils.createPagingLink(links, theRequest, searchId, start, pageSize, theRequest.getParameters()));
 				}
 			}
 		}

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<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;
+		}
+
+	}
+
+
+}