Contained bug (#6402)

* Contained bug * more tests * changelog, tests, implementation * code review * backwards logic
hapifhir · Oct 26, 2024 · e78a0b8 · e78a0b8
1 parent 60ed1fd
commit e78a0b8
Show file tree

Hide file tree

Showing 11 changed files with 209 additions and 38 deletions.
diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseParser.java
@@ -105,7 +105,6 @@ public abstract class BaseParser implements IParser {
 	private static final Set<String> notEncodeForContainedResource =
 			new HashSet<>(Arrays.asList("security", "versionId", "lastUpdated"));
 
-	private FhirTerser.ContainedResources myContainedResources;
 	private boolean myEncodeElementsAppliesToChildResourcesOnly;
 	private final FhirContext myContext;
 	private Collection<String> myDontEncodeElements;
@@ -183,12 +182,15 @@ protected Iterable<CompositeChildElement> compositeChildIterator(
 	}
 
 	private String determineReferenceText(
-			IBaseReference theRef, CompositeChildElement theCompositeChildElement, IBaseResource theResource) {
+			IBaseReference theRef,
+			CompositeChildElement theCompositeChildElement,
+			IBaseResource theResource,
+			EncodeContext theContext) {
 		IIdType ref = theRef.getReferenceElement();
 		if (isBlank(ref.getIdPart())) {
 			String reference = ref.getValue();
 			if (theRef.getResource() != null) {
-				IIdType containedId = getContainedResources().getResourceId(theRef.getResource());
+				IIdType containedId = theContext.getContainedResources().getResourceId(theRef.getResource());
 				if (containedId != null && !containedId.isEmpty()) {
 					if (containedId.isLocal()) {
 						reference = containedId.getValue();
@@ -262,7 +264,8 @@ public String encodeResourceToString(IBaseResource theResource) throws DataForma
 	@Override
 	public final void encodeResourceToWriter(IBaseResource theResource, Writer theWriter)
 			throws IOException, DataFormatException {
-		EncodeContext encodeContext = new EncodeContext(this, myContext.getParserOptions());
+		EncodeContext encodeContext =
+				new EncodeContext(this, myContext.getParserOptions(), new FhirTerser.ContainedResources());
 		encodeResourceToWriter(theResource, theWriter, encodeContext);
 	}
 
@@ -285,7 +288,8 @@ public void encodeToWriter(IBase theElement, Writer theWriter) throws DataFormat
 		} else if (theElement instanceof IPrimitiveType) {
 			theWriter.write(((IPrimitiveType<?>) theElement).getValueAsString());
 		} else {
-			EncodeContext encodeContext = new EncodeContext(this, myContext.getParserOptions());
+			EncodeContext encodeContext =
+					new EncodeContext(this, myContext.getParserOptions(), new FhirTerser.ContainedResources());
 			encodeToWriter(theElement, theWriter, encodeContext);
 		}
 	}
@@ -404,10 +408,6 @@ protected String getCompositeElementId(IBase theElement) {
 		return elementId;
 	}
 
-	FhirTerser.ContainedResources getContainedResources() {
-		return myContainedResources;
-	}
-
 	@Override
 	public Set<String> getDontStripVersionsFromReferencesAtPaths() {
 		return myDontStripVersionsFromReferencesAtPaths;
@@ -539,10 +539,11 @@ public boolean getSuppressNarratives() {
 		return mySuppressNarratives;
 	}
 
-	protected boolean isChildContained(BaseRuntimeElementDefinition<?> childDef, boolean theIncludedResource) {
+	protected boolean isChildContained(
+			BaseRuntimeElementDefinition<?> childDef, boolean theIncludedResource, EncodeContext theContext) {
 		return (childDef.getChildType() == ChildTypeEnum.CONTAINED_RESOURCES
 						|| childDef.getChildType() == ChildTypeEnum.CONTAINED_RESOURCE_LIST)
-				&& getContainedResources().isEmpty() == false
+				&& theContext.getContainedResources().isEmpty() == false
 				&& theIncludedResource == false;
 	}
 
@@ -788,7 +789,8 @@ protected List<? extends IBase> preProcessValues(
 			 */
 			if (next instanceof IBaseReference) {
 				IBaseReference nextRef = (IBaseReference) next;
-				String refText = determineReferenceText(nextRef, theCompositeChildElement, theResource);
+				String refText =
+						determineReferenceText(nextRef, theCompositeChildElement, theResource, theEncodeContext);
 				if (!StringUtils.equals(refText, nextRef.getReferenceElement().getValue())) {
 
 					if (retVal == theValues) {
@@ -980,7 +982,7 @@ protected boolean shouldEncodeResource(String theName, EncodeContext theEncodeCo
 		return true;
 	}
 
-	protected void containResourcesInReferences(IBaseResource theResource) {
+	protected void containResourcesInReferences(IBaseResource theResource, EncodeContext theContext) {
 
 		/*
 		 * If a UUID is present in Bundle.entry.fullUrl but no value is present
@@ -1003,7 +1005,7 @@ protected void containResourcesInReferences(IBaseResource theResource) {
 			}
 		}
 
-		myContainedResources = getContext().newTerser().containResources(theResource);
+		theContext.setContainedResources(getContext().newTerser().containResources(theResource));
 	}
 
 	static class ChildNameAndDef {
@@ -1034,8 +1036,12 @@ class EncodeContext extends EncodeContextPath {
 		private final List<EncodeContextPath> myEncodeElementPaths;
 		private final Set<String> myEncodeElementsAppliesToResourceTypes;
 		private final List<EncodeContextPath> myDontEncodeElementPaths;
+		private FhirTerser.ContainedResources myContainedResources;
 
-		public EncodeContext(BaseParser theParser, ParserOptions theParserOptions) {
+		public EncodeContext(
+				BaseParser theParser,
+				ParserOptions theParserOptions,
+				FhirTerser.ContainedResources theContainedResources) {
 			Collection<String> encodeElements = theParser.myEncodeElements;
 			Collection<String> dontEncodeElements = theParser.myDontEncodeElements;
 			if (isSummaryMode()) {
@@ -1058,13 +1064,23 @@ public EncodeContext(BaseParser theParser, ParserOptions theParserOptions) {
 						dontEncodeElements.stream().map(EncodeContextPath::new).collect(Collectors.toList());
 			}
 
+			myContainedResources = theContainedResources;
+
 			myEncodeElementsAppliesToResourceTypes =
 					ParserUtil.determineApplicableResourceTypesForTerserPaths(myEncodeElementPaths);
 		}
 
 		private Map<Key, List<BaseParser.CompositeChildElement>> getCompositeChildrenCache() {
 			return myCompositeChildrenCache;
 		}
+
+		public FhirTerser.ContainedResources getContainedResources() {
+			return myContainedResources;
+		}
+
+		public void setContainedResources(FhirTerser.ContainedResources theContainedResources) {
+			myContainedResources = theContainedResources;
+		}
 	}
 
 	protected class CompositeChildElement {

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java
@@ -54,6 +54,7 @@
 import ca.uhn.fhir.parser.json.jackson.JacksonStructure;
 import ca.uhn.fhir.rest.api.EncodingEnum;
 import ca.uhn.fhir.util.ElementUtil;
+import ca.uhn.fhir.util.FhirTerser;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.apache.commons.text.WordUtils;
@@ -386,12 +387,14 @@ public String toString() {
 			}
 			case CONTAINED_RESOURCE_LIST:
 			case CONTAINED_RESOURCES: {
-				List<IBaseResource> containedResources = getContainedResources().getContainedResources();
+				List<IBaseResource> containedResources =
+						theEncodeContext.getContainedResources().getContainedResources();
 				if (containedResources.size() > 0) {
 					beginArray(theEventWriter, theChildName);
 
 					for (IBaseResource next : containedResources) {
-						IIdType resourceId = getContainedResources().getResourceId(next);
+						IIdType resourceId =
+								theEncodeContext.getContainedResources().getResourceId(next);
 						String value = resourceId.getValue();
 						encodeResourceToJsonStreamWriter(
 								theResDef,
@@ -554,7 +557,8 @@ private void encodeCompositeElementChildrenToStreamWriter(
 
 				if (nextValue == null || nextValue.isEmpty()) {
 					if (nextValue instanceof BaseContainedDt) {
-						if (theContainedResource || getContainedResources().isEmpty()) {
+						if (theContainedResource
+								|| theEncodeContext.getContainedResources().isEmpty()) {
 							continue;
 						}
 					} else {
@@ -838,7 +842,8 @@ public void encodeResourceToJsonLikeWriter(IBaseResource theResource, BaseJsonLi
 					+ theResource.getStructureFhirVersionEnum());
 		}
 
-		EncodeContext encodeContext = new EncodeContext(this, getContext().getParserOptions());
+		EncodeContext encodeContext =
+				new EncodeContext(this, getContext().getParserOptions(), new FhirTerser.ContainedResources());
 		String resourceName = getContext().getResourceType(theResource);
 		encodeContext.pushPath(resourceName, true);
 		doEncodeResourceToJsonLikeWriter(theResource, theJsonLikeWriter, encodeContext);
@@ -894,7 +899,7 @@ private void encodeResourceToJsonStreamWriter(
 		}
 
 		if (!theContainedResource) {
-			containResourcesInReferences(theResource);
+			containResourcesInReferences(theResource, theEncodeContext);
 		}
 
 		RuntimeResourceDefinition resDef = getContext().getResourceDefinition(theResource);

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/RDFParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/RDFParser.java
@@ -191,7 +191,7 @@ private Resource encodeResourceToRDFStreamWriter(
 		}
 
 		if (!containedResource) {
-			containResourcesInReferences(resource);
+			containResourcesInReferences(resource, encodeContext);
 		}
 
 		if (!(resource instanceof IAnyResource)) {
@@ -354,7 +354,7 @@ private Model encodeChildElementToStreamWriter(
 		try {
 
 			if (element == null || element.isEmpty()) {
-				if (!isChildContained(childDef, includedResource)) {
+				if (!isChildContained(childDef, includedResource, theEncodeContext)) {
 					return rdfModel;
 				}
 			}

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/XmlParser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/XmlParser.java
@@ -295,7 +295,7 @@ private void encodeChildElementToStreamWriter(
 		try {
 
 			if (theElement == null || theElement.isEmpty()) {
-				if (isChildContained(childDef, theIncludedResource)) {
+				if (isChildContained(childDef, theIncludedResource, theEncodeContext)) {
 					// We still want to go in..
 				} else {
 					return;
@@ -359,8 +359,10 @@ private void encodeChildElementToStreamWriter(
 					 * theEventWriter.writeStartElement("contained"); encodeResourceToXmlStreamWriter(next, theEventWriter, true, fixContainedResourceId(next.getId().getValue()));
 					 * theEventWriter.writeEndElement(); }
 					 */
-					for (IBaseResource next : getContainedResources().getContainedResources()) {
-						IIdType resourceId = getContainedResources().getResourceId(next);
+					for (IBaseResource next :
+							theEncodeContext.getContainedResources().getContainedResources()) {
+						IIdType resourceId =
+								theEncodeContext.getContainedResources().getResourceId(next);
 						theEventWriter.writeStartElement("contained");
 						String value = resourceId.getValue();
 						encodeResourceToXmlStreamWriter(
@@ -682,7 +684,7 @@ private void encodeResourceToXmlStreamWriter(
 		}
 
 		if (!theContainedResource) {
-			containResourcesInReferences(theResource);
+			containResourcesInReferences(theResource, theEncodeContext);
 		}
 
 		theEventWriter.writeStartElement(resDef.getName());

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/FhirTerser.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/util/FhirTerser.java
@@ -61,6 +61,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
@@ -83,10 +84,23 @@ public class FhirTerser {
 
 	private static final Pattern COMPARTMENT_MATCHER_PATH =
 			Pattern.compile("([a-zA-Z.]+)\\.where\\(resolve\\(\\) is ([a-zA-Z]+)\\)");
+
 	private static final String USER_DATA_KEY_CONTAIN_RESOURCES_COMPLETED =
 			FhirTerser.class.getName() + "_CONTAIN_RESOURCES_COMPLETED";
+
 	private final FhirContext myContext;
 
+	/**
+	 * This comparator sorts IBaseReferences, and places any that are missing an ID at the end. Those with an ID go to the front.
+	 */
+	private static final Comparator<IBaseReference> REFERENCES_WITH_IDS_FIRST =
+			Comparator.nullsLast(Comparator.comparing(ref -> {
+				if (ref.getResource() == null) return true;
+				if (ref.getResource().getIdElement() == null) return true;
+				if (ref.getResource().getIdElement().getValue() == null) return true;
+				return false;
+			}));
+
 	public FhirTerser(FhirContext theContext) {
 		super();
 		myContext = theContext;
@@ -1418,6 +1432,13 @@ public boolean acceptUndeclaredExtension(
 	private void containResourcesForEncoding(
 			ContainedResources theContained, IBaseResource theResource, boolean theModifyResource) {
 		List<IBaseReference> allReferences = getAllPopulatedChildElementsOfType(theResource, IBaseReference.class);
+
+		// Note that we process all contained resources that have arrived here with an ID contained resources first, so
+		// that we don't accidentally auto-assign an ID
+		// which may collide with a resource we have yet to process.
+		// See: https://github.com/hapifhir/hapi-fhir/issues/6403
+		allReferences.sort(REFERENCES_WITH_IDS_FIRST);
+
 		for (IBaseReference next : allReferences) {
 			IBaseResource resource = next.getResource();
 			if (resource == null && next.getReferenceElement().isLocal()) {
@@ -1437,11 +1458,11 @@ private void containResourcesForEncoding(
 			IBaseResource resource = next.getResource();
 			if (resource != null) {
 				if (resource.getIdElement().isEmpty() || resource.getIdElement().isLocal()) {
-					if (theContained.getResourceId(resource) != null) {
-						// Prevent infinite recursion if there are circular loops in the contained resources
+
+					IIdType id = theContained.addContained(resource);
+					if (id == null) {
 						continue;
 					}
-					IIdType id = theContained.addContained(resource);
 					if (theModifyResource) {
 						getContainedResourceList(theResource).add(resource);
 						next.setReference(id.getValue());
@@ -1769,7 +1790,6 @@ private interface ICompartmentOwnerVisitor {
 
 	public static class ContainedResources {
 		private long myNextContainedId = 1;
-
 		private List<IBaseResource> myResourceList;
 		private IdentityHashMap<IBaseResource, IIdType> myResourceToIdMap;
 		private Map<String, IBaseResource> myExistingIdToContainedResourceMap;
@@ -1782,6 +1802,11 @@ public Map<String, IBaseResource> getExistingIdToContainedResource() {
 		}
 
 		public IIdType addContained(IBaseResource theResource) {
+			if (this.getResourceId(theResource) != null) {
+				// Prevent infinite recursion if there are circular loops in the contained resources
+				return null;
+			}
+
 			IIdType existing = getResourceToIdMap().get(theResource);
 			if (existing != null) {
 				return existing;
@@ -1796,7 +1821,10 @@ public IIdType addContained(IBaseResource theResource) {
 				if (substring(idPart, 0, 1).equals("#")) {
 					idPart = idPart.substring(1);
 					if (StringUtils.isNumeric(idPart)) {
-						myNextContainedId = Long.parseLong(idPart) + 1;
+						// If there is a user-supplied numeric contained ID, our auto-assigned IDs should exceed the
+						// largest
+						// client-assigned contained ID.
+						myNextContainedId = Math.max(myNextContainedId, Long.parseLong(idPart) + 1);
 					}
 				}
 			}
@@ -1864,6 +1892,7 @@ public boolean hasExistingIdToContainedResource() {
 		}
 
 		public void assignIdsToContainedResources() {
+			// TODO JA: Dead code?
 
 			if (!getContainedResources().isEmpty()) {
 

diff --git a/...-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6403-json-parser-bugs.yaml b/...-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6403-json-parser-bugs.yaml
@@ -0,0 +1,6 @@
+---
+type: fix
+jira: SMILE-8969
+title: "Fixed a rare bug in the JSON Parser, wherein client-assigned contained resource IDs could collide with server-assigned contained IDs. For example if a 
+resource had a client-assigned contained ID of `#2`, and a contained resource with no ID, then depending on the processing order, the parser could occasionally 
+provide duplicate contained resource IDs, leading to non-deterministic behaviour."
diff --git a/...ir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/ColumnTupleObject.java b/...ir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/ColumnTupleObject.java
@@ -1,3 +1,22 @@
+/*-
+ * #%L
+ * HAPI FHIR JPA Server
+ * %%
+ * Copyright (C) 2014 - 2024 Smile CDR, Inc.
+ * %%
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * #L%
+ */
 package ca.uhn.fhir.jpa.search.builder.sql;
 
 import com.healthmarketscience.common.util.AppendableExt;

diff --git a/...paserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java b/...paserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java
@@ -1,6 +1,7 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
 import ca.uhn.fhir.i18n.Msg;
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
@@ -23,6 +24,8 @@
 import org.hl7.fhir.r4.model.ServiceRequest;
 import org.hl7.fhir.r4.model.ServiceRequest.ServiceRequestIntent;
 import org.hl7.fhir.r4.model.ServiceRequest.ServiceRequestStatus;
+import org.hl7.fhir.r4.model.Specimen;
+import org.hl7.fhir.r4.model.StringType;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;

diff --git a/...r-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/...r-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java
@@ -263,6 +263,8 @@ public void before() throws Exception {
 		myStorageSettings.setSearchPreFetchThresholds(new JpaStorageSettings().getSearchPreFetchThresholds());
 	}
 
+
+
 	@Test
 	public void testParameterWithNoValueThrowsError_InvalidChainOnCustomSearch() throws IOException {
 		SearchParameter searchParameter = new SearchParameter();