From 3974239c48a85894c7660b6fa99f160072bc3da5 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 21 Feb 2024 13:32:59 -0500 Subject: [PATCH] Make history pages stable (#5720) * Make history pages stable * Add changelog * Apply spotless --- .../7_2_0/5720-stable-history-pages.yaml | 7 + .../ca/uhn/fhir/jpa/dao/HistoryBuilder.java | 15 ++- .../provider/r5/ResourceProviderR5Test.java | 123 +++++++++++++----- 3 files changed, 115 insertions(+), 30 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5720-stable-history-pages.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5720-stable-history-pages.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5720-stable-history-pages.yaml new file mode 100644 index 00000000000..d4afe3e8396 --- /dev/null +++ b/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." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/HistoryBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/HistoryBuilder.java index 1349afaa758..21f40b20ee5 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/HistoryBuilder.java +++ b/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 query = myEntityManager.createQuery(criteriaQuery); diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/provider/r5/ResourceProviderR5Test.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/provider/r5/ResourceProviderR5Test.java index 21a41aef519..961621e7a6b 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/provider/r5/ResourceProviderR5Test.java +++ b/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"); - comp.setCode(cc); + cc.addCoding().setCode("2345-7").setSystem("http://loinc.org"); + 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"); - comp.setCode(cc); + cc.addCoding().setCode("2345-7").setSystem("http://loinc.org"); + 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"); - comp.setCode(cc); + cc.addCoding().setCode("2345-7").setSystem("http://loinc.org"); + 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"); - comp.setCode(cc); + cc.addCoding().setCode("2345-7").setSystem("http://loinc.org"); + 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 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 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 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 toUnqualifiedVersionlessIds(Bundle theFound) { List retVal = new ArrayList<>(); for (BundleEntryComponent next : theFound.getEntry()) { - if (next.getResource()!= null) { + if (next.getResource() != null) { retVal.add(next.getResource().getIdElement().toUnqualifiedVersionless()); } }