From 2e27912b30cc69b62f225656f3e0ac379e22a4eb Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Mon, 26 May 2025 21:26:47 +0200 Subject: [PATCH 01/10] log error if there re multiple different variadic concepts with the same name --- src/pynxtools/dataconverter/helpers.py | 8 ++++ src/pynxtools/dataconverter/validation.py | 56 ++++++++++++++++++++++- tests/dataconverter/test_validation.py | 37 +++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/pynxtools/dataconverter/helpers.py b/src/pynxtools/dataconverter/helpers.py index 61b774ca8..fc668ee64 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() @@ -85,6 +86,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.error( + 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." diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index b5457dea7..de9f65c02 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -850,6 +850,59 @@ def recurse_tree( handling_map.get(child.type, handle_unknown_type)(child, keys, prev_path) + def find_instance_name_conflicts( + mapping: MutableMapping[str, str], keys_to_remove: List[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. + + 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. + + Parameters: + 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[^\]]+)\]") + + # Map from instance name to list of (concept_name, full_key) where it's used + instance_usage: Dict[str, List[Tuple[str, str]]] = defaultdict(list) + + for key in mapping: + for match in pattern.finditer(key): + concept_name, instance_name = match.groups() + instance_usage[instance_name].append((concept_name, key)) + + for instance_name, 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, + ) + for key in keys: + collector.collect_and_log( + key, + ValidationProblem.KeyToBeRemoved, + "key", + ) + keys_to_remove += keys + def check_attributes_of_nonexisting_field( node: NexusNode, ): @@ -1174,10 +1227,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, keys_to_remove) 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..19f7fc945 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -234,6 +234,43 @@ def listify_template(data_dict: Template): @pytest.mark.parametrize( "data_dict,error_messages", [ + 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]/USER[some_name]/name", + "A user name", + ), + "/ENTRY[my_entry]/MONITOR[some_name]/name", + "A monitor name", + ), + "/ENTRY[my_entry]/MONITOR[another_name]/name", + "Another monitor name", + ), + "/ENTRY[my_entry]/SAMPLE[another_name]/name", + "Another sample name", + ), + [ + "Instance name 'another_name' used for multiple different concepts: MONITOR, SAMPLE. " + "The following keys are affected: /ENTRY[my_entry]/MONITOR[another_name]/name, " + "/ENTRY[my_entry]/SAMPLE[another_name]/name.", + "The key /ENTRY[my_entry]/MONITOR[another_name]/name will not be written.", + "The key /ENTRY[my_entry]/SAMPLE[another_name]/name will not be written.", + "Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. " + "The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/name, " + "/ENTRY[my_entry]/SAMPLE[some_name]/name, /ENTRY[my_entry]/USER[some_name]/name.", + "The key /ENTRY[my_entry]/MONITOR[some_name]/name 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( From eaba076c611663ae55ad6bc0e3e46f8883cb13c7 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Mon, 26 May 2025 21:42:29 +0200 Subject: [PATCH 02/10] fix for groups that are not on the same level --- src/pynxtools/dataconverter/validation.py | 25 ++++++++++---- tests/dataconverter/test_validation.py | 42 +++++++++++++---------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index de9f65c02..f0d724517 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -862,6 +862,14 @@ def find_instance_name_conflicts( 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. @@ -878,15 +886,20 @@ def find_instance_name_conflicts( """ pattern = re.compile(r"(?P[^\[\]/]+)\[(?P[^\]]+)\]") - # Map from instance name to list of (concept_name, full_key) where it's used - instance_usage: Dict[str, List[Tuple[str, str]]] = defaultdict(list) + # 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: - for match in pattern.finditer(key): - concept_name, instance_name = match.groups() - instance_usage[instance_name].append((concept_name, key)) + matches = list(pattern.finditer(key)) + for i, match in enumerate(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()] + instance_usage[(instance_name, parent_path)].append((concept_name, key)) - for instance_name, entries in sorted(instance_usage.items()): + 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) diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index 19f7fc945..e1eeec42a 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -240,32 +240,36 @@ def listify_template(data_dict: Template): alter_dict( alter_dict( alter_dict( - TEMPLATE, - "/ENTRY[my_entry]/SAMPLE[some_name]/name", - "A sample name", + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/SAMPLE[some_name]/name", + "A sample name", + ), + "/ENTRY[my_entry]/USER[some_name]/name", + "A user name", ), - "/ENTRY[my_entry]/USER[some_name]/name", - "A user name", + "/ENTRY[my_entry]/APERTURE[some_name]/name", + "An monitor name", ), - "/ENTRY[my_entry]/MONITOR[some_name]/name", - "A monitor name", + "/ENTRY[my_entry]/APERTURE[another_name]/name", + "Another monitor name", ), - "/ENTRY[my_entry]/MONITOR[another_name]/name", - "Another monitor name", + "/ENTRY[my_entry]/SAMPLE[another_name]/name", + "Another sample name", ), - "/ENTRY[my_entry]/SAMPLE[another_name]/name", - "Another sample name", + "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", + "Another c name within an instrument.", ), [ - "Instance name 'another_name' used for multiple different concepts: MONITOR, SAMPLE. " - "The following keys are affected: /ENTRY[my_entry]/MONITOR[another_name]/name, " + "Instance name 'another_name' used for multiple different concepts: APERTURE, SAMPLE. " + "The following keys are affected: /ENTRY[my_entry]/APERTURE[another_name]/name, " "/ENTRY[my_entry]/SAMPLE[another_name]/name.", - "The key /ENTRY[my_entry]/MONITOR[another_name]/name will not be written.", + "The key /ENTRY[my_entry]/APERTURE[another_name]/name will not be written.", "The key /ENTRY[my_entry]/SAMPLE[another_name]/name will not be written.", - "Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. " - "The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/name, " + "Instance name 'some_name' used for multiple different concepts: APERTURE, SAMPLE, USER. " + "The following keys are affected: /ENTRY[my_entry]/APERTURE[some_name]/name, " "/ENTRY[my_entry]/SAMPLE[some_name]/name, /ENTRY[my_entry]/USER[some_name]/name.", - "The key /ENTRY[my_entry]/MONITOR[some_name]/name will not be written.", + "The key /ENTRY[my_entry]/APERTURE[some_name]/name 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.", ], @@ -1116,11 +1120,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", ), From 29f290e1adf18372bdf83a9a83ed14e16405c210 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Tue, 27 May 2025 21:59:07 +0200 Subject: [PATCH 03/10] remove unneeded enumeration --- src/pynxtools/dataconverter/validation.py | 2 +- tests/dataconverter/test_validation.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index f0d724517..19e11cfd4 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -891,7 +891,7 @@ def find_instance_name_conflicts( for key in mapping: matches = list(pattern.finditer(key)) - for i, match in enumerate(matches): + for match in matches: concept_name = match.group("concept_name") instance_name = match.group("instance") diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index e1eeec42a..34b9cac06 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -258,7 +258,7 @@ def listify_template(data_dict: Template): "Another sample name", ), "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", - "Another c name within an instrument.", + "Another aperture name within an instrument.", ), [ "Instance name 'another_name' used for multiple different concepts: APERTURE, SAMPLE. " From d499cc6ea4e84f116ba9c859ce47b042013c6318 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Tue, 27 May 2025 22:06:18 +0200 Subject: [PATCH 04/10] continue if keys has no variable concepts --- src/pynxtools/dataconverter/validation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index 19e11cfd4..a1c6459aa 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -891,6 +891,9 @@ def find_instance_name_conflicts( 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") From 0225333cd1a36e29fdf880e7e1ed975c3cd268ed Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Wed, 28 May 2025 10:59:18 +0200 Subject: [PATCH 05/10] use existing docs style --- src/pynxtools/dataconverter/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index a1c6459aa..4e85a2213 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -874,7 +874,7 @@ def find_instance_name_conflicts( and the conflicting concept names. Additionally, all keys involved in the conflict are logged and added to the `keys_to_remove` list. - Parameters: + Args: mapping (MutableMapping[str, str]): The mapping containing the data to validate. This should be a dict of `/` separated paths, such as From 40295fa3d8374108c96c2ec32a374770048a9f17 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Wed, 28 May 2025 18:31:54 +0200 Subject: [PATCH 06/10] only remove conflicting keys if each of them is valid --- src/pynxtools/dataconverter/helpers.py | 2 +- src/pynxtools/dataconverter/validation.py | 27 ++++++++++-- tests/dataconverter/test_validation.py | 54 ++++++++++++++--------- 3 files changed, 57 insertions(+), 26 deletions(-) diff --git a/src/pynxtools/dataconverter/helpers.py b/src/pynxtools/dataconverter/helpers.py index fc668ee64..3074a6469 100644 --- a/src/pynxtools/dataconverter/helpers.py +++ b/src/pynxtools/dataconverter/helpers.py @@ -88,7 +88,7 @@ def _log(self, path: str, log_type: ValidationProblem, value: Optional[Any], *ar if log_type == ValidationProblem.DifferentVariadicNodesWithTheSameName: value = cast(Any, value) - logger.error( + 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)))}." diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index 4e85a2213..41526caea 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -911,13 +911,32 @@ def find_instance_name_conflicts( ValidationProblem.DifferentVariadicNodesWithTheSameName, entries, ) + # Now that we have name conflicts, we still need to check that there are + # at least two valid keys in that conclit. 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", + key, ValidationProblem.KeyToBeRemoved, "key" ) - keys_to_remove += keys + keys_to_remove.append(key) + + if len(valid_keys_with_name_conflicts) > 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, diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index 34b9cac06..b5eb50abc 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -241,35 +241,47 @@ def listify_template(data_dict: Template): alter_dict( alter_dict( alter_dict( - TEMPLATE, - "/ENTRY[my_entry]/SAMPLE[some_name]/name", - "A sample name", + alter_dict( + alter_dict( + TEMPLATE, + "/ENTRY[my_entry]/SAMPLE[some_name]/name", + "A sample name", + ), + "/ENTRY[my_entry]/USER[some_name]/name", + "A user name", + ), + "/ENTRY[my_entry]/MONITOR[some_name]/name", + "An monitor name", ), - "/ENTRY[my_entry]/USER[some_name]/name", - "A user name", + "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", + "An aperture within an instrument", ), - "/ENTRY[my_entry]/APERTURE[some_name]/name", - "An monitor name", + "/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name", + "A detector within an instrument", ), - "/ENTRY[my_entry]/APERTURE[another_name]/name", - "Another monitor name", + "/ENTRY[my_entry]/INSTRUMENT[instrument]/SOURCE[my_source]/APERTURE[another_name]/name", + "An aperture within a source inside an instrument", ), - "/ENTRY[my_entry]/SAMPLE[another_name]/name", - "Another sample name", + "/ENTRY[my_entry]/USER[a_third_name]/name", + "A tird user name", ), - "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", - "Another aperture name within an instrument.", + "/ENTRY[my_entry]/USERS[a_third_name]/name", + "An invalid group of the same name", ), [ - "Instance name 'another_name' used for multiple different concepts: APERTURE, SAMPLE. " - "The following keys are affected: /ENTRY[my_entry]/APERTURE[another_name]/name, " - "/ENTRY[my_entry]/SAMPLE[another_name]/name.", - "The key /ENTRY[my_entry]/APERTURE[another_name]/name will not be written.", - "The key /ENTRY[my_entry]/SAMPLE[another_name]/name will not be written.", - "Instance name 'some_name' used for multiple different concepts: APERTURE, SAMPLE, USER. " - "The following keys are affected: /ENTRY[my_entry]/APERTURE[some_name]/name, " + "Instance name 'a_third_name' used for multiple different concepts: USER, USERS. " + "The following keys are affected: /ENTRY[my_entry]/USERS[a_third_name]/name, " + "/ENTRY[my_entry]/USER[a_third_name]/name.", + "The key /ENTRY[my_entry]/USERS[a_third_name]/name will not be written.", + "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.", + "Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. " + "The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/name, " "/ENTRY[my_entry]/SAMPLE[some_name]/name, /ENTRY[my_entry]/USER[some_name]/name.", - "The key /ENTRY[my_entry]/APERTURE[some_name]/name 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]/name will not be written.", "The key /ENTRY[my_entry]/USER[some_name]/name will not be written.", ], From 8d0e62b5aabe3edcbbf1aaf35f922962e590e3c4 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Thu, 5 Jun 2025 10:46:20 +0200 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Laurenz Rettig <53396064+rettigl@users.noreply.github.com> --- src/pynxtools/dataconverter/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index 41526caea..4d3b95905 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -912,7 +912,7 @@ def find_instance_name_conflicts( entries, ) # Now that we have name conflicts, we still need to check that there are - # at least two valid keys in that conclit. Only then we remove these. + # 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. From aacf70e8e5d5e415d5905c9207691710c1adfa15 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Thu, 5 Jun 2025 10:49:52 +0200 Subject: [PATCH 08/10] use keys_to_remove from global context --- src/pynxtools/dataconverter/validation.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index 4d3b95905..4dc86c2e7 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -850,9 +850,7 @@ def recurse_tree( handling_map.get(child.type, handle_unknown_type)(child, keys, prev_path) - def find_instance_name_conflicts( - mapping: MutableMapping[str, str], keys_to_remove: List[str] - ) -> None: + 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. @@ -1266,7 +1264,7 @@ def check_reserved_prefix( tree = generate_tree_from(appdef) collector.clear() - find_instance_name_conflicts(mapping, keys_to_remove) + find_instance_name_conflicts(mapping) nested_keys = build_nested_dict_from(mapping) not_visited = list(mapping) keys = _follow_link(nested_keys, "") From 6daffdc2f4c12f40c0a01a49f519ed54b7086016 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Thu, 5 Jun 2025 13:26:25 +0200 Subject: [PATCH 09/10] catch case where there are multiple fields in the conflicting groups --- src/pynxtools/dataconverter/validation.py | 23 +++- tests/dataconverter/test_validation.py | 132 ++++++++++++++++------ 2 files changed, 116 insertions(+), 39 deletions(-) diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py index 4dc86c2e7..6f30e1e99 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -929,12 +929,23 @@ def find_instance_name_conflicts(mapping: MutableMapping[str, str]) -> None: ) keys_to_remove.append(key) - if len(valid_keys_with_name_conflicts) > 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) + 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, diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index b5eb50abc..4f20c76a0 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -240,53 +240,119 @@ def listify_template(data_dict: Template): alter_dict( alter_dict( alter_dict( - alter_dict( - alter_dict( - alter_dict( - TEMPLATE, - "/ENTRY[my_entry]/SAMPLE[some_name]/name", - "A sample name", - ), - "/ENTRY[my_entry]/USER[some_name]/name", - "A user name", - ), - "/ENTRY[my_entry]/MONITOR[some_name]/name", - "An monitor name", - ), - "/ENTRY[my_entry]/INSTRUMENT[instrument]/APERTURE[another_name]/name", - "An aperture within an instrument", + TEMPLATE, + "/ENTRY[my_entry]/SAMPLE[some_name]/name", + "A sample name", ), - "/ENTRY[my_entry]/INSTRUMENT[instrument]/DETECTOR[another_name]/name", - "A detector within an instrument", + "/ENTRY[my_entry]/SAMPLE[some_name]/description", + "A sample description", ), - "/ENTRY[my_entry]/INSTRUMENT[instrument]/SOURCE[my_source]/APERTURE[another_name]/name", - "An aperture within a source inside an instrument", + "/ENTRY[my_entry]/USER[some_name]/name", + "A user name", ), - "/ENTRY[my_entry]/USER[a_third_name]/name", - "A tird user name", + "/ENTRY[my_entry]/MONITOR[some_name]/name", + "A monitor name", ), - "/ENTRY[my_entry]/USERS[a_third_name]/name", - "An invalid group of the same name", + "/ENTRY[my_entry]/MONITOR[some_name]/description", + "A monitor description", ), [ - "Instance name 'a_third_name' used for multiple different concepts: USER, USERS. " - "The following keys are affected: /ENTRY[my_entry]/USERS[a_third_name]/name, " - "/ENTRY[my_entry]/USER[a_third_name]/name.", - "The key /ENTRY[my_entry]/USERS[a_third_name]/name will not be written.", - "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.", "Instance name 'some_name' used for multiple different concepts: MONITOR, SAMPLE, USER. " - "The following keys are affected: /ENTRY[my_entry]/MONITOR[some_name]/name, " + "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( From 14f956211b30739a7a0955801647d3f83dbbdef6 Mon Sep 17 00:00:00 2001 From: Lukas Pielsticker <50139597+lukaspie@users.noreply.github.com> Date: Thu, 5 Jun 2025 14:04:24 +0200 Subject: [PATCH 10/10] catch case where a key with a concept name has an equivalent non-variadic key in the template already --- src/pynxtools/dataconverter/helpers.py | 6 ++++++ src/pynxtools/dataconverter/validation.py | 24 +++++++++++++++++++++++ tests/dataconverter/test_validation.py | 14 +++++++++++++ 3 files changed, 44 insertions(+) diff --git a/src/pynxtools/dataconverter/helpers.py b/src/pynxtools/dataconverter/helpers.py index 3074a6469..04061ef1c 100644 --- a/src/pynxtools/dataconverter/helpers.py +++ b/src/pynxtools/dataconverter/helpers.py @@ -73,6 +73,7 @@ class ValidationProblem(Enum): ReservedSuffixWithoutField = auto() ReservedPrefixInWrongContext = auto() InvalidNexusTypeForNamedConcept = auto() + KeysWithAndWithoutConcept = auto() class Collector: @@ -185,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 6f30e1e99..af743506d 100644 --- a/src/pynxtools/dataconverter/validation.py +++ b/src/pynxtools/dataconverter/validation.py @@ -898,6 +898,30 @@ def find_instance_name_conflicts(mapping: MutableMapping[str, str]) -> None: # 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()): diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py index 4f20c76a0..f6217c7df 100644 --- a/tests/dataconverter/test_validation.py +++ b/tests/dataconverter/test_validation.py @@ -234,6 +234,20 @@ 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(