Skip to content

Commit

Permalink
issue #2207 - consistently throw FHIRPatchException for bad patches
Browse files Browse the repository at this point in the history 
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Apr 7, 2021
1 parent ebffbea commit 933f63a
Show file tree
Hide file tree
Showing 8 changed files with 263 additions and 84 deletions.
30 changes: 13 additions & 17 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 @@ -20,8 +20,6 @@
import javax.lang.model.SourceVersion;

import com.ibm.fhir.model.builder.Builder;
import com.ibm.fhir.model.resource.Bundle;
import com.ibm.fhir.model.resource.Parameters;
import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.type.Code;
import com.ibm.fhir.model.type.Element;
Expand Down Expand Up @@ -171,16 +169,14 @@ private void _visitEnd(java.lang.String elementName, int index, Visitable visite
Builder<?> parentBuilder = parent.getBuilder();
Object obj = wrapper.getBuilder().build();

MethodHandle methodHandle;
Class<?> expectedType = ModelSupport.getElementType(parentBuilder.getClass().getEnclosingClass(), elementName);
if (obj != null && !expectedType.isInstance(obj)) {
throw new IllegalStateException("Expected argument of type " + expectedType + " but found " + obj.getClass());
}

try {
MethodType mt;
if ((visited instanceof Element && ModelSupport.isChoiceElement(parentBuilder.getClass().getEnclosingClass(), elementName))
|| (visited instanceof Resource && isResourceContainer(parentBuilder, elementName))) {
mt = MethodType.methodType(parentBuilder.getClass(), elementOrResource);
} else {
mt = MethodType.methodType(parentBuilder.getClass(), visited.getClass());
}
methodHandle = MethodHandles.publicLookup().findVirtual(parentBuilder.getClass(), setterName(elementName), mt);
MethodType mt = MethodType.methodType(parentBuilder.getClass(), expectedType);
MethodHandle methodHandle = MethodHandles.publicLookup().findVirtual(parentBuilder.getClass(), setterName(elementName), mt);
methodHandle.invoke(parentBuilder, obj);
} catch (Throwable t) {
throw new RuntimeException("Unexpected error while visiting " + parentBuilder.getClass() + "." + elementName, t);
Expand All @@ -191,12 +187,6 @@ private void _visitEnd(java.lang.String elementName, int index, Visitable visite
}
}

private boolean isResourceContainer(Builder<?> parentBuilder, String elementName) {
return (parentBuilder instanceof Bundle.Entry.Builder && "resource".equals(elementName)) ||
(parentBuilder instanceof Bundle.Entry.Response.Builder && "outcome".equals(elementName)) ||
(parentBuilder instanceof Parameters.Parameter.Builder && "resource".equals(elementName));
}

/**
* Subclasses may override doVisitListStart
*/
Expand All @@ -214,10 +204,16 @@ public void visitEnd(String elementName, List<? extends Visitable> visitables, C
doVisitListEnd(elementName, visitables, type);
ListWrapper listWrapper = listStack.pop();
if (listWrapper.isDirty()) {
for (Visitable obj : listWrapper.getList()) {
if (!type.isInstance(obj)) {
throw new IllegalStateException("Expected argument of type " + type + " but found " + obj.getClass());
}
}
BuilderWrapper parent = builderStack.peek();
parent.dirty(true);
Builder<?> parentBuilder = parent.getBuilder();
MethodHandles.Lookup lookup = MethodHandles.publicLookup();

MethodType mt = MethodType.methodType(parentBuilder.getClass(), java.util.Collection.class);
MethodHandle methodHandle;
try {
Expand Down
40 changes: 20 additions & 20 deletions fhir-path/src/main/java/com/ibm/fhir/path/patch/FHIRPathPatch.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2020
* (C) Copyright IBM Corp. 2020, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -26,15 +26,15 @@ public class FHIRPathPatch implements FHIRPatch {
private FHIRPathPatch(Builder builder) {
this.operations = Collections.unmodifiableList(builder.operations);
}

@Override
public <T extends Resource> T apply(T resource) throws FHIRPatchException {
for (FHIRPathPatchOperation fhirPathPatchOperation : operations) {
resource = fhirPathPatchOperation.apply(resource);
}
return resource;
}

/**
* Convert the FHIRPathPatch to a FHIR Parameters resource
*/
Expand All @@ -45,84 +45,84 @@ public Parameters toParameters() {
}
return builder.build();
}

public Builder toBuilder() {
return new Builder().from(this);
}

public static Builder builder() {
return new Builder();
}
public static class Builder {

public static class Builder {
private List<FHIRPathPatchOperation> operations = new ArrayList<>(3);

private Builder() {
// hidden constructor
}

/**
* Add an add operation to the FHIRPathPatch
*/
public Builder add(String path, String elementName, Element element) {
operations.add(new FHIRPathPatchAdd(path, elementName, element));
return this;
}

/**
* Add a delete operation to the FHIRPathPatch
*/
public Builder delete(String path) {
operations.add(new FHIRPathPatchDelete(path));
return this;
}

/**
* Add an insert operation to the FHIRPathPatch
*/
public Builder insert(String path, Element element, Integer index) {
operations.add(new FHIRPathPatchInsert(path, element, index));
return this;
}

/**
* Add a move operation to the FHIRPathPatch
*/
public Builder move(String path, Integer source, Integer destination) {
operations.add(new FHIRPathPatchMove(path, source, destination));
return this;
}

/**
* Add an add operation to the FHIRPathPatch
*/
public Builder replace(String path, Element element) {
operations.add(new FHIRPathPatchReplace(path, element));
return this;
}

/**
* Add all patch operations from the passed FHIRPathPatch
*/
public Builder from(FHIRPathPatch patch) {
operations.addAll(patch.operations);
return this;
}

/**
* Build the {@link FHIRPathPatch}
*
*
* @return
* An immutable object of type {@link FHIRPathPatch}
*/
public FHIRPathPatch build() {
return new FHIRPathPatch(this);
}
}

/**
* Parse a FHIRPathPatch from a FHIR Parameters resource
*
*
* @throws IllegalArgumentException if the Parameters object does not satisfy the requirements of a FHIRPath Patch
*/
public static FHIRPathPatch from(Parameters params) {
Expand All @@ -138,10 +138,10 @@ public static FHIRPathPatch from(Parameters params) {

return builder.build();
}

/**
* Parse the passed Parameter and add it to the builder
*
*
* @throws IllegalArgumentException if the Parameter object does not represent a valid FHIRPath Patch operation
*/
private static void addOperation(Builder builder, Parameter operation) {
Expand Down
18 changes: 10 additions & 8 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
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2020
* (C) Copyright IBM Corp. 2020, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -26,6 +26,7 @@ class AddingVisitor<T extends Visitable> extends CopyingVisitor<T> {
* @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
* @throws IllegalArgumentException
*/
public AddingVisitor(Visitable parent, String parentPath, String elementName, Visitable value) {
this.path = Objects.requireNonNull(parentPath);
Expand All @@ -34,17 +35,18 @@ public AddingVisitor(Visitable parent, String parentPath, String elementName, Vi
this.value = Objects.requireNonNull(value) instanceof Code ?
convertToCodeSubtype(parent, elementName, (Code)value) : value;
}

@Override
protected void doVisitListEnd(String elementName, List<? extends Visitable> visitables, Class<?> type) {
if (getPath().equals(path) &&
type.isAssignableFrom(value.getClass()) &&
elementName.equals(this.elementNameToAdd)) {
if (getPath().equals(path) && elementName.equals(this.elementNameToAdd)) {
if (!type.isAssignableFrom(value.getClass())) {
throw new IllegalStateException("target " + type + " is not assignable from " + value.getClass());
}
getList().add(value);
markListDirty();
}
}

@Override
public boolean visit(String elementName, int index, Visitable value) {
if (!isRepeatingElement) {
Expand All @@ -56,7 +58,7 @@ public boolean visit(String elementName, int index, Visitable value) {
}
return true;
}

@Override
protected void doVisitEnd(String elementName, int elementIndex, Resource resource) {
if (!isRepeatingElement) {
Expand All @@ -65,7 +67,7 @@ protected void doVisitEnd(String elementName, int elementIndex, Resource resourc
}
}
}

@Override
protected void doVisitEnd(String elementName, int elementIndex, Element element) {
if (!isRepeatingElement) {
Expand Down
49 changes: 28 additions & 21 deletions fhir-path/src/main/java/com/ibm/fhir/path/util/FHIRPathUtil.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019, 2020
* (C) Copyright IBM Corp. 2019, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -865,14 +865,13 @@ public static <T extends Visitable> T add(T elementOrResource, String fhirPath,
Visitable parent = node.isResourceNode() ?
node.asResourceNode().resource() : node.asElementNode().element();

AddingVisitor<T> addingVisitor = new AddingVisitor<>(parent, node.path(), elementName, value);

try {
AddingVisitor<T> addingVisitor = new AddingVisitor<>(parent, node.path(), elementName, value);
elementOrResource.accept(addingVisitor);
} catch (IllegalStateException e) {
return addingVisitor.getResult();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new FHIRPatchException("An error occurred while adding the value", fhirPath, e);
}
return addingVisitor.getResult();
}

/**
Expand All @@ -883,14 +882,14 @@ public static <T extends Visitable> T add(T elementOrResource, String fhirPath,
*/
public static <T extends Visitable> T delete(T elementOrResource, String fhirPath) throws FHIRPathException, FHIRPatchException {
FHIRPathNode node = evaluateToSingle(elementOrResource, fhirPath);
DeletingVisitor<T> deletingVisitor = new DeletingVisitor<T>(node.path());

try {
DeletingVisitor<T> deletingVisitor = new DeletingVisitor<T>(node.path());
elementOrResource.accept(deletingVisitor);
} catch (IllegalStateException e) {
return deletingVisitor.getResult();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new FHIRPatchException("An error occurred while deleting the value", fhirPath, e);
}
return deletingVisitor.getResult();
}

/**
Expand All @@ -909,16 +908,22 @@ public static <T extends Visitable> T replace(T elementOrResource, String fhirPa
Visitable parent = parentNode.isResourceNode() ?
parentNode.asResourceNode().resource() : parentNode.asElementNode().element();

ReplacingVisitor<T> replacingVisitor = new ReplacingVisitor<T>(parent, elementName, node.path(), value);

try {
ReplacingVisitor<T> replacingVisitor = new ReplacingVisitor<T>(parent, elementName, node.path(), value);
elementOrResource.accept(replacingVisitor);
} catch (IllegalStateException e) {
return replacingVisitor.getResult();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new FHIRPatchException("An error occurred while replacing the value", fhirPath, e);
}
return replacingVisitor.getResult();
}

/**
* @param elementOrResource
* @param fhirPath
* @return
* @throws FHIRPathException
* @throws FHIRPatchException if the fhirPath does not evaluate to a non-null singleton collection
*/
private static FHIRPathNode evaluateToSingle(Visitable elementOrResource, String fhirPath) throws FHIRPathException, FHIRPatchException {
/*
* 1. The FHIRPath statement must return a single element.
Expand All @@ -933,7 +938,11 @@ private static FHIRPathNode evaluateToSingle(Visitable elementOrResource, String
* 5. Except for the delete operation, it is an error if no element matches the specified path.
*/
Collection<FHIRPathNode> nodes = evaluator.evaluate(elementOrResource, fhirPath);
return getSingleton(nodes);
try {
return getSingleton(nodes);
} catch (IllegalArgumentException e) {
throw new FHIRPatchException("The FHIRPath '" + fhirPath + "' did not evaluate to a singleton", e);
}
}

/**
Expand All @@ -959,14 +968,13 @@ public static <T extends Visitable> T insert(T elementOrResource, String fhirPat
Visitable parent = parentNode.isResourceNode() ?
parentNode.asResourceNode().resource() : parentNode.asElementNode().element();

InsertingVisitor<T> insertingVisitor = new InsertingVisitor<T>(parent, parentNode.path(), elementName, index, value);

try {
InsertingVisitor<T> insertingVisitor = new InsertingVisitor<T>(parent, parentNode.path(), elementName, index, value);
elementOrResource.accept(insertingVisitor);
} catch (IllegalStateException e) {
return insertingVisitor.getResult();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new FHIRPatchException("An error occurred while inserting the value", fhirPath, e);
}
return insertingVisitor.getResult();
}

/**
Expand All @@ -990,14 +998,13 @@ public static <T extends Visitable> T move(T elementOrResource, String fhirPath,
FHIRPathTree tree = evaluator.getEvaluationContext().getTree();
FHIRPathNode parent = getCommonParent(fhirPath, nodes, tree);

MovingVisitor<T> movingVisitor = new MovingVisitor<T>(parent.path(), elementName, source, target);

try {
MovingVisitor<T> movingVisitor = new MovingVisitor<T>(parent.path(), elementName, source, target);
elementOrResource.accept(movingVisitor);
} catch (IllegalStateException e) {
return movingVisitor.getResult();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new FHIRPatchException("An error occurred while moving the value", fhirPath, e);
}
return movingVisitor.getResult();
}

/**
Expand Down
Loading

0 comments on commit 933f63a

Please sign in to comment.