diff --git a/src/pynxtools/dataconverter/helpers.py b/src/pynxtools/dataconverter/helpers.py index 61b774ca8..04061ef1c 100644 --- a/src/pynxtools/dataconverter/helpers.py +++ b/src/pynxtools/dataconverter/helpers.py @@ -46,6 +46,7 @@ class ValidationProblem(Enum): + DifferentVariadicNodesWithTheSameName = auto() UnitWithoutDocumentation = auto() InvalidEnum = auto() OpenEnumWithNewItem = auto() @@ -72,6 +73,7 @@ class ValidationProblem(Enum): ReservedSuffixWithoutField = auto() ReservedPrefixInWrongContext = auto() InvalidNexusTypeForNamedConcept = auto() + KeysWithAndWithoutConcept = auto() class Collector: @@ -85,6 +87,13 @@ def _log(self, path: str, log_type: ValidationProblem, value: Optional[Any], *ar if value is None: value = "" + if log_type == ValidationProblem.DifferentVariadicNodesWithTheSameName: + value = cast(Any, value) + logger.warning( + f"Instance name '{path}' used for multiple different concepts: " + f"{', '.join(sorted(set(c for c, _ in value)))}. " + f"The following keys are affected: {', '.join(sorted(set(k for _, k in value)))}." + ) if log_type == ValidationProblem.UnitWithoutDocumentation: logger.info( f"The unit, {path} = {value}, is being written but has no documentation." @@ -177,6 +186,11 @@ def _log(self, path: str, log_type: ValidationProblem, value: Optional[Any], *ar f"The type ('{args[0] if args else ''}') of the given concept '{path}' " f"conflicts with another existing concept of the same name, which is of type '{value.type}'." ) + elif log_type == ValidationProblem.KeysWithAndWithoutConcept: + value = cast(Any, value) + logger.warning( + f"The key '{path}' uses the valid concept name '{args[0]}', but there is another valid key {value} that uses the non-variadic name of the node.'" + ) def collect_and_log( self, diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index b5457dea7..af743506d 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -850,6 +850,127 @@ def recurse_tree( handling_map.get(child.type, handle_unknown_type)(child, keys, prev_path) + def find_instance_name_conflicts(mapping: MutableMapping[str, str]) -> None: + """ + Detect and log conflicts where the same variadic instance name is reused across + different concept names. + + This function ensures that a given instance name (e.g., 'my_name') is only used + for a single concept (e.g., SAMPLE or USER, but not both). Reusing the same instance + name for different concept names (e.g., SAMPLE[my_name] and USER[my_name]) is + considered a conflict. + + For example, this is a conflict: + /ENTRY[entry1]/SAMPLE[my_name]/... + /ENTRY[entry1]/USER[my_name]/... + + But this is NOT a conflict: + /ENTRY[entry1]/INSTRUMENT[instrument]/FIELD[my_name]/... + /ENTRY[entry1]/INSTRUMENT[instrument]/DETECTOR[detector]/FIELD[my_name]/... + + When such conflicts are found, an error is logged indicating the instance name + and the conflicting concept names. Additionally, all keys involved in the conflict + are logged and added to the `keys_to_remove` list. + + Args: + mapping (MutableMapping[str, str]): + The mapping containing the data to validate. + This should be a dict of `/` separated paths, such as + "/ENTRY[entry1]/SAMPLE[sample1]/name". + keys_to_remove (List[str]): + List of keys that will be removed from the template. This is extended here + in the case of conflicts. + + """ + pattern = re.compile(r"(?P[^\[\]/]+)\[(?P[^\]]+)\]") + + # Tracks instance usage with respect to their parent group + instance_usage: Dict[Tuple[str, str], List[Tuple[str, str]]] = defaultdict(list) + + for key in mapping: + matches = list(pattern.finditer(key)) + if not matches: + # The keys contains no concepts with variable name, no need to further check + continue + for match in matches: + concept_name = match.group("concept_name") + instance_name = match.group("instance") + + # Determine the parent path up to just before this match + parent_path = key[: match.start()] + child_path = key[match.start() :].split("/", 1)[-1] + + # Here we check if for this key with a concept name, another valid key + # with a non-concept name exists. + non_concept_key = f"{parent_path}{instance_name}/{child_path}" + + if non_concept_key in mapping: + try: + node = add_best_matches_for(non_concept_key, tree) + if node is not None: + collector.collect_and_log( + key, + ValidationProblem.KeysWithAndWithoutConcept, + non_concept_key, + concept_name, + ) + collector.collect_and_log( + key, ValidationProblem.KeyToBeRemoved, "key" + ) + keys_to_remove.append(key) + continue + except TypeError: + pass + + instance_usage[(instance_name, parent_path)].append((concept_name, key)) + + for (instance_name, parent_path), entries in sorted(instance_usage.items()): + concept_names = {c for c, _ in entries} + if len(concept_names) > 1: + keys = sorted(k for _, k in entries) + collector.collect_and_log( + instance_name, + ValidationProblem.DifferentVariadicNodesWithTheSameName, + entries, + ) + # Now that we have name conflicts, we still need to check that there are + # at least two valid keys in that conflict. Only then we remove these. + # This takes care of the example with keys like + # /ENTRY[my_entry]/USER[some_name]/name and /ENTRY[my_entry]/USERS[some_name]/name, + # where we only want to keep the first one. + valid_keys_with_name_conflicts = [] + + for key in keys: + try: + node = add_best_matches_for(key, tree) + if node is not None: + valid_keys_with_name_conflicts.append(key) + continue + except TypeError: + pass + collector.collect_and_log( + key, ValidationProblem.KeyToBeRemoved, "key" + ) + keys_to_remove.append(key) + + if len(valid_keys_with_name_conflicts) >= 1: + # At this point, all invalid keys have been removed. + # If more than one valid concept still uses the same instance name under the same parent path, + # this indicates a semantic ambiguity (e.g., USER[alex] and SAMPLE[alex]). + # We remove these keys as well to avoid conflicts in the writer. + remaining_concepts = { + pattern.findall(k)[-1][0] + for k in valid_keys_with_name_conflicts + if pattern.findall(k) + } + # If multiple valid concept names reuse the same instance name, remove them too + if len(remaining_concepts) > 1: + for valid_key in valid_keys_with_name_conflicts: + collector.collect_and_log( + valid_key, ValidationProblem.KeyToBeRemoved, "key" + ) + keys_to_remove.append(valid_key) + def check_attributes_of_nonexisting_field( node: NexusNode, ): @@ -1174,10 +1295,11 @@ def check_reserved_prefix( "choice": handle_choice, } - keys_to_remove = [] + keys_to_remove: List[str] = [] tree = generate_tree_from(appdef) collector.clear() + find_instance_name_conflicts(mapping) nested_keys = build_nested_dict_from(mapping) not_visited = list(mapping) keys = _follow_link(nested_keys, "") diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index 3a40c39e1..f6217c7df 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -234,6 +234,139 @@ def listify_template(data_dict: Template): @pytest.mark.parametrize( "data_dict,error_messages", [ + pytest.param( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/NOTE[required_group2]/description", + "an additional description", + ), + [ + "The key '/ENTRY[my_entry]/NOTE[required_group2]/description' uses the valid concept name 'NOTE', " + "but there is another valid key /ENTRY[my_entry]/required_group2/description that uses the non-variadic " + "name of the node.'", + "The key /ENTRY[my_entry]/NOTE[required_group2]/description will not be written.", + ], + id="same-concept-with-and-without-concept-name", + ), + pytest.param( + alter_dict( + alter_dict( + alter_dict( + alter_dict( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/SAMPLE[some_name]/name", + "A sample name", + ), + "/ENTRY[my_entry]/SAMPLE[some_name]/description", + "A sample description", + ), + "/ENTRY[my_entry]/USER[some_name]/name", + "A user name", + ), + "/ENTRY[my_entry]/MONITOR[some_name]/name", + "A monitor name", + ), + "/ENTRY[my_entry]/MONITOR[some_name]/description", + "A monitor description", + ), + [ + "Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. " + "The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/description, " + "/ENTRY[my_entry]/MONITOR[some_name]/name, /ENTRY[my_entry]/SAMPLE[some_name]/description, " + "/ENTRY[my_entry]/SAMPLE[some_name]/name, /ENTRY[my_entry]/USER[some_name]/name.", + "The key /ENTRY[my_entry]/MONITOR[some_name]/description will not be written.", + "The key /ENTRY[my_entry]/MONITOR[some_name]/name will not be written.", + "The key /ENTRY[my_entry]/SAMPLE[some_name]/description will not be written.", + "The key /ENTRY[my_entry]/SAMPLE[some_name]/name will not be written.", + "The key /ENTRY[my_entry]/USER[some_name]/name will not be written.", + ], + id="variadic-groups-of-the-same-name", + ), + pytest.param( + alter_dict( + alter_dict( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", + "An aperture within an instrument", + ), + "/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name", + "A detector within an instrument", + ), + "/ENTRY[my_entry]/INSTRUMENT[instrument]/SOURCE[my_source]/APERTURE[another_name]/name", + "An aperture within a source inside an instrument", + ), + [ + "Instance name 'another_name' used for multiple different concepts: APERTURE, DETECTOR. " + "The following keys are affected: /ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name, " + "/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name.", + "The key /ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name will not be written.", + "The key /ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name will not be written.", + ], + id="variadic-groups-of-the-same-name-but-at-different-levels", + ), + pytest.param( + alter_dict( + alter_dict( + alter_dict( + alter_dict( + alter_dict( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/USER[user]/name", + "A user name", + ), + "/ENTRY[my_entry]/USER[user]/role", + "A user role", + ), + "/ENTRY[my_entry]/USER[user]/affiliation", + "A user affiliation", + ), + "/ENTRY[my_entry]/ILLEGAL[user]/name", + "An illegal user name", + ), + "/ENTRY[my_entry]/ILLEGAL[user]/role", + "An illegal user role", + ), + "/ENTRY[my_entry]/ILLEGAL[user]/affiliation", + "An illegal user affiliation", + ), + [ + "Instance name 'user' used for multiple different concepts: ILLEGAL, USER. " + "The following keys are affected: /ENTRY[my_entry]/ILLEGAL[user]/affiliation, /ENTRY[my_entry]/ILLEGAL[user]/name, " + "/ENTRY[my_entry]/ILLEGAL[user]/role, /ENTRY[my_entry]/USER[user]/affiliation, /ENTRY[my_entry]/USER[user]/name, " + "/ENTRY[my_entry]/USER[user]/role.", + "The key /ENTRY[my_entry]/ILLEGAL[user]/affiliation will not be written.", + "The key /ENTRY[my_entry]/ILLEGAL[user]/name will not be written.", + "The key /ENTRY[my_entry]/ILLEGAL[user]/role will not be written.", + ], + id="variadic-groups-of-the-same-name-illegal-concept-multiple-fields", + ), + pytest.param( + alter_dict( + alter_dict( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/USER[user]/name", + "A user name", + ), + "/ENTRY[my_entry]/USERS[user]/name", + "An invalid group of the same name", + ), + "/ENTRY[my_entry]/SAMPLE[user]/name", + "A sample group called user with a name", + ), + [ + "Instance name 'user' used for multiple different concepts: SAMPLE, USER, USERS. " + "The following keys are affected: /ENTRY[my_entry]/SAMPLE[user]/name, " + "/ENTRY[my_entry]/USERS[user]/name, /ENTRY[my_entry]/USER[user]/name.", + "The key /ENTRY[my_entry]/USERS[user]/name will not be written.", + "The key /ENTRY[my_entry]/SAMPLE[user]/name will not be written.", + "The key /ENTRY[my_entry]/USER[user]/name will not be written.", + ], + id="variadic-groups-of-the-same-name-mix-of-valid-and-illegal-concepts", + ), pytest.param( alter_dict( alter_dict( @@ -1079,11 +1212,11 @@ def listify_template(data_dict: Template): pytest.param( alter_dict( TEMPLATE, - "/ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source]/type", + "/ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source2]/type", 1, ), [ - "Field /ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source]/type written without documentation." + "Field /ENTRY[my_entry]/INSTRUMENT[my_instrument]/ILLEGAL[my_source2]/type written without documentation." ], id="bad-namefitting", ),