From 19d3952b11392173c0153cc09985984b0e07c0cc Mon Sep 17 00:00:00 2001 From: Grahame Grieve Date: Mon, 19 Jun 2023 22:42:02 +1000 Subject: [PATCH] Validate FHIRPath constraints in IGs and profiles --- .../hl7/fhir/r5/elementmodel/FmlParser.java | 2 +- .../java/org/hl7/fhir/r5/utils/FHIRLexer.java | 17 +++++-- .../org/hl7/fhir/r5/utils/FHIRPathEngine.java | 21 ++++++--- .../org/hl7/fhir/r5/utils/LiquidEngine.java | 2 +- .../structuremap/StructureMapUtilities.java | 2 +- .../r5/context/BaseWorkerContextTests.java | 6 +++ .../org/hl7/fhir/r5/utils/FHIRLexerTest.java | 2 +- .../hl7/fhir/utilities/SimpleHTTPClient.java | 6 ++- .../instance/InstanceValidator.java | 22 +++++++--- .../type/StructureDefinitionValidator.java | 44 ++++++++++++++++--- .../validation/profile/ProfileValidator.java | 10 +++++ 11 files changed, 108 insertions(+), 26 deletions(-) diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/FmlParser.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/FmlParser.java index f4629aeeb..0664857d8 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/FmlParser.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/elementmodel/FmlParser.java @@ -57,7 +57,7 @@ public class FmlParser extends ParserBase { } public Element parse(String text) throws FHIRException { - FHIRLexer lexer = new FHIRLexer(text, "source", true); + FHIRLexer lexer = new FHIRLexer(text, "source", true, true); if (lexer.done()) throw lexer.error("Map Input cannot be empty"); Element result = Manager.build(context, context.fetchTypeDefinition("StructureMap")); diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRLexer.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRLexer.java index 1b521b471..af0183b4e 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRLexer.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/FHIRLexer.java @@ -88,6 +88,7 @@ public class FHIRLexer { private boolean liquidMode; // in liquid mode, || terminates the expression and hands the parser back to the host private SourceLocation commentLocation; private boolean metadataFormat; + private boolean allowDoubleQuotes; public FHIRLexer(String source, String name) throws FHIRLexerException { this.source = source == null ? "" : source; @@ -101,10 +102,18 @@ public class FHIRLexer { currentLocation = new SourceLocation(1, 1); next(); } - public FHIRLexer(String source, String name, boolean metadataFormat) throws FHIRLexerException { + public FHIRLexer(String source, int i, boolean allowDoubleQuotes) throws FHIRLexerException { + this.source = source; + this.cursor = i; + this.allowDoubleQuotes = allowDoubleQuotes; + currentLocation = new SourceLocation(1, 1); + next(); + } + public FHIRLexer(String source, String name, boolean metadataFormat, boolean allowDoubleQuotes) throws FHIRLexerException { this.source = source == null ? "" : source; this.name = name == null ? "??" : name; this.metadataFormat = metadataFormat; + this.allowDoubleQuotes = allowDoubleQuotes; currentLocation = new SourceLocation(1, 1); next(); } @@ -235,7 +244,7 @@ public class FHIRLexer { if (ch == '}') cursor++; current = source.substring(currentStart, cursor); - } else if (ch == '"') { + } else if (ch == '"' && allowDoubleQuotes) { cursor++; boolean escape = false; while (cursor < source.length() && (escape || source.charAt(cursor) != '"')) { @@ -588,5 +597,7 @@ public class FHIRLexer { return null; } } - + public boolean isAllowDoubleQuotes() { + return allowDoubleQuotes; + } } \ 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 2d805a672..cf570ac85 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 @@ -280,6 +280,7 @@ public class FHIRPathEngine { private boolean liquidMode; // in liquid mode, || terminates the expression and hands the parser back to the host private boolean doNotEnforceAsSingletonRule; private boolean doNotEnforceAsCaseSensitive; + private boolean allowDoubleQuotes; // 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 @@ -511,7 +512,7 @@ public class FHIRPathEngine { } public ExpressionNode parse(String path, String name) throws FHIRLexerException { - FHIRLexer lexer = new FHIRLexer(path, name); + FHIRLexer lexer = new FHIRLexer(path, name, false, allowDoubleQuotes); if (lexer.done()) { throw lexer.error("Path cannot be empty"); } @@ -548,7 +549,7 @@ public class FHIRPathEngine { * @throws Exception */ public ExpressionNodeWithOffset parsePartial(String path, int i) throws FHIRLexerException { - FHIRLexer lexer = new FHIRLexer(path, i); + FHIRLexer lexer = new FHIRLexer(path, i, allowDoubleQuotes); if (lexer.done()) { throw lexer.error("Path cannot be empty"); } @@ -5816,7 +5817,11 @@ public class FHIRPathEngine { String tail = ""; StructureDefinition sd = worker.fetchResource(StructureDefinition.class, url); if (sd == null) { - throw makeException(expr, I18nConstants.FHIRPATH_NO_TYPE, url, "getChildTypesByName"); + if (url.startsWith(TypeDetails.FP_NS)) { + return; + } else { + throw makeException(expr, I18nConstants.FHIRPATH_UNKNOWN_TYPE, url, "getChildTypesByName"); + } } List sdl = new ArrayList(); ElementDefinitionMatch m = null; @@ -5826,14 +5831,14 @@ public class FHIRPathEngine { if (m.fixedType != null) { StructureDefinition dt = worker.fetchResource(StructureDefinition.class, ProfileUtilities.sdNs(m.fixedType, null), sd); if (dt == null) { - throw makeException(expr, I18nConstants.FHIRPATH_NO_TYPE, ProfileUtilities.sdNs(m.fixedType, null), "getChildTypesByName"); + throw makeException(expr, I18nConstants.FHIRPATH_UNKNOWN_TYPE, ProfileUtilities.sdNs(m.fixedType, null), "getChildTypesByName"); } sdl.add(dt); } else for (TypeRefComponent t : m.definition.getType()) { StructureDefinition dt = worker.fetchResource(StructureDefinition.class, ProfileUtilities.sdNs(t.getCode(), null)); if (dt == null) { - throw makeException(expr, I18nConstants.FHIRPATH_NO_TYPE, ProfileUtilities.sdNs(t.getCode(), null), "getChildTypesByName"); + throw makeException(expr, I18nConstants.FHIRPATH_UNKNOWN_TYPE, ProfileUtilities.sdNs(t.getCode(), null), "getChildTypesByName"); } addTypeAndDescendents(sdl, dt, cu.allStructures()); // also add any descendant types @@ -6392,4 +6397,10 @@ public class FHIRPathEngine { return profileUtilities; } + public boolean isAllowDoubleQuotes() { + return allowDoubleQuotes; + } + public void setAllowDoubleQuotes(boolean allowDoubleQuotes) { + this.allowDoubleQuotes = allowDoubleQuotes; + } } diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/LiquidEngine.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/LiquidEngine.java index c9f4772dc..e4bbf2103 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/LiquidEngine.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/LiquidEngine.java @@ -176,7 +176,7 @@ public class LiquidEngine implements IEvaluationContext { @Override public void evaluate(StringBuilder b, Base resource, LiquidEngineContext ctxt) throws FHIRException { if (compiled.size() == 0) { - FHIRLexer lexer = new FHIRLexer(statement, "liquid statement"); + FHIRLexer lexer = new FHIRLexer(statement, "liquid statement", false, true); lexer.setLiquidMode(true); compiled.add(new LiquidExpressionNode(null, engine.parse(lexer))); while (!lexer.done()) { diff --git a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/structuremap/StructureMapUtilities.java b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/structuremap/StructureMapUtilities.java index 5e91f5438..9e5420cb8 100644 --- a/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/structuremap/StructureMapUtilities.java +++ b/org.hl7.fhir.r5/src/main/java/org/hl7/fhir/r5/utils/structuremap/StructureMapUtilities.java @@ -626,7 +626,7 @@ public class StructureMapUtilities { } public StructureMap parse(String text, String srcName) throws FHIRException { - FHIRLexer lexer = new FHIRLexer(Utilities.stripBOM(text), srcName, true); + FHIRLexer lexer = new FHIRLexer(Utilities.stripBOM(text), srcName, true, true); if (lexer.done()) throw lexer.error("Map Input cannot be empty"); StructureMap result = new StructureMap(); diff --git a/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/context/BaseWorkerContextTests.java b/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/context/BaseWorkerContextTests.java index e32537104..f49ae01e1 100644 --- a/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/context/BaseWorkerContextTests.java +++ b/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/context/BaseWorkerContextTests.java @@ -3,6 +3,7 @@ package org.hl7.fhir.r5.context; import org.hl7.fhir.exceptions.FHIRException; import org.hl7.fhir.r5.model.PackageInformation; import org.hl7.fhir.r5.model.Parameters; +import org.hl7.fhir.r5.model.Resource; import org.hl7.fhir.r5.model.ValueSet; import org.hl7.fhir.r5.utils.validation.IResourceValidator; import org.hl7.fhir.utilities.npm.BasePackageCacheManager; @@ -75,6 +76,11 @@ public class BaseWorkerContextTests { public String getSpecUrl() { return null; } + + @Override + public T fetchResourceRaw(Class class_, String uri) { + return null; + } }; baseWorkerContext.expParameters = new Parameters(); return baseWorkerContext; diff --git a/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/utils/FHIRLexerTest.java b/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/utils/FHIRLexerTest.java index 73637fcde..79b7ad31f 100644 --- a/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/utils/FHIRLexerTest.java +++ b/org.hl7.fhir.r5/src/test/java/org/hl7/fhir/r5/utils/FHIRLexerTest.java @@ -8,7 +8,7 @@ class FHIRLexerTest { @Test @DisplayName("Test that a 'null' current value returns 'false' when FHIRLexer.isConstant() is called, and not NPE.") void getCurrent() { - FHIRLexer lexer = new FHIRLexer(null, null); + FHIRLexer lexer = new FHIRLexer(null, null, false, true); String lexerCurrent = lexer.getCurrent(); Assertions.assertNull(lexerCurrent); Assertions.assertFalse(lexer.isConstant()); diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/SimpleHTTPClient.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/SimpleHTTPClient.java index f97cef2f9..7f85adbcd 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/SimpleHTTPClient.java +++ b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/SimpleHTTPClient.java @@ -163,8 +163,10 @@ public class SimpleHTTPClient { } private void setHeaders(HttpURLConnection c) { - for (Header h : headers) { - c.setRequestProperty(h.getName(), h.getValue()); + if (headers != null) { + for (Header h : headers) { + c.setRequestProperty(h.getName(), h.getValue()); + } } c.setConnectTimeout(15000); c.setReadTimeout(15000); diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java index c4fb250b1..4e9b9139a 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java @@ -466,7 +466,7 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat private boolean noUnicodeBiDiControlChars; private HtmlInMarkdownCheck htmlInMarkdownCheck; private boolean allowComments; - private boolean displayWarnings; + private boolean allowDoubleQuotesInFHIRPath; private List igs = new ArrayList<>(); private List extensionDomains = new ArrayList(); @@ -518,6 +518,7 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat fpe.setLegacyMode(true); source = Source.InstanceValidator; fpe.setDoNotEnforceAsSingletonRule(!VersionUtilities.isR5VerOrLater(theContext.getVersion())); + fpe.setAllowDoubleQuotes(allowDoubleQuotesInFHIRPath); } @Override @@ -2036,12 +2037,11 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat } } else if (ctxt.getType() == ExtensionContextType.EXTENSION) { contexts.append("x:" + ctxt.getExpression()); - NodeStack estack = stack.getParent(); - if (estack != null && estack.getElement().fhirType().equals("Extension")) { - String ext = estack.getElement().getNamedChildValue("url"); - if (ctxt.getExpression().equals(ext)) { - ok = true; - } + String ext = stack.getElement().getNamedChildValue("url"); + if (ctxt.getExpression().equals(ext)) { + ok = true; + } else { + plist.add(ext); } } else if (ctxt.getType() == ExtensionContextType.FHIRPATH) { contexts.append("p:" + ctxt.getExpression()); @@ -6490,6 +6490,14 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat } + public boolean isAllowDoubleQuotesInFHIRPath() { + return allowDoubleQuotesInFHIRPath; + } + + public void setAllowDoubleQuotesInFHIRPath(boolean allowDoubleQuotesInFHIRPath) { + this.allowDoubleQuotesInFHIRPath = allowDoubleQuotesInFHIRPath; + } + public static void setParents(Element element) { if (element != null && !element.hasParentForValidator()) { element.setParentForValidator(null); diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/StructureDefinitionValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/StructureDefinitionValidator.java index 643771e34..2e8e5924e 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/StructureDefinitionValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/StructureDefinitionValidator.java @@ -5,7 +5,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Comparator; import java.util.HashSet; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Set; import org.hl7.fhir.convertors.factory.VersionConvertorFactory_10_50; @@ -129,10 +131,10 @@ public class StructureDefinitionValidator extends BaseValidator { boolean logical = "logical".equals(src.getNamedChildValue("kind")); boolean constraint = "constraint".equals(src.getNamedChildValue("derivation")); for (Element differential : differentials) { - ok = validateElementList(errors, differential, stack.push(differential, -1, null, null), false, snapshots.size() > 0, sd, typeName, logical, constraint) && ok; + ok = validateElementList(errors, differential, stack.push(differential, -1, null, null), false, snapshots.size() > 0, sd, typeName, logical, constraint, src.getNamedChildValue("type"), src.getNamedChildValue("url")) && ok; } for (Element snapshotE : snapshots) { - ok = validateElementList(errors, snapshotE, stack.push(snapshotE, -1, null, null), true, true, sd, typeName, logical, constraint) && ok; + ok = validateElementList(errors, snapshotE, stack.push(snapshotE, -1, null, null), true, true, sd, typeName, logical, constraint, src.getNamedChildValue("type"), src.getNamedChildValue("url")) && ok; } // obligation profile support @@ -330,18 +332,19 @@ public class StructureDefinitionValidator extends BaseValidator { } } - private boolean validateElementList(List errors, Element elementList, NodeStack stack, boolean snapshot, boolean hasSnapshot, StructureDefinition sd, String typeName, boolean logical, boolean constraint) { + private boolean validateElementList(List errors, Element elementList, NodeStack stack, boolean snapshot, boolean hasSnapshot, StructureDefinition sd, String typeName, boolean logical, boolean constraint, String rootPath, String profileUrl) { + Map invariantMap = new HashMap<>(); boolean ok = true; List elements = elementList.getChildrenByName("element"); int cc = 0; for (Element element : elements) { - ok = validateElementDefinition(errors, element, stack.push(element, cc, null, null), snapshot, hasSnapshot, sd, typeName, logical, constraint) && ok; + ok = validateElementDefinition(errors, element, stack.push(element, cc, null, null), snapshot, hasSnapshot, sd, typeName, logical, constraint, invariantMap, rootPath, profileUrl) && ok; cc++; } return ok; } - private boolean validateElementDefinition(List errors, Element element, NodeStack stack, boolean snapshot, boolean hasSnapshot, StructureDefinition sd, String typeName, boolean logical, boolean constraint) { + private boolean validateElementDefinition(List errors, Element element, NodeStack stack, boolean snapshot, boolean hasSnapshot, StructureDefinition sd, String typeName, boolean logical, boolean constraint, Map invariantMap, String rootPath, String profileUrl) { boolean ok = true; boolean typeMustSupport = false; String path = element.getNamedChildValue("path"); @@ -466,9 +469,40 @@ public class StructureDefinitionValidator extends BaseValidator { } // if we see fixed[x] or pattern[x] applied to a repeating element, we'll give the user a hint } + List constraints = element.getChildrenByName("constraint"); + int cc = 0; + for (Element invariant : constraints) { + ok = validateElementDefinitionInvariant(errors, invariant, stack.push(invariant, cc, null, null), invariantMap, element.getNamedChildValue("path"), rootPath, profileUrl) && ok; + cc++; + } return ok; } + private boolean validateElementDefinitionInvariant(List errors, Element invariant, NodeStack stack, Map invariantMap, String path, String rootPath, String profileUrl) { + boolean ok = true; + String key = invariant.getNamedChildValue("key"); + String expression = invariant.getNamedChildValue("expression"); + String source = invariant.getNamedChildValue("source"); + if (warning(errors, "2023-06-19", IssueType.INFORMATIONAL, stack, !Utilities.noString(key), I18nConstants.ED_INVARIANT_NO_KEY)) { + if (hint(errors, "2023-06-19", IssueType.INFORMATIONAL, stack, !Utilities.noString(expression), I18nConstants.ED_INVARIANT_NO_EXPRESSION, key)) { + if (invariantMap.containsKey(key)) { + // it's legal - and common - for a list of elemnts to contain the same invariant more than once, but it's not valid if it's not always the same + ok = rule(errors, "2023-06-19", IssueType.INVALID, stack, expression.equals(invariantMap.get(key)), I18nConstants.ED_INVARIANT_EXPRESSION_CONFLICT, key, expression, invariantMap.get(key)); + } else { + invariantMap.put(key, expression); + } + if (Utilities.noString(source) || (source.equals(profileUrl))) { // no need to revalidate FHIRPath from elsewhere + try { + fpe.check(invariant, rootPath, path, fpe.parse(expression)); + } catch (Exception e) { + ok = rule(errors, "2023-06-19", IssueType.INVALID, stack, false, I18nConstants.ED_INVARIANT_EXPRESSION_ERROR, key, expression, e.getMessage()) && ok; + } + } + } + } + return ok; + } + private boolean meaningWhenMissingAllowed(Element element) { // allowed to use meaningWhenMissing on the root of an element to say what it means when the extension // is not present. diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/profile/ProfileValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/profile/ProfileValidator.java index 7066d12d9..2b3ebec25 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/profile/ProfileValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/profile/ProfileValidator.java @@ -51,11 +51,13 @@ public class ProfileValidator extends BaseValidator { private boolean checkAggregation = false; private boolean checkMustSupport = false; + private boolean allowDoubleQuotesInFHIRPath = false; private FHIRPathEngine fpe; public ProfileValidator(IWorkerContext context, XVerExtensionManager xverManager) { super(context, xverManager); fpe = new FHIRPathEngine(context); + fpe.setAllowDoubleQuotes(allowDoubleQuotesInFHIRPath); } public boolean isCheckAggregation() { @@ -74,6 +76,14 @@ public class ProfileValidator extends BaseValidator { this.checkMustSupport = checkMustSupport; } + public boolean isAllowDoubleQuotesInFHIRPath() { + return allowDoubleQuotesInFHIRPath; + } + + public void setAllowDoubleQuotesInFHIRPath(boolean allowDoubleQuotesInFHIRPath) { + this.allowDoubleQuotesInFHIRPath = allowDoubleQuotesInFHIRPath; + } + protected boolean rule(List errors, IssueType type, String path, boolean b, String msg) { String rn = path.contains(".") ? path.substring(0, path.indexOf(".")) : path; return super.ruleHtml(errors, NO_RULE_DATE, type, path, b, msg, ""+rn+": "+Utilities.escapeXml(msg));