Support "permissive" UIDs in URI. (#49)

* Support "permissive" UIDs in `URI`.

* Include `permissive` flag in `URI.update()` and `URI.from_string()`.

Author: agharwal

src/dicomweb_client/uri.py

@@ -24,6 +24,7 @@ class URISuffix(enum.Enum):
 # For DICOM Standard spec validation of UID components in `URI`.
 _MAX_UID_LENGTH = 64
 _REGEX_UID = re.compile(r'[0-9]+([.][0-9]+)*')
+_REGEX_PERMISSIVE_UID = re.compile(r'[^/@]+')
 
 
 class URI:
@@ -60,12 +61,15 @@ def __init__(self,
                  series_instance_uid: Optional[str] = None,
                  sop_instance_uid: Optional[str] = None,
                  frames: Optional[Sequence[int]] = None,
-                 suffix: Optional[URISuffix] = None):
+                 suffix: Optional[URISuffix] = None,
+                 permissive: bool = False):
         """Instantiates an object.
 
         As per the DICOM Standard, the Study, Series, and Instance UIDs must be
         a series of numeric components (``0``-``9``) separated by the period
         ``.`` character, with a maximum length of 64 characters.
+        If the ``permissive`` flag is set to ``True``, any alpha-numeric or
+        special characters (except for ``/`` and ``@``) may be used.
 
         Parameters
         ----------
@@ -83,6 +87,14 @@ def __init__(self,
         suffix: URISuffix, optional
             Suffix attached to the DICOM resource URI. This could refer to a
             metadata, rendered, or thumbnail resource.
+        permissive: bool
+            If ``True``, relaxes the DICOM Standard validation for UIDs (see
+            main docstring for details). This option is made available since
+            users may be occasionally forced to work with DICOMs or services
+            that may be in violation of the standard. Unless required, use of
+            this flag is **not** recommended, since non-conformant UIDs may
+            lead to unexpected errors downstream, e.g., rejection by a DICOMweb
+            server, etc.
 
         Raises
         ------
@@ -112,6 +124,7 @@ def __init__(self,
         """
         _validate_base_url(base_url)
         _validate_resource_identifiers_and_suffix(
+            permissive,
             study_instance_uid,
             series_instance_uid,
             sop_instance_uid,
@@ -124,6 +137,7 @@ def __init__(self,
         self._instance_uid = sop_instance_uid
         self._frames = None if frames is None else tuple(frames)
         self._suffix = suffix
+        self._permissive = permissive
 
     def __str__(self) -> str:
         """Returns the object as a DICOMweb URI string."""
@@ -207,6 +221,11 @@ def type(self) -> URIType:
             return URIType.INSTANCE
         return URIType.FRAME
 
+    @property
+    def permissive(self) -> bool:
+        """Returns the ``permissive`` parameter value in the initializer."""
+        return self._permissive
+
     def base_uri(self) -> 'URI':
         """Returns `URI` for the DICOM Service within this object."""
         return URI(self.base_url)
@@ -248,7 +267,8 @@ def update(self,
                series_instance_uid: Optional[str] = None,
                sop_instance_uid: Optional[str] = None,
                frames: Optional[Sequence[int]] = None,
-               suffix: Optional[URISuffix] = None) -> 'URI':
+               suffix: Optional[URISuffix] = None,
+               permissive: Optional[bool] = False) -> 'URI':
         """Creates a new `URI` object based on the current one.
 
         Replaces the specified `URI` components in the current `URI` to create
@@ -274,6 +294,9 @@ def update(self,
         suffix: URISuffix, optional
             Suffix to use in the new `URI` or `None` if the `suffix` from the
             current `URI` should be used.
+        permissive: bool, optional
+            Set if permissive handling of UIDs (if any) in the updated ``URI``
+            is required. See the class initializer docstring for details.
 
         Returns
         -------
@@ -297,6 +320,7 @@ def update(self,
             if sop_instance_uid is not None else self.sop_instance_uid,
             frames if frames is not None else self.frames,
             suffix if suffix is not None else self.suffix,
+            permissive if permissive is not None else self.permissive,
         )
 
     @property
@@ -353,7 +377,8 @@ def parent(self) -> 'URI':
     @classmethod
     def from_string(cls,
                     dicomweb_uri: str,
-                    uri_type: Optional[URIType] = None) -> 'URI':
+                    uri_type: Optional[URIType] = None,
+                    permissive: bool = False) -> 'URI':
         """Parses the string to return the URI.
 
         Any valid DICOMweb compatible HTTP[S] URI is permitted, e.g.,
@@ -367,6 +392,9 @@ def from_string(cls,
             The expected DICOM resource type referenced by the object. If set,
             it validates that the resource-scope of the `dicomweb_uri` matches
             the expected type.
+        permissive: bool
+            Set if permissive handling of UIDs (if any) in ``dicomweb_uri`` is
+            required. See the class initializer docstring for details.
 
         Returns
         -------
@@ -428,7 +456,7 @@ def from_string(cls,
                         f'URI: {dicomweb_uri!r}')
 
         uri = cls(base_url, study_instance_uid, series_instance_uid,
-                  sop_instance_uid, frames, suffix)
+                  sop_instance_uid, frames, suffix, permissive)
         # Validate that the URI is of the specified type, if applicable.
         if uri_type is not None and uri.type != uri_type:
             raise ValueError(
@@ -450,6 +478,7 @@ def _validate_base_url(url: str) -> None:
 
 
 def _validate_resource_identifiers_and_suffix(
+        permissive: bool,
         study_instance_uid: Optional[str],
         series_instance_uid: Optional[str],
         sop_instance_uid: Optional[str],
@@ -473,7 +502,7 @@ def _validate_resource_identifiers_and_suffix(
 
     for uid in (study_instance_uid, series_instance_uid, sop_instance_uid):
         if uid is not None:
-            _validate_uid(uid)
+            _validate_uid(uid, permissive)
 
     if suffix in (URISuffix.RENDERED, URISuffix.THUMBNAIL) and (
             study_instance_uid is None):
@@ -487,13 +516,17 @@ def _validate_resource_identifiers_and_suffix(
                          'resources: Study, Series, or SOP Instance UID')
 
 
-def _validate_uid(uid: str) -> None:
+def _validate_uid(uid: str, permissive: bool) -> None:
     """Validates a DICOM UID."""
     if len(uid) > _MAX_UID_LENGTH:
         raise ValueError('UID cannot have more than 64 chars. '
                          f'Actual count in {uid!r}: {len(uid)}')
-    if _REGEX_UID.fullmatch(uid) is None:
-        raise ValueError(f'UID {uid!r} must match regex {_REGEX_UID!r}.')
+    if not permissive and _REGEX_UID.fullmatch(uid) is None:
+        raise ValueError(f'UID {uid!r} must match regex {_REGEX_UID!r} in '
+                         'conformance with the DICOM Standard.')
+    elif permissive and _REGEX_PERMISSIVE_UID.fullmatch(uid) is None:
+        raise ValueError(f'Permissive mode is enabled. UID {uid!r} must match '
+                         f'regex {_REGEX_PERMISSIVE_UID!r}.')
 
 
 def _validate_frames(frames: Sequence[int]) -> None:

tests/test_uri.py

@@ -18,22 +18,22 @@
 @pytest.mark.parametrize('illegal_char', ['/', '@', 'a', 'A'])
 def test_uid_illegal_character(illegal_char):
     """Checks *ValueError* is raised when a UID contains an illegal char."""
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, f'1.2{illegal_char}3')
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, '1.2.3', f'4.5{illegal_char}6')
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, '1.2.3', '4.5.6', f'7.8{illegal_char}9')
 
 
 @pytest.mark.parametrize('illegal_uid', ['.23', '1.2..4', '1.2.', '.'])
 def test_uid_illegal_format(illegal_uid):
     """Checks *ValueError* is raised if a UID is in an illegal format."""
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, illegal_uid)
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, '1.2.3', illegal_uid)
-    with pytest.raises(ValueError, match='must match'):
+    with pytest.raises(ValueError, match='in conformance'):
         URI(_BASE_URL, '1.2.3', '4.5.6', illegal_uid)
 
 
@@ -55,6 +55,21 @@ def test_uid_length():
         URI(_BASE_URL, '1.2.3', '4.5.6', uid_65)
 
 
+@pytest.mark.parametrize('uid', ['13-abc', 'hello', 'is it', 'me you\'re'])
+def test_uid_permissive_valid(uid):
+    """Tests valid "permissive" UIDs are accommodated iff the flag is set."""
+    with pytest.raises(ValueError, match='in conformance'):
+        URI(_BASE_URL, uid, permissive=False)
+    URI(_BASE_URL, uid, permissive=True)
+
+
+@pytest.mark.parametrize('uid', ['1.23.5@', '1/23.4'])
+def test_uid_permissive_invalid(uid):
+    """Tests that invalid "permissive" UIDs are rejected."""
+    with pytest.raises(ValueError, match='Permissive mode'):
+        URI(_BASE_URL, uid, permissive=True)
+
+
 def test_uid_missing_error():
     """Checks *ValueError* is raised when an expected UID is missing."""
     with pytest.raises(ValueError, match='`study_instance_uid` missing with'):
@@ -417,6 +432,27 @@ def test_update(uri_args, update_args, expected_uri_args):
     assert actual_uri == expected_uri
 
 
+@pytest.mark.parametrize('original,update,expected', [
+    (None, None, False),
+    (None, False, False),
+    (None, True, True),
+    (False, None, False),
+    (True, None, True),
+    (False, False, False),
+    (False, True, True),
+    (True, False, False),
+    (True, True, True),
+])
+def test_update_permissive(original, update, expected):
+    """Tests for the expected value of `permissive` flag in `URI.update()`."""
+    if original is None:
+        original_uri = URI(_BASE_URL)
+    else:
+        original_uri = URI(_BASE_URL, permissive=original)
+    updated_uri = original_uri.update(permissive=update)
+    assert updated_uri.permissive == expected
+
+
 @pytest.mark.parametrize('uri_args,update_args,error_msg', [
     ((_BASE_URL, ), (None, None, '1', None, None),
      '`study_instance_uid` missing'),