feat: use uri's _validate_uid method in web. Skip uid lenght validation in permissive mode.

Author: jaya-krishnan-sr

src/dicomweb_client/uri.py

@@ -629,12 +629,13 @@ def _validate_resource_identifiers_and_suffix(
 
 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 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.')
+    if not permissive:
+        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} 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}.')

src/dicomweb_client/web.py

@@ -32,15 +32,11 @@
 import requests
 import retrying
 
-from dicomweb_client.uri import build_query_string, parse_query_parameters
+from dicomweb_client.uri import build_query_string, parse_query_parameters, _validate_uid
 
 
 logger = logging.getLogger(__name__)
 
-# For DICOM Standard spec validation of UID components in `DICOMwebClient`.
-_REGEX_UID = re.compile(r'[0-9]+([.][0-9]+)*')
-_REGEX_PERMISSIVE_UID = re.compile(r'[^/@]+')
-
 
 def _load_xml_dataset(dataset: Element) -> pydicom.dataset.Dataset:
     """Load DICOM Data Set in DICOM XML format.
@@ -205,7 +201,7 @@ def __init__(
         headers: Optional[Dict[str, str]] = None,
         callback: Optional[Callable] = None,
         chunk_size: int = 10**6,
-        permissive: bool = False
+        permissive_uid: bool = False
     ) -> None:
         """Instatiate client.
 
@@ -327,7 +323,7 @@ def __init__(
         if callback is not None:
             self._session.hooks = {'response': [callback, ]}
         self._chunk_size = chunk_size
-        self._permissive = permissive
+        self._permissive_uid = permissive_uid
         self.set_http_retry_params()
 
     def _get_transaction_url(self, transaction: _Transaction) -> str:
@@ -2177,31 +2173,6 @@ def delete_study(
             url += f'?{additional_params_query_string}'
         self._http_delete(url)
 
-    def _assert_uid_format(self, uid: str) -> None:
-        """Check whether a DICOM UID has the correct format.
-
-        Parameters
-        ----------
-        uid: str
-            DICOM UID
-
-        Raises
-        ------
-        TypeError
-            When `uid` is not a string
-        ValueError
-            When `uid` doesn't match the regular expression pattern for
-            DICOM UIDs (strict or permissive regex)
-
-        """
-        if not isinstance(uid, str):
-            raise TypeError('DICOM UID must be a string.')
-        if not self._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 self._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 search_for_series(
         self,
@@ -2255,7 +2226,7 @@ def search_for_series(
 
         """  # noqa: E501
         if study_instance_uid is not None:
-            self._assert_uid_format(study_instance_uid)
+            _validate_uid(study_instance_uid, self._permissive_uid)
             logger.info(f'search for series of study "{study_instance_uid}"')
         else:
             logger.info('search for series')
@@ -2314,12 +2285,12 @@ def _get_series(
             f'retrieve series "{series_instance_uid}" '
             f'of study "{study_instance_uid}"'
         )
-        self._assert_uid_format(study_instance_uid)
+        _validate_uid(study_instance_uid, self._permissive_uid)
         if series_instance_uid is None:
             raise ValueError(
                 'Series Instance UID is required for retrieval of series.'
             )
-        self._assert_uid_format(series_instance_uid)
+        _validate_uid(series_instance_uid, self._permissive_uid)
         url = self._get_series_url(
             _Transaction.RETRIEVE,
             study_instance_uid,
@@ -2474,7 +2445,7 @@ def retrieve_series_metadata(
                 'Study Instance UID is required for retrieval of '
                 'series metadata.'
             )
-        self._assert_uid_format(study_instance_uid)
+        _validate_uid(study_instance_uid, self._permissive_uid)
         if series_instance_uid is None:
             raise ValueError(
                 'Series Instance UID is required for retrieval of '
@@ -2484,7 +2455,7 @@ def retrieve_series_metadata(
             f'retrieve metadata of series "{series_instance_uid}" '
             f'of study "{study_instance_uid}"'
         )
-        self._assert_uid_format(series_instance_uid)
+        _validate_uid(series_instance_uid, self._permissive_uid)
         url = self._get_series_url(
             _Transaction.RETRIEVE,
             study_instance_uid,
@@ -2677,7 +2648,7 @@ def search_for_instances(
         if series_instance_uid is not None:
             message += f' of series "{series_instance_uid}"'
         if study_instance_uid is not None:
-            self._assert_uid_format(study_instance_uid)
+            _validate_uid(study_instance_uid, self._permissive_uid)
             message += f' of study "{study_instance_uid}"'
         logger.info(message)
         url = self._get_instances_url(
@@ -2744,12 +2715,12 @@ def retrieve_instance(
             raise ValueError(
                 'Study Instance UID is required for retrieval of instance.'
             )
-        self._assert_uid_format(study_instance_uid)
+        _validate_uid(study_instance_uid, self._permissive_uid)
         if series_instance_uid is None:
             raise ValueError(
                 'Series Instance UID is required for retrieval of instance.'
             )
-        self._assert_uid_format(series_instance_uid)
+        _validate_uid(series_instance_uid, self._permissive_uid)
         if sop_instance_uid is None:
             raise ValueError(
                 'SOP Instance UID is required for retrieval of instance.'
@@ -2759,7 +2730,7 @@ def retrieve_instance(
             f'of series "{series_instance_uid}" '
             f'of study "{study_instance_uid}"'
         )
-        self._assert_uid_format(sop_instance_uid)
+        _validate_uid(sop_instance_uid, self._permissive_uid)
         url = self._get_instances_url(
             _Transaction.RETRIEVE,
             study_instance_uid,

tests/test_uri.py

@@ -70,6 +70,21 @@ def test_uid_permissive_invalid(uid):
         URI(_BASE_URL, uid, permissive=True)
 
 
+def test_uid_length_permissive():
+    """Tests that UIDs longer than 64 chars are allowed in permissive mode."""
+    # Create a UID with 65 characters
+    uid_65 = '1' * 65
+
+    # Should fail in strict mode
+    with pytest.raises(ValueError, match='UID cannot have more than 64 chars'):
+        URI(_BASE_URL, uid_65, permissive=False)
+
+    # Should succeed in permissive mode
+    URI(_BASE_URL, uid_65, permissive=True)
+    URI(_BASE_URL, '1.2.3', uid_65, permissive=True)
+    URI(_BASE_URL, '1.2.3', '4.5.6', uid_65, 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'):

tests/test_web.py

@@ -1614,16 +1614,16 @@ def test_load_xml_response(httpserver, client, cache_dir):
     '1.2/3.4', '[email protected]', '1.2.a.4', '1.2.A.4', '.23.4', '1.2..4', '1.2.', '.'
 ])
 def test_uid_strict_validation_fail(httpserver, invalid_uid):
-    client = DICOMwebClient(httpserver.url, permissive=False)
+    client = DICOMwebClient(httpserver.url, permissive_uid=False)
     with pytest.raises(ValueError,
                        match=f'UID {invalid_uid!r} must match regex'):
         client.search_for_series(study_instance_uid=invalid_uid)
 
 
 @pytest.mark.parametrize('uid', ['13-abc', 'hello', 'is it', "you're me"])
 def test_uid_permissive_validation_pass(httpserver, uid):
-    client_strict = DICOMwebClient(httpserver.url, permissive=False)
-    client_permissive = DICOMwebClient(httpserver.url, permissive=True)
+    client_strict = DICOMwebClient(httpserver.url, permissive_uid=False)
+    client_permissive = DICOMwebClient(httpserver.url, permissive_uid=True)
 
     # Should fail in strict mode
     with pytest.raises(ValueError, match=f'UID {uid!r} must match regex'):
@@ -1636,7 +1636,7 @@ def test_uid_permissive_validation_pass(httpserver, uid):
 
 @pytest.mark.parametrize('uid', ['1.23.5@', '1/23.4'])
 def test_uid_permissive_validation_fail(httpserver, uid):
-    client = DICOMwebClient(httpserver.url, permissive=True)
+    client = DICOMwebClient(httpserver.url, permissive_uid=True)
     with pytest.raises(ValueError,
                        match=f'UID {uid!r} must match regex'):
         client.search_for_series(study_instance_uid=uid)