From 5b6f3d57ab71ce609fc961b5156f0f49142a1562 Mon Sep 17 00:00:00 2001 From: Samuel Kwong Date: Tue, 29 Sep 2026 00:48:30 +0200 Subject: [PATCH] Stop mass transfer partitions failing on an empty StudyTime pydicom's TM("") and DA("") return None, so convert_to_python_time and convert_to_python_date hit their assert on an empty value and raised a bare AssertionError. The handlers added in #400 only catch ValueError, so one study with an empty StudyTime failed its whole mass transfer partition, with an empty task message and again on every retry. StudyDate and StudyTime are Type 2 and may legitimately be empty, so this is PACS input rather than an internal invariant. The converters now raise ValueError for empty values, as pydicom does for malformed ones: an empty StudyTime falls back to midnight and an empty StudyDate skips only that study. Co-Authored-By: Claude Opus 5.5 --- adit/core/tests/utils/test_dicom_utils.py | 10 ++++++++++ adit/core/utils/dicom_utils.py | 16 ++++++++++++---- adit/mass_transfer/tests/test_processor.py | 16 ++++++++++++---- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/adit/core/tests/utils/test_dicom_utils.py b/adit/core/tests/utils/test_dicom_utils.py index 2fe924c2..1052aa0d 100644 --- a/adit/core/tests/utils/test_dicom_utils.py +++ b/adit/core/tests/utils/test_dicom_utils.py @@ -61,10 +61,20 @@ def test_convert_to_python_date(): assert convert_to_python_date("20230831") == date(2023, 8, 31) +def test_convert_to_python_date_rejects_empty_value(): + with pytest.raises(ValueError): + convert_to_python_date("") + + def test_convert_to_python_time(): assert convert_to_python_time("080000") == time(8, 0, 0) +def test_convert_to_python_time_rejects_empty_value(): + with pytest.raises(ValueError): + convert_to_python_time("") + + def test_convert_to_python_regex_exact_match(): # No wildcard = exact whole-value match (DICOM Single Value Matching) pattern = convert_to_python_regex("query") diff --git a/adit/core/utils/dicom_utils.py b/adit/core/utils/dicom_utils.py index 9f60e47d..5c5703d8 100644 --- a/adit/core/utils/dicom_utils.py +++ b/adit/core/utils/dicom_utils.py @@ -143,16 +143,24 @@ def convert_to_dicom_datetime(value: str | datetime.datetime | DateTimeRange) -> def convert_to_python_date(value: str) -> datetime.date: - """Convert a DICOM date string to a Python date object.""" + """Convert a DICOM date string to a Python date object. + + Raises ValueError if the value is empty or invalid. + """ date_valuerep = valuerep.DA(value) - assert date_valuerep is not None + if date_valuerep is None: + raise ValueError(f"Empty DICOM date: {value!r}") return datetime.date.fromisoformat(date_valuerep.isoformat()) def convert_to_python_time(value: str) -> datetime.time: - """Convert a DICOM date string to a Python date object.""" + """Convert a DICOM time string to a Python time object. + + Raises ValueError if the value is empty or invalid. + """ time_valuerep = valuerep.TM(value) - assert time_valuerep is not None + if time_valuerep is None: + raise ValueError(f"Empty DICOM time: {value!r}") return datetime.time.fromisoformat(time_valuerep.isoformat()) diff --git a/adit/mass_transfer/tests/test_processor.py b/adit/mass_transfer/tests/test_processor.py index b662f98b..0631e38b 100644 --- a/adit/mass_transfer/tests/test_processor.py +++ b/adit/mass_transfer/tests/test_processor.py @@ -699,8 +699,17 @@ def test_discover_series_exclude_by_institution(mocker: MockerFixture): assert series_uids == {"1.2.3.901"} -def test_discover_series_salvages_malformed_study_time(mocker: MockerFixture): - """A non-conformant StudyTime (as returned by some PACS) falls back to midnight.""" +@pytest.mark.parametrize( + "study_time", + [ + # TM only allows a fractional part after full HHMMSS, so HHMM.fff is invalid + pytest.param("1113.672", id="non-conformant"), + # StudyTime is Type 2, so a PACS may return it empty + pytest.param("", id="empty"), + ], +) +def test_discover_series_salvages_unparseable_study_time(mocker: MockerFixture, study_time: str): + """A StudyTime that can't be parsed (as returned by some PACS) falls back to midnight.""" processor = _make_processor(mocker) processor.mass_task.partition_start = datetime(2024, 1, 1, 0, 0) processor.mass_task.partition_end = datetime(2024, 1, 1, 23, 59, 59) @@ -709,8 +718,7 @@ def test_discover_series_salvages_malformed_study_time(mocker: MockerFixture): operator.server = mocker.MagicMock(max_search_results=200) study = _make_study("1.2.3.100") - # TM only allows a fractional part after full HHMMSS, so HHMM.fff is invalid - _set_nonconformant(study.dataset, "StudyTime", "1113.672") + _set_nonconformant(study.dataset, "StudyTime", study_time) operator.find_studies.return_value = [study] operator.find_series.return_value = [_make_series_result("1.2.3.201")]