From 4eff9a3b68e34ec642ab49b9aa3ada4f06d82d01 Mon Sep 17 00:00:00 2001 From: Grahame Grieve Date: Thu, 27 Jul 2023 17:49:57 +1000 Subject: [PATCH] fix up validation of FHIRPaths in differentials, and add warnings when collection status isn't right --- .../org/hl7/fhir/r5/model/ExpressionNode.java | 4 + .../org/hl7/fhir/r5/model/TypeDetails.java | 5 +- .../org/hl7/fhir/r5/utils/FHIRPathEngine.java | 74 ++++++++++++++++--- .../fhir/utilities/i18n/I18nConstants.java | 3 + .../src/main/resources/Messages.properties | 12 +-- .../src/main/resources/Messages_es.properties | 3 + 6 files changed, 83 insertions(+), 18 deletions(-) diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/ExpressionNode.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/ExpressionNode.java index e5af0adef..5330ca00b 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/ExpressionNode.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/ExpressionNode.java @@ -359,6 +359,10 @@ public class ExpressionNode { public enum CollectionStatus { SINGLETON, ORDERED, UNORDERED; + + boolean isList() { + return this == ORDERED || this == UNORDERED; + } } //the expression will have one of either name or constant diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/TypeDetails.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/TypeDetails.java index d3a122dca..f0d0b02e1 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/TypeDetails.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/model/TypeDetails.java @@ -285,7 +285,7 @@ public class TypeDetails { public void update(TypeDetails source) { for (ProfiledType pt : source.types) addType(pt); - if (collectionStatus == null) + if (collectionStatus == null || collectionStatus == CollectionStatus.SINGLETON) collectionStatus = source.collectionStatus; else if (source.collectionStatus == CollectionStatus.UNORDERED) collectionStatus = source.collectionStatus; @@ -516,5 +516,8 @@ public class TypeDetails { public static TypeDetails empty() { return new TypeDetails(CollectionStatus.SINGLETON); } + public boolean isList() { + return collectionStatus != null && collectionStatus.isList(); + } } \ No newline at end of file diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRPathEngine.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRPathEngine.java index 657bd6c1f..e760163df 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRPathEngine.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRPathEngine.java @@ -281,6 +281,7 @@ public class FHIRPathEngine { private boolean doNotEnforceAsSingletonRule; private boolean doNotEnforceAsCaseSensitive; private boolean allowDoubleQuotes; + private List typeWarnings = new ArrayList<>(); // if the fhir path expressions are allowed to use constants beyond those defined in the specification // the application can implement them by providing a constant resolver @@ -656,7 +657,8 @@ public class FHIRPathEngine { * @throws PathEngineException * @if the path is not valid */ - public TypeDetails checkOnTypes(Object appContext, String resourceType, List typeList, ExpressionNode expr) throws FHIRLexerException, PathEngineException, DefinitionException { + public TypeDetails checkOnTypes(Object appContext, String resourceType, List typeList, ExpressionNode expr, List warnings) throws FHIRLexerException, PathEngineException, DefinitionException { + typeWarnings.clear(); // if context is a path that refers to a type, do that conversion now TypeDetails types = new TypeDetails(CollectionStatus.SINGLETON); @@ -689,7 +691,9 @@ public class FHIRPathEngine { } } } - return executeType(new ExecutionTypeContext(appContext, resourceType, types, types), types, expr, null, true, false); + TypeDetails res = executeType(new ExecutionTypeContext(appContext, resourceType, types, types), types, expr, null, true, false); + warnings.addAll(typeWarnings); + return res; } /** @@ -2017,18 +2021,54 @@ public class FHIRPathEngine { } + private void checkCardinalityForComparabilitySame(TypeDetails left, Operation operation, TypeDetails right, ExpressionNode expr) { + if (left.isList() && !right.isList()) { + typeWarnings.add(worker.formatMessage(I18nConstants.FHIRPATH_COLLECTION_STATUS_OPERATION_LEFT, expr.toString())); + } else if (!left.isList() && right.isList()) { + typeWarnings.add(worker.formatMessage(I18nConstants.FHIRPATH_COLLECTION_STATUS_OPERATION_RIGHT, expr.toString())); + } + } + + private void checkCardinalityForSingle(TypeDetails left, Operation operation, TypeDetails right, ExpressionNode expr) { + if (left.isList()) { + typeWarnings.add(worker.formatMessage(I18nConstants.FHIRPATH_COLLECTION_STATUS_OPERATION_LEFT, expr.toString())); + } + if (right.isList()) { + typeWarnings.add(worker.formatMessage(I18nConstants.FHIRPATH_COLLECTION_STATUS_OPERATION_RIGHT, expr.toString())); + } + } + private TypeDetails operateTypes(TypeDetails left, Operation operation, TypeDetails right, ExpressionNode expr) { switch (operation) { - case Equals: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case Equivalent: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case NotEquals: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case NotEquivalent: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case LessThan: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case Greater: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case LessOrEqual: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case GreaterOrEqual: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); - case Is: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case Equals: + checkCardinalityForComparabilitySame(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case Equivalent: + checkCardinalityForComparabilitySame(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case NotEquals: + checkCardinalityForComparabilitySame(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case NotEquivalent: + checkCardinalityForComparabilitySame(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case LessThan: + checkCardinalityForSingle(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case Greater: + checkCardinalityForSingle(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case LessOrEqual: + checkCardinalityForSingle(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case GreaterOrEqual: + checkCardinalityForSingle(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); + case Is: + checkCardinalityForSingle(left, operation, right, expr); + return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); case As: + checkCardinalityForSingle(left, operation, right, expr); TypeDetails td = new TypeDetails(CollectionStatus.SINGLETON, right.getTypes()); if (td.typesHaveTargets()) { td.addTargets(left.getTargets()); @@ -2040,6 +2080,7 @@ public class FHIRPathEngine { case Xor: return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); case Implies : return new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_Boolean); case Times: + checkCardinalityForSingle(left, operation, right, expr); TypeDetails result = new TypeDetails(CollectionStatus.SINGLETON); if (left.hasType(worker, "integer") && right.hasType(worker, "integer")) { result.addType(TypeDetails.FP_Integer); @@ -2048,6 +2089,7 @@ public class FHIRPathEngine { } return result; case DivideBy: + checkCardinalityForSingle(left, operation, right, expr); result = new TypeDetails(CollectionStatus.SINGLETON); if (left.hasType(worker, "integer") && right.hasType(worker, "integer")) { result.addType(TypeDetails.FP_Decimal); @@ -2056,9 +2098,11 @@ public class FHIRPathEngine { } return result; case Concatenate: + checkCardinalityForSingle(left, operation, right, expr); result = new TypeDetails(CollectionStatus.SINGLETON, TypeDetails.FP_String); return result; case Plus: + checkCardinalityForSingle(left, operation, right, expr); result = new TypeDetails(CollectionStatus.SINGLETON); if (left.hasType(worker, "integer") && right.hasType(worker, "integer")) { result.addType(TypeDetails.FP_Integer); @@ -2075,6 +2119,7 @@ public class FHIRPathEngine { } return result; case Minus: + checkCardinalityForSingle(left, operation, right, expr); result = new TypeDetails(CollectionStatus.SINGLETON); if (left.hasType(worker, "integer") && right.hasType(worker, "integer")) { result.addType(TypeDetails.FP_Integer); @@ -2092,6 +2137,7 @@ public class FHIRPathEngine { return result; case Div: case Mod: + checkCardinalityForSingle(left, operation, right, expr); result = new TypeDetails(CollectionStatus.SINGLETON); if (left.hasType(worker, "integer") && right.hasType(worker, "integer")) { result.addType(TypeDetails.FP_Integer); @@ -3222,7 +3268,7 @@ public class FHIRPathEngine { if (atEntry && Character.isUpperCase(exp.getName().charAt(0)) && hashTail(type).equals(exp.getName())) { // special case for start up return new TypeDetails(CollectionStatus.SINGLETON, type); } - TypeDetails result = new TypeDetails(null); + TypeDetails result = new TypeDetails(focus.getCollectionStatus()); getChildTypesByName(type, exp.getName(), result, exp, focus, elementDependencies); return result; } @@ -3265,6 +3311,10 @@ public class FHIRPathEngine { } while (changed); paramTypes.clear(); paramTypes.add(base); + } else if (exp.getFunction() == Function.Where || exp.getFunction() == Function.Select || exp.getFunction() == Function.Exists || + exp.getFunction() == Function.All || exp.getFunction() == Function.AllTrue || exp.getFunction() == Function.AnyTrue + || exp.getFunction() == Function.AllFalse || exp.getFunction() == Function.AnyFalse) { + evaluateParameters(context, focus.toSingleton(), exp, elementDependencies, paramTypes, false); } else { evaluateParameters(context, focus, exp, elementDependencies, paramTypes, false); } diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/i18n/I18nConstants.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/i18n/I18nConstants.java index 77f2afc9b..2277deed8 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/i18n/I18nConstants.java +++ b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/i18n/I18nConstants.java @@ -943,6 +943,9 @@ public class I18nConstants { public static final String LIQUID_VARIABLE_ALREADY_ASSIGNED = "LIQUID_VARIABLE_ALREADY_ASSIGNED"; public static final String LIQUID_VARIABLE_ILLEGAL = "LIQUID_VARIABLE_ILLEGAL"; public static final String TERMINOLOGY_TX_SYSTEM_NOT_USABLE = "TERMINOLOGY_TX_SYSTEM_NOT_USABLE"; + public static final String ED_INVARIANT_DIFF_NO_SOURCE = "ED_INVARIANT_DIFF_NO_SOURCE"; + public static final String FHIRPATH_COLLECTION_STATUS_OPERATION_LEFT = "FHIRPATH_COLLECTION_STATUS_OPERATION_LEFT"; + public static final String FHIRPATH_COLLECTION_STATUS_OPERATION_RIGHT = "FHIRPATH_COLLECTION_STATUS_OPERATION_RIGHT"; diff --git a/org.hl7.fhir.utilities/src/main/resources/Messages.properties b/org.hl7.fhir.utilities/src/main/resources/Messages.properties index 530ec44f4..c95a25b9f 100644 --- a/org.hl7.fhir.utilities/src/main/resources/Messages.properties +++ b/org.hl7.fhir.utilities/src/main/resources/Messages.properties @@ -184,7 +184,7 @@ Terminology_TX_NoValid_8 = None of the codes provided are in the maximum value s Terminology_TX_NoValid_9 = The code provided ({2}) could not be validated against the maximum value set {0}, (error = {1}) Terminology_TX_System_Invalid = Invalid System URI: {0} Terminology_TX_System_NotKnown = Code System URI ''{0}'' is unknown so the code cannot be validated -TERMINOLOGY_TX_SYSTEM_NOT_USABLE = Code System with URI ''{0}'' has no content so the code cannot be validated +TERMINOLOGY_TX_SYSTEM_NOT_USABLE = The definition for the Code System with URI ''{0}'' doesn't provide any codes so the code cannot be validated Terminology_TX_System_Relative = Coding.system must be an absolute reference, not a local reference Terminology_TX_System_Unknown = Unknown Code System ''{0}'' Terminology_TX_System_ValueSet = Invalid System URI: {0} - cannot use a value set URI as a system @@ -990,13 +990,15 @@ LIQUID_SYNTAX_EXPECTING = Script {0}: Found ''{1}'' expecting ''{2}'' parsing cy LIQUID_SYNTAX_UNTERMINATED = Script {0}: Found unterminated string parsing cycle LIQUID_UNKNOWN_FLOW_STMT = Script {0}: Unknown flow control statement ''{1}'' LIQUID_UNKNOWN_NOEND = Script {0}: Found end of script looking for {1} -LIQUID_SYNTAX_INCLUDE, = Script {0}: Error reading include: {1} +LIQUID_SYNTAX_INCLUDE = Script {0}: Error reading include: {1} LIQUID_SYNTAX_LOOP = Script {0}: Error reading loop: {1} LIQUID_SYNTAX_NOTERM = Script {0}: Unterminated Liquid statement {1} LIQUID_UNKNOWN_NOTERM = Script {0}: Unterminated Liquid statement {1} LIQUID_SYNTAX_COLON = Exception evaluating {0}: limit is not followed by '':'' LIQUID_SYNTAX_NUMBER = Exception evaluating {0}: limit is not followed by a number LIQUID_SYNTAX_UNEXPECTED = Exception evaluating {0}: unexpected content at {1} -LIQUID_VARIABLE_ALREADY_ASSIGNED = "Liquid Exception: The variable ''{0}'' already has an assigned value -LIQUID_VARIABLE_ILLEGAL = "Liquid Exception: The variable name ''{0}'' cannot be used - \ No newline at end of file +LIQUID_VARIABLE_ALREADY_ASSIGNED = Liquid Exception: The variable ''{0}'' already has an assigned value +LIQUID_VARIABLE_ILLEGAL = Liquid Exception: The variable name ''{0}'' cannot be used +ED_INVARIANT_DIFF_NO_SOURCE = The invariant {0} defined in the differential must have no source, or the source must be the same as the profile +FHIRPATH_COLLECTION_STATUS_OPERATION_LEFT = The left side is inherently a collection, and so the expression ''{0}'' may fail or return false if there is more than one item in the content being evaluated +FHIRPATH_COLLECTION_STATUS_OPERATION_RIGHT = The right side is inherently a collection, and so this expression ''{0}'' may fail or return false if there is more than one item in the content being evaluated diff --git a/org.hl7.fhir.utilities/src/main/resources/Messages_es.properties b/org.hl7.fhir.utilities/src/main/resources/Messages_es.properties index 246ca98c8..b209a888d 100644 --- a/org.hl7.fhir.utilities/src/main/resources/Messages_es.properties +++ b/org.hl7.fhir.utilities/src/main/resources/Messages_es.properties @@ -829,3 +829,6 @@ NO_VALID_DISPLAY_FOUND_other = PRIMITIVE_VALUE_ALTERNATIVES_MESSAGE_one = PRIMITIVE_VALUE_ALTERNATIVES_MESSAGE_many = PRIMITIVE_VALUE_ALTERNATIVES_MESSAGE_other = +UNICODE_XML_BAD_CHARS_one = +UNICODE_XML_BAD_CHARS_many = +UNICODE_XML_BAD_CHARS_other = \ No newline at end of file