From 81f02da5c2c0b18fcf7cb06652fe111640012873 Mon Sep 17 00:00:00 2001 From: Max Bureck Date: Tue, 7 May 2024 15:24:14 +0200 Subject: [PATCH 1/3] Fix split logic of canonical into url and version in several places Removed - org.hl7.fhir.r4b.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r4b.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#versionFromCanonical - org.hl7.fhir.r5.renderers.DataRenderer#systemFromCanonical - org.hl7.fhir.validation.BaseValidator#versionFromCanonical - org.hl7.fhir.validation.BaseValidator#systemFromCanonical Replaced all usages of aforementioned methods by the use of org.hl7.fhir.utilities.CanonicalPair. Improved CanonicalPair to avoid multiple lookup of the | character. And added a few convenience methods to the class. This fixes the issues of #1611 (not replacing all canonical splits in other places of the code). --- .../org/hl7/fhir/utilities/CanonicalPair.java | 66 ++++++++++++-- .../hl7/fhir/utilities/CanonicalPairTest.java | 90 +++++++++++++++++++ .../hl7/fhir/validation/BaseValidator.java | 18 ---- .../instance/type/CodeSystemValidator.java | 4 +- 4 files changed, 154 insertions(+), 24 deletions(-) create mode 100644 org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/CanonicalPairTest.java diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/CanonicalPair.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/CanonicalPair.java index 6fe972e01..221b90e55 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/CanonicalPair.java +++ b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/CanonicalPair.java @@ -1,26 +1,82 @@ package org.hl7.fhir.utilities; +import org.apache.commons.lang3.StringUtils; + +/** + * Abstraction that splits a canonical in form of {@code |} into a URL and a version part. + */ public class CanonicalPair { - private String url; - private String version; + private final String url; + private final String version; + /** + * Static factory method, that invokes the {@link CanonicalPair#CanonicalPair(String) CanonicalPair constructor} + * with the given argument. + * @param target the canonical to be split. May be {@code null}. + * @return new instance of CanonicalPair + */ + public static CanonicalPair of(String target) { + return new CanonicalPair(target); + } + + /** + * Wraps the given canonical and if needed splits off the version part.

+ * The given parameter {@code target} is expected to be a canonical. + * @param target canonical to create a url, version pair from. May be {@code null}. + */ public CanonicalPair(String target) { - if (target != null && target.contains("|")) { - this.url = target.substring(0, target.indexOf("|")); - this.version = target.substring(target.indexOf("|")+1); + int pipeIndex = target != null ? target.indexOf('|') : -1; + if (pipeIndex >= 0) { + this.url = target.substring(0, pipeIndex); + this.version = target.substring(pipeIndex+1); } else { this.url = target; this.version = null; } } + /** + * Returns the URL part of the canonical (everything before the {@code "|"} character, or the complete + * canonical if the character is not present). If the source + * @return URL part of the source canonical. May be {@code null}, if source canonical was {@code null} + */ public String getUrl() { return url; } + /** + * Returns the version part of the source canonical (everything after the {@code "|"} character. + * @return version part of the canonical, may be {@code null}, if canonical was {@code null}, or canonical contains no + * {@code "|"} character. + */ public String getVersion() { return version; } + /** + * Determines if the version part of the canonical is not {@code null} + * @return {@code true} if version is not {@code null}, otherwise {@code false} + */ + public boolean hasVersion() { + return version != null; + } + + /** + * Returns the version of this pair, or the parameter {@code alternative}, + * if the version of this pair is {@code null}. + * @param alternative to be returned from this method if the encapsulated version is {@code null}. + * @return either the held version, or {@code alternative}, if version is {@code null} + */ + public String getVersionOr(String alternative) { + return hasVersion() ? version : alternative; + } + + /** + * Determines if the encapsulated version of this pair is not {@code null} and not an empty string. + * @return {@code true} if the version of this pair is not {@code null} and not an empty string, {@code false} otherwise + */ + public boolean hasNonEmptyVersion() { + return StringUtils.isNotEmpty(version); + } } diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/CanonicalPairTest.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/CanonicalPairTest.java new file mode 100644 index 000000000..3b67bf81b --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/CanonicalPairTest.java @@ -0,0 +1,90 @@ +package org.hl7.fhir.utilities; + +import static org.junit.jupiter.api.Assertions.*; + +import org.junit.jupiter.api.Test; + +class CanonicalPairTest { + + @Test + void testCanonicalNull() { + var canonical = new CanonicalPair(null); + assertNull(canonical.getUrl()); + assertNull(canonical.getVersion()); + assertFalse(canonical.hasVersion()); + assertFalse(canonical.hasNonEmptyVersion()); + String expectedVer = "1.0"; + String actualVer = canonical.getVersionOr(expectedVer); + assertEquals(expectedVer, actualVer); + } + + @Test + void testCanonicalEmpty() { + var url = ""; + var canonical = new CanonicalPair(url); + assertEquals(url, canonical.getUrl()); + assertFalse(canonical.hasVersion()); + assertFalse(canonical.hasNonEmptyVersion()); + String expectedVer = "1.0"; + String actualVer = canonical.getVersionOr(expectedVer); + assertEquals(expectedVer, actualVer); + } + + @Test + void testCanonicalWithoutVersion() { + var url = "https://www.test.org"; + var canonical = new CanonicalPair(url); + assertEquals(url, canonical.getUrl()); + assertNull(canonical.getVersion()); + assertFalse(canonical.hasVersion()); + assertFalse(canonical.hasNonEmptyVersion()); + String expectedVer = "1.0"; + String actualVer = canonical.getVersionOr(expectedVer); + assertEquals(expectedVer, actualVer); + } + + @Test + void testCanonicalWithEmptyVersion() { + var expectedUrl = "https://www.test.org"; + var url = expectedUrl + "|"; + var canonical = new CanonicalPair(url); + assertEquals(expectedUrl, canonical.getUrl()); + assertEquals("", canonical.getVersion()); + assertTrue(canonical.hasVersion()); + assertFalse(canonical.hasNonEmptyVersion()); + String alternativeVer = "1.0"; + String actualVer = canonical.getVersionOr(alternativeVer); + assertEquals("", actualVer); + } + + @Test + void testCanonicalWithVersion() { + var expectedUrl = "https://www.test.org"; + var expectedVersion = "2.6"; + var url = expectedUrl + "|" + expectedVersion; + var canonical = new CanonicalPair(url); + assertEquals(expectedUrl, canonical.getUrl()); + assertEquals(expectedVersion, canonical.getVersion()); + assertTrue(canonical.hasVersion()); + assertTrue(canonical.hasNonEmptyVersion()); + String alternativeVer = "1.0"; + String actualVer = canonical.getVersionOr(alternativeVer); + assertEquals(expectedVersion, actualVer); + } + + @Test + void testCanonicalWithVersionIncludingPipe() { + var expectedUrl = "https://www.test.org"; + var expectedVersion = "2024|05"; + var url = expectedUrl + "|" + expectedVersion; + var canonical = new CanonicalPair(url); + assertEquals(expectedUrl, canonical.getUrl()); + assertEquals(expectedVersion, canonical.getVersion()); + assertTrue(canonical.hasVersion()); + assertTrue(canonical.hasNonEmptyVersion()); + String alternativeVer = "1.0"; + String actualVer = canonical.getVersionOr(alternativeVer); + assertEquals(expectedVersion, actualVer); + } + +} diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/BaseValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/BaseValidator.java index 827710f3b..9e88c6b4c 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/BaseValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/BaseValidator.java @@ -1428,25 +1428,7 @@ public class BaseValidator implements IValidationContextResourceLoader { return null; } - protected String versionFromCanonical(String system) { - if (system == null) { - return null; - } else if (system.contains("|")) { - return system.substring(0, system.indexOf("|")); - } else { - return system; - } - } - protected String systemFromCanonical(String system) { - if (system == null) { - return null; - } else if (system.contains("|")) { - return system.substring(system.indexOf("|")+1); - } else { - return system; - } - } @Override public Resource loadContainedResource(List errors, String path, Element resource, String id, Class class1) throws FHIRException { diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/CodeSystemValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/CodeSystemValidator.java index 6363c37b6..e87456b7c 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/CodeSystemValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/type/CodeSystemValidator.java @@ -13,6 +13,7 @@ import org.hl7.fhir.r5.model.CodeSystem; import org.hl7.fhir.r5.model.CodeSystem.ConceptDefinitionComponent; import org.hl7.fhir.r5.model.ValueSet; import org.hl7.fhir.r5.terminologies.CodeSystemUtilities; +import org.hl7.fhir.utilities.CanonicalPair; import org.hl7.fhir.utilities.Utilities; import org.hl7.fhir.utilities.VersionUtilities; import org.hl7.fhir.utilities.i18n.I18nConstants; @@ -620,7 +621,8 @@ public class CodeSystemValidator extends BaseValidator { private boolean validateSupplementConcept(List errors, Element concept, NodeStack stack, String supp, ValidationOptions options) { String code = concept.getChildValue("code"); if (!Utilities.noString(code)) { - org.hl7.fhir.r5.terminologies.utilities.ValidationResult res = context.validateCode(options, systemFromCanonical(supp), versionFromCanonical(supp), code, null); + var canonical = new CanonicalPair(supp); + org.hl7.fhir.r5.terminologies.utilities.ValidationResult res = context.validateCode(options, canonical.getUrl(), canonical.getVersion(), code, null); return rule(errors, NO_RULE_DATE, IssueType.BUSINESSRULE, stack.getLiteralPath(), res.isOk(), I18nConstants.CODESYSTEM_CS_SUPP_INVALID_CODE, supp, code); } else { return true; From 962aee14a37464070a0b6ef66641f28838313571 Mon Sep 17 00:00:00 2001 From: Max Bureck Date: Tue, 7 May 2024 21:53:52 +0200 Subject: [PATCH 2/3] Added test case for CodeSystem.supplements with versioned canonical --- .../validation/ResourceValidationTests.java | 31 +++++++++++++++++-- pom.xml | 2 +- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java index 3cf91da2e..2db355613 100644 --- a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java +++ b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java @@ -1,5 +1,9 @@ package org.hl7.fhir.validation; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.MatcherAssert.assertThat; + import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; @@ -25,7 +29,7 @@ public class ResourceValidationTests { private static InstanceValidator val; - private void runTest(String filename) throws IOException, FileNotFoundException, Exception { + private List runTest(String filename) throws IOException, FileNotFoundException, Exception { TestingUtilities.injectCorePackageLoader(); if (val == null) { ctxt = TestingUtilities.getSharedWorkerContext(); @@ -37,6 +41,7 @@ public class ResourceValidationTests { Resource res = (Resource) new XmlParser().parse(TestingUtilities.loadTestResourceStream("r5", filename)); val.validate(val, errors, res); Assertions.assertNotNull(errors); + return errors; } @@ -123,4 +128,26 @@ public class ResourceValidationTests { } -} + @Test + public void testCodesystemSupplementsVersion() throws Exception { + List errors = runTest("codesystem-example-supplement-version.xml"); + assertNoErrors(errors); + } + + + private void assertNoErrors(List errors) { + List errorMessages = new ArrayList<>(); + for(ValidationMessage message : errors) { + // we will skip the message that WG citation is needed + if("VALIDATION_HL7_WG_NEEDED".equals(message.getMessageId())) { + continue; + } + if(message.getLevel().isError()) { + errorMessages.add(message.getMessage()); + } + } + assertThat("No error message expected in validation outcome.", errorMessages, is(empty())); + } + + +} \ No newline at end of file diff --git a/pom.xml b/pom.xml index f414bd97e..4c169bd7f 100644 --- a/pom.xml +++ b/pom.xml @@ -21,7 +21,7 @@ 1.26.0 32.0.1-jre 6.4.1 - 1.5.6 + 1.5.7-SNAPSHOT 2.17.0 5.9.2 1.8.2 From be0d718ac720bd8122bb5f33839ea543d6e670b1 Mon Sep 17 00:00:00 2001 From: Max Bureck Date: Wed, 8 May 2024 14:50:50 +0200 Subject: [PATCH 3/3] Changed use of message ID string in test to use constant instead --- .../java/org/hl7/fhir/validation/ResourceValidationTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java index 2db355613..e49726978 100644 --- a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java +++ b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/ResourceValidationTests.java @@ -14,6 +14,7 @@ import org.hl7.fhir.r5.formats.XmlParser; import org.hl7.fhir.r5.model.Resource; import org.hl7.fhir.r5.test.utils.TestingUtilities; import org.hl7.fhir.utilities.FhirPublication; +import org.hl7.fhir.utilities.i18n.I18nConstants; import org.hl7.fhir.utilities.settings.FhirSettings; import org.hl7.fhir.utilities.validation.ValidationMessage; import org.hl7.fhir.validation.instance.InstanceValidator; @@ -139,7 +140,7 @@ public class ResourceValidationTests { List errorMessages = new ArrayList<>(); for(ValidationMessage message : errors) { // we will skip the message that WG citation is needed - if("VALIDATION_HL7_WG_NEEDED".equals(message.getMessageId())) { + if(I18nConstants.VALIDATION_HL7_WG_NEEDED.equals(message.getMessageId())) { continue; } if(message.getLevel().isError()) {