The Lenient error handler will now throw exceptions when encode invalid extension containing value and nested extensions (#4747)

* The Lenient error handler will throw exceptions when encode invalid extension containing value and nested extensions

* Addressing suggestions

* Minor changes

* Minor fix

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

@@ -26,7 +26,7 @@ import ca.uhn.fhir.parser.json.BaseJsonLikeValue.ValueType;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 
 /**
- * The default error handler, which logs issues but does not abort parsing, with only one exception:
+ * The default error handler, which logs issues but does not abort parsing, with only two exceptions:
  * <p>
  * The {@link #invalidValue(ca.uhn.fhir.parser.IParserErrorHandler.IParseLocation, String, String)}
  * method will throw a {@link DataFormatException} by default since ignoring this type of error
@@ -36,12 +36,20 @@ import static org.apache.commons.lang3.StringUtils.isBlank;
  * 
  * @see IParser#setParserErrorHandler(IParserErrorHandler)
  * @see FhirContext#setParserErrorHandler(IParserErrorHandler)
+ *
+ * <p>
+ * The {@link #extensionContainsValueAndNestedExtensions(ca.uhn.fhir.parser.IParserErrorHandler.IParseLocation)}
+ * method will throw a {@link DataFormatException} by default since ignoring this type of error will allow malformed
+ * resouces to be created and result in errors when attempts to read, update or delete the resource in the future.
+ *  See {@link #setErrorOnInvalidExtension(boolean)} for information on this.
+ * </p>
  */
 public class LenientErrorHandler extends ParseErrorHandler implements IParserErrorHandler {
 
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(LenientErrorHandler.class);
 	private static final StrictErrorHandler STRICT_ERROR_HANDLER = new StrictErrorHandler();
 	private boolean myErrorOnInvalidValue = true;
+	private boolean myErrorOnInvalidExtension = true;
 	private boolean myLogErrors;
 
 	/**
@@ -92,7 +100,7 @@ public class LenientErrorHandler extends ParseErrorHandler implements IParserErr
 
 	/**
 	 * If set to <code>false</code> (default is <code>true</code>) invalid values will be logged. By
-	 * default invalid attribute values cause this error handler to throw a {@link DataFormatException} (unlike
+	 * default, invalid attribute values cause this error handler to throw a {@link DataFormatException} (unlike
 	 * other methods in this class which default to simply logging errors).
 	 * <p>
 	 * Note that empty values (e.g. <code>""</code>) will not lead to an error when this is set to
@@ -105,6 +113,17 @@ public class LenientErrorHandler extends ParseErrorHandler implements IParserErr
 		return myErrorOnInvalidValue;
 	}
 
+	/**
+	 * If set to <code>false</code> (default is <code>true</code>) invalid extensions will be logged. By
+	 * default, invalid resource extensions cause this error handler to throw a {@link DataFormatException} (unlike
+	 * other methods in this class which default to simply logging errors).
+	 *
+	 * @see #setErrorOnInvalidExtension(boolean)
+	 */
+	public boolean isErrorOnInvalidExtension() {
+		return myErrorOnInvalidExtension;
+	}
+
 	@Override
 	public void missingRequiredElement(IParseLocation theLocation, String theElementName) {
 		if (myLogErrors) {
@@ -114,7 +133,7 @@ public class LenientErrorHandler extends ParseErrorHandler implements IParserErr
 
 	/**
 	 * If set to <code>false</code> (default is <code>true</code>) invalid values will be logged. By
-	 * default invalid attribute values cause this error handler to throw a {@link DataFormatException} (unlike
+	 * default, invalid attribute values cause this error handler to throw a {@link DataFormatException} (unlike
 	 * other methods in this class which default to simply logging errors).
 	 * <p>
 	 * Note that empty values (e.g. <code>""</code>) will not lead to an error when this is set to
@@ -129,6 +148,28 @@ public class LenientErrorHandler extends ParseErrorHandler implements IParserErr
 		return this;
 	}
 
+	/**
+	 * If set to <code>false</code> (default is <code>true</code>) invalid extensions will be logged. By
+	 * default, invalid resource extensions cause this error handler to throw a {@link DataFormatException} (unlike
+	 * other methods in this class which default to simply logging errors).
+	 *
+	 * @return Returns a reference to <code>this</code> for easy method chaining
+	 * @see #isErrorOnInvalidExtension()
+	 */
+	public LenientErrorHandler setErrorOnInvalidExtension(boolean theErrorOnInvalidExtension) {
+		myErrorOnInvalidExtension = theErrorOnInvalidExtension;
+		return this;
+	}
+
+	/**
+	 * If this method is called, both invalid resource extensions and invalid attribute values will set to simply logging errors.
+	 */
+	public LenientErrorHandler disableAllErrors() {
+		myErrorOnInvalidValue = false;
+		myErrorOnInvalidExtension = false;
+		return this;
+	}
+
 	@Override
 	public void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName) {
 		if (myLogErrors) {
@@ -158,8 +199,10 @@ public class LenientErrorHandler extends ParseErrorHandler implements IParserErr
 	}
 
 	@Override
-	public void extensionContainsValueAndNestedExtensions(IParseLocation theLocation) {
+	public void extensionContainsValueAndNestedExtensions(IParseLocation theLocation){
-		if (myLogErrors) {
+		if (myErrorOnInvalidExtension) {
+			STRICT_ERROR_HANDLER.extensionContainsValueAndNestedExtensions(theLocation);
+		} else if (myLogErrors) {
 			ourLog.warn("{}Extension contains both a value and nested extensions", describeLocation(theLocation));
 		}
 	}

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4746-hl7v2-inbound-endpoint-should-reject-malformed-resources.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 4746
+jira: SMILE-4710
+title: "Previously, the Lenient error handler will accept invalid extension containing value and nested extensions by default.
+This will lead to errors when client further attempt to read, update or delete the resources. Now, this has been fixed and 
+a DataFormatException will be thrown instead."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java

@@ -76,7 +76,7 @@ import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class JpaStorageResourceParser implements IJpaStorageResourceParser {
-	public static final LenientErrorHandler LENIENT_ERROR_HANDLER = new LenientErrorHandler(false).setErrorOnInvalidValue(false);
+	public static final LenientErrorHandler LENIENT_ERROR_HANDLER = new LenientErrorHandler(false).disableAllErrors();
 	private static final Logger ourLog = LoggerFactory.getLogger(JpaStorageResourceParser.class);
 	@Autowired
 	private FhirContext myFhirContext;

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TolerantJsonParser.java

@@ -110,7 +110,7 @@ public class TolerantJsonParser extends JsonParser {
 	}
 
 	public static TolerantJsonParser createWithLenientErrorHandling(FhirContext theContext, @Nullable Long theResourcePid) {
-		LenientErrorHandler errorHandler = new LenientErrorHandler(false).setErrorOnInvalidValue(false);
+		LenientErrorHandler errorHandler = new LenientErrorHandler(false).disableAllErrors();
 		return new TolerantJsonParser(theContext, errorHandler, theResourcePid);
 	}
 }

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/ErrorHandlerTest.java

@@ -50,6 +50,7 @@ public class ErrorHandlerTest {
 		new LenientErrorHandler().unknownReference(null, null);
 		new LenientErrorHandler().incorrectJsonType(null, null, ValueType.ARRAY, null, ValueType.SCALAR, null);
 		new LenientErrorHandler().setErrorOnInvalidValue(false).invalidValue(null, "FOO", "");
+		new LenientErrorHandler().setErrorOnInvalidExtension(false).extensionContainsValueAndNestedExtensions(null);
 		new LenientErrorHandler().invalidValue(null, null, "");
 		try {
 			new LenientErrorHandler().invalidValue(null, "FOO", "");
@@ -57,6 +58,13 @@ public class ErrorHandlerTest {
 		} catch (DataFormatException e) {
 			// good, this one method defaults to causing an error
 		}
+
+		try {
+			new LenientErrorHandler().extensionContainsValueAndNestedExtensions(null);
+			fail();
+		} catch (DataFormatException e) {
+			// good, this one method defaults to causing an error
+		}
 	}
 
 	@Test

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java

@@ -1619,7 +1619,7 @@ public class XmlParserDstu3Test {
 		child.setValue(new StringType("CHILD_VALUE"));
 
 		// Lenient error handler
-		IParser parser = ourCtx.newXmlParser();
+		IParser parser = ourCtx.newXmlParser().setParserErrorHandler(new LenientErrorHandler(false).disableAllErrors());
 		String output = parser.encodeResourceToString(p);
 		ourLog.info("Output: {}", output);
 		assertThat(output, containsString("http://root"));

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

@@ -473,7 +473,6 @@ public class JsonParserR4Test extends BaseTest {
 
 	}
 
-
 	@Test
 	public void testEncodeWithInvalidExtensionContainingValueAndNestedExtensions() {
 
@@ -485,14 +484,15 @@ public class JsonParserR4Test extends BaseTest {
 		child.setUrl("http://child");
 		child.setValue(new StringType("CHILD_VALUE"));
 
+		// According to issue4129, all error handlers should reject malformed resources
 		// Lenient error handler
 		IParser parser = ourCtx.newJsonParser();
-		String output = parser.encodeResourceToString(p);
+		try {
-		ourLog.info("Output: {}", output);
+			parser.encodeResourceToString(p);
-		assertThat(output, containsString("http://root"));
+			fail();
-		assertThat(output, containsString("ROOT_VALUE"));
+		} catch (DataFormatException e) {
-		assertThat(output, containsString("http://child"));
+			assertEquals(Msg.code(1827) + "[element=\"Patient(res).extension\"] Extension contains both a value and nested extensions", e.getMessage());
-		assertThat(output, containsString("CHILD_VALUE"));
+		}
 
 		// Strict error handler
 		try {
@@ -505,6 +505,30 @@ public class JsonParserR4Test extends BaseTest {
 
 	}
 
+	@Test
+	public void testEncodeWithInvalidExtensionContainingValueAndNestedExtensions_withDisableAllErrorsShouldSucceed() {
+
+		Patient p = new Patient();
+		Extension root = p.addExtension();
+		root.setUrl("http://root");
+		root.setValue(new StringType("ROOT_VALUE"));
+		Extension child = root.addExtension();
+		child.setUrl("http://child");
+		child.setValue(new StringType("CHILD_VALUE"));
+
+		// Lenient error handler - should parse successfully with no error
+		LenientErrorHandler errorHandler = new LenientErrorHandler(true).disableAllErrors();
+		IParser parser = ourCtx.newJsonParser().setParserErrorHandler(errorHandler);
+		String output = parser.encodeResourceToString(p);
+		ourLog.info("Output: {}", output);
+		assertThat(output, containsString("http://root"));
+		assertThat(output, containsString("ROOT_VALUE"));
+		assertThat(output, containsString("http://child"));
+		assertThat(output, containsString("CHILD_VALUE"));
+		assertEquals(false, errorHandler.isErrorOnInvalidExtension());
+		assertEquals(false, errorHandler.isErrorOnInvalidValue());
+	}
+
 	@Test
 	public void testEncodeResourceWithMixedManualAndAutomaticContainedResourcesLocalFirst() {