Fix handling of missing values

mrbean-bremen · mrbean-bremen · commit 6ce341a7c6f6 · 2023-07-22T19:35:09.000+02:00
- if the values for a condition needing a value cannot be parsed, the condition is now ignored - fixes pydicom#20 - add handling of a few more simple conditions
diff --git a/CHANGES.md b/CHANGES.md
@@ -4,12 +4,15 @@ The released versions correspond to PyPi releases.
 ## [Version 0.4.0] (Unreleased)
 
 ### Fixes
-* condition parser: multiple or expressions handled correctly
+* Condition parser: multiple or expressions handled correctly
 * Condition parser: handle a few more complicated conditions
 * Condition parser: handle conditions without values,
   see [#15](../.. /issues/15)
 * Condition parser: fixed handling of "zero" and "non-zero" values
   see [#17](../.. /issues/17)
+* Condition parser: handle a few more simple expressions
+* Condition parser: ignore equality conditions with missing values
+  (caused crash, see [#20](../.. /issues/20)
 
 ### Changes
 * Removed support for Python 3.6
diff --git a/dicom_validator/spec_reader/condition.py b/dicom_validator/spec_reader/condition.py
@@ -15,6 +15,8 @@ class Condition:
                 otherwise not
             'MU': the object is mandatory if the condition is fulfilled,
                 otherwise is user defined
+            'MC': the object is mandatory if the condition is fulfilled,
+                otherwise another condition will be checked
         tag: the ID of the required tag in the form '(####,####)' or None
         index: the index of the tag for multi-valued tags or 0
         values: a list of values the tag shall have if the condition
@@ -38,6 +40,9 @@ def __init__(self, ctype: Optional[str] = None,
         self.or_conditions: List[Condition] = []
         self.other_condition: Optional[Condition] = None
 
+    def __repr__(self):
+        return f"Condition type={self.type} op='{self.operator}' tag={self.tag} values={self.values}"
+
     @classmethod
     def read_condition(cls, condition_dict: Dict,
                        condition: Optional["Condition"] = None) -> "Condition":
diff --git a/dicom_validator/spec_reader/condition_parser.py b/dicom_validator/spec_reader/condition_parser.py
@@ -31,10 +31,13 @@ class ConditionParser:
         r'(?P<id>\([\dA-Fa-f]{4},[\dA-Fa-f]{4}\))?(,? Value (?P<index>\d))?$')
 
     operators = OrderedDict([
+        (' is present and the value is ', '='),
+        (' is present and has a value of ', '='),
         (' is greater than ', '>'),
         (' is present and equals ', '='),
         (' is present with a value of ', '='),
-        (' value is ', '='),
+        (' is set to ', '='),
+        (' equals one of the following values: ', '='),
         (' has a value of more than ', '>'),
         (' has a value greater than ', '>'),
         (' has a value of ', '='),
@@ -43,16 +46,17 @@ class ConditionParser:
         (' equals other than ', '!='),
         (' equals ', '='),
         (' is other than ', '!='),
-        (' is present and the value is ', '='),
-        (' is present and has a value of ', '='),
         (' is one of the following: ', '='),
         (' is present and has a value', '++'),
         (' is present', '+'),
+        (' value is not ', '!='),
+        (' value is ', '='),
         (' is sent', '+'),
         (' is not sent', '-'),
         (' is not present', '-'),
         (' is absent', '-'),
         (' is not equal to ', '!='),
+        (' is equal to ', '='),
         (' is not ', '!='),
         (' is ', '='),
         (' is: ', '='),
@@ -115,9 +119,18 @@ def _parse_tag_expression(
         if operator in ('=', '!=', '>', '<'):
             values, rest = self._parse_tag_values(rest)
             # fixup special values
-            if values and values[0].startswith('non-zero'):
-                operator = '!='
-                values = ['0'] if values[0] == 'non-zero' else ['']
+            if values:
+                if values[0].startswith('non-zero'):
+                    operator = '!='
+                    values = ['0'] if values[0] == 'non-zero' else ['']
+                elif values[0].startswith('non-null'):
+                    operator = '++'
+                    values = []
+                elif values[0].startswith('zero-length'):
+                    values = ['']
+            else:
+                # failed to parse mandatory values - ignore the condition
+                return Condition(ctype='U'), None
         elif operator == '=>':
             value_string, rest = self._split_value_part(rest)
             tag, _ = self._parse_tag(value_string)
@@ -203,7 +216,7 @@ def _parse_tag_values(
     def _split_value_part(self, value_string: str) -> Tuple[str, str]:
         value_string = value_string.strip()
         end_index = self._end_index_for_stop_chars(
-            value_string, [';', '.', ', and ', ' and '])
+            value_string, [';', '.', ', and ', ' and ', ':'])
         return value_string[:end_index], value_string[end_index:]
 
     @staticmethod
@@ -232,13 +245,17 @@ def _get_const_value(self, value: str) -> Tuple[Optional[str], str]:
         value = value.strip()
         if value[0] == value[-1] == '"':
             return value[1:-1], ''
-        if re.match('^[A-Z0-9_ ]+$', value) is not None:
+        if re.match('^[A-Z0-9][A-Za-z0-9_ ]*$', value) is not None:
             return value, ''
+        # sometimes a value explanation is present in scopes
+        match = re.match(r'^([A-Z0-9_ ]+)\([A-Za-z ]+\)+$', value)
+        if match is not None:
+            return match.group(1).strip(), ''
         if value == 'zero length':
             return '', ''
         if value == 'zero':
             return '0', ''
-        if value in ('non-zero', 'non-zero length'):
+        if value in ('non-zero', 'non-zero length', 'non-null', 'zero-length'):
             return value, ''
         match = re.match(r'^.* \(\"([\d.]+)\"\)(.*)$', value)
         if match is not None:
diff --git a/dicom_validator/spec_reader/tests/test_condition_parser.py b/dicom_validator/spec_reader/tests/test_condition_parser.py
@@ -45,15 +45,13 @@ def test_ignore_condition_without_tag(self):
         )
         self.assertEqual('U', result.type)
 
-    def test_handle_condition_without_value(self):
+    def test_ignore_condition_without_value(self):
         # regression test for #15
         result = self.parser.parse(
             'required if Selector Attribute (0072,0026) is nested in '
             'one or more Sequences or is absent.'
         )
-        self.assertEqual('MN', result.type)
-        self.assertEqual('(0072,0026)', result.tag)
-        self.assertEqual('=', result.operator)
+        self.assertEqual('U', result.type)
 
 
 class SimpleConditionParserTest(ConditionParserTest):
@@ -86,6 +84,42 @@ def test_is_present(self):
         self.assertEqual('+', result.operator)
         self.assertEqual([], result.values)
 
+    def test_equal_sign(self):
+        result = self.parser.parse(
+            'Required if Pixel Component Organization = Bit aligned.'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('(0018,6044)', result.tag)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['Bit aligned'], result.values)
+
+    def test_value_has_explanation(self):
+        result = self.parser.parse(
+            'Required if Conversion Type (0008,0064) is DF (Digitized Film).'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('(0008,0064)', result.tag)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['DF'], result.values)
+
+    def test_values_have_explanation(self):
+        result = self.parser.parse(
+            'Required if Conversion Type (0008,0064) is SD (Scanned Document) or SI (Scanned Image).'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('(0008,0064)', result.tag)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['SD', 'SI'], result.values)
+
+    def test_is_with_colon_after_value(self):
+        result = self.parser.parse(
+            'Required if the value of Reformatting Operation Type (0072,0510) is 3D_RENDERING:'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('(0072,0510)', result.tag)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['3D_RENDERING'], result.values)
+
     def test_is_present_with_value(self):
         result = self.parser.parse(
             'Required if Responsible Person is present and has a value.'
@@ -95,6 +129,15 @@ def test_is_present_with_value(self):
         self.assertEqual('++', result.operator)
         self.assertEqual([], result.values)
 
+    def test_is_set_to(self):
+        result = self.parser.parse(
+            'Required if Ophthalmic Volumetric Properties Flag (0022,1622) is set to YES. May be present otherwise.'
+        )
+        self.assertEqual('MU', result.type)
+        self.assertEqual('(0022,1622)', result.tag)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['YES'], result.values)
+
     def test_is_present_tag_name_with_digit(self):
         result = self.parser.parse(
             'Required if 3D Mating Point (0068,64C0) is present.'
@@ -135,6 +178,14 @@ def test_required_only_if(self):
         self.assertEqual('-', result.operator)
         self.assertEqual([], result.values)
 
+    def test_values_failed_parsing(self):
+        """Regression test for #20 (if the value could not be parsed ignore the condition)"""
+        result = self.parser.parse(
+            'Required if Constraint Violation Significance (0082,0036) '
+            'is only significant under certain conditions.'
+        )
+        self.assertEqual('U', result.type)
+
 
 class ValueConditionParserTest(ConditionParserTest):
     def test_equality_tag(self):
@@ -318,6 +369,14 @@ def test_present_with_value(self):
         self.assertEqual('=', result.operator)
         self.assertEqual(['AS'], result.values)
 
+    def test_value_is_not(self):
+        result = self.parser.parse(
+            'Required if Shadow Style (0070,0244) value is not OFF.'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('!=', result.operator)
+        self.assertEqual(['OFF'], result.values)
+
     def test_other_than(self):
         result = self.parser.parse(
             'Required if Decay Correction (0054,1102) is other than NONE.'
@@ -335,6 +394,14 @@ def test_not_equal_to(self):
         self.assertEqual('!=', result.operator)
         self.assertEqual(['UNDEFINED'], result.values)
 
+    def test_equal_to(self):
+        result = self.parser.parse(
+            'Required if Blending Mode (0070,1B06) is equal to FOREGROUND.'
+        )
+        self.assertEqual('MN', result.type)
+        self.assertEqual('=', result.operator)
+        self.assertEqual(['FOREGROUND'], result.values)
+
     def test_present_with_value_of(self):
         result = self.parser.parse(
             'Required if Partial View '
@@ -361,6 +428,26 @@ def test_non_zero(self):
         self.assertEqual('!=', result.operator)
         self.assertEqual(['0'], result.values)
 
+    def test_non_null(self):
+        result = self.parser.parse(
+            'Required if value Transfer Tube Number (300A,02A2) is non-null.'
+        )
+        assert result.type == 'MN'
+        assert result.tag == '(300A,02A2)'
+        assert result.operator == '++'
+        self.assertEqual([], result.values)
+
+    def test_zero_length(self):
+        result = self.parser.parse(
+            'Required if Material ID (300A,00E1) is zero-length. '
+            'May be present if Material ID (300A,00E1) is non-zero length.'
+        )
+        assert result.type == 'MC'
+        assert result.tag == '(300A,00E1)'
+        assert result.operator == '='
+        self.assertEqual([''], result.values)
+        assert result.other_condition is not None
+
     def test_greater_than_zero(self):
         result = self.parser.parse(
             'Required if Number of Beams (300A,0080) is greater than zero'
diff --git a/dicom_validator/tests/fixtures/2021d/json/dict_info.json b/dicom_validator/tests/fixtures/2021d/json/dict_info.json
@@ -47,6 +47,12 @@
     "vm": "1",
     "vr": "PN"
   },
+  "(0018,6044)": {
+    "name": "Pixel Component Organization",
+    "prop": "",
+    "vm": "1",
+    "vr": "US"
+  },
   "(0028,0008)": {
     "name": "Number of Frames",
     "prop": "",
