Skip to content

Commit

Permalink
Improve performance when parsing contained resources
Browse files Browse the repository at this point in the history
  • Loading branch information
@jamesagnew
jamesagnew committed Aug 25, 2016
1 parent 9d3bd1a commit 17940b8
Show file tree
Hide file tree
Showing 16 changed files with 1,219 additions and 110 deletions.
18 changes: 14 additions & 4 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ErrorHandlerAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* 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
* 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,
Expand All @@ -24,9 +24,14 @@
* Adapter implementation with NOP implementations of all {@link IParserErrorHandler} methods.
*/
public class ErrorHandlerAdapter implements IParserErrorHandler {

@Override
public void unknownElement(IParseLocation theLocation, String theElementName) {
public void containedResourceWithNoId(IParseLocation theLocation) {
// NOP
}

@Override
public void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName) {
// NOP
}

Expand All @@ -36,7 +41,12 @@ public void unknownAttribute(IParseLocation theLocation, String theElementName)
}

@Override
public void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName) {
public void unknownElement(IParseLocation theLocation, String theElementName) {
// NOP
}

@Override
public void unknownReference(IParseLocation theLocation, String theReference) {
// NOP
}

Expand Down
51 changes: 41 additions & 10 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/IParserErrorHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* 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
* 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,
Expand All @@ -25,32 +25,63 @@
*/
public interface IParserErrorHandler {

/**
* Invoked when a contained resource is parsed that has no ID specified (and is therefore invalid)
*
* @param theLocation
* The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without
* changing the API.
* @since 2.0
*/
void containedResourceWithNoId(IParseLocation theLocation);

/**
* Invoked when an element repetition (e.g. a second repetition of something) is found for a field
* which is non-repeating.
*
* @param theLocation The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without changing the API.
* @param theElementName The name of the element that was found.
* @param theLocation
* The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without
* changing the API.
* @param theElementName
* The name of the element that was found.
* @since 1.2
*/
void unexpectedRepeatingElement(IParseLocation theLocation, String theElementName);

/**
* Invoked when an unknown element is found in the document.
* Invoked when an unknown element is found in the document.
*
* @param theLocation The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without changing the API.
* @param theAttributeName The name of the attribute that was found.
* @param theLocation
* The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without
* changing the API.
* @param theAttributeName
* The name of the attribute that was found.
*/
void unknownAttribute(IParseLocation theLocation, String theAttributeName);

/**
* Invoked when an unknown element is found in the document.
* Invoked when an unknown element is found in the document.
*
* @param theLocation The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without changing the API.
* @param theElementName The name of the element that was found.
* @param theLocation
* The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without
* changing the API.
* @param theElementName
* The name of the element that was found.
*/
void unknownElement(IParseLocation theLocation, String theElementName);

/**
* Resource contained a reference that could not be resolved and needs to be resolvable (e.g. because
* it is a local reference to an unknown contained resource)
*
* @param theLocation
* The location in the document. WILL ALWAYS BE NULL currently, as this is not yet implemented, but this parameter is included so that locations can be added in the future without
* changing the API.
* @param theReference The actual invalid reference (e.g. "#3")
* @since 2.0
*/
void unknownReference(IParseLocation theLocation, String theReference);

/**
* For now this is an empty interface. Error handling methods include a parameter of this
* type which will currently always be set to null. This interface is included here so that
Expand All @@ -59,5 +90,5 @@ public interface IParserErrorHandler {
public interface IParseLocation {
// nothing for now
}

}
14 changes: 14 additions & 0 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/LenientErrorHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,18 @@ public void unexpectedRepeatingElement(IParseLocation theLocation, String theEle
}
}

@Override
public void containedResourceWithNoId(IParseLocation theLocation) {
if (myLogErrors) {
ourLog.warn("Resource has contained child resource with no ID");
}
}

@Override
public void unknownReference(IParseLocation theLocation, String theReference) {
if (myLogErrors) {
ourLog.warn("Resource has invalid reference: {}", theReference);
}
}

}
99 changes: 53 additions & 46 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/ParserState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1393,7 +1393,9 @@ public void wereBack() {
IResource res = (IResource) getCurrentElement();
assert res != null;
if (res.getId() == null || res.getId().isEmpty()) {
ourLog.debug("Discarding contained resource with no ID!");
// If there is no ID, we don't keep the resource because it's useless (contained resources
// need an ID to be referred to)
myErrorHandler.containedResourceWithNoId(null);
} else {
if (!res.getId().isLocal()) {
res.setId(new IdDt('#' + res.getId().getIdPart()));
Expand Down Expand Up @@ -1433,11 +1435,11 @@ public void wereBack() {
IBaseResource res = getCurrentElement();
assert res != null;
if (res.getIdElement() == null || res.getIdElement().isEmpty()) {
ourLog.debug("Discarding contained resource with no ID!");
// If there is no ID, we don't keep the resource because it's useless (contained resources
// need an ID to be referred to)
myErrorHandler.containedResourceWithNoId(null);
} else {
if (!res.getIdElement().isLocal()) {
res.getIdElement().setValue('#' + res.getIdElement().getIdPart());
}
res.getIdElement().setValue('#' + res.getIdElement().getIdPart());
getPreResourceState().getContainedResources().put(res.getIdElement().getValue(), res);
}

Expand Down Expand Up @@ -1989,11 +1991,54 @@ public PreResourceState(PreResourceState thePreResourcesState, FhirVersionEnum t

@Override
public void endingElement() throws DataFormatException {
// postProcess();
stitchBundleCrossReferences();
pop();
}

protected void weaveContainedResources() {
FhirTerser terser = myContext.newTerser();
terser.visit(myInstance, new IModelVisitor() {

@Override
public void acceptElement(IBase theElement, List<String> thePathToElement, BaseRuntimeChildDefinition theChildDefinition, BaseRuntimeElementDefinition<?> theDefinition) {
if (theElement instanceof BaseResourceReferenceDt) {
BaseResourceReferenceDt nextRef = (BaseResourceReferenceDt) theElement;
String ref = nextRef.getReference().getValue();
if (isNotBlank(ref)) {
if (ref.startsWith("#")) {
IResource target = (IResource) myContainedResources.get(ref);
if (target != null) {
ourLog.debug("Resource contains local ref {} in field {}", ref, thePathToElement);
nextRef.setResource(target);
} else {
myErrorHandler.unknownReference(null, ref);
}
}
}
} else if (theElement instanceof IBaseReference) {
IBaseReference nextRef = (IBaseReference) theElement;
String ref = nextRef.getReferenceElement().getValue();
if (isNotBlank(ref)) {
if (ref.startsWith("#")) {
IBaseResource target = myContainedResources.get(ref);
if (target != null) {
ourLog.debug("Resource contains local ref {} in field {}", ref, thePathToElement);
nextRef.setResource(target);
} else {
myErrorHandler.unknownReference(null, ref);
}
}
}
}
}

@Override
public void acceptUndeclaredExtension(ISupportsUndeclaredExtensions theContainingElement, List<String> thePathToElement, BaseRuntimeChildDefinition theChildDefinition, BaseRuntimeElementDefinition<?> theDefinition, ExtensionDt theNextExt) {
acceptElement(theNextExt.getValue(), null, null, null);
}
});
}

@Override
public void enteringNewElement(String theNamespaceUri, String theLocalPart) throws DataFormatException {
BaseRuntimeElementDefinition<?> definition;
Expand Down Expand Up @@ -2111,46 +2156,6 @@ private void postProcess() {
}
}

FhirTerser terser = myContext.newTerser();
terser.visit(myInstance, new IModelVisitor() {

@Override
public void acceptElement(IBase theElement, List<String> thePathToElement, BaseRuntimeChildDefinition theChildDefinition, BaseRuntimeElementDefinition<?> theDefinition) {
if (theElement instanceof BaseResourceReferenceDt) {
BaseResourceReferenceDt nextRef = (BaseResourceReferenceDt) theElement;
String ref = nextRef.getReference().getValue();
if (isNotBlank(ref)) {
if (ref.startsWith("#")) {
IResource target = (IResource) myContainedResources.get(ref);
if (target != null) {
nextRef.setResource(target);
} else {
ourLog.warn("Resource contains unknown local ref: " + ref);
}
}
}
} else if (theElement instanceof IBaseReference) {
IBaseReference nextRef = (IBaseReference) theElement;
String ref = nextRef.getReferenceElement().getValue();
if (isNotBlank(ref)) {
if (ref.startsWith("#")) {
IBaseResource target = myContainedResources.get(ref);
if (target != null) {
nextRef.setResource(target);
} else {
ourLog.warn("Resource contains unknown local ref: " + ref);
}
}
}
}
}

@Override
public void acceptUndeclaredExtension(ISupportsUndeclaredExtensions theContainingElement, List<String> thePathToElement, BaseRuntimeChildDefinition theChildDefinition, BaseRuntimeElementDefinition<?> theDefinition, ExtensionDt theNextExt) {
acceptElement(theNextExt.getValue(), null, null, null);
}
});

populateTarget();
}

Expand Down Expand Up @@ -2232,6 +2237,7 @@ public PreResourceStateHapi(Object theTarget, IMutator theMutator, Class<? exten

@Override
protected void populateTarget() {
weaveContainedResources();
if (myEntry != null) {
myEntry.setResource((IResource) getCurrentElement());
}
Expand Down Expand Up @@ -2277,6 +2283,7 @@ public PreResourceStateHl7Org(Object theTarget, IMutator theMutator, Class<? ext

@Override
protected void populateTarget() {
weaveContainedResources();
if (myMutator != null) {
myMutator.setValue(myTarget, getCurrentElement());
}
Expand Down
10 changes: 10 additions & 0 deletions hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/StrictErrorHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,15 @@ public void unexpectedRepeatingElement(IParseLocation theLocation, String theEle
throw new DataFormatException("Multiple repetitions of non-repeatable element '" + theElementName + "' found during parse");
}

@Override
public void containedResourceWithNoId(IParseLocation theLocation) {
throw new DataFormatException("Resource has contained child resource with no ID");
}

@Override
public void unknownReference(IParseLocation theLocation, String theReference) {
throw new DataFormatException("Resource has invalid reference: " + theReference);
}


}
59 changes: 59 additions & 0 deletions hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/parser/XmlParserDstu2Test.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.io.IOException;
import java.io.StringReader;
import java.nio.charset.StandardCharsets;
import java.util.*;

import org.apache.commons.io.IOUtils;
Expand Down Expand Up @@ -67,6 +68,64 @@ public void before() {
}
}

/**
* If a contained resource refers to a contained resource that comes after it, it should still be successfully
* woven together.
*/
@Test
public void testParseWovenContainedResources() throws IOException {
String string = IOUtils.toString(getClass().getResourceAsStream("/bundle_with_woven_obs.xml"), StandardCharsets.UTF_8);

IParser parser = ourCtx.newXmlParser();
parser.setParserErrorHandler(new StrictErrorHandler());
ca.uhn.fhir.model.dstu2.resource.Bundle bundle = parser.parseResource(ca.uhn.fhir.model.dstu2.resource.Bundle.class, string);

DiagnosticReport resource = (DiagnosticReport) bundle.getEntry().get(0).getResource();
Observation obs = (Observation) resource.getResult().get(1).getResource();
assertEquals("#2", obs.getId().getValue());
ResourceReferenceDt performerFirstRep = obs.getPerformer().get(0);
Practitioner performer = (Practitioner) performerFirstRep.getResource();
assertEquals("#3", performer.getId().getValue());
}

@Test(expected=DataFormatException.class)
public void testContainedResourceWithNoId() throws IOException {
String string = IOUtils.toString(getClass().getResourceAsStream("/bundle_with_contained_with_no_id.xml"), StandardCharsets.UTF_8);

IParser parser = ourCtx.newXmlParser();
parser.setParserErrorHandler(new StrictErrorHandler());
parser.parseResource(ca.uhn.fhir.model.dstu2.resource.Bundle.class, string);
}


@Test()
public void testContainedResourceWithNoIdLenient() throws IOException {
String string = IOUtils.toString(getClass().getResourceAsStream("/bundle_with_contained_with_no_id.xml"), StandardCharsets.UTF_8);

IParser parser = ourCtx.newXmlParser();
parser.setParserErrorHandler(new LenientErrorHandler());
parser.parseResource(ca.uhn.fhir.model.dstu2.resource.Bundle.class, string);
}

@Test(expected=DataFormatException.class)
public void testParseWithInvalidLocalRef() throws IOException {
String string = IOUtils.toString(getClass().getResourceAsStream("/bundle_with_invalid_contained_ref.xml"), StandardCharsets.UTF_8);

IParser parser = ourCtx.newXmlParser();
parser.setParserErrorHandler(new StrictErrorHandler());
parser.parseResource(ca.uhn.fhir.model.dstu2.resource.Bundle.class, string);
}

@Test()
public void testParseWithInvalidLocalRefLenient() throws IOException {
String string = IOUtils.toString(getClass().getResourceAsStream("/bundle_with_invalid_contained_ref.xml"), StandardCharsets.UTF_8);

IParser parser = ourCtx.newXmlParser();
parser.setParserErrorHandler(new LenientErrorHandler());
parser.parseResource(ca.uhn.fhir.model.dstu2.resource.Bundle.class, string);
}


@Test
public void testBundleWithBinary() {
//@formatter:off
Expand Down
Loading

0 comments on commit 17940b8

Please sign in to comment.