From 2a43aa3b6ccc39cab1ecbd4a1fd98f95ec70d965 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Thu, 9 Apr 2020 09:53:13 -0400 Subject: [PATCH] Improve error message reporting for invalid json - Fix #1793 --- .../ca/uhn/fhir/parser/BaseErrorHandler.java | 14 +++++++++ .../java/ca/uhn/fhir/parser/JsonParser.java | 20 ++++++------ .../uhn/fhir/parser/LenientErrorHandler.java | 14 ++++----- .../ca/uhn/fhir/parser/ParseLocation.java | 9 +++++- .../java/ca/uhn/fhir/parser/ParserState.java | 31 +++++++++++-------- .../uhn/fhir/parser/StrictErrorHandler.java | 15 ++++----- ...report-attr-name-in-json-parse-errors.yaml | 7 +++++ .../ca/uhn/fhir/parser/JsonParserR4Test.java | 26 ++++++++++++++++ 8 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseErrorHandler.java create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1793-report-attr-name-in-json-parse-errors.yaml diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseErrorHandler.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseErrorHandler.java new file mode 100644 index 00000000000..34a657d0a83 --- /dev/null +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/BaseErrorHandler.java @@ -0,0 +1,14 @@ +package ca.uhn.fhir.parser; + +class BaseErrorHandler { + + String describeLocation(IParserErrorHandler.IParseLocation theLocation) { + if (theLocation == null) { + return ""; + } else { + return theLocation.toString() + " "; + } + } + +} + 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 index 4dea1260aca..6a5917853ed 100644 --- 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 @@ -174,7 +174,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { theEventWriter.init(); RuntimeResourceDefinition resDef = myContext.getResourceDefinition(theResource); - encodeResourceToJsonStreamWriter(resDef, theResource, theEventWriter, null, false, theEncodeContext); + encodeResourceToJsonStreamWriter(resDef, theResource, theEventWriter, null, false, theEncodeContext); theEventWriter.flush(); } @@ -258,9 +258,9 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { break; } else if (valueObj instanceof Long) { if (theChildName != null) { - theEventWriter.write(theChildName, (long)valueObj); + theEventWriter.write(theChildName, (long) valueObj); } else { - theEventWriter.write((long)valueObj); + theEventWriter.write((long) valueObj); } break; } @@ -604,7 +604,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { return maxCardinality > 1 || maxCardinality == Child.MAX_UNLIMITED; } - private void encodeCompositeElementToStreamWriter(RuntimeResourceDefinition theResDef, IBaseResource theResource, IBase theNextValue, JsonLikeWriter theEventWriter, boolean theContainedResource, CompositeChildElement theParent, EncodeContext theEncodeContext) throws IOException, DataFormatException { + private void encodeCompositeElementToStreamWriter(RuntimeResourceDefinition theResDef, IBaseResource theResource, IBase theNextValue, JsonLikeWriter theEventWriter, boolean theContainedResource, CompositeChildElement theParent, EncodeContext theEncodeContext) throws IOException, DataFormatException { writeCommentsPreAndPost(theNextValue, theEventWriter); encodeCompositeElementChildrenToStreamWriter(theResDef, theResource, theNextValue, theEventWriter, theContainedResource, theParent, theEncodeContext); @@ -1387,10 +1387,6 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { } } - private static void write(JsonLikeWriter theWriter, String theName, String theValue) throws IOException { - theWriter.write(theName, theValue); - } - private class HeldExtension implements Comparable { private CompositeChildElement myChildElem; @@ -1485,7 +1481,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { extractAndWriteExtensionsAsDirectChild(myValue, theEventWriter, def, theResDef, theResource, myChildElem, null, theEncodeContext, theContainedResource); } else { String childName = myDef.getChildNameByDatatype(myValue.getClass()); - encodeChildElementToStreamWriter(theResDef, theResource, theEventWriter, myValue, def, childName, false, myParent, false, theEncodeContext); + encodeChildElementToStreamWriter(theResDef, theResource, theEventWriter, myValue, def, childName, false, myParent, false, theEncodeContext); managePrimitiveExtension(myValue, theResDef, theResource, theEventWriter, def, childName, theEncodeContext, theContainedResource); } @@ -1562,7 +1558,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { if (childDef == null) { throw new ConfigurationException("Unable to encode extension, unrecognized child element type: " + value.getClass().getCanonicalName()); } - encodeChildElementToStreamWriter(theResDef, theResource, theEventWriter, value, childDef, childName, false, myParent,false, theEncodeContext); + encodeChildElementToStreamWriter(theResDef, theResource, theEventWriter, value, childDef, childName, false, myParent, false, theEncodeContext); managePrimitiveExtension(value, theResDef, theResource, theEventWriter, childDef, childName, theEncodeContext, theContainedResource); theEncodeContext.popPath(); @@ -1574,4 +1570,8 @@ public class JsonParser extends BaseParser implements IJsonLikeParser { theEventWriter.endObject(); } } + + private static void write(JsonLikeWriter theWriter, String theName, String theValue) throws IOException { + theWriter.write(theName, theValue); + } } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/LenientErrorHandler.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/LenientErrorHandler.java index a43a429508c..5082d3cdad7 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/LenientErrorHandler.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/LenientErrorHandler.java @@ -38,7 +38,7 @@ import ca.uhn.fhir.parser.json.JsonLikeValue.ValueType; * @see IParser#setParserErrorHandler(IParserErrorHandler) * @see FhirContext#setParserErrorHandler(IParserErrorHandler) */ -public class LenientErrorHandler implements IParserErrorHandler { +public class LenientErrorHandler extends BaseErrorHandler implements IParserErrorHandler { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(LenientErrorHandler.class); private static final StrictErrorHandler STRICT_ERROR_HANDLER = new StrictErrorHandler(); @@ -84,7 +84,7 @@ public class LenientErrorHandler implements IParserErrorHandler { public void invalidValue(IParseLocation theLocation, String theValue, String theError) { if (isBlank(theValue) || myErrorOnInvalidValue == false) { if (myLogErrors) { - ourLog.warn("Invalid attribute value \"{}\": {}", theValue, theError); + ourLog.warn("{}Invalid attribute value \"{}\": {}", describeLocation(theLocation), theValue, theError); } } else { STRICT_ERROR_HANDLER.invalidValue(theLocation, theValue, theError); @@ -133,35 +133,35 @@ public class LenientErrorHandler implements IParserErrorHandler { @Override public void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName) { if (myLogErrors) { - ourLog.warn("Multiple repetitions of non-repeatable element '{}' found while parsing", theElementName); + ourLog.warn("{}Multiple repetitions of non-repeatable element '{}' found while parsing", describeLocation(theLocation), theElementName); } } @Override public void unknownAttribute(IParseLocation theLocation, String theElementName) { if (myLogErrors) { - ourLog.warn("Unknown attribute '{}' found while parsing", theElementName); + ourLog.warn("{}Unknown attribute '{}' found while parsing",describeLocation(theLocation), theElementName); } } @Override public void unknownElement(IParseLocation theLocation, String theElementName) { if (myLogErrors) { - ourLog.warn("Unknown element '{}' found while parsing", theElementName); + ourLog.warn("{}Unknown element '{}' found while parsing", describeLocation(theLocation), theElementName); } } @Override public void unknownReference(IParseLocation theLocation, String theReference) { if (myLogErrors) { - ourLog.warn("Resource has invalid reference: {}", theReference); + ourLog.warn("{}Resource has invalid reference: {}", describeLocation(theLocation), theReference); } } @Override public void extensionContainsValueAndNestedExtensions(IParseLocation theLocation) { if (myLogErrors) { - ourLog.warn("Extension contains both a value and nested extensions: {}", theLocation); + ourLog.warn("Extension contains both a value and nested extensions: {}", describeLocation(theLocation)); } } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParseLocation.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParseLocation.java index f8451143806..fc150cdf2fc 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParseLocation.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParseLocation.java @@ -54,6 +54,13 @@ class ParseLocation implements IParseLocation { @Override public String toString() { - return defaultString(myParentElementName); + return "[element=\"" + defaultString(myParentElementName) + "\"]"; + } + + /** + * Factory method + */ + static ParseLocation fromElementName(String theChildName) { + return new ParseLocation(theChildName); } } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParserState.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParserState.java index d3f6f7193f6..34a2b765a0c 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParserState.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParserState.java @@ -191,7 +191,7 @@ class ParserState { return thePrimitiveTarget.newInstance(theDefinition.getInstanceConstructorArguments()); } - public IPrimitiveType getPrimitiveInstance(BaseRuntimeChildDefinition theChild, RuntimePrimitiveDatatypeDefinition thePrimitiveTarget) { + public IPrimitiveType getPrimitiveInstance(BaseRuntimeChildDefinition theChild, RuntimePrimitiveDatatypeDefinition thePrimitiveTarget, String theChildName) { return thePrimitiveTarget.newInstance(theChild.getInstanceConstructorArguments()); } @@ -456,7 +456,7 @@ class ParserState { RuntimePrimitiveDatatypeDefinition primitiveTarget = (RuntimePrimitiveDatatypeDefinition) target; IPrimitiveType newChildInstance = newPrimitiveInstance(myDefinition, primitiveTarget); myDefinition.getMutator().addValue(myParentInstance, newChildInstance); - PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance); + PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance, theLocalPart); push(newState); return; } @@ -583,9 +583,9 @@ class ParserState { case PRIMITIVE_DATATYPE: { RuntimePrimitiveDatatypeDefinition primitiveTarget = (RuntimePrimitiveDatatypeDefinition) target; IPrimitiveType newChildInstance; - newChildInstance = getPrimitiveInstance(child, primitiveTarget); + newChildInstance = getPrimitiveInstance(child, primitiveTarget, theChildName); child.getMutator().addValue(myInstance, newChildInstance); - PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance); + PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance, theChildName); push(newState); return; } @@ -752,7 +752,7 @@ class ParserState { RuntimePrimitiveDatatypeDefinition primitiveTarget = (RuntimePrimitiveDatatypeDefinition) target; IPrimitiveType newChildInstance = newInstance(primitiveTarget); myExtension.setValue(newChildInstance); - PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance); + PrimitiveState newState = new PrimitiveState(getPreResourceState(), newChildInstance, theLocalPart); push(newState); return; } @@ -824,7 +824,7 @@ class ParserState { break; case "lastUpdated": InstantDt updated = new InstantDt(); - push(new PrimitiveState(getPreResourceState(), updated)); + push(new PrimitiveState(getPreResourceState(), updated, theLocalPart)); myMap.put(ResourceMetadataKeyEnum.UPDATED, updated); break; case "security": @@ -850,7 +850,7 @@ class ParserState { newProfiles = new ArrayList<>(1); } IdDt profile = new IdDt(); - push(new PrimitiveState(getPreResourceState(), profile)); + push(new PrimitiveState(getPreResourceState(), profile, theLocalPart)); newProfiles.add(profile); myMap.put(ResourceMetadataKeyEnum.PROFILES, Collections.unmodifiableList(newProfiles)); break; @@ -891,7 +891,7 @@ class ParserState { private class MetaVersionElementState extends BaseState { - private ResourceMetadataMap myMap; + private final ResourceMetadataMap myMap; MetaVersionElementState(ParserState.PreResourceState thePreResourceState, ResourceMetadataMap theMap) { super(thePreResourceState); @@ -1268,23 +1268,27 @@ class ParserState { } private class PrimitiveState extends BaseState { + private final String myChildName; private IPrimitiveType myInstance; - PrimitiveState(PreResourceState thePreResourceState, IPrimitiveType theInstance) { + PrimitiveState(PreResourceState thePreResourceState, IPrimitiveType theInstance, String theChildName) { super(thePreResourceState); myInstance = theInstance; + myChildName = theChildName; } @Override public void attributeValue(String theName, String theValue) throws DataFormatException { if ("value".equals(theName)) { if ("".equals(theValue)) { - myErrorHandler.invalidValue(null, theValue, "Attribute values must not be empty (\"\")"); + ParseLocation location = ParseLocation.fromElementName(myChildName); + myErrorHandler.invalidValue(location, theValue, "Attribute value for element must not be empty (\"\")"); } else { try { myInstance.setValueAsString(theValue); } catch (DataFormatException | IllegalArgumentException e) { - myErrorHandler.invalidValue(null, theValue, e.getMessage()); + ParseLocation location = ParseLocation.fromElementName(myChildName); + myErrorHandler.invalidValue(location, theValue, e.getMessage()); } } } else if ("id".equals(theName)) { @@ -1295,7 +1299,8 @@ class ParserState { } else if (myInstance instanceof IBaseResource) { new IdDt(theValue).applyTo((org.hl7.fhir.instance.model.api.IBaseResource) myInstance); } else { - myErrorHandler.unknownAttribute(null, theName); + ParseLocation location = ParseLocation.fromElementName(myChildName); + myErrorHandler.unknownAttribute(location, theName); } } else { super.attributeValue(theName, theValue); @@ -1343,7 +1348,7 @@ class ParserState { @Override public void enteringNewElement(String theNamespace, String theChildName) throws DataFormatException { if ("id".equals(theChildName)) { - push(new PrimitiveState(getPreResourceState(), myInstance.getId())); + push(new PrimitiveState(getPreResourceState(), myInstance.getId(), theChildName)); } else if ("meta".equals(theChildName)) { push(new MetaElementState(getPreResourceState(), myInstance.getResourceMetadata())); } else { diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/StrictErrorHandler.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/StrictErrorHandler.java index 433b3f0cbf5..7c35006b255 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/StrictErrorHandler.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/StrictErrorHandler.java @@ -3,6 +3,7 @@ package ca.uhn.fhir.parser; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.parser.json.JsonLikeValue.ScalarType; import ca.uhn.fhir.parser.json.JsonLikeValue.ValueType; +import ca.uhn.fhir.util.UrlUtil; /* * #%L @@ -31,7 +32,7 @@ import ca.uhn.fhir.parser.json.JsonLikeValue.ValueType; * @see IParser#setParserErrorHandler(IParserErrorHandler) * @see FhirContext#setParserErrorHandler(IParserErrorHandler) */ -public class StrictErrorHandler implements IParserErrorHandler { +public class StrictErrorHandler extends BaseErrorHandler implements IParserErrorHandler { @Override public void containedResourceWithNoId(IParseLocation theLocation) { @@ -46,7 +47,7 @@ public class StrictErrorHandler implements IParserErrorHandler { @Override public void invalidValue(IParseLocation theLocation, String theValue, String theError) { - throw new DataFormatException("Invalid attribute value \"" + theValue + "\": " + theError); + throw new DataFormatException(describeLocation(theLocation) + "Invalid attribute value \"" + UrlUtil.sanitizeUrlPart(theValue) + "\": " + theError); } @Override @@ -65,27 +66,27 @@ public class StrictErrorHandler implements IParserErrorHandler { @Override public void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName) { - throw new DataFormatException("Multiple repetitions of non-repeatable element '" + theElementName + "' found during parse"); + throw new DataFormatException(describeLocation(theLocation) + "Multiple repetitions of non-repeatable element '" + theElementName + "' found during parse"); } @Override public void unknownAttribute(IParseLocation theLocation, String theAttributeName) { - throw new DataFormatException("Unknown attribute '" + theAttributeName + "' found during parse"); + throw new DataFormatException(describeLocation(theLocation) + "Unknown attribute '" + theAttributeName + "' found during parse"); } @Override public void unknownElement(IParseLocation theLocation, String theElementName) { - throw new DataFormatException("Unknown element '" + theElementName + "' found during parse"); + throw new DataFormatException(describeLocation(theLocation) + "Unknown element '" + theElementName + "' found during parse"); } @Override public void unknownReference(IParseLocation theLocation, String theReference) { - throw new DataFormatException("Resource has invalid reference: " + theReference); + throw new DataFormatException(describeLocation(theLocation) + "Resource has invalid reference: " + theReference); } @Override public void extensionContainsValueAndNestedExtensions(IParseLocation theLocation) { - throw new DataFormatException("Extension contains both a value and nested extensions: " + theLocation); + throw new DataFormatException(describeLocation(theLocation) + "Extension contains both a value and nested extensions: " + theLocation); } } diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1793-report-attr-name-in-json-parse-errors.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1793-report-attr-name-in-json-parse-errors.yaml new file mode 100644 index 00000000000..ad2b30fa92a --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1793-report-attr-name-in-json-parse-errors.yaml @@ -0,0 +1,7 @@ +--- +type: add +issue: 1793 +title: "When parsing JSON resources, if an element contains an invalid value, the Parser Error Handler + did not have access to the actual name of the element being parsed. This meant that errors lacked useful + detail in order to diagnose the issue. This has been corrected. Thanks to GitHub user + @jwalter for reporting!" diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/parser/JsonParserR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/parser/JsonParserR4Test.java index 350746625aa..b3363a6a8b6 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/parser/JsonParserR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/parser/JsonParserR4Test.java @@ -569,6 +569,32 @@ public class JsonParserR4Test extends BaseTest { } + /** + * See #1793 + */ + @Test + public void testParseEmptyAttribute() { + String input = "{\n" + + " \"resourceType\": \"Patient\",\n" + + " \"identifier\": [\n" + + " {\n" + + " \"system\": \"https://example.com\",\n" + + " \"value\": \"\"\n" + + " }\n" + + " ]\n" + + "}"; + + IParser jsonParser = ourCtx.newJsonParser(); + jsonParser.setParserErrorHandler(new StrictErrorHandler()); + try { + jsonParser.parseResource(Patient.class, input); + fail(); + } catch (DataFormatException e) { + assertEquals("[element=\"value\"] Invalid attribute value \"\": Attribute value for element must not be empty (\"\")", e.getMessage()); + } + + } + @Test public void testParseExtensionOnPrimitive() throws IOException { String input = IOUtils.toString(JsonParserR4Test.class.getResourceAsStream("/extension-on-line.txt"), Constants.CHARSET_UTF8);