Incorrect version of auto versioned reference for conditional update with urn id placeholder (#5625)

* Incorrect version from versioned_references.auto_at_paths for conditional update - implementation

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."

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());
 
 	}

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());
+			Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(),
-			ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
+				List.of("200 OK", "200 OK", "201 Created"), List.of("2", "1", "1"));
-			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());
 			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());
+			Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(),
-			ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
+				List.of("200 OK", "201 Created"), List.of("3", "1"));
-			assertEquals("200 OK", outcome.getEntry().get(0).getResponse().getStatus());
-			assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus());
 			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());
+			Bundle outcome = createAndValidateBundle((Bundle) builder.getBundle(),
-			ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
+				List.of("200 OK", "201 Created"), List.of("3", "1"));
-			assertEquals("200 OK", outcome.getEntry().get(0).getResponse().getStatus());
-			assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus());
 			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 = createAndValidateBundle((Bundle) builder.getBundle(),
-			Bundle outcome = mySystemDao.transaction(mySrd, (Bundle) builder.getBundle());
+				List.of("201 Created"), List.of("1"));
-			ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
-			assertEquals("201 Created", outcome.getEntry().get(0).getResponse().getStatus());
-
 			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<Bundle> 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<String> theOutcomeStatuses,
+											   List<String> 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);

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);
+			theIdSubstitutions.updateTargets(newId);
-			if (target != null) {
-				target.setValue(newId.getValue());
-			}
 
 			if (theDaoMethodOutcome.getOperationOutcome() != null) {
 				IBase responseEntry = entriesToProcess.getResponseBundleEntryWithVersionlessComparison(newId);

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;

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