From 9de430628c940efea449f844c46a74a4ba42c953 Mon Sep 17 00:00:00 2001 From: alexfurmenkov Date: Mon, 26 Jan 2026 20:28:35 +0100 Subject: [PATCH 1/4] Refactor target name handling to preserve order --- cdisc_rules_engine/models/actions.py | 67 ++++++++++++------- .../models/validation_error_container.py | 2 +- .../utilities/rule_processor.py | 3 +- 3 files changed, 46 insertions(+), 26 deletions(-) diff --git a/cdisc_rules_engine/models/actions.py b/cdisc_rules_engine/models/actions.py index 9ee50801c..8eccf107f 100644 --- a/cdisc_rules_engine/models/actions.py +++ b/cdisc_rules_engine/models/actions.py @@ -1,4 +1,4 @@ -from typing import List, Optional, Set, Hashable +from typing import List, Optional, Set, Hashable, Iterable from os import path import pandas as pd from business_rules.actions import BaseActions, rule_action @@ -48,7 +48,8 @@ def generate_record_message(self, message, target=None): def generate_dataset_error_objects(self, message: str, results: pd.Series): # leave only those columns where errors have been found rows_with_error = self.variable.dataset.get_error_rows(results) - target_names: Set[str] = RuleProcessor.extract_target_names_from_rule( + # get targets in the order they appear in rule.output_variables + target_names: List[str] = RuleProcessor.extract_target_names_from_rule( self.rule, self.dataset_metadata.domain_cleaned, self.variable.dataset.columns.tolist(), @@ -67,21 +68,29 @@ def generate_dataset_error_objects(self, message: str, results: pd.Series): def generate_single_error(self, message): self.output_container.append(message) - def _get_target_names_from_list_values(self, target_names, rows_with_error): - expanded_target_names = set(target_names) - expanded_target_names.update( - value - for target in target_names - if target in rows_with_error - for candidate_list in rows_with_error[target] - if isinstance(candidate_list, list) - for value in candidate_list - if value in self.variable.dataset.columns - ) - return expanded_target_names + def _get_target_names_from_list_values( + self, target_names: List[str], rows_with_error: pd.DataFrame + ) -> List[str]: + """Expand target names with column names found inside list values, preserving order. + + New targets are appended in the order they are discovered, duplicates are ignored. + """ + expanded: List[str] = list(target_names) + existing = set(expanded) + for target in target_names: + if target not in rows_with_error: + continue + for candidate_list in rows_with_error[target]: + if not isinstance(candidate_list, list): + continue + for value in candidate_list: + if value in self.variable.dataset.columns and value not in existing: + expanded.append(value) + existing.add(value) + return expanded def generate_targeted_error_object( # noqa: C901 - self, targets: Set[str], data: pd.DataFrame, message: str + self, targets: Iterable[str], data: pd.DataFrame, message: str ) -> ValidationErrorContainer: """ Generates a targeted error object. @@ -116,9 +125,12 @@ def generate_targeted_error_object( # noqa: C901 "message": "AESTDY and DOMAIN are equal to test", } """ + # preserve incoming order for representation but use sets for membership tests + targets_list: List[str] = list(targets) + targets_set: Set[str] = set(targets_list) df_columns: set = set(data) - targets_in_dataset = targets.intersection(df_columns) - targets_not_in_dataset = targets.difference(df_columns) + targets_in_dataset = targets_set.intersection(df_columns) + targets_not_in_dataset = targets_set.difference(df_columns) all_targets_missing = ( len(targets_in_dataset) == 0 and len(targets_not_in_dataset) > 0 ) @@ -126,7 +138,7 @@ def generate_targeted_error_object( # noqa: C901 errors_df = data[list(targets_in_dataset)] else: errors_df = data - if not targets: + if not targets_set: errors_df = data if self.rule.get("sensitivity") == Sensitivity.DATASET.value: @@ -200,7 +212,7 @@ def generate_targeted_error_object( # noqa: C901 if self.dataset_metadata.is_supp else (self.dataset_metadata.domain or self.dataset_metadata.name) ), - targets=sorted(targets), + targets=targets_list, message="Invalid or undefined sensitivity in the rule", errors=[error_entity], ) @@ -217,7 +229,7 @@ def generate_targeted_error_object( # noqa: C901 dataset=", ".join( sorted(set(error.dataset or "" for error in errors_list)) ), - targets=sorted(targets), + targets=targets_list, errors=errors_list, message=message.replace("--", self.dataset_metadata.domain_cleaned or ""), ) @@ -324,13 +336,14 @@ def _create_configuration_error(self, message, targets): USUBJID="N/A", SEQ=0, ) + targets_list = list(targets) return ValidationErrorContainer( domain=( f"SUPP{self.dataset_metadata.rdomain}" if self.dataset_metadata.is_supp else (self.dataset_metadata.domain or self.dataset_metadata.name) ), - targets=sorted(targets), + targets=targets_list, message=message, errors=[error_entity], ) @@ -485,8 +498,16 @@ def _create_error_object( ) return error_object - def extract_target_names_from_value_level_metadata(self): - return set([item["define_variable_name"] for item in self.value_level_metadata]) + def extract_target_names_from_value_level_metadata(self) -> List[str]: + """Return target names from value-level metadata preserving input order.""" + seen = set() + ordered: List[str] = [] + for item in self.value_level_metadata: + name = item.get("define_variable_name") + if name and name not in seen: + seen.add(name) + ordered.append(name) + return ordered @staticmethod def _sequence_exists(sequence: pd.Series, row_name: Hashable) -> bool: diff --git a/cdisc_rules_engine/models/validation_error_container.py b/cdisc_rules_engine/models/validation_error_container.py index 03a45cd2c..7f0c77f9f 100644 --- a/cdisc_rules_engine/models/validation_error_container.py +++ b/cdisc_rules_engine/models/validation_error_container.py @@ -28,7 +28,7 @@ def to_representation(self) -> dict: "executionStatus": self.executionStatus, "dataset": self.dataset, "domain": self.domain, - "variables": sorted(self.targets), + "variables": self.targets, "message": self.message, "errors": [error.to_representation() for error in self.errors], **({"entity": self.entity} if self.entity else {}), diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index c90ac7310..e511a75bc 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -782,8 +782,7 @@ def extract_target_names_from_rule( ) else: target_names.append(target) - target_names.sort() - return set(target_names) + return target_names @staticmethod def extract_referenced_variables_from_rule(rule: dict): From 42224290b0b888c8bc684c2263f2aed1d706e194 Mon Sep 17 00:00:00 2001 From: alexfurmenkov Date: Wed, 28 Jan 2026 17:46:58 +0100 Subject: [PATCH 2/4] Reorder output variables and ensure uniqueness in target name extraction --- .../utilities/rule_processor.py | 32 +++++++++++-------- tests/unit/test_rules_engine.py | 14 ++++---- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index e511a75bc..03de2bc9e 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -2,7 +2,7 @@ import copy import os -from typing import Iterable, List, Optional, Set, Union, Tuple +from typing import Iterable, List, Optional, Union, Tuple from cdisc_rules_engine.enums.rule_types import RuleTypes from cdisc_rules_engine.interfaces.cache_service_interface import ( CacheServiceInterface, @@ -739,7 +739,7 @@ def is_suitable_for_validation( @staticmethod def extract_target_names_from_rule( rule: dict, domain: str, column_names: List[str] - ) -> Set[str]: + ) -> List[str]: r""" Extracts target from each item of condition list. @@ -754,13 +754,17 @@ def extract_target_names_from_rule( pattern: ^TSVAL\d+$ (starts with TSVAL and ends with number) additional columns: TSVAL1, TSVAL2, TSVAL3 etc. """ + # flake8: noqa: C901 output_variables: List[str] = rule.get("output_variables", []) + target_names: List[str] = [] + seen: set[str] = set() if output_variables: - target_names: List[str] = [ - var.replace("--", domain or "", 1) for var in output_variables - ] + for var in output_variables: + name = var.replace("--", domain or "", 1) + if name not in seen: + seen.add(name) + target_names.append(name) else: - target_names: List[str] = [] conditions: ConditionInterface = rule["conditions"] for condition in conditions.values(): if condition.get("operator") == "not_exists": @@ -774,14 +778,16 @@ def extract_target_names_from_rule( ) if op_related_pattern is not None: # if pattern exists -> return only matching column names - target_names.extend( - filter( - lambda name: re.match(op_related_pattern, name), - column_names, - ) - ) + for name in column_names: + if re.match(op_related_pattern, name): + if name not in seen: + seen.add(name) + target_names.append(name) else: - target_names.append(target) + if target not in seen: + seen.add(target) + target_names.append(target) + return target_names @staticmethod diff --git a/tests/unit/test_rules_engine.py b/tests/unit/test_rules_engine.py index c27468bfb..c19c209d8 100644 --- a/tests/unit/test_rules_engine.py +++ b/tests/unit/test_rules_engine.py @@ -952,7 +952,7 @@ def test_validate_variable_metadata_wrong_metadata( { "domain": "EC", "dataset": "bundle", - "variables": ["variable_data_type", "variable_label", "variable_name"], + "variables": ["variable_name", "variable_label", "variable_data_type"], "executionStatus": ExecutionStatus.SUCCESS.value, "errors": [ { @@ -1289,7 +1289,7 @@ def test_validate_single_dataset_not_equal_to( "executionStatus": "success", "dataset": "ae.xpt", "domain": "AE", - "variables": ["dataset_label", "dataset_location", "dataset_name"], + "variables": ["dataset_label", "dataset_name", "dataset_location"], "message": "Dataset metadata does not correspond to Define XML", "errors": [ { @@ -1887,7 +1887,7 @@ def test_validate_split_dataset_variables_metadata( "domain": "EC", "dataset": "ec_2.xpt", "executionStatus": ExecutionStatus.SUCCESS.value, - "variables": ["variable_data_type", "variable_label", "variable_name"], + "variables": ["variable_name", "variable_label", "variable_data_type"], "errors": [ { "dataset": "ec_2.xpt", @@ -2011,7 +2011,7 @@ def test_validate_record_in_parent_domain( "executionStatus": "success", "domain": "EC", "dataset": "ec.xpt", - "variables": ["ECPRESP", "ECREASOC"], + "variables": ["ECREASOC", "ECPRESP"], "message": "Dataset contents is wrong.", "errors": [ { @@ -2187,8 +2187,8 @@ def test_validate_dataset_contents_against_define_and_library_variable_metadata( "dataset": "filename", "domain": "AE", "variables": [ - "AESER", "AESEV", + "AESER", ], # AELNKID must not be included since its core status is not "Perm" "message": RuleProcessor.extract_message_from_rule( rule_check_dataset_against_library_and_define @@ -2686,10 +2686,10 @@ def mock_cached_method(*args, **kwargs): "variables": [ "$column_order_from_dataset", "$column_order_from_library", - "AESEQ", - "AETERM", "DOMAIN", + "AESEQ", "STUDYID", + "AETERM", ], "message": "Order of variables is invalid", "errors": [ From 3926072ccadb05cd82c608c061d43fbf1093357f Mon Sep 17 00:00:00 2001 From: alexfurmenkov Date: Wed, 28 Jan 2026 18:07:15 +0100 Subject: [PATCH 3/4] fix tests --- .../unit/test_utilities/test_jsonata_processor.py | 2 +- tests/unit/test_utilities/test_rule_processor.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_utilities/test_jsonata_processor.py b/tests/unit/test_utilities/test_jsonata_processor.py index 67d11601e..b6f7dd4cb 100644 --- a/tests/unit/test_utilities/test_jsonata_processor.py +++ b/tests/unit/test_utilities/test_jsonata_processor.py @@ -53,7 +53,7 @@ class TestJSONataProcessor(TestCase): "executionStatus": "success", "dataset": None, "domain": None, - "variables": ["A", "B", "row"], + "variables": ["row", "A", "B"], "message": "A equals B", "errors": [ { diff --git a/tests/unit/test_utilities/test_rule_processor.py b/tests/unit/test_utilities/test_rule_processor.py index 4be952e66..f2356f73d 100644 --- a/tests/unit/test_utilities/test_rule_processor.py +++ b/tests/unit/test_utilities/test_rule_processor.py @@ -1,4 +1,4 @@ -from typing import List, Set +from typing import List from unittest.mock import MagicMock, patch import pandas as pd @@ -1101,7 +1101,7 @@ def test_extract_target_names_from_rule(): rule: dict = { "conditions": ConditionCompositeFactory.get_condition_composite(conditions), } - target_names: Set[str] = RuleProcessor.extract_target_names_from_rule( + target_names: List[str] = RuleProcessor.extract_target_names_from_rule( rule, "AE", [ @@ -1110,11 +1110,11 @@ def test_extract_target_names_from_rule(): "TARGET", ], ) - assert target_names == { + assert target_names == [ "AESTDY", "USUBJID", "TARGET", - } + ] def test_extract_target_names_from_rule_output_variables(): @@ -1130,7 +1130,7 @@ def test_extract_target_names_from_rule_output_variables(): "output_variables": ["AESTDY", "USUBJID", "TARGET"], "conditions": ConditionCompositeFactory.get_condition_composite(conditions), } - target_names: Set[str] = RuleProcessor.extract_target_names_from_rule( + target_names: List[str] = RuleProcessor.extract_target_names_from_rule( rule, "AE", [ @@ -1139,11 +1139,11 @@ def test_extract_target_names_from_rule_output_variables(): "TARGET", ], ) - assert target_names == { + assert target_names == [ "AESTDY", "USUBJID", "TARGET", - } + ] @pytest.mark.parametrize( From a7c0fb71fd18795d3c7b145631e19d4253f28b75 Mon Sep 17 00:00:00 2001 From: alexfurmenkov Date: Thu, 29 Jan 2026 17:09:42 +0100 Subject: [PATCH 4/4] Refactor target name extraction to improve clarity and maintainability --- .../utilities/rule_processor.py | 80 +++++++++++-------- 1 file changed, 45 insertions(+), 35 deletions(-) diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index 03de2bc9e..b72dc9b44 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -736,6 +736,46 @@ def is_suitable_for_validation( return False, reason return self.log_suitable_for_validation(rule_id, dataset_name) + @staticmethod + def _extract_targets_from_output_variables(rule: dict, domain: str) -> List[str]: + output_variables: List[str] = rule.get("output_variables", []) + target_names: List[str] = [] + seen: set[str] = set() + for var in output_variables: + name = var.replace("--", domain or "", 1) + if name not in seen: + seen.add(name) + target_names.append(name) + return target_names + + @staticmethod + def _extract_targets_from_conditions( + rule: dict, domain: str, column_names: List[str] + ) -> List[str]: + target_names: List[str] = [] + seen: set[str] = set() + conditions: ConditionInterface = rule["conditions"] + for condition in conditions.values(): + if condition.get("operator") == "not_exists": + continue + target: str = condition["value"].get("target") + if target is None: + continue + target = target.replace("--", domain or "") + op_related_pattern: str = RuleProcessor.get_operator_related_pattern( + condition.get("operator"), target + ) + if op_related_pattern is not None: + for name in column_names: + if re.match(op_related_pattern, name) and name not in seen: + seen.add(name) + target_names.append(name) + else: + if target not in seen: + seen.add(target) + target_names.append(target) + return target_names + @staticmethod def extract_target_names_from_rule( rule: dict, domain: str, column_names: List[str] @@ -754,41 +794,11 @@ def extract_target_names_from_rule( pattern: ^TSVAL\d+$ (starts with TSVAL and ends with number) additional columns: TSVAL1, TSVAL2, TSVAL3 etc. """ - # flake8: noqa: C901 - output_variables: List[str] = rule.get("output_variables", []) - target_names: List[str] = [] - seen: set[str] = set() - if output_variables: - for var in output_variables: - name = var.replace("--", domain or "", 1) - if name not in seen: - seen.add(name) - target_names.append(name) - else: - conditions: ConditionInterface = rule["conditions"] - for condition in conditions.values(): - if condition.get("operator") == "not_exists": - continue - target: str = condition["value"].get("target") - if target is None: - continue - target = target.replace("--", domain or "") - op_related_pattern: str = RuleProcessor.get_operator_related_pattern( - condition.get("operator"), target - ) - if op_related_pattern is not None: - # if pattern exists -> return only matching column names - for name in column_names: - if re.match(op_related_pattern, name): - if name not in seen: - seen.add(name) - target_names.append(name) - else: - if target not in seen: - seen.add(target) - target_names.append(target) - - return target_names + if rule.get("output_variables"): + return RuleProcessor._extract_targets_from_output_variables(rule, domain) + return RuleProcessor._extract_targets_from_conditions( + rule, domain, column_names + ) @staticmethod def extract_referenced_variables_from_rule(rule: dict):