Merge branch '#49_fix_suid_only_query_handling'

Author: sjoerdk

dicomtrolley/core.py

@@ -556,7 +556,7 @@ def init_from_query(
                 f"parameters. You are probably converting between incompatible "
                 f"query subtypes. Use a basic Query instance to avoid this error. "
                 f"See ValidationError for details on which parameter is causing "
-                f"the problem"
+                f"the problem. Original error: {str(e)}"
             ) from e
 
     @staticmethod

dicomtrolley/qido_rs.py

@@ -122,17 +122,14 @@ def uri_search_params(self) -> Dict[str, Union[str, List[str]]]:
         if self.include_fields:
             search_params["includefield"] = self.include_fields
 
-        # now collect all other Query() fields that can be search params
+        # now collect all other Query() fields that can be search params.
+        # Exclude fields that should not be sent to server
         exclude_fields = {
             "min_study_date",  # addressed above
             "max_study_date",
-            "include_fields",
-            "StudyInstanceUID",  # potential part of url, not search params
-            "SeriesInstanceUID",
-            "SOPClassInstanceUID",
-            "query_level",
+            "include_fields",  # addressed above
+            "query_level",  # encoded in url structure
         }
-
         other_search_params = {
             key: val
             for key, val in self.dict().items()
@@ -235,6 +232,44 @@ def uri_base(self) -> str:
                 f'Should be one of "{QueryLevels}"'
             )
 
+    def uri_search_params(self) -> Dict[str, Union[str, List[str]]]:
+        """The search parameter part of the URI as defined in
+        DICOM PS3.18 section 10.6 table 10.6.1-2
+
+        Returns
+        -------
+        Dict[str, Union[str, List[str]]]
+            Output that can be fed directly into a requests post request.
+            format is param_name:param_value. If param_value is a list, the param
+            will be included multiple times.
+            See https://docs.python-requests.org/en/latest/user/quickstart/
+            #passing-parameters-in-urls
+
+        Notes
+        -----
+        Will not output any parameters with Null or empty value (bool(value)==False).
+        This does not affect query functionality but makes output cleaner in strings
+        """
+        search_params: Dict[
+            str, Union[str, List[str]]
+        ] = super().uri_search_params()
+
+        # Depending on query level, some UIDs in Hierarchical queries are part
+        # of url and should not be part of parameter
+        exclude_fields = set()
+        if self.query_level == QueryLevels.INSTANCE:
+            exclude_fields = {"StudyInstanceUID", "SeriesInstanceUID"}
+        if self.query_level == QueryLevels.SERIES:
+            exclude_fields = {"StudyInstanceUID"}
+        if self.query_level == QueryLevels.STUDY:
+            pass  # for series level all uids are part of parameters. Don't exclude
+
+        return {
+            key: val
+            for key, val in search_params.items()
+            if key not in exclude_fields
+        }
+
 
 # used in RelationalQuery
 STUDY_VALUE_ERROR_TEXT: str = (
@@ -287,7 +322,7 @@ def uri_base(self) -> str:
         if self.query_level == QueryLevels.STUDY:
             raise ValueError(STUDY_VALUE_ERROR_TEXT)
         elif self.query_level == QueryLevels.SERIES:
-            return "/studies"
+            return "/series"
         elif self.query_level == QueryLevels.INSTANCE:
             if self.StudyInstanceUID:
                 # all instances for this study
@@ -364,10 +399,11 @@ def ensure_query_type(cls, query: Query) -> QidoRSQueryBase:
                     "to HierarchicalQuery"
                 )
                 return HierarchicalQuery.init_from_query(query)
-            except UnSupportedParameterError:
+            except UnSupportedParameterError as e:
                 logger.debug(
                     "Converting to HierarchicalQuery did not work. "
-                    "Trying slower but less stringent RelationalQuery"
+                    "Trying slower but less stringent RelationalQuery. Error was: "
+                    f"{str(e)}"
                 )
                 return RelationalQuery.init_from_query(query)
         else:

tests/test_qido_rs.py

@@ -2,7 +2,7 @@
 
 import pytest
 
-from dicomtrolley.core import QueryLevels
+from dicomtrolley.core import Query, QueryLevels
 from dicomtrolley.qido_rs import HierarchicalQuery, QidoRS, RelationalQuery
 from tests.conftest import set_mock_response
 from tests.mock_responses import (
@@ -60,6 +60,47 @@ def test_hierarchical_query_uris():
     assert url == "/studies/123/series"
 
 
+@pytest.mark.parametrize(
+    "query,expected_url",
+    [
+        # For this basic study-level query suid should be a parameter
+        (
+            HierarchicalQuery(
+                StudyInstanceUID="123", query_level=QueryLevels.STUDY
+            ),
+            "/qido/studies?StudyInstanceUID=123",
+        ),
+        # But for series-level query suid becomes part of the path, not parameter
+        (
+            HierarchicalQuery(
+                StudyInstanceUID="123", query_level=QueryLevels.SERIES
+            ),
+            "/qido/studies/123/series",
+        ),
+    ],
+)
+def test_hierarchical_query_uri_suid_only(
+    requests_mock, a_qido, query, expected_url
+):
+    """Exposes issue #49 - Query containing SeriesInstanceUID only is not
+    translated properly. This test check more broadly how hierarchical queries
+    generate they urls to make sure this is sane.
+    """
+    # Return a valid response to avoid errors. We're only interested in the called url
+    set_mock_response(requests_mock, QIDO_RS_STUDY_LEVEL)
+
+    # Simple query for a single StudyInstanceUID
+    a_qido.find_studies(query)
+
+    # This should translate to a qido url call including the StudyInstanceUID
+    hist = requests_mock.request_history
+    assert len(hist) == 1  # sanity check
+    called = hist[0].path
+    if hist[0].query:
+        called = called + "?" + hist[0].query
+    assert called.lower() == expected_url.lower()
+
+
 @pytest.mark.parametrize(
     "query_params",
     [
@@ -117,3 +158,11 @@ def test_qido_searcher_204(requests_mock, a_qido):
     set_mock_response(requests_mock, QIDO_RS_204_NO_RESULTS)
     result = a_qido.find_studies(HierarchicalQuery())
     assert len(result) == 0
+
+
+def test_ensure_query_type(a_qido):
+    """Exposes bug where Series level Relational query calles a study url"""
+    ensured = a_qido.ensure_query_type(
+        Query(AccessionNumber="123", query_level=QueryLevels.SERIES)
+    )
+    assert ensured.uri_base() == "/series"