From 82f6b8d6363152502d13676cec6d5f28328aa86f Mon Sep 17 00:00:00 2001 From: Rakesh Date: Wed, 11 Feb 2026 12:02:51 -0500 Subject: [PATCH 1/4] Handle missing external dictionaries without crashing and log as execution errors --- scripts/script_utils.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts/script_utils.py b/scripts/script_utils.py index b08035d3a..83bc68476 100644 --- a/scripts/script_utils.py +++ b/scripts/script_utils.py @@ -27,6 +27,7 @@ from cdisc_rules_engine.services.define_xml.define_xml_reader_factory import ( DefineXMLReaderFactory, ) +from cdisc_rules_engine.exceptions.custom_exceptions import MissingDataError def get_library_metadata_from_cache(args) -> LibraryMetadataContainer: # noqa @@ -194,7 +195,16 @@ def fill_cache_with_dictionaries( f'MAIN/{dictionary_path.get("edition")}/{dictionary_path.get("version")}' ) continue - terms = extract_dictionary_terms(data_service, dictionary_type, dictionary_path) + try: + terms = extract_dictionary_terms( + data_service, dictionary_type, dictionary_path + ) + except MissingDataError as e: + engine_logger.warning( + f"External dictionary '{dictionary_type}' at '{dictionary_path}' " + f"could not be loaded and will be skipped: {getattr(e, 'message', str(e))}" + ) + continue cache.add(dictionary_path, terms) versions_map[dictionary_type] = terms.version From b935f654098363a621823947e585be812cb32a58 Mon Sep 17 00:00:00 2001 From: Rakesh Date: Fri, 13 Feb 2026 17:01:55 -0500 Subject: [PATCH 2/4] Add parametrized unit tests for fill_cache_with_dictionaries (MissingDataError skip and related cases) --- tests/unit/test_script_utils.py | 141 +++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_script_utils.py b/tests/unit/test_script_utils.py index 8122defdc..79a025a01 100644 --- a/tests/unit/test_script_utils.py +++ b/tests/unit/test_script_utils.py @@ -1,11 +1,21 @@ from types import SimpleNamespace from os import path +from unittest.mock import MagicMock, patch import pytest import scripts.script_utils as script_utils -from scripts.script_utils import load_rules_from_local, load_specified_rules +from scripts.script_utils import ( + fill_cache_with_dictionaries, + load_rules_from_local, + load_specified_rules, +) from cdisc_rules_engine.utilities.utils import get_rules_cache_key +from cdisc_rules_engine.exceptions.custom_exceptions import MissingDataError +from cdisc_rules_engine.models.dictionaries.base_external_dictionary import ( + ExternalDictionary, +) +from cdisc_rules_engine.models.dictionaries.dictionary_types import DictionaryTypes @pytest.fixture @@ -193,3 +203,132 @@ def test_load_rules_from_local_exclude(monkeypatch, standard_context): returned_ids = {rule["core_id"] for rule in rules} assert returned_ids == {"CORE.Local.0001", "CORE.Local.0003"} + + +def _make_args(mapping): + return SimpleNamespace( + external_dictionaries=SimpleNamespace(dictionary_path_mapping=mapping) + ) + + +_NO_EXTRACT = object() + + +def _extract_meddra_fails_whodrug_ok(whodrug_terms): + def fn(_ds, dtype, _dpath): + if dtype == "meddra": + raise MissingDataError("Missing files") + return whodrug_terms + + return fn + + +@pytest.mark.parametrize( + "mapping,expected,extract_ret,raise_exc,warn_has,cache_with", + [ + ({"meddra": ""}, {}, _NO_EXTRACT, None, None, None), + ({"meddra": None}, {}, _NO_EXTRACT, None, None, None), + ({}, {}, _NO_EXTRACT, None, None, None), + ( + {DictionaryTypes.SNOMED.value: {"edition": "MAIN", "version": "2024-01"}}, + {DictionaryTypes.SNOMED.value: "MAIN/MAIN/2024-01"}, + _NO_EXTRACT, + None, + None, + None, + ), + ( + {DictionaryTypes.SNOMED.value: {"edition": "MAIN"}}, + {}, + _NO_EXTRACT, + None, + None, + None, + ), + ( + {"meddra": r".\bad_meddra"}, + {}, + MissingDataError("Required files not found"), + None, + [ + "meddra", + r".\bad_meddra", + "could not be loaded", + "Required files not found", + ], + None, + ), + ( + {"meddra": r".\valid_meddra"}, + {"meddra": "26.0"}, + ExternalDictionary(terms={"PT1": {}}, version="26.0"), + None, + None, + (r".\valid_meddra", "26.0"), + ), + ( + {"meddra": r".\bad_meddra", "whodrug": r".\valid_whodrug"}, + {"whodrug": "2024-01"}, + "side_effect", + None, + None, + (r".\valid_whodrug", "2024-01"), + ), + ( + {"meddra": r".\some_path"}, + None, + OSError("Permission denied"), + OSError, + None, + None, + ), + ], + ids=[ + "empty_path", + "none_path", + "empty_mapping", + "snomed_full", + "snomed_incomplete", + "missing_data_error", + "success", + "one_fails_others_ok", + "other_exception_propagates", + ], +) +def test_fill_cache_with_dictionaries( + mapping, expected, extract_ret, raise_exc, warn_has, cache_with +): + """Unit test for fill_cache_with_dictionaries: falsy/SNOMED skip, MissingDataError skip+warn, + success, one-fails-others-succeed, other exception propagates (per reviewer request). + """ + cache, args, ds = MagicMock(), _make_args(mapping), MagicMock() + with patch.object(script_utils, "extract_dictionary_terms") as ext: + if extract_ret is _NO_EXTRACT: + pass + elif extract_ret == "side_effect": + terms = ExternalDictionary(terms={}, version="2024-01") + ext.side_effect = _extract_meddra_fails_whodrug_ok(terms) + elif isinstance(extract_ret, BaseException): + ext.side_effect = extract_ret + else: + ext.return_value = extract_ret + with patch.object(script_utils, "engine_logger") as log: + if raise_exc: + with pytest.raises(raise_exc, match="Permission denied"): + fill_cache_with_dictionaries(cache, args, ds) + else: + result = fill_cache_with_dictionaries(cache, args, ds) + assert result == expected + if warn_has: + log.warning.assert_called_once() + msg = log.warning.call_args[0][0] + for s in warn_has: + assert s in msg or s in msg.lower() + if extract_ret is _NO_EXTRACT: + ext.assert_not_called() + if cache_with: + cache.add.assert_called_once() + assert cache.add.call_args[0][0] == cache_with[0] + assert cache.add.call_args[0][1].version == cache_with[1] + else: + cache.add.assert_not_called() From f3d57481453143cd89217ac42243d4a730ee84af Mon Sep 17 00:00:00 2001 From: Rakesh Date: Tue, 17 Feb 2026 17:25:35 -0500 Subject: [PATCH 3/4] remove unnecessary comments --- tests/unit/test_script_utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_script_utils.py b/tests/unit/test_script_utils.py index 79a025a01..7b42d6de3 100644 --- a/tests/unit/test_script_utils.py +++ b/tests/unit/test_script_utils.py @@ -298,9 +298,8 @@ def fn(_ds, dtype, _dpath): def test_fill_cache_with_dictionaries( mapping, expected, extract_ret, raise_exc, warn_has, cache_with ): - """Unit test for fill_cache_with_dictionaries: falsy/SNOMED skip, MissingDataError skip+warn, - success, one-fails-others-succeed, other exception propagates (per reviewer request). - """ + """Parametrized: falsy/SNOMED skip, MissingDataError skip+warn, success, + one-fails-others-succeed, other exception propagates.""" cache, args, ds = MagicMock(), _make_args(mapping), MagicMock() with patch.object(script_utils, "extract_dictionary_terms") as ext: if extract_ret is _NO_EXTRACT: From 1ececdfe2e1978147e19cc12d3de428c1f968aab Mon Sep 17 00:00:00 2001 From: Rakesh Date: Thu, 12 Mar 2026 11:51:36 -0400 Subject: [PATCH 4/4] Include underlying exception message in EXECUTION_ERROR report for missing external dictionary --- cdisc_rules_engine/rules_engine.py | 3 ++- cdisc_rules_engine/utilities/rule_processor.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cdisc_rules_engine/rules_engine.py b/cdisc_rules_engine/rules_engine.py index 2d69de749..6404ee841 100644 --- a/cdisc_rules_engine/rules_engine.py +++ b/cdisc_rules_engine/rules_engine.py @@ -528,10 +528,11 @@ def handle_validation_exceptions( # noqa ) elif isinstance(exception, OperationError): + error_msg = getattr(exception, "message", None) or str(exception) error_obj = FailedValidationEntity( dataset=filename, error=OperationError.description, - message=str(exception), + message=error_msg, ) message = "rule evaluation error - operation failed" errors = [error_obj] diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index 407f67cb7..470c5928b 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -410,11 +410,12 @@ def perform_rule_operations( operation_params, dataset_copy, previous_operations ) except Exception as e: + error_detail = getattr(e, "message", None) or str(e) raise OperationError( f"Failed to execute rule operation. " f"Operation: {operation_params.operation_name}, " f"Target: {target}, Domain: {domain}, " - f"Error: {str(e)}" + f"Error: {error_detail}" ) previous_operations.append(operation_params.operation_name)