diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5619-fix-auto-version-reference-for-conditional-update-with-id-placeholder.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5619-fix-auto-version-reference-for-conditional-update-with-id-placeholder.yaml new file mode 100644 index 00000000000..3077b0e9595 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5619-fix-auto-version-reference-for-conditional-update-with-id-placeholder.yaml @@ -0,0 +1,7 @@ +--- +type: fix +issue: 5619 +jira: SMILE-7909 +title: "Previously, when a transaction was posted with a resource that had placeholder references and auto versioning +references enabled for that path, if the target resource was included in the Bundle but not modified, the reference was +saved with a version number that didn't exist. This has been fixed." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index afbd4fff725..33fdc826980 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java @@ -3379,7 +3379,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test assertEquals(8, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); myCaptureQueriesListener.logInsertQueries(); assertEquals(4, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); - assertEquals(7, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); + assertEquals(6, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countDeleteQueriesForCurrentThread()); ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); @@ -3462,7 +3462,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test myCaptureQueriesListener.logSelectQueriesForCurrentThread(); assertEquals(8, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(2, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); - assertEquals(6, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); + assertEquals(5, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countDeleteQueriesForCurrentThread()); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4VersionedReferenceTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4VersionedReferenceTest.java index 49a714afd35..da64fee8df5 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4VersionedReferenceTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4VersionedReferenceTest.java @@ -380,17 +380,11 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test { observation.getEncounter().setReference(encounter.getId()); // not versioned builder.addTransactionCreateEntry(observation); - Bundle outcome = mySystemDao.transaction(mySrd, (Bundle) builder.getBundle()); - ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); - assertEquals("200 OK", outcome.getEntry().get(0).getResponse().getStatus()); - assertEquals("200 OK", outcome.getEntry().get(1).getResponse().getStatus()); - assertEquals("201 Created", outcome.getEntry().get(2).getResponse().getStatus()); + Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(), + List.of("200 OK", "200 OK", "201 Created"), List.of("2", "1", "1")); IdType patientId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); IdType encounterId = new IdType(outcome.getEntry().get(1).getResponse().getLocation()); IdType observationId = new IdType(outcome.getEntry().get(2).getResponse().getLocation()); - assertEquals("2", patientId.getVersionIdPart()); - assertEquals("1", encounterId.getVersionIdPart()); - assertEquals("1", observationId.getVersionIdPart()); // Read back and verify that reference is now versioned observation = myObservationDao.read(observationId); @@ -429,14 +423,10 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test { builder.addTransactionCreateEntry(observation); myCaptureQueriesListener.clear(); - Bundle outcome = mySystemDao.transaction(mySrd, (Bundle) builder.getBundle()); - ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); - assertEquals("200 OK", outcome.getEntry().get(0).getResponse().getStatus()); - assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus()); + Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(), + List.of("200 OK", "201 Created"), List.of("3", "1")); IdType patientId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); IdType observationId = new IdType(outcome.getEntry().get(1).getResponse().getLocation()); - assertEquals("3", patientId.getVersionIdPart()); - assertEquals("1", observationId.getVersionIdPart()); // Make sure we're not introducing any extra DB operations assertEquals(3, myCaptureQueriesListener.logSelectQueries().size()); @@ -468,14 +458,10 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test { myCaptureQueriesListener.clear(); - Bundle outcome = mySystemDao.transaction(mySrd, (Bundle) builder.getBundle()); - ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); - assertEquals("200 OK", outcome.getEntry().get(0).getResponse().getStatus()); - assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus()); + Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(), + List.of("200 OK", "201 Created"), List.of("3", "1")); IdType patientId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); IdType observationId = new IdType(outcome.getEntry().get(1).getResponse().getLocation()); - assertEquals("3", patientId.getVersionIdPart()); - assertEquals("1", observationId.getVersionIdPart()); // Make sure we're not introducing any extra DB operations assertEquals(4, myCaptureQueriesListener.logSelectQueries().size()); @@ -563,20 +549,91 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test { BundleBuilder builder = new BundleBuilder(myFhirContext); builder.addTransactionCreateEntry(messageHeader); - ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(builder.getBundle())); - Bundle outcome = mySystemDao.transaction(mySrd, (Bundle) builder.getBundle()); - ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); - assertEquals("201 Created", outcome.getEntry().get(0).getResponse().getStatus()); - + Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(), + List.of("201 Created"), List.of("1")); IdType messageHeaderId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); assertEquals("2", patient.getIdElement().getVersionIdPart()); - assertEquals("1", messageHeaderId.getVersionIdPart()); // read back and verify that reference is versioned messageHeader = myMessageHeaderDao.read(messageHeaderId); assertEquals(patient.getIdElement().getValue(), messageHeader.getFocus().get(0).getReference()); } + @Test + @DisplayName("#5619 Incorrect version of auto versioned reference for conditional update with urn id placeholder") + public void testInsertVersionedReferencesByPath_conditionalUpdateNoOpInTransaction_addsCorrectVersionToReference() { + Supplier supplier = () -> { + // create patient + Patient patient = new Patient(); + patient.setActive(true); + patient.addIdentifier().setSystem("http://example.com").setValue("test"); + + // add patient to the Bundle - conditional update with placeholder url + Bundle bundle = new Bundle(); + bundle.setType(Bundle.BundleType.TRANSACTION); + bundle.addEntry() + .setResource(patient) + .setFullUrl("urn:uuid:00001") + .getRequest() + .setMethod(Bundle.HTTPVerb.PUT) + .setUrl("Patient?identifier=http://example.com|test"); + + // create MessageHeader + MessageHeader messageHeader = new MessageHeader(); + messageHeader.getMeta().setExtension(messageHeaderAutoVersionExtension); + // add reference + messageHeader.addFocus().setReference("urn:uuid:00001"); + + bundle.addEntry() + .setResource(messageHeader) + .getRequest() + .setMethod(Bundle.HTTPVerb.POST) + .setUrl("/MessageHeader"); + + return bundle; + }; + + // create bundle first time + Bundle outcome = createAndValidateBundle(supplier.get(), + List.of("201 Created", "201 Created"), List.of("1", "1")); + IdType patientId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); + IdType messageHeaderId = new IdType(outcome.getEntry().get(1).getResponse().getLocation()); + + // read back and verify that reference is versioned and correct + Patient patient = myPatientDao.read(patientId); + MessageHeader messageHeader = myMessageHeaderDao.read(messageHeaderId); + assertEquals(patient.getIdElement().getValue(), messageHeader.getFocus().get(0).getReference()); + + // create bundle second time + outcome = createAndValidateBundle(supplier.get(), List.of("200 OK", "201 Created"), List.of("1", "1")); + patientId = new IdType(outcome.getEntry().get(0).getResponse().getLocation()); + messageHeaderId = new IdType(outcome.getEntry().get(1).getResponse().getLocation()); + + // read back and verify that reference is versioned and correct + patient = myPatientDao.read(patientId); + messageHeader = myMessageHeaderDao.read(messageHeaderId); + assertEquals(patient.getIdElement().getValue(), messageHeader.getFocus().get(0).getReference()); + } + + private Bundle createAndValidateBundle(Bundle theBundle, List theOutcomeStatuses, + List theOutcomeVersions) { + assertEquals(theBundle.getEntry().size(), theOutcomeStatuses.size(), + "Size of OutcomeStatuses list is incorrect"); + assertEquals(theBundle.getEntry().size(), theOutcomeVersions.size(), + "Size of OutcomeVersions list is incorrect"); + + ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(theBundle)); + Bundle result = mySystemDao.transaction(mySrd, theBundle); + ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(theBundle)); + + for (int i = 0; i < result.getEntry().size(); i++) { + assertEquals(theOutcomeStatuses.get(i), result.getEntry().get(i).getResponse().getStatus()); + IIdType resultId = new IdType(result.getEntry().get(i).getResponse().getLocation()); + assertEquals(theOutcomeVersions.get(i), resultId.getVersionIdPart()); + } + return result; + } + private Patient createAndUpdatePatient(String thePatientId) { Patient patient = new Patient(); patient.setId(thePatientId); diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 08cb4b8b03f..05f912d0150 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -1806,10 +1806,7 @@ public abstract class BaseTransactionProcessor { theDaoMethodOutcome.setId(newId); - IIdType target = theIdSubstitutions.getForSource(newId); - if (target != null) { - target.setValue(newId.getValue()); - } + theIdSubstitutions.updateTargets(newId); if (theDaoMethodOutcome.getOperationOutcome() != null) { IBase responseEntry = entriesToProcess.getResponseBundleEntryWithVersionlessComparison(newId); diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMap.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMap.java index 26130af1b2d..66a17baedef 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMap.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMap.java @@ -27,6 +27,7 @@ import org.hl7.fhir.instance.model.api.IIdType; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; public class IdSubstitutionMap { @@ -87,6 +88,22 @@ public class IdSubstitutionMap { return myMap.isEmpty(); } + /** + * Updates all targets of the map with a new id value if the input id has + * the same ResourceType and IdPart as the target id. + */ + public void updateTargets(IIdType theNewId) { + if (theNewId == null) { + return; + } + String newUnqualifiedVersionLessId = theNewId.toUnqualifiedVersionless().getValue(); + entrySet().stream() + .map(Pair::getValue) + .filter(targetId -> + Objects.equals(targetId.toUnqualifiedVersionless().getValue(), newUnqualifiedVersionLessId)) + .forEach(targetId -> targetId.setValue(theNewId.getValue())); + } + private static class Entry { private final String myUnversionedId; diff --git a/hapi-fhir-storage/src/test/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMapTest.java b/hapi-fhir-storage/src/test/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMapTest.java new file mode 100644 index 00000000000..1bfb76dd375 --- /dev/null +++ b/hapi-fhir-storage/src/test/java/ca/uhn/fhir/jpa/dao/IdSubstitutionMapTest.java @@ -0,0 +1,68 @@ +package ca.uhn.fhir.jpa.dao; + +import org.hl7.fhir.r4.model.IdType; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +@ExtendWith(MockitoExtension.class) +public class IdSubstitutionMapTest { + + private IdSubstitutionMap idSubstitutions; + + @BeforeEach + void setUp() { + idSubstitutions = new IdSubstitutionMap(); + } + + @ParameterizedTest + @CsvSource({ + "Patient/123/_history/3, Patient/123/_history/2", + "Patient/123/_history/3, Patient/123" + }) + void testUpdateTargets_inputMatchesTarget_onlyMatchedTargetUpdated(String theInputId, String theTargetId) { + idSubstitutions.put(new IdType("urn:uuid:1234"), new IdType(theTargetId)); + idSubstitutions.put(new IdType("urn:uuid:5000"), new IdType("Patient/5000")); + idSubstitutions.put(new IdType("urn:uuid:6000"), new IdType("Patient/6000_history/3")); + + idSubstitutions.updateTargets(new IdType(theInputId)); + + assertEquals(theInputId, idSubstitutions.getForSource("urn:uuid:1234").getValue()); + assertEquals("Patient/5000", idSubstitutions.getForSource("urn:uuid:5000").getValue()); + assertEquals("Patient/6000_history/3", idSubstitutions.getForSource("urn:uuid:6000").getValue()); + } + + @Test + void testUpdateTargets_inputMatchesAllTargets_allTargetsUpdated() { + idSubstitutions.put(new IdType("urn:uuid:1234"), new IdType("Patient/123/_history/1")); + idSubstitutions.put(new IdType("urn:uuid:5000"), new IdType("Patient/123/_history/2")); + idSubstitutions.put(new IdType("urn:uuid:6000"), new IdType("Patient/123/_history/4")); + + idSubstitutions.updateTargets(new IdType("Patient/123/_history/3")); + + assertEquals("Patient/123/_history/3", idSubstitutions.getForSource("urn:uuid:1234").getValue()); + assertEquals("Patient/123/_history/3", idSubstitutions.getForSource("urn:uuid:5000").getValue()); + assertEquals("Patient/123/_history/3", idSubstitutions.getForSource("urn:uuid:6000").getValue()); + } + + @ParameterizedTest + @ValueSource(strings = {"Patient/124", "Patient/124/_history/3", "Patient", ""}) + void testUpdateTargets_noMatchingTarget_noUpdate(String theInputId) { + idSubstitutions.put(new IdType("urn:uuid:1234"), new IdType("Patient/123/_history/3")); + idSubstitutions.updateTargets(new IdType(theInputId)); + assertEquals("Patient/123/_history/3", idSubstitutions.getForSource("urn:uuid:1234").getValue()); + } + + @Test + void testUpdateTargets_nullInputId_noExceptionAndNoUpdate() { + idSubstitutions.put(new IdType("urn:uuid:1234"), new IdType("Patient/123/_history/3")); + idSubstitutions.updateTargets(null); + assertEquals("Patient/123/_history/3", idSubstitutions.getForSource("urn:uuid:1234").getValue()); + } +}