Correctly handle encoding contained resources where some have user

assigned IDs and some do not

hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java

@@ -193,6 +193,19 @@ public abstract class BaseParser implements IParser {
 
 		{
 			List<IBaseReference> allElements = myContext.newTerser().getAllPopulatedChildElementsOfType(theResource, IBaseReference.class);
+			for (IBaseReference next : allElements) {
+				IBaseResource resource = next.getResource();
+				if (resource == null && next.getReferenceElement().isLocal()) {
+					if (existingIdToContainedResource != null) {
+						IBaseResource potentialTarget = existingIdToContainedResource.remove(next.getReferenceElement().getValue());
+						if (potentialTarget != null) {
+							theContained.addContained(next.getReferenceElement(), potentialTarget);
+							containResourcesForEncoding(theContained, potentialTarget, theTarget);
+						}
+					}
+				}
+			}
+
 			for (IBaseReference next : allElements) {
 				IBaseResource resource = next.getResource();
 				if (resource != null) {
@@ -210,15 +223,8 @@ public abstract class BaseParser implements IParser {
 					}
 
 					containResourcesForEncoding(theContained, resource, theTarget);
-				} else if (next.getReferenceElement().isLocal()) {
-					if (existingIdToContainedResource != null) {
-						IBaseResource potentialTarget = existingIdToContainedResource.remove(next.getReferenceElement().getValue());
-						if (potentialTarget != null) {
-							theContained.addContained(next.getReferenceElement(), potentialTarget);
-							containResourcesForEncoding(theContained, potentialTarget, theTarget);
-						}
-					}
 				}
+
 			}
 		}
 
@@ -1170,8 +1176,9 @@ public abstract class BaseParser implements IParser {
 	static class ContainedResources {
 		private long myNextContainedId = 1;
 
-		private List<IBaseResource> myResources = new ArrayList<IBaseResource>();
+		private List<IBaseResource> myResources = new ArrayList<>();
-		private IdentityHashMap<IBaseResource, IIdType> myResourceToId = new IdentityHashMap<IBaseResource, IIdType>();
+		private IdentityHashMap<IBaseResource, IIdType> myResourceToId = new IdentityHashMap<>();
+		private Set<String> myExistingLocalIds = new HashSet<>();
 
 		public void addContained(IBaseResource theResource) {
 			if (myResourceToId.containsKey(theResource)) {
@@ -1184,7 +1191,20 @@ public abstract class BaseParser implements IParser {
 			} else {
 				// TODO: make this configurable between the two below (and something else?)
 				// newId = new IdDt(UUID.randomUUID().toString());
-				newId = new IdDt(myNextContainedId++);
+
+				// Generally we won't even go into this loop once since
+				// our next ID shouldn't already exist, but there is
+				// always a chance that it does if someone is mixing
+				// manually assigned local IDs with HAPI assigned ones
+				// See JsonParser#testEncodeResourceWithMixedManualAndAutomaticContainedResourcesLocalFirst
+				// and JsonParser#testEncodeResourceWithMixedManualAndAutomaticContainedResourcesLocalLast
+				// for examples of where this is needed...
+				while (!myExistingLocalIds.add("#" + myNextContainedId)) {
+					myNextContainedId++;
+				}
+				newId = new IdDt(myNextContainedId);
+
+				myNextContainedId++;
 			}
 
 			myResourceToId.put(theResource, newId);
@@ -1192,7 +1212,7 @@ public abstract class BaseParser implements IParser {
 		}
 
 		public void addContained(IIdType theId, IBaseResource theResource) {
-			if (!hasId(theId)) {
+			if (myExistingLocalIds.add(theId.getValue())) {
 				myResourceToId.put(theResource, theId);
 				myResources.add(theResource);
 			}
@@ -1206,15 +1226,6 @@ public abstract class BaseParser implements IParser {
 			return myResourceToId.get(theNext);
 		}
 
-		public boolean hasId(IIdType theId) {
-			for (IIdType next : myResourceToId.values()) {
-				if (Objects.equals(next.getValue(), theId.getValue())) {
-					return true;
-				}
-			}
-			return false;
-		}
-
 		public boolean isEmpty() {
 			return myResourceToId.isEmpty();
 		}

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java

@@ -152,6 +152,36 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest {
 		return input;
 	}
 
+	@Test
+	public void testTransactionUpdateTwoResourcesWithSameId() {
+		Bundle request = new Bundle();
+
+		Patient p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue("DDD");
+		p.setId("Patient/ABC");
+		request.addEntry()
+			.setResource(p)
+			.getRequest()
+			.setMethod(HTTPVerb.PUT)
+			.setUrl("Patient/ABC");
+
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue("DDD");
+		p.setId("Patient/ABC");
+		request.addEntry()
+			.setResource(p)
+			.getRequest()
+			.setMethod(HTTPVerb.PUT)
+			.setUrl("Patient/ABC");
+
+		try {
+			mySystemDao.transaction(mySrd, request);
+			fail();
+		} catch (InvalidRequestException e) {
+			assertThat(e.getMessage(), containsString("Transaction bundle contains multiple resources with ID: Patient/ABC"));
+		}
+	}
+
 	@SuppressWarnings("unchecked")
 	private <T extends org.hl7.fhir.dstu3.model.Resource> T find(Bundle theBundle, Class<T> theType, int theIndex) {
 		int count = 0;

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java

@@ -15,7 +15,6 @@ import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.server.exceptions.*;
 import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails;
 import ca.uhn.fhir.util.TestUtil;
-import com.google.common.base.Charsets;
 import org.apache.commons.io.IOUtils;
 import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IIdType;
@@ -696,6 +695,36 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
 
 	}
 
+	@Test
+	public void testTransactionUpdateTwoResourcesWithSameId() {
+		Bundle request = new Bundle();
+
+		Patient p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue("DDD");
+		p.setId("Patient/ABC");
+		request.addEntry()
+			.setResource(p)
+			.getRequest()
+			.setMethod(HTTPVerb.PUT)
+			.setUrl("Patient/ABC");
+
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue("DDD");
+		p.setId("Patient/ABC");
+		request.addEntry()
+			.setResource(p)
+			.getRequest()
+			.setMethod(HTTPVerb.PUT)
+			.setUrl("Patient/ABC");
+
+		try {
+			mySystemDao.transaction(mySrd, request);
+			fail();
+		} catch (InvalidRequestException e) {
+			assertThat(e.getMessage(), containsString("Transaction bundle contains multiple resources with ID: Patient/ABC"));
+		}
+	}
+
 	@Test
 	public void testTransactionCreateInlineMatchUrlWithOneMatch2() {
 		String methodName = "testTransactionCreateInlineMatchUrlWithOneMatch2";

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/parser/JsonParserR4Test.java

@@ -36,6 +36,64 @@ public class JsonParserR4Test {
 		return b;
 	}
 
+	@Test
+	public void testEncodeResourceWithMixedManualAndAutomaticContainedResourcesLocalFirst() {
+
+		Observation obs = new Observation();
+
+		Patient pt = new Patient();
+		pt.setId("#1");
+		pt.addName().setFamily("FAM");
+		obs.getSubject().setReference("#1");
+		obs.getContained().add(pt);
+
+		Encounter enc = new Encounter();
+		enc.setStatus(Encounter.EncounterStatus.ARRIVED);
+		obs.getContext().setResource(enc);
+
+		String encoded = ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs);
+		ourLog.info(encoded);
+
+		obs = ourCtx.newJsonParser().parseResource(Observation.class, encoded);
+		assertEquals("#1", obs.getContained().get(0).getId());
+		assertEquals("#2", obs.getContained().get(1).getId());
+
+		pt = (Patient) obs.getSubject().getResource();
+		assertEquals("FAM", pt.getNameFirstRep().getFamily());
+
+		enc = (Encounter) obs.getContext().getResource();
+		assertEquals(Encounter.EncounterStatus.ARRIVED, enc.getStatus());
+	}
+
+	@Test
+	public void testEncodeResourceWithMixedManualAndAutomaticContainedResourcesLocalLast() {
+
+		Observation obs = new Observation();
+
+		Patient pt = new Patient();
+		pt.addName().setFamily("FAM");
+		obs.getSubject().setResource(pt);
+
+		Encounter enc = new Encounter();
+		enc.setId("#1");
+		enc.setStatus(Encounter.EncounterStatus.ARRIVED);
+		obs.getContext().setReference("#1");
+		obs.getContained().add(enc);
+
+		String encoded = ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs);
+		ourLog.info(encoded);
+
+		obs = ourCtx.newJsonParser().parseResource(Observation.class, encoded);
+		assertEquals("#1", obs.getContained().get(0).getId());
+		assertEquals("#2", obs.getContained().get(1).getId());
+
+		pt = (Patient) obs.getSubject().getResource();
+		assertEquals("FAM", pt.getNameFirstRep().getFamily());
+
+		enc = (Encounter) obs.getContext().getResource();
+		assertEquals(Encounter.EncounterStatus.ARRIVED, enc.getStatus());
+	}
+
 	/**
 	 * See #814
 	 */
@@ -153,7 +211,7 @@ public class JsonParserR4Test {
 		b = parser.parseResource(Bundle.class, encoded);
 
 		assertEquals("BUNDLEID", b.getIdElement().getIdPart());
-		assertEquals("Patient/PATIENTID", ((Patient) b.getEntry().get(0).getResource()).getId());
+		assertEquals("Patient/PATIENTID", b.getEntry().get(0).getResource().getId());
 		assertEquals("GIVEN", ((Patient) b.getEntry().get(0).getResource()).getNameFirstRep().getGivenAsSingleString());
 	}
 
@@ -179,7 +237,7 @@ public class JsonParserR4Test {
 		b = parser.parseResource(Bundle.class, encoded);
 
 		assertNotEquals("BUNDLEID", b.getIdElement().getIdPart());
-		assertEquals("Patient/PATIENTID", ((Patient) b.getEntry().get(0).getResource()).getId());
+		assertEquals("Patient/PATIENTID", b.getEntry().get(0).getResource().getId());
 		assertEquals("GIVEN", ((Patient) b.getEntry().get(0).getResource()).getNameFirstRep().getGivenAsSingleString());
 	}
 
@@ -205,7 +263,7 @@ public class JsonParserR4Test {
 		b = parser.parseResource(Bundle.class, encoded);
 
 		assertNotEquals("BUNDLEID", b.getIdElement().getIdPart());
-		assertNotEquals("Patient/PATIENTID", ((Patient) b.getEntry().get(0).getResource()).getId());
+		assertNotEquals("Patient/PATIENTID", b.getEntry().get(0).getResource().getId());
 		assertEquals("GIVEN", ((Patient) b.getEntry().get(0).getResource()).getNameFirstRep().getGivenAsSingleString());
 	}
 

src/changes/changes.xml

@@ -45,6 +45,14 @@
 				This work was sponsored by the Regenstrief Institute. Thanks
 				to Regenstrief for their support!
 			</action>
+			<action type="fix">
+				When encoding a resource that had contained resources with user-supplied
+				local IDs (e.g. resource.setId("#1")) as well as contained resources
+				with no IDs (meaning HAPI should automatically assign a local ID
+				for these resources) it was possible for HAPI to generate
+				a local ID that already existed, making the resulting 
+				serialization invalid. This has been corrected.
+			</action>
 		</release>
 		<release version="3.3.0" date="2018-03-29">
 			<action type="add">