Error for multiple different variadic concepts with the same name

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 = "<unknown>"
 
+        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 '<unknown>'}') 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,

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<concept_name>[^\[\]/]+)\[(?P<instance>[^\]]+)\]")
+
+        # 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, "")

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",
         ),