5771 patch operation failing for complex extension (#5776)

* initial failing test

* tightening initial test.

* fix with changelog

* addressing comment from code review.

* clean up from code analysis recommendations

---------

Co-authored-by: peartree <etienne.poirier@smilecdr.com>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5711-fix-patch-operation-failing-for-complex-extension.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 5771
+jira: SMILE-7837
+title: "Previously, a Patch operation would fail when adding a complex extension, i.e. an extension
+comprised of another extension.  This issue has been fixed."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/patch/FhirPatchApplyR4Test.java

@@ -12,6 +12,7 @@ import org.hl7.fhir.r4.model.BooleanType;
 import org.hl7.fhir.r4.model.CodeType;
 import org.hl7.fhir.r4.model.CodeableConcept;
 import org.hl7.fhir.r4.model.Coding;
+import org.hl7.fhir.r4.model.DateTimeType;
 import org.hl7.fhir.r4.model.Extension;
 import org.hl7.fhir.r4.model.HumanName;
 import org.hl7.fhir.r4.model.Identifier;
@@ -37,6 +38,7 @@ import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -531,6 +533,59 @@ public class FhirPatchApplyR4Test {
 		assertThat(patient.getExtension().get(0).getValueAsPrimitive().getValueAsString(), is(equalTo("foo")));
 	}
 
+	@Test
+	public void testAddExtensionWithExtension() {
+		final String extensionUrl  = "http://foo/fhir/extension/foo";
+		final String innerExtensionUrl  = "http://foo/fhir/extension/innerExtension";
+		final String innerExtensionValue = "2021-07-24T13:23:30-04:00";
+
+		FhirPatch svc = new FhirPatch(ourCtx);
+		Patient patient = new Patient();
+
+		Parameters patch = new Parameters();
+		Parameters.ParametersParameterComponent addOperation = createPatchAddOperation("Patient", "extension", null);
+		addOperation
+			.addPart()
+			.setName("value")
+			.addPart(
+				new Parameters.ParametersParameterComponent()
+					.setName("url")
+					.setValue(new UriType(extensionUrl))
+			)
+			.addPart(
+				new Parameters.ParametersParameterComponent()
+					.setName("extension")
+					.addPart(
+						new Parameters.ParametersParameterComponent()
+							.setName("url")
+							.setValue(new UriType(innerExtensionUrl))
+					)
+					.addPart(
+						new Parameters.ParametersParameterComponent()
+							.setName("value")
+							.setValue(new DateTimeType(innerExtensionValue))
+					)
+			);
+
+		patch.addParameter(addOperation);
+
+		ourLog.info("Patch:\n{}", ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(patch));
+
+		svc.apply(patient, patch);
+		ourLog.debug("Outcome:\n{}", ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(patient));
+
+		//Then: it adds the new extension correctly.
+		assertThat(patient.getExtension(), hasSize(1));
+		Extension extension = patient.getExtension().get(0);
+		assertThat(extension.getUrl(), is(equalTo(extensionUrl)));
+		Extension innerExtension = extension.getExtensionFirstRep();
+
+		assertThat(innerExtension, notNullValue());
+		assertThat(innerExtension.getUrl(), is(equalTo(innerExtensionUrl)));
+		assertThat(innerExtension.getValue().primitiveValue(), is(equalTo(innerExtensionValue)));
+
+	}
+
 	private Parameters.ParametersParameterComponent createPatchAddOperation(String thePath, String theName, Type theValue) {
 		return createPatchOperation("add", thePath, theName, theValue, null);
 	}

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/patch/FhirPatch.java

@@ -30,10 +30,10 @@ import ca.uhn.fhir.util.IModelVisitor2;
 import ca.uhn.fhir.util.ParametersUtil;
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.hl7.fhir.instance.model.api.IBase;
 import org.hl7.fhir.instance.model.api.IBaseEnumeration;
-import org.hl7.fhir.instance.model.api.IBaseExtension;
 import org.hl7.fhir.instance.model.api.IBaseParameters;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
@@ -49,7 +49,6 @@ import java.util.Optional;
 import java.util.Set;
 
 import static org.apache.commons.lang3.StringUtils.defaultString;
-import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class FhirPatch {
 
@@ -319,12 +318,9 @@ public class FhirPatch {
 		if (valuePartValue.isPresent()) {
 			newValue = valuePartValue.get();
 		} else {
-			newValue = theChildDefinition.getChildElement().newInstance();
+			List<IBase> partParts = valuePart.map(this::extractPartsFromPart).orElse(Collections.emptyList());
 
-			if (valuePart.isPresent()) {
-				IBase theValueElement = valuePart.get();
-				populateNewValue(theChildDefinition, newValue, theValueElement);
-			}
+			newValue = createAndPopulateNewElement(theChildDefinition, partParts);
 		}
 
 		if (IBaseEnumeration.class.isAssignableFrom(
@@ -350,31 +346,65 @@ public class FhirPatch {
 		return newValue;
 	}
 
-	private void populateNewValue(ChildDefinition theChildDefinition, IBase theNewValue, IBase theValueElement) {
-		List<IBase> valuePartParts = myContext.newTerser().getValues(theValueElement, "part");
-		for (IBase nextValuePartPart : valuePartParts) {
+	@Nonnull
+	private List<IBase> extractPartsFromPart(IBase theParametersParameterComponent) {
+		return myContext.newTerser().getValues(theParametersParameterComponent, "part");
+	}
+
+	/**
+	 * this method will instantiate an element according to the provided Definition and it according to
+	 * the properties found in thePartParts.  a part usually represent a datatype as a name/value[X] pair.
+	 * it may also represent a complex type like an Extension.
+	 *
+	 * @param theDefinition wrapper around the runtime definition of the element to be populated
+	 * @param thePartParts list of Part to populate the element that will be created from theDefinition
+	 * @return an element that was created from theDefinition and populated with the parts
+	 */
+	private IBase createAndPopulateNewElement(ChildDefinition theDefinition, List<IBase> thePartParts) {
+		IBase newElement = theDefinition.getChildElement().newInstance();
+
+		for (IBase nextValuePartPart : thePartParts) {
 
 			String name = myContext
 					.newTerser()
 					.getSingleValue(nextValuePartPart, PARAMETER_NAME, IPrimitiveType.class)
 					.map(IPrimitiveType::getValueAsString)
 					.orElse(null);
-			if (isNotBlank(name)) {
 
-				Optional<IBase> value =
-						myContext.newTerser().getSingleValue(nextValuePartPart, "value[x]", IBase.class);
-				if (value.isPresent()) {
+			if (StringUtils.isBlank(name)) {
+				continue;
+			}
 
-					BaseRuntimeChildDefinition partChildDef =
-							theChildDefinition.getChildElement().getChildByName(name);
-					if (partChildDef == null) {
-						name = name + "[x]";
-						partChildDef = theChildDefinition.getChildElement().getChildByName(name);
-					}
-					partChildDef.getMutator().addValue(theNewValue, value.get());
+			Optional<IBase> value = myContext.newTerser().getSingleValue(nextValuePartPart, "value[x]", IBase.class);
+
+			if (value.isPresent()) {
+				// we have a dataType. let's extract its value and assign it.
+				BaseRuntimeChildDefinition partChildDef =
+						theDefinition.getChildElement().getChildByName(name);
+				if (partChildDef == null) {
+					name = name + "[x]";
+					partChildDef = theDefinition.getChildElement().getChildByName(name);
 				}
+				partChildDef.getMutator().addValue(newElement, value.get());
+
+				// a part represent a datatype or a complexType but not both at the same time.
+				continue;
+			}
+
+			List<IBase> part = extractPartsFromPart(nextValuePartPart);
+
+			if (!part.isEmpty()) {
+				// we have a complexType.  let's find its definition and recursively process
+				// them till all complexTypes are processed.
+				ChildDefinition childDefinition = findChildDefinition(newElement, name);
+
+				IBase childNewValue = createAndPopulateNewElement(childDefinition, part);
+
+				childDefinition.getChildDef().getMutator().setValue(newElement, childNewValue);
 			}
 		}
+
+		return newElement;
 	}
 
 	private void deleteSingleElement(IBase theElementToDelete) {
@@ -390,17 +420,6 @@ public class FhirPatch {
 				}
 				return true;
 			}
-
-			@Override
-			public boolean acceptUndeclaredExtension(
-					IBaseExtension<?, ?> theNextExt,
-					List<IBase> theContainingElementPath,
-					List<BaseRuntimeChildDefinition> theChildDefinitionPath,
-					List<BaseRuntimeElementDefinition<?>> theElementDefinitionPath) {
-				theNextExt.setUrl(null);
-				theNextExt.setValue(null);
-				return true;
-			}
 		});
 	}
 
@@ -565,7 +584,7 @@ public class FhirPatch {
 		 * If the value is a Resource or a datatype, we can put it into the part.value and that will cover
 		 * all of its children. If it's an infrastructure element though, such as Patient.contact we can't
 		 * just put it into part.value because it isn't an actual type. So we have to put all of its
-		 * childen in instead.
+		 * children in instead.
 		 */
 		if (valueDef.isStandardType()) {
 			ParametersUtil.addPart(myContext, operation, PARAMETER_VALUE, value);