Skip to content

Commit

Permalink
issue #679 - fixed major issue caused by use of parent identity
Browse files Browse the repository at this point in the history 
The previous approach failed in certain cases when the Visitable tree
contains the exact same object in multiple different places (e.g. adding
to a name when that same name object is repeated twice in teh same
list).

With this change, the CopyingVisitor now keeps its own pathStack and we
use the normalized path of FHIRPath nodes and/or their parent to ensure
we're only modifying the proper paths in the tree.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Feb 8, 2020
1 parent 5687778 commit 8ab77de
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 242 deletions.
68 changes: 64 additions & 4 deletions fhir-model/src/main/java/com/ibm/fhir/model/visitor/CopyingVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@

package com.ibm.fhir.model.visitor;

import static com.ibm.fhir.model.util.ModelSupport.delimit;
import static com.ibm.fhir.model.util.ModelSupport.isKeyword;

import java.lang.invoke.MethodHandle;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.util.ArrayList;
import java.util.List;
import java.util.Stack;
import java.util.stream.Collectors;

import javax.lang.model.SourceVersion;

Expand All @@ -35,13 +39,13 @@
*
* Note: this class is NOT threadsafe. Only one object should be visited at a time.
*
* @author lmsurpre
* @param <T> The type to copy. Only visitables of this type should be visited.
*/
@NotThreadSafe
public class CopyingVisitor<T extends Visitable> extends DefaultVisitor {
private final Stack<String> pathStack = new Stack<>();
private final Stack<BuilderWrapper> builderStack = new Stack<>();
private Stack<ListWrapper> listStack = new Stack<>();
private final Stack<ListWrapper> listStack = new Stack<>();
private Object result;

// subclasses may implement these to customize visit behavior without messing up our stacks
Expand All @@ -51,6 +55,7 @@ protected void doVisitStart(String elementName, int elementIndex, Element elemen
protected void doVisitStart(String elementName, int elementIndex, Resource resource) {}
protected void doVisitListStart(String elementName, List<? extends Visitable> visitables, Class<?> type) {}
protected void doVisitListEnd(String elementName, List<? extends Visitable> visitables, Class<?> type) {}

/**
* Retrieve a copy of the resource last visited.
*
Expand All @@ -61,17 +66,52 @@ protected void doVisitListEnd(String elementName, List<? extends Visitable> visi
public T getResult() {
return (T)result;
}

/**
* Get the FHIRPath path of the Resource or Element currently being visited.
*
* This method is primarily for subclasses but can also be used externally to retrieve a path to the Resource
* or Element that was being visited when an Exception occurs.
*
* @return The path of the Resource or Element currently being visited, the path that was being visited when an
* exception was thrown, or null if there is no Resource or Element being visited.
* @implSpec Path segments are appended in the visitStart methods and removed in the visitEnd methods.
*/
public final String getPath() {
if (!pathStack.isEmpty()) {
return pathStack.stream().collect(Collectors.joining("."));
}
return null;
}

public CopyingVisitor() {
super(true);
}


/**
* Reset the state of the CopyingVisitor.
*
* Invoke this method when visiting has failed and you want to clear the state in order to re-use the visitor.
*/
public final void reset() {
if (!pathStack.isEmpty()) {
pathStack.clear();
}
if (!builderStack.isEmpty()) {
builderStack.clear();
}
if (!listStack.isEmpty()) {
listStack.clear();
}
}

/**
* Subclasses may override doVisitStart
*/
@Override
public final void visitStart(java.lang.String elementName, int index, Element element) {
builderStack.push(new ElementWrapper(element.toBuilder()));
pathStackPush(elementName, index);
doVisitStart(elementName, index, element);
}

Expand All @@ -81,6 +121,7 @@ public final void visitStart(java.lang.String elementName, int index, Element el
@Override
public final void visitStart(java.lang.String elementName, int index, Resource resource) {
builderStack.push(new ResourceWrapper(resource.toBuilder()));
pathStackPush(elementName, index);
doVisitStart(elementName, index, resource);
}

Expand All @@ -103,14 +144,18 @@ public final void visitEnd(java.lang.String elementName, int index, Resource res
}

private void _visitEnd(java.lang.String elementName, int index, Visitable visited, Class<? extends Visitable> elementOrResource) {
pathStackPop();
BuilderWrapper wrapper = builderStack.pop();
if (index != -1) {
ListWrapper listWrapper = listStack.peek();
if (wrapper.isDirty()) {
listWrapper.dirty(true);
}
// No way to know if one of the other elements in the list will be dirty so we need to build them all
listWrapper.getList().add((Visitable) wrapper.getBuilder().build());
Visitable item = wrapper.getBuilder().build();
if (item != null) {
listWrapper.getList().add((Visitable) wrapper.getBuilder().build());
}
} else {
if (builderStack.isEmpty()) {
if (wrapper.isDirty()) {
Expand Down Expand Up @@ -201,6 +246,21 @@ public void postVisit(Element element) {
public void postVisit(Resource resource) {
}

private void pathStackPop() {
pathStack.pop();
}

private void pathStackPush(String elementName, int index) {
if (isKeyword(elementName)) {
elementName = delimit(elementName);
}
if (index != -1) {
pathStack.push(elementName + "[" + index + "]");
} else {
pathStack.push(elementName);
}
}

protected Builder<?> getBuilder() {
return builderStack.peek().getBuilder();
}
Expand Down
65 changes: 28 additions & 37 deletions fhir-path/src/main/java/com/ibm/fhir/path/util/AddingVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import java.util.List;
import java.util.Objects;
import java.util.Stack;

import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.type.Code;
Expand All @@ -17,70 +16,62 @@
import com.ibm.fhir.model.visitor.Visitable;

class AddingVisitor<T extends Visitable> extends CopyingVisitor<T> {
private Stack<Visitable> visitStack;

private Visitable parent;
private String path;
private String elementNameToAdd;
private boolean isRepeatingElement;
private Visitable value;

/**
* @param parent
* @param elementName
* @param value
* @param parent the resource or element to add to
* @param parentPath a "simple" FHIRPath path to the parent of the element being added
* @param elementName the name of the element to add
* @param value the element to add
*/
public AddingVisitor(Visitable parent, String elementName, Visitable value) {
this.visitStack = new Stack<Visitable>();
this.parent = Objects.requireNonNull(parent);
public AddingVisitor(Visitable parent, String parentPath, String elementName, Visitable value) {
this.path = Objects.requireNonNull(parentPath);
this.elementNameToAdd = Objects.requireNonNull(elementName);
this.isRepeatingElement = ModelSupport.isRepeatingElement(parent.getClass(), elementName);
this.value = Objects.requireNonNull(value) instanceof Code ?
convertToCodeSubtype(parent, elementName, (Code)value) : value;
}

@Override
protected void doVisitStart(String elementName, int elementIndex, Resource resource) {
visitStack.push(resource);
}

@Override
protected void doVisitStart(String elementName, int elementIndex, Element element) {
visitStack.push(element);
}

@Override
protected void doVisitListEnd(String elementName, List<? extends Visitable> visitables, Class<?> type) {
if (visitStack.peek() == parent && type.isAssignableFrom(value.getClass()) && elementName.equals(this.elementNameToAdd)) {
if (getPath().equals(path) &&
type.isAssignableFrom(value.getClass()) &&
elementName.equals(this.elementNameToAdd)) {
getList().add(value);
markListDirty();
}
}

@Override
public boolean visit(String elementName, int index, Visitable value) {
if (!isRepeatingElement) {
if (this.value == value) {
markDirty();
} else if (getPath().equals(path + "." + elementNameToAdd)) {
throw new IllegalStateException("Add cannot replace an existing value at " + getPath());
}
}
return true;
}

@Override
protected void doVisitEnd(String elementName, int elementIndex, Resource resource) {
if (!ModelSupport.isRepeatingElement(parent.getClass(), this.elementNameToAdd)) {
if (resource == parent) {
if (!isRepeatingElement) {
if (getPath().equals(path)) {
value.accept(this.elementNameToAdd, this);
} else if (resource == value) {
markDirty();
} else if (visitStack.peek() == parent && elementName.equals(this.elementNameToAdd)) {
// XXX: assuming that we have the right parent is potentially dangerous
throw new IllegalStateException("Add cannot replace an existing value");
}
}
visitStack.pop();
}

@Override
protected void doVisitEnd(String elementName, int elementIndex, Element element) {
if (!ModelSupport.isRepeatingElement(parent.getClass(), this.elementNameToAdd)) {
if (element == parent) {
if (!isRepeatingElement) {
if (getPath().equals(path)) {
value.accept(this.elementNameToAdd, this);
} else if (element == value) {
markDirty();
} else if (visitStack.peek() == parent && elementName.equals(this.elementNameToAdd)) {
// XXX: assuming that we have the right parent is potentially dangerous
throw new IllegalStateException("Add cannot replace an existing value");
}
}
visitStack.pop();
}
}
71 changes: 11 additions & 60 deletions fhir-path/src/main/java/com/ibm/fhir/path/util/DeletingVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,74 +5,25 @@
*/
package com.ibm.fhir.path.util;

import java.util.List;
import java.util.Objects;
import java.util.Stack;

import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.type.Element;
import com.ibm.fhir.model.util.ModelSupport;
import com.ibm.fhir.model.visitor.CopyingVisitor;
import com.ibm.fhir.model.visitor.Visitable;

class DeletingVisitor<T extends Visitable> extends CopyingVisitor<T> {
private Stack<Visitable> visitStack;

private Visitable parent;
private String elementNameToDelete;
private Visitable toDelete;
private String pathToDelete;

/**
* @param parent
* @param elementName
* @param toDelete
*/
public DeletingVisitor(Visitable parent, String elementName, Visitable toDelete) {
this.visitStack = new Stack<Visitable>();
this.parent = Objects.requireNonNull(parent);
this.elementNameToDelete = Objects.requireNonNull(elementName);
this.toDelete = Objects.requireNonNull(toDelete);
public DeletingVisitor(String pathToDelete) {
this.pathToDelete = Objects.requireNonNull(pathToDelete);
}

@Override
protected void doVisitStart(String elementName, int elementIndex, Resource resource) {
visitStack.push(resource);
}

@Override
protected void doVisitStart(String elementName, int elementIndex, Element element) {
visitStack.push(element);
}

@Override
protected void doVisitListEnd(String elementName, List<? extends Visitable> visitables, Class<?> type) {
if (visitStack.peek() == parent && type.isAssignableFrom(toDelete.getClass()) && elementName.equals(this.elementNameToDelete)) {
for (int i = 0; i < visitables.size(); i++) {
if (visitables.get(i) == toDelete) {
getList().remove(i);
markListDirty();
}
}
}
}

@Override
protected void doVisitEnd(String elementName, int elementIndex, Resource resource) {
if (!ModelSupport.isRepeatingElement(parent.getClass(), this.elementNameToDelete)) {
if (elementName.equals(this.elementNameToDelete) && resource == toDelete) {
delete();
}
}
visitStack.pop();
}


@Override
protected void doVisitEnd(String elementName, int elementIndex, Element element) {
if (!ModelSupport.isRepeatingElement(parent.getClass(), this.elementNameToDelete)) {
if (elementName.equals(this.elementNameToDelete) && element == toDelete) {
delete();
}
public boolean visit(String elementName, int index, Visitable value) {
if (pathToDelete.equals(getPath())) {
delete();
markDirty();
return false;
}
visitStack.pop();
return true;
}
}
}
Loading

0 comments on commit 8ab77de

Please sign in to comment.