Issue #694 - Perform syntax validation against ucum-units value set #1430

michaelwschroeder · 2020-08-19T20:44:32Z

Signed-off-by: Mike Schroeder mschroed@us.ibm.com

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

prb112 · 2020-08-19T20:56:29Z

2020-08-19T20:49:09.1349282Z WARNING: xml/ibm/complete-absent/RiskEvidenceSynthesis-1.xml: wanted:         OK got:      PARSE
2020-08-19T20:49:09.3893166Z [ERROR] Tests run: 100, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 89.686 s <<< FAILURE! - in TestSuite
2020-08-19T20:49:09.3900551Z [ERROR] copyTest(com.ibm.fhir.model.spec.test.R4ExamplesTest)  Time elapsed: 39.589 s  <<< FAILURE!
2020-08-19T20:49:09.3913400Z com.ibm.fhir.model.parser.exception.FHIRParserException: 'unitOfMeasure' does not contain a Coding with a UCUM system of 'http://unitsofmeasure.org' and a valid UCUM code [EffectEvidenceSynthesis]
2020-08-19T20:49:09.3919255Z 	at com.ibm.fhir.model.spec.test.R4ExamplesTest.copyTest(R4ExamplesTest.java:44)
2020-08-19T20:49:09.3929542Z Caused by: java.lang.IllegalStateException: 'unitOfMeasure' does not contain a Coding with a UCUM system of 'http://unitsofmeasure.org' and a valid UCUM code
2020-08-19T20:49:09.3935791Z 	at com.ibm.fhir.model.spec.test.R4ExamplesTest.copyTest(R4ExamplesTest.java:44)
2020-08-19T20:49:09.3936903Z 
2020-08-19T20:49:09.3944208Z [ERROR] serializationTest(com.ibm.fhir.model.spec.test.R4ExamplesTest)  Time elapsed: 43.295 s  <<< FAILURE!
2020-08-19T20:49:09.3956733Z com.ibm.fhir.model.parser.exception.FHIRParserException: 'unitOfMeasure' does not contain a Coding with a UCUM system of 'http://unitsofmeasure.org' and a valid UCUM code [EffectEvidenceSynthesis]
2020-08-19T20:49:09.3962984Z 	at com.ibm.fhir.model.spec.test.R4ExamplesTest.serializationTest(R4ExamplesTest.java:36)
2020-08-19T20:49:09.3973647Z Caused by: java.lang.IllegalStateException: 'unitOfMeasure' does not contain a Coding with a UCUM system of 'http://unitsofmeasure.org' and a valid UCUM code
2020-08-19T20:49:09.3979936Z 	at com.ibm.fhir.model.spec.test.R4ExamplesTest.serializationTest(R4ExamplesTest.java:36)
2020-08-19T20:49:09.3981029Z

prb112 · 2020-08-19T21:01:16Z

fhir-model/src/test/java/com/ibm/fhir/model/util/test/ValidationSupportTest.java

+            fail();
+        }


style consistency - we merge the catch line with the prior line

as a second comment, to this approach, if they are individual tests, then they can succeed and fail independently.

There is a code template and formatting rules for Eclipse here (near the bottom of the page):
https://github.com/IBM/FHIR/wiki/Style-Guide

Will fix style and break into individual testcases.

prb112 · 2020-08-19T21:02:59Z

fhir-tools/src/main/java/com/ibm/fhir/tools/CodeGenerator.java

@@ -1260,6 +1261,8 @@ private String getSyntaxBasedValueSetValidationMethod(String valueSet, String fi
        String suffix = fieldType + (isRepeating ? "s" : "");
        if (ALL_LANG_VALUE_SET_URL.equals(valueSet)) {
            return "checkLanguage" + suffix;
+        } else if (UCUM_UNITS_VALUE_SET_URL.equals(valueSet)) {
+            return "checkUcum" + suffix;


I'm surprised it didn't hit Location as well.

Not sure what you're referring to here - don't see any UCUM value sets used in the Location resource.

I'm just surprised there were not any. It feels like a resource that can benefit from ucum consistency.

prb112 · 2020-08-19T21:04:33Z

fhir-model/src/main/java/com/ibm/fhir/model/util/ValidationSupport.java

+     * @param elementName the element name
+     * @throws IllegalStateException if the passed UCUM codeable concept is not valid
+     */
+    public static void checkUcumCodeableConcept(CodeableConcept ucumCodeableConcept, String elementName) {


It'd be great to turn this on / and off like we've done with some other validation features.
That way we have an opportunity to still extract or tell someone how to extract bad data.

We discussed doing this more widely - something we'd address in issue #1421 - and making sure it applies to the UCUM and language syntax validation as well.

prb112 · 2020-08-19T23:46:23Z

fhir-model/src/main/java/com/ibm/fhir/model/util/ValidationSupport.java

+        if (ucumCoding != null) {
+            if (hasSystemAndCodeValues(ucumCoding)) {
+                if (!UCUM_CODE_SYSTEM_URL.equals(ucumCoding.getSystem().getValue())) {


This could be refactored a little into one if statement and the else could be the checkUcumCode (it feels super nested) also ucumCoding.getSystem is this ever null on 355? maybe just combine this with 354 and 355

I need the if check on line 353 by itself b/c I don't want to return an exception if the coding is null. The if check on line 354 is just to verify that I have values for both system and code. If I don't, I can skip validating those fields separately since I know the coding is not valid without both system and code values. I can move the checkUcumCode to an else leg for the if check on line 355 (ucumCoding.getSystem will never be null here due to the checking in line above in the hasSystemAndCodeValues method)

prb112 · 2020-08-20T00:01:05Z

fhir-model/src/main/java/com/ibm/fhir/model/util/ValidationSupport.java

+            if (ucumCodeableConcept.getCoding() != null) {
+                for (Coding coding : ucumCodeableConcept.getCoding()) {
+                    if (coding != null) {
+                        try {
+                            checkUcumCoding(coding, elementName);
+                            return;
+                        }
+                        catch (IllegalStateException e) {}
+                    }
+                }
+            }
+            throw new IllegalStateException(String.format("'%s' does not contain a Coding with a UCUM system of '%s' and a valid UCUM code", elementName, UCUM_CODE_SYSTEM_URL));


when the for loop succeeds and the coding is checked it can throw an illegalState? I don't think it should return elementName as part of the String.format, it's difficult to back track in the error to the actual failing element

In this case we're just looking for one valid Coding. The checkUcumCoding method will return an IllegalStateException if a coding is not valid, so we want to just catch the exception and continue processing codings until we find a valid one, at which point we return from the method. If no valid codings are found we drop out of the loop and throw the IllegalStateException. For the error msg, do you suggest I just omit the CodeableConcept element name, or should I be providing something else there?

tbieste · 2020-08-21T17:18:08Z

Looks good. Once the testcases are working, I'll approve it.

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

fhir-model/src/main/java/com/ibm/fhir/model/util/ValidationSupport.java

JohnTimm

LGTM

Issue #694 - Perform syntax validation against ucum-units value set

7725fe0

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

michaelwschroeder requested review from prb112, JohnTimm and tbieste August 19, 2020 20:44

prb112 reviewed Aug 19, 2020

View reviewed changes

prb112 reviewed Aug 20, 2020

View reviewed changes

Issue #694 - Update validation per new rules in issue #1421

bca4959

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

tbieste reviewed Aug 21, 2020

View reviewed changes

fhir-model/src/main/java/com/ibm/fhir/model/util/ValidationSupport.java Show resolved Hide resolved

tbieste approved these changes Aug 21, 2020

View reviewed changes

prb112 approved these changes Aug 21, 2020

View reviewed changes

JohnTimm approved these changes Aug 22, 2020

View reviewed changes

JohnTimm merged commit ebb749d into master Aug 22, 2020

JohnTimm deleted the issue-694 branch August 22, 2020 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #694 - Perform syntax validation against ucum-units value set #1430

Issue #694 - Perform syntax validation against ucum-units value set #1430

michaelwschroeder commented Aug 19, 2020

prb112 commented Aug 19, 2020

prb112 Aug 19, 2020

prb112 Aug 19, 2020

JohnTimm Aug 20, 2020

michaelwschroeder Aug 20, 2020

prb112 Aug 19, 2020

michaelwschroeder Aug 20, 2020

prb112 Aug 21, 2020

prb112 Aug 19, 2020

michaelwschroeder Aug 20, 2020

prb112 Aug 19, 2020

michaelwschroeder Aug 20, 2020

prb112 Aug 20, 2020

michaelwschroeder Aug 20, 2020

tbieste commented Aug 21, 2020

JohnTimm left a comment

Issue #694 - Perform syntax validation against ucum-units value set #1430

Issue #694 - Perform syntax validation against ucum-units value set #1430

Conversation

michaelwschroeder commented Aug 19, 2020

prb112 commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbieste commented Aug 21, 2020

JohnTimm left a comment

Choose a reason for hiding this comment