review fixes

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5192-include-in-search-paging-fix.yaml

@@ -1,7 +1,7 @@
 ---
 type: fix
 issue: 5192
-title: "Fixed a bug where searches with an _include filter would
-      break paging resulting in 'next' links to blank pages when
-      no more results are available.
+title: "Fixed a bug where search Bundles with `include` entries from an _include query parameter might
+      trigger a 'next' link to blank pages when
+      no more results `match` results are available.
 "

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java

@@ -64,15 +64,15 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 
+import javax.annotation.Nonnull;
+import javax.persistence.EntityManager;
+import javax.persistence.PersistenceContext;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
-import javax.annotation.Nonnull;
-import javax.persistence.EntityManager;
-import javax.persistence.PersistenceContext;
 
 public class PersistedJpaBundleProvider implements IBundleProvider {
 
@@ -152,13 +152,6 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
 		myRequestPartitionHelperSvc = theRequestPartitionHelperSvc;
 	}
 
-	@Override
-	public ResponsePage.ResponsePageBuilder getResponsePageBuilder() {
-		ResponsePage.ResponsePageBuilder builder = new ResponsePage.ResponsePageBuilder();
-
-		return builder;
-	}
-
 	protected Search getSearchEntity() {
 		return mySearchEntity;
 	}
@@ -365,7 +358,7 @@ public class PersistedJpaBundleProvider implements IBundleProvider {
 	@Nonnull
 	@Override
 	public List<IBaseResource> getResources(int theFromIndex, int theToIndex) {
-		return getResources(theFromIndex, theToIndex, getResponsePageBuilder());
+		return getResources(theFromIndex, theToIndex, new ResponsePage.ResponsePageBuilder());
 	}
 
 	@Override

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/IBundleProvider.java

@@ -26,23 +26,15 @@ import org.apache.commons.lang3.Validate;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
 
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.stream.Collectors;
-import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
 
 public interface IBundleProvider {
 
-	/**
-	 * Returns a ResponsePageBuilder for constructing
-	 * pages that return results.
-	 */
-	default ResponsePage.ResponsePageBuilder getResponsePageBuilder() {
-		return new ResponsePage.ResponsePageBuilder();
-	}
-
 	/**
 	 * If this method is implemented, provides an ID for the current
 	 * page of results. This ID should be unique (at least within
@@ -123,8 +115,6 @@ public interface IBundleProvider {
 	IPrimitiveType<Date> getPublished();
 
 	/**
-	 * Deprecated: Use the getResources(int from, int to, ResourcePageBuilder builder) instead.
-	 * <p>
 	 * Load the given collection of resources by index, plus any additional resources per the
 	 * server's processing rules (e.g. _include'd resources, OperationOutcome, etc.). For example,
 	 * if the method is invoked with index 0,10 the method might return 10 search results, plus an
@@ -137,15 +127,17 @@ public interface IBundleProvider {
 	 * previous page, then the indexes should be ignored and the
 	 * whole page returned.
 	 * </p>
+	 * Note that this implementation should not be used if accurate paging is required,
+	 * as page calculation depends on _include'd resource counts.
+	 * For accurate paging, use {@link IBundleProvider#getResources(int, int, ResponsePage.ResponsePageBuilder)}
 	 *
 	 * @param theFromIndex The low index (inclusive) to return
 	 * @param theToIndex   The high index (exclusive) to return
 	 * @return A list of resources. The size of this list must be at least <code>theToIndex - theFromIndex</code>.
 	 */
 	@Nonnull
-	@Deprecated(forRemoval = true, since = "7.0.0")
 	default List<IBaseResource> getResources(int theFromIndex, int theToIndex) {
-		return getResources(theFromIndex, theToIndex, getResponsePageBuilder());
+		return getResources(theFromIndex, theToIndex, new ResponsePage.ResponsePageBuilder());
 	}
 
 	/**

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilder.java

@@ -90,7 +90,7 @@ public class ResponseBundleBuilder {
 		final List<IBaseResource> resourceList;
 		final int pageSize;
 
-		ResponsePage.ResponsePageBuilder responsePageBuilder = bundleProvider.getResponsePageBuilder();
+		ResponsePage.ResponsePageBuilder responsePageBuilder = new ResponsePage.ResponsePageBuilder();
 
 		int numToReturn;
 		String searchId = null;
@@ -125,7 +125,7 @@ public class ResponseBundleBuilder {
 				.setSearchId(searchId)
 				.setPageSize(pageSize)
 				.setNumToReturn(numToReturn)
-				.setNumTotalResults(bundleProvider.size())
+				.setBundleProvider(bundleProvider)
 				.setResources(resourceList);
 
 		return responsePageBuilder.build();
@@ -265,7 +265,6 @@ public class ResponseBundleBuilder {
 				|| myIsOffsetModeHistory);
 		theResponsePage.setResponseBundleRequest(theResponseBundleRequest);
 		theResponsePage.setRequestedPage(pageRequest);
-		theResponsePage.setBundleProvider(bundleProvider);
 
 		// generate our links
 		theResponsePage.setNextPageIfNecessary(retval);

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ResponsePage.java

@@ -53,7 +53,8 @@ public class ResponsePage {
 	 */
 	private final int myPageSize;
 	/**
-	 * The number of resources that should be returned in the bundle.  Can be smaller than pageSize when the bundleProvider
+	 * The number of resources that should be returned in the bundle.
+	 * Can be smaller than pageSize when the bundleProvider
 	 * has fewer results than the page size.
 	 */
 	private final int myNumToReturn;
@@ -71,6 +72,11 @@ public class ResponsePage {
 	 */
 	private final int myOmittedResourceCount;
 
+	/**
+	 * The bundle provider.
+	 */
+	private final IBundleProvider myBundleProvider;
+
 	// Properties below here are set for calculation of pages;
 	// not part of the response pages in and of themselves
 
@@ -89,11 +95,6 @@ public class ResponsePage {
 	 */
 	private RequestedPage myRequestedPage;
 
-	/**
-	 * The bundle provider.
-	 */
-	private IBundleProvider myBundleProvider;
-
 	/**
 	 * The paging style being used.
 	 * This is determined by a number of conditions,
@@ -106,16 +107,18 @@ public class ResponsePage {
 			List<IBaseResource> theResourceList,
 			int thePageSize,
 			int theNumToReturn,
-			Integer theNumTotalResults,
 			int theIncludedResourceCount,
-			int theOmittedResourceCount) {
+			int theOmittedResourceCount,
+			IBundleProvider theBundleProvider) {
 		mySearchId = theSearchId;
 		myResourceList = theResourceList;
 		myPageSize = thePageSize;
 		myNumToReturn = theNumToReturn;
-		myNumTotalResults = theNumTotalResults;
 		myIncludedResourceCount = theIncludedResourceCount;
 		myOmittedResourceCount = theOmittedResourceCount;
+		myBundleProvider = theBundleProvider;
+
+		myNumTotalResults = myBundleProvider.size();
 	}
 
 	public int size() {
@@ -155,10 +158,10 @@ public class ResponsePage {
 		} else if (StringUtils.isNotBlank(mySearchId)) {
 			myPagingStyle = PagingStyle.SAVED_SEARCH;
 		} else {
-			myPagingStyle = PagingStyle.UNKNOWN;
-			// only our unit tests end up here
-			ourLog.warn("Response page requires more information to determine paging style. "
-					+ "Did you remember to mock out all proper values?");
+			myPagingStyle = PagingStyle.NONE;
+			// only end up here if no paging is desired
+			ourLog.debug("No accurate paging will be generated." +
+				" If accurate paging is desired, ResponsePageBuilder must be provided with additioanl information.");
 		}
 	}
 
@@ -170,10 +173,6 @@ public class ResponsePage {
 		return myBundleProvider;
 	}
 
-	public void setBundleProvider(IBundleProvider theBundleProvider) {
-		myBundleProvider = theBundleProvider;
-	}
-
 	public void setUseOffsetPaging(boolean theIsUsingOffsetPaging) {
 		myIsUsingOffsetPages = theIsUsingOffsetPaging;
 	}
@@ -351,11 +350,11 @@ public class ResponsePage {
 
 		private String mySearchId;
 		private List<IBaseResource> myResources;
-		private Integer myNumTotalResults;
 		private int myPageSize;
 		private int myNumToReturn;
 		private int myIncludedResourceCount;
 		private int myOmittedResourceCount;
+		private IBundleProvider myBundleProvider;
 
 		public ResponsePageBuilder setOmittedResourceCount(int theOmittedResourceCount) {
 			myOmittedResourceCount = theOmittedResourceCount;
@@ -377,8 +376,8 @@ public class ResponsePage {
 			return this;
 		}
 
-		public ResponsePageBuilder setNumTotalResults(Integer theNumTotalResults) {
-			myNumTotalResults = theNumTotalResults;
+		public ResponsePageBuilder setBundleProvider(IBundleProvider theBundleProvider) {
+			myBundleProvider = theBundleProvider;
 			return this;
 		}
 
@@ -398,10 +397,10 @@ public class ResponsePage {
 					myResources, // resource list
 					myPageSize, // page size
 					myNumToReturn, // num to return
-					myNumTotalResults, // total results
 					myIncludedResourceCount, // included count
-					myOmittedResourceCount // omitted resources
-					);
+					myOmittedResourceCount, // omitted resources
+					myBundleProvider // the bundle provider
+			);
 		}
 	}
 
@@ -442,12 +441,11 @@ public class ResponsePage {
 		 */
 		SAVED_SEARCH,
 		/**
-		 * Our unit tests have a tendency to not include all the mocked out
-		 * values, resulting in invalid search pages being returned.
-		 *
-		 * These tests also do not check links, so correctness was never verified.
-		 * This search status should never appear in production.
+		 * No paging is done at all.
+		 * No previous nor next links will be available, even if previous or next
+		 * links exist.
+		 * If paging is required, a different paging method must be specified.
 		 */
-		UNKNOWN;
+		NONE;
 	}
 }

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/api/server/method/ResponsePageTest.java

@@ -72,6 +72,7 @@ public class ResponsePageTest {
 	) {
 		// setup
 		myBundleBuilder
+			.setBundleProvider(myBundleProvider)
 			.setResources(myList);
 		RequestedPage requestedPage = new RequestedPage(
 			0, // offset
@@ -81,7 +82,6 @@ public class ResponsePageTest {
 
 		page.setResponseBundleRequest(myRequest);
 		page.setRequestedPage(requestedPage);
-		page.setBundleProvider(myBundleProvider);
 
 		// when
 		if (theHasNextBoolean) {
@@ -117,6 +117,7 @@ public class ResponsePageTest {
 		// setup
 		// setup
 		myBundleBuilder
+			.setBundleProvider(myBundleProvider)
 			.setResources(myList)
 		;
 		RequestedPage requestedPage = new RequestedPage(
@@ -127,7 +128,6 @@ public class ResponsePageTest {
 
 		page.setResponseBundleRequest(myRequest);
 		page.setRequestedPage(requestedPage);
-		page.setBundleProvider(myBundleProvider);
 
 		// when
 		if (theHasNextBoolean) {
@@ -175,17 +175,26 @@ public class ResponsePageTest {
 	) {
 		// setup
 		myBundleBuilder
+			.setBundleProvider(myBundleProvider)
 			.setResources(myList);
 
 		int offset = theHasPreviousBoolean ? 10 : 0;
-		if (!theNumTotalResultsIsNull) {
-			myBundleBuilder.setNumTotalResults(10 + offset);
-		}
 
 		if (!theHasNextBoolean) {
 			myBundleBuilder.setNumToReturn(10);
 		}
 
+		// when
+		when(myBundleProvider.getCurrentPageOffset())
+			.thenReturn(null);
+		if (!theNumTotalResultsIsNull) {
+			when(myBundleProvider.size())
+				.thenReturn(10 + offset);
+		} else {
+			when(myBundleProvider.size())
+				.thenReturn(null);
+		}
+
 		RequestedPage requestedPage = new RequestedPage(
 			offset, // offset
 			10 // limit
@@ -194,13 +203,8 @@ public class ResponsePageTest {
 
 		page.setResponseBundleRequest(myRequest);
 		page.setRequestedPage(requestedPage);
-		page.setBundleProvider(myBundleProvider);
 		page.setUseOffsetPaging(true);
 
-		// when
-		when(myBundleProvider.getCurrentPageOffset())
-			.thenReturn(null);
-
 		// test
 		page.setNextPageIfNecessary(myLinks);
 		page.setPreviousPageIfNecessary(myLinks);
@@ -230,6 +234,7 @@ public class ResponsePageTest {
 		myBundleBuilder
 			.setResources(myList)
 			.setSearchId("search-id")
+			.setBundleProvider(myBundleProvider)
 			.setPageSize(pageSize);
 
 		int offset = 0;
@@ -247,14 +252,23 @@ public class ResponsePageTest {
 		myBundleBuilder.setIncludedResourceCount(includeResourceCount);
 
 		if (!theNumTotalResultsIsNull) {
-			// accurate total (myNumTotalResults has a value)
-			myBundleBuilder.setNumTotalResults(offset + pageSize);
-
 			if (!theHasNextBoolean) {
 				myBundleBuilder.setNumToReturn(pageSize + offset + includeResourceCount);
 			}
 		}
 
+		// when
+		when(myBundleProvider.getCurrentPageOffset())
+			.thenReturn(null);
+		if (!theNumTotalResultsIsNull) {
+			// accurate total (myNumTotalResults has a value)
+			when(myBundleProvider.size())
+				.thenReturn(offset + pageSize);
+		} else {
+			when(myBundleProvider.size())
+				.thenReturn(null);
+		}
+
 		RequestedPage requestedPage = new RequestedPage(
 			0, // offset
 			10 // limit
@@ -263,11 +277,6 @@ public class ResponsePageTest {
 
 		page.setResponseBundleRequest(myRequest);
 		page.setRequestedPage(requestedPage);
-		page.setBundleProvider(myBundleProvider);
-
-		// when
-		when(myBundleProvider.getCurrentPageOffset())
-			.thenReturn(null);
 
 		// test
 		page.setNextPageIfNecessary(myLinks);

hapi-fhir-storage/src/test/java/ca/uhn/fhir/rest/server/method/ResponseBundleBuilderTest.java

@@ -152,24 +152,15 @@ class ResponseBundleBuilderTest {
 	}
 
 	@Test
-	public void buildResponseBundle_withIncludeFilterAndFewerResultsThanPageSize_doesNotReturnNextLink() {
+	public void buildResponseBundle_withIncludeParamAndFewerResultsThanPageSize_doesNotReturnNextLink() {
 		// setup
 		int includeResources = 4;
 		// we want the number of resources returned to be equal to the pagesize
 		List<IBaseResource> list = buildXPatientList(DEFAULT_PAGE_SIZE - includeResources);
 
-		ResponsePage.ResponsePageBuilder builder = new ResponsePage.ResponsePageBuilder();
-		builder.setIncludedResourceCount(includeResources);
-
 		ResponseBundleBuilder svc = new ResponseBundleBuilder(false);
 
 		SimpleBundleProvider provider = new SimpleBundleProvider() {
-
-			@Override
-			public ResponsePage.ResponsePageBuilder getResponsePageBuilder() {
-				return builder;
-			}
-
 			@Nonnull
 			@Override
 			public List<IBaseResource> getResources(int theFrom, int theTo, @Nonnull ResponsePage.ResponsePageBuilder theResponsePageBuilder) {
@@ -178,6 +169,7 @@ class ResponseBundleBuilderTest {
 				for (int i = 0; i < includeResources; i++) {
 					retList.add(new Organization().setId("Organization/" + i));
 				}
+				theResponsePageBuilder.setIncludedResourceCount(includeResources);
 				return retList;
 			}
 		};