Skip to content

Commit

Permalink
Fixed bugs in recursion depth and added a path example (google#761)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bashir2
bashir2 authored Jul 21, 2023
1 parent ea2de8d commit 892ad88
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 216 deletions.
4 changes: 2 additions & 2 deletions .../bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ public HapiConverter<Schema> visitComposite(String elementName,
String elementTypeUrl,
List<StructureField<HapiConverter<Schema>>> children) {

Preconditions.checkArgument(!children.isEmpty());
String recordName = DefinitionVisitorsUtil.recordNameFor(elementPath);
String recordNamespace = DefinitionVisitorsUtil.namespaceFor(basePackage, elementTypeUrl);
String fullName = recordNamespace + "." + recordName;
Expand Down Expand Up @@ -743,8 +744,7 @@ private static String lowercase(String string) {

@Override
public int getMaxDepth(String elementTypeUrl, String path) {
// should be an odd number!
// return 3;
// return 2;
return 1;
}
}
2 changes: 1 addition & 1 deletion bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/R4AvroConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ public void testCompile() throws IOException {

// Ensure common types were generated
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/r4/avro/Period.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/r4/avro/Coding.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/r4/avro/PatientCoding.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/r4/avro/ValueSet.java"));

// The specific profile should be created in the expecter4b-package.
Expand Down
2 changes: 1 addition & 1 deletion bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/Stu3AvroConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ public void testCompile() throws IOException {

// Ensure common types were generated
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/stu3/avro/Period.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/stu3/avro/Coding.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/stu3/avro/PatientCoding.java"));
Assert.assertTrue(javaFiles.contains("com/cerner/bunsen/stu3/avro/ValueSet.java"));

// The specific profile should be created in the expected sub-package.
Expand Down
171 changes: 68 additions & 103 deletions ...bunsen-core-r4/src/main/java/com/cerner/bunsen/definitions/r4/R4StructureDefinitions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.cerner.bunsen.definitions.QualifiedPath;
import com.cerner.bunsen.definitions.StructureDefinitions;
import com.cerner.bunsen.definitions.StructureField;
import com.google.common.base.Verify;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
Expand All @@ -18,6 +19,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.hl7.fhir.r4.model.CanonicalType;
import org.hl7.fhir.r4.model.ElementDefinition;
import org.hl7.fhir.r4.model.ElementDefinition.TypeRefComponent;
Expand Down Expand Up @@ -83,33 +85,24 @@ private StructureDefinition getDefinition(ElementDefinition element) {
}

private <T> List<StructureField<T>> singleField(String elementName, T result) {

if (result == null) {
return Collections.emptyList();
}
return Collections.singletonList(StructureField.property(elementName, result));
}

private boolean shouldTerminateRecursive(DefinitionVisitor visitor,
StructureDefinition structureDefinition,
Deque<QualifiedPath> stack) {

ElementDefinition definitionRootElement = structureDefinition.getSnapshot().getElement().get(0);

return shouldTerminateRecursive(visitor, structureDefinition, definitionRootElement, stack);
}

private boolean shouldTerminateRecursive(DefinitionVisitor visitor,
StructureDefinition rootDefinition,
ElementDefinition elementDefinition, Deque<QualifiedPath> stack) {


return shouldTerminateRecursive(visitor,
new QualifiedPath(rootDefinition.getUrl(), elementDefinition.getPath()),
stack);
}

private boolean shouldTerminateRecursive(DefinitionVisitor visitor,
QualifiedPath newPath,
Deque<QualifiedPath> stack) {

// TODO we should add configuration parameters for exceptional paths
// that require deeper recursion; this is where to apply that logic:
// String elementPath = DefinitionVisitorsUtil.pathFromStack(
// DefinitionVisitorsUtil.elementName(newPath.getElementPath()), stack);
// if ("QuestionnaireResponse.item.item.item.item.item.item".startsWith(elementPath)) {
// return false;
// }

int maxDepth = visitor.getMaxDepth(newPath.getParentTypeUrl(), newPath.getElementPath());

return stack.stream().filter(path -> path.equals(newPath)).count() > maxDepth;
Expand Down Expand Up @@ -149,26 +142,17 @@ private <T> List<StructureField<T>> extensionElementToFields(DefinitionVisitor<T

if (definition != null) {

if (shouldTerminateRecursive(visitor,
new QualifiedPath(definition.getUrl(), element.getPath()),
stack)) {

return Collections.emptyList();

} else {

List<ElementDefinition> extensionDefinitions = definition.getSnapshot().getElement();
List<ElementDefinition> extensionDefinitions = definition.getSnapshot().getElement();

ElementDefinition extensionRoot = extensionDefinitions.get(0);
ElementDefinition extensionRoot = extensionDefinitions.get(0);

extensions = visitExtensionDefinition(visitor,
rootDefinition,
element.getSliceName(),
stack,
definition.getUrl(),
extensionDefinitions,
extensionRoot);
}
extensions = visitExtensionDefinition(visitor,
rootDefinition,
element.getSliceName(),
stack,
definition.getUrl(),
extensionDefinitions,
extensionRoot);

} else {

Expand Down Expand Up @@ -286,11 +270,7 @@ private <T> List<StructureField<T>> elementToFields(DefinitionVisitor<T> visitor

String elementName = DefinitionVisitorsUtil.elementName(element.getPath());

if (shouldTerminateRecursive(visitor, rootDefinition, element, stack)) {

return Collections.emptyList();

} else if (element.getMax().equals("0")) {
if (element.getMax().equals("0")) {

// Fields with max of zero are omitted.
return Collections.emptyList();
Expand Down Expand Up @@ -338,8 +318,12 @@ private <T> List<StructureField<T>> elementToFields(DefinitionVisitor<T> visitor
(StructureDefinition) validationSupport
.fetchStructureDefinition(typeRef.getCode());

// TODO document why we are resetting the stack here; it is not clear
// why this cannot lead to infinite recursion for choice types. If
// we don't reset the stack, then we should handle null returns.
T child = transform(visitor, element, structureDefinition, new ArrayDeque<>());

Verify.verify(child != null,
"Unexpected null choice type {} for element {}", typeRef, element);
choiceTypes.put(typeRef.getCode(), child);
}
}
Expand All @@ -360,22 +344,19 @@ private <T> List<StructureField<T>> elementToFields(DefinitionVisitor<T> visitor
// Handle defined data types.
StructureDefinition definition = getDefinition(element);

if (shouldTerminateRecursive(visitor, definition, stack)) {

return Collections.emptyList();

} else {

T type = transform(visitor, element, definition, stack);
T type = transform(visitor, element, definition, stack);

return singleField(elementName,
visitor.visitMultiValued(elementName, type));
}
return singleField(elementName,
visitor.visitMultiValued(elementName, type));

} else {

List<StructureField<T>> childElements = transformChildren(visitor,
rootDefinition, definitions, stack, element);
if (childElements.isEmpty()) {
// All children were dropped because of recursion depth limit.
return Collections.emptyList();
}

T result = visitor.visitComposite(elementName,
DefinitionVisitorsUtil.pathFromStack(elementName, stack),
Expand All @@ -399,27 +380,26 @@ private <T> List<StructureField<T>> elementToFields(DefinitionVisitor<T> visitor

} else if (getDefinition(element) != null) {

// TODO refactor this and the similar block above for handling defined data types.
// Handle defined data types.
StructureDefinition definition = getDefinition(element);

if (shouldTerminateRecursive(visitor, definition, stack)) {

return Collections.emptyList();

} else {
T type = transform(visitor, element, definition, stack);
T type = transform(visitor, element, definition, stack);

return singleField(DefinitionVisitorsUtil.elementName(element.getPath()), type);
}
return singleField(DefinitionVisitorsUtil.elementName(element.getPath()), type);

} else {

// Handle composite type
List<StructureField<T>> childElements = transformChildren(visitor, rootDefinition,
definitions, stack, element);
if (childElements.isEmpty()) {
// All children were dropped because of recursion depth limit.
return Collections.emptyList();
}

T result = visitor.visitComposite(elementName,
element.getPath(),
DefinitionVisitorsUtil.pathFromStack(elementName, stack),
elementName,
rootDefinition.getUrl(),
childElements);
Expand All @@ -428,6 +408,12 @@ private <T> List<StructureField<T>> elementToFields(DefinitionVisitor<T> visitor
}
}

/**
* Goes through the list of children of the given `element` and convert each
* of those `ElementDefinision`s to `StructureField`s.
* NOTE: This is the only place where the traversal stack can grow. It is also
* best if this is the only place where `shouldTerminateRecursive` is called.
*/
private <T> List<StructureField<T>> transformChildren(DefinitionVisitor<T> visitor,
StructureDefinition rootDefinition,
List<ElementDefinition> definitions,
Expand Down Expand Up @@ -475,15 +461,13 @@ private <T> StructureField<T> transformContained(DefinitionVisitor<T> visitor,

List<ElementDefinition> childDefinitions = containedDefinition.getSnapshot().getElement();

stack.push(new QualifiedPath(containedDefinition.getUrl(), containedRootElement.getPath()));

List<StructureField<T>> childElements = transformChildren(visitor,
containedDefinition,
childDefinitions,
stack,
containedRootElement);

stack.pop();
// At this level no child should be dropped because of recursion limit.
Verify.verify(!childElements.isEmpty());

String rootName = DefinitionVisitorsUtil.elementName(containedRootElement.getPath());

Expand Down Expand Up @@ -511,26 +495,7 @@ public FhirConversionSupport conversionSupport() {

@Override
public <T> T transform(DefinitionVisitor<T> visitor, String resourceTypeUrl) {

StructureDefinition definition = (StructureDefinition) context.getValidationSupport()
.fetchStructureDefinition(resourceTypeUrl);

if (definition == null) {

throw new IllegalArgumentException("Unable to find definition for " + resourceTypeUrl);
}

return transform(visitor, definition);
}

/**
* Returns the Spark struct type used to encode the given FHIR composite.
*
* @return The schema as a Spark StructType
*/
public <T> T transform(DefinitionVisitor<T> visitor, StructureDefinition definition) {

return transform(visitor, null, definition, new ArrayDeque<>());
return transform(visitor, resourceTypeUrl, Collections.emptyList());
}

@Override
Expand Down Expand Up @@ -562,9 +527,10 @@ public <T> T transform(DefinitionVisitor<T> visitor,
})
.collect(Collectors.toList());

return transformRoot(visitor, definition, containedDefinitions, new ArrayDeque<>());
return transformRoot(visitor, definition, containedDefinitions);
}

// TODO make the separation between this and `elementToFields` more clear.
/**
* Transforms the given FHIR structure definition.
*
Expand All @@ -576,6 +542,7 @@ public <T> T transform(DefinitionVisitor<T> visitor,
*
* @return the transformed structure, or null if it should not be included in the parent.
*/
@Nullable
private <T> T transform(DefinitionVisitor<T> visitor,
ElementDefinition parentElement,
StructureDefinition definition,
Expand All @@ -585,13 +552,9 @@ private <T> T transform(DefinitionVisitor<T> visitor,

ElementDefinition root = definitions.get(0);

stack.push(new QualifiedPath(definition.getUrl(), root.getPath()));

List<StructureField<T>> childElements = transformChildren(visitor, definition,
definitions, stack, root);

stack.pop();

if ("Reference".equals(definition.getType())) {

// TODO: if this is in an option there may be other non-reference types here?
Expand Down Expand Up @@ -628,8 +591,12 @@ private <T> T transform(DefinitionVisitor<T> visitor,
// https://github.com/FHIR/sql-on-fhir/blob/master/sql-on-fhir.md#id-fields-omitted
childElements.removeIf(field -> field.fieldName().equals("id"));

if (childElements.isEmpty()) {
// All children were dropped because of recursion depth limit.
return null;
}
return visitor.visitComposite(rootName,
rootName,
DefinitionVisitorsUtil.pathFromStack(root.getPath(), stack),
rootName,
definition.getUrl(),
childElements);
Expand All @@ -638,22 +605,22 @@ private <T> T transform(DefinitionVisitor<T> visitor,

private <T> T transformRoot(DefinitionVisitor<T> visitor,
StructureDefinition definition,
List<StructureDefinition> containedDefinitions,
Deque<QualifiedPath> stack) {
List<StructureDefinition> containedDefinitions) {

ElementDefinition definitionRootElement = definition.getSnapshot().getElementFirstRep();

List<ElementDefinition> definitions = definition.getSnapshot().getElement();

ElementDefinition root = definitions.get(0);

stack.push(new QualifiedPath(definition.getUrl(), definitionRootElement.getPath()));
Deque<QualifiedPath> stack = new ArrayDeque<>();

List<StructureField<T>> childElements = transformChildren(visitor,
definition,
definitions,
stack,
root);
definitionRootElement);
// At this level no child should be dropped because of recursion limit.
Verify.verify(!childElements.isEmpty());
Verify.verify(stack.isEmpty());

// If there are contained definitions, create a Resource Container StructureField
if (containedDefinitions.size() > 0) {
Expand All @@ -662,16 +629,14 @@ private <T> T transformRoot(DefinitionVisitor<T> visitor,
definition,
containedDefinitions,
stack,
root);
definitionRootElement);

// Replace default StructureField with constructed Resource Container StructureField
// TODO make this future proof instead of using a hard-coded index for `contained`.
childElements.set(5, containedElement);
}

stack.pop();

String rootName = DefinitionVisitorsUtil.elementName(root.getPath());
String rootName = DefinitionVisitorsUtil.elementName(definitionRootElement.getPath());

return visitor.visitComposite(rootName,
rootName,
Expand Down
Loading

0 comments on commit 892ad88

Please sign in to comment.