Make history pages stable (#5720)

* Make history pages stable

* Add changelog

* Apply spotless

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5720-stable-history-pages.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 5720
+jira: SMILE-8091
+title: "System-level and Type-level History operations on the JPA server (i.e. `_history`) could sometimes
+   contain duplicates or miss entries when a large number of matching resources on the server had identical
+   update timestamps. This has been corrected."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/HistoryBuilder.java

@@ -124,7 +124,20 @@ public class HistoryBuilder {
 
 		from.fetch("myProvenance", JoinType.LEFT);
 
-		criteriaQuery.orderBy(cb.desc(from.get("myUpdated")));
+		/*
+		 * The sort on myUpdated is the important one for _history operations, but there are
+		 * cases where multiple pages of results all have the exact same myUpdated value (e.g.
+		 * if they were all ingested in a single FHIR transaction). So we put a secondary sort
+		 * on the resource PID just to ensure that the sort is stable across queries.
+		 *
+		 * There are indexes supporting the myUpdated sort at each level (system/type/instance)
+		 * but those indexes don't include myResourceId. I don't think that should be an issue
+		 * since myUpdated should generally be unique anyhow. If this ever becomes an issue,
+		 * we might consider adding the resource PID to indexes IDX_RESVER_DATE and
+		 * IDX_RESVER_TYPE_DATE in the future.
+		 * -JA 2024-04-21
+		 */
+		criteriaQuery.orderBy(cb.desc(from.get("myUpdated")), cb.desc(from.get("myResourceId")));
 
 		TypedQuery<ResourceHistoryTable> query = myEntityManager.createQuery(criteriaQuery);
 

hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/provider/r5/ResourceProviderR5Test.java

@@ -4,9 +4,11 @@ import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.entity.Search;
 import ca.uhn.fhir.jpa.model.search.SearchStatusEnum;
 import ca.uhn.fhir.parser.StrictErrorHandler;
+import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.client.interceptor.CapturingInterceptor;
 import ca.uhn.fhir.rest.server.exceptions.NotImplementedOperationException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
+import ca.uhn.fhir.util.BundleBuilder;
 import ca.uhn.fhir.util.UrlUtil;
 import com.google.common.base.Charsets;
 import org.apache.commons.io.IOUtils;
@@ -26,7 +28,6 @@ import org.hl7.fhir.r5.model.DateTimeType;
 import org.hl7.fhir.r5.model.MedicationRequest;
 import org.hl7.fhir.r5.model.Observation;
 import org.hl7.fhir.r5.model.Observation.ObservationComponentComponent;
-import org.hl7.fhir.r5.model.OperationOutcome;
 import org.hl7.fhir.r5.model.Organization;
 import org.hl7.fhir.r5.model.Parameters;
 import org.hl7.fhir.r5.model.Patient;
@@ -34,14 +35,18 @@ import org.hl7.fhir.r5.model.Quantity;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.util.comparator.Comparators;
 
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
 
+import static org.apache.commons.lang3.StringUtils.leftPad;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
@@ -278,7 +283,7 @@ public class ResourceProviderR5Test extends BaseResourceProviderR5Test {
 
 	@Test
 	public void testSearchWithCompositeSort() throws IOException {
-		
+
 		IIdType pid0;
 		IIdType oid1;
 		IIdType oid2;
@@ -294,76 +299,76 @@ public class ResourceProviderR5Test extends BaseResourceProviderR5Test {
 			Observation obs = new Observation();
 			obs.addIdentifier().setSystem("urn:system").setValue("FOO");
 			obs.getSubject().setReferenceElement(pid0);
-			
+
 			ObservationComponentComponent comp = obs.addComponent();
 			CodeableConcept cc = new CodeableConcept();
-			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");			
+			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");
-			comp.setCode(cc);			
+			comp.setCode(cc);
 			comp.setValue(new Quantity().setValue(200));
-			
+
 			oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless();
-			
+
 			ourLog.debug("Observation: \n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs));
 		}
-		
+
 		{
 			Observation obs = new Observation();
 			obs.addIdentifier().setSystem("urn:system").setValue("FOO");
 			obs.getSubject().setReferenceElement(pid0);
-			
+
 			ObservationComponentComponent comp = obs.addComponent();
 			CodeableConcept cc = new CodeableConcept();
-			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");			
+			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");
-			comp.setCode(cc);			
+			comp.setCode(cc);
 			comp.setValue(new Quantity().setValue(300));
-			
+
 			oid2 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless();
-			
+
 			ourLog.debug("Observation: \n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs));
 		}
-		
+
 		{
 			Observation obs = new Observation();
 			obs.addIdentifier().setSystem("urn:system").setValue("FOO");
 			obs.getSubject().setReferenceElement(pid0);
-			
+
 			ObservationComponentComponent comp = obs.addComponent();
 			CodeableConcept cc = new CodeableConcept();
-			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");			
+			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");
-			comp.setCode(cc);			
+			comp.setCode(cc);
 			comp.setValue(new Quantity().setValue(150));
-			
+
 			oid3 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless();
-			
+
 			ourLog.debug("Observation: \n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs));
 		}
-		
+
 		{
 			Observation obs = new Observation();
 			obs.addIdentifier().setSystem("urn:system").setValue("FOO");
 			obs.getSubject().setReferenceElement(pid0);
-			
+
 			ObservationComponentComponent comp = obs.addComponent();
 			CodeableConcept cc = new CodeableConcept();
-			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");			
+			cc.addCoding().setCode("2345-7").setSystem("http://loinc.org");
-			comp.setCode(cc);			
+			comp.setCode(cc);
 			comp.setValue(new Quantity().setValue(250));
 			oid4 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless();
-			
+
 			ourLog.debug("Observation: \n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs));
 		}
-		
+
 		String uri = myServerBase + "/Observation?_sort=combo-code-value-quantity";
 		Bundle found;
-		
+
 		HttpGet get = new HttpGet(uri);
 		try (CloseableHttpResponse resp = ourHttpClient.execute(get)) {
 			String output = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);
 			found = myFhirCtx.newXmlParser().parseResource(Bundle.class, output);
 		}
-		
+
 		ourLog.debug("Bundle: \n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(found));
-		
+
 		List<IIdType> list = toUnqualifiedVersionlessIds(found);
 		assertEquals(4, found.getEntry().size());
 		assertEquals(oid3, list.get(0));
@@ -496,6 +501,66 @@ public class ResourceProviderR5Test extends BaseResourceProviderR5Test {
 		myClient.transaction().withResources(carePlans).execute();
 	}
 
+	@ParameterizedTest
+	@ValueSource(booleans = {true, false})
+	public void testHistoryPaging(boolean theTypeLevel) {
+		// Setup
+		BundleBuilder bb = new BundleBuilder(myFhirContext);
+		List<String> expectedIdentifiers = new ArrayList<>();
+		for (int i = 0; i < 500; i++) {
+			Patient p = new Patient();
+			String identifier = leftPad(Integer.toString(i), 4, '0');
+			expectedIdentifiers.add(identifier);
+			p.addIdentifier().setValue(identifier);
+			bb.addTransactionCreateEntry(p);
+		}
+
+		ourLog.info("Starting transaction with {} entries...", expectedIdentifiers.size());
+		mySystemDao.transaction(mySrd, bb.getBundleTyped());
+
+		// Test
+		ourLog.info("Loading type history, expecting identifiers from {} to {}...", expectedIdentifiers.get(0), expectedIdentifiers.get(expectedIdentifiers.size() - 1));
+		List<String> actualIdentifiers = new ArrayList<>();
+		Bundle historyBundle;
+		if (theTypeLevel) {
+			historyBundle = myClient.history().onType(Patient.class).returnBundle(Bundle.class).execute();
+		} else {
+			historyBundle = myClient.history().onServer().returnBundle(Bundle.class).execute();
+		}
+        while (true) {
+			historyBundle
+				.getEntry()
+				.stream()
+				.map(t -> (Patient) t.getResource())
+				.map(t -> t.getIdentifierFirstRep().getValue())
+				.forEach(actualIdentifiers::add);
+
+			BundleEntryComponent firstEntry = historyBundle.getEntry().get(0);
+			BundleEntryComponent lastEntry = historyBundle.getEntry().get(historyBundle.getEntry().size() - 1);
+			ourLog.info("""
+                            Loaded history page:
+                             * First ID[ {} ] LastUpdated: {}
+                             * Last  ID[ {} ] LastUpdated: {}""",
+				((Patient) firstEntry.getResource()).getIdentifierFirstRep().getValue(),
+				firstEntry.getResource().getMeta().getLastUpdatedElement().getValueAsString(),
+				((Patient) lastEntry.getResource()).getIdentifierFirstRep().getValue(),
+				lastEntry.getResource().getMeta().getLastUpdatedElement().getValueAsString()
+			);
+
+			if (historyBundle.getLink(Constants.LINK_NEXT) != null) {
+				historyBundle = myClient.loadPage().next(historyBundle).execute();
+			} else {
+				break;
+			}
+		}
+
+		// Verify
+		actualIdentifiers.sort(Comparators.comparable());
+
+		assertEquals(expectedIdentifiers, actualIdentifiers);
+	}
+
+
 	private IIdType createOrganization(String methodName, String s) {
 		Organization o1 = new Organization();
 		o1.setName(methodName + s);
@@ -543,7 +608,7 @@ public class ResourceProviderR5Test extends BaseResourceProviderR5Test {
 	protected List<IIdType> toUnqualifiedVersionlessIds(Bundle theFound) {
 		List<IIdType> retVal = new ArrayList<>();
 		for (BundleEntryComponent next : theFound.getEntry()) {
-			if (next.getResource()!= null) {
+			if (next.getResource() != null) {
 				retVal.add(next.getResource().getIdElement().toUnqualifiedVersionless());
 			}
 		}