Merge pull request #639 from FAIRmat-NFDI/reserved-prefixes-suffixes

lukaspie · web-flow · commit f50a82712f20 · 2025-05-19T14:31:14.000+02:00
Validation check for reserved prefixes and suffixes
diff --git a/src/pynxtools/data/NXtest.nxdl.xml b/src/pynxtools/data/NXtest.nxdl.xml
@@ -23,6 +23,15 @@
             <field name="required_field" required="true" type="NX_INT">
                 <doc>A dummy entry to test optional parent check for a required child.</doc>
             </field>
+            <field name="required_field_set" optional="true" type="NX_INT">
+                <doc>A dummy entry to test reserved suffixes.</doc>
+            </field>
+            <field name="some_field_set" optional="true" type="NX_INT">
+                <doc>
+                    A dummy entry to test reserved suffixes where the actual field is not given.
+                    Note that this is not allowed by NeXus, but we do this here to test the validation.
+                </doc>
+            </field>
             <field name="optional_field" optional="true" type="NX_INT">
                 <doc>A dummy entry to test optional parent check for an optional child.</doc>
             </field>
diff --git a/src/pynxtools/dataconverter/helpers.py b/src/pynxtools/dataconverter/helpers.py
@@ -68,6 +68,8 @@ class ValidationProblem(Enum):
     NXdataAxisMismatch = 20
     KeyToBeRemoved = 21
     InvalidConceptForNonVariadic = 22
+    ReservedSuffixWithoutField = 23
+    ReservedPrefixInWrongContext = 24
 
 
 class Collector:
@@ -157,6 +159,16 @@ def _log(self, path: str, log_type: ValidationProblem, value: Optional[Any], *ar
             if value.type == "group":
                 log_text += f", which should be of type {value.nx_class}."
             logger.warning(log_text)
+        elif log_type == ValidationProblem.ReservedSuffixWithoutField:
+            logger.warning(
+                f"Reserved suffix '{args[0]}' was used in {path}, but there is no associated field {value}."
+            )
+        elif log_type == ValidationProblem.ReservedPrefixInWrongContext:
+            log_text = f"Reserved prefix {path} was used in key {args[0] if args else '<unknown>'}, but is not valid here."
+            # Note that value=None" gets converted to "<unknown>"
+            if value != "<unknown>":
+                log_text += f" It is only valid in the context of {value}."
+            logger.error(log_text)
 
     def collect_and_log(
         self,
diff --git a/src/pynxtools/dataconverter/validation.py b/src/pynxtools/dataconverter/validation.py
@@ -20,7 +20,18 @@
 from collections import defaultdict
 from functools import reduce
 from operator import getitem
-from typing import Any, Iterable, List, Mapping, MutableMapping, Optional, Tuple, Union
+from typing import (
+    Any,
+    Iterable,
+    List,
+    Mapping,
+    MutableMapping,
+    Optional,
+    Tuple,
+    Union,
+    Dict,
+    Literal,
+)
 
 import h5py
 import lxml.etree as ET
@@ -515,6 +526,9 @@ def handle_field(node: NexusNode, keys: Mapping[str, Any], prev_path: str):
                 f"{prev_path}/{variant}",
             )
 
+            _ = check_reserved_suffix(f"{prev_path}/{variant}", mapping)
+            _ = check_reserved_prefix(f"{prev_path}/{variant}", mapping, "field")
+
             # Check unit category
             if node.unit is not None:
                 remove_from_not_visited(f"{prev_path}/{variant}/@units")
@@ -532,9 +546,6 @@ def handle_field(node: NexusNode, keys: Mapping[str, Any], prev_path: str):
                 prev_path=f"{prev_path}/{variant}",
             )
 
-        # TODO: Build variadic map for fields and attributes
-        # Introduce variadic siblings in NexusNode?
-
     def handle_attribute(node: NexusNode, keys: Mapping[str, Any], prev_path: str):
         full_path = remove_from_not_visited(f"{prev_path}/@{node.name}")
         variants = get_variations_of(node, keys)
@@ -559,6 +570,7 @@ def handle_attribute(node: NexusNode, keys: Mapping[str, Any], prev_path: str):
                 node.open_enum,
                 f"{prev_path}/{variant if variant.startswith('@') else f'@{variant}'}",
             )
+            _ = check_reserved_prefix(f"{prev_path}/{variant}", mapping, "attribute")
 
     def handle_choice(node: NexusNode, keys: Mapping[str, Any], prev_path: str):
         global collector
@@ -847,6 +859,146 @@ def startswith_with_variations(
         # default
         return (False, 0)
 
+    def check_reserved_suffix(key: str, mapping: MutableMapping[str, Any]) -> bool:
+        """
+        Check if an associated field exists for a key with a reserved suffix.
+
+        Reserved suffixes imply the presence of an associated base field (e.g.,
+        "temperature_errors" implies "temperature" must exist in the mapping).
+
+        Args:
+            key (str):
+                The full key path (e.g., "/ENTRY[entry1]/sample/temperature_errors").
+            mapping (MutableMapping[str, Any]):
+                The mapping containing the data to validate.
+                This should be a dict of `/` separated paths.
+
+        Returns:
+            bool:
+                True if the suffix usage is valid or not applicable.
+                False if the suffix is used without the expected associated base field.
+        """
+        reserved_suffixes = (
+            "_end",
+            "_increment_set",
+            "_errors",
+            "_indices",
+            "_mask",
+            "_set",
+            "_weights",
+            "_scaling_factor",
+            "_offset",
+        )
+
+        parent_path, name = key.rsplit("/", 1)
+        concept_name, instance_name = split_class_and_name_of(name)
+
+        for suffix in reserved_suffixes:
+            if instance_name.endswith(suffix):
+                associated_field = instance_name.rsplit(suffix, 1)[0]
+
+                if not any(
+                    k.startswith(parent_path + "/")
+                    and (
+                        k.endswith(associated_field)
+                        or k.endswith(f"[{associated_field}]")
+                    )
+                    for k in mapping
+                ):
+                    collector.collect_and_log(
+                        key,
+                        ValidationProblem.ReservedSuffixWithoutField,
+                        associated_field,
+                        suffix,
+                    )
+                    return False
+                break  # We found the suffix and it passed
+
+        return True
+
+    def check_reserved_prefix(
+        key: str,
+        mapping: MutableMapping[str, Any],
+        nx_type: Literal["group", "field", "attribute"],
+    ) -> bool:
+        """
+        Check if a reserved prefix was used in the correct context.
+
+        Args:
+            key (str): The full key path (e.g., "/ENTRY[entry1]/instrument/detector/@DECTRIS_config").
+            mapping (MutableMapping[str, Any]):
+                The mapping containing the data to validate.
+                This should be a dict of `/` separated paths.
+                Attributes are denoted with `@` in front of the last element.
+            nx_type (Literal["group", "field", "attribute"]):
+                The NeXus type the key represents. Determines which reserved prefixes are relevant.
+
+
+        Returns:
+            bool:
+                True if the prefix usage is valid or not applicable.
+                False if an invalid or misapplied reserved prefix is detected.
+        """
+        reserved_prefixes = {
+            "attribute": {
+                "@BLUESKY_": None,  # do not use anywhere
+                "@DECTRIS_": "NXmx",
+                "@IDF_": None,  # do not use anywhere
+                "@NDAttr": None,
+                "@NX_": "all",
+                "@PDBX_": None,  # do not use anywhere
+                "@SAS_": "NXcanSAS",
+                "@SILX_": None,  # do not use anywhere
+            },
+            "field": {
+                "DECTRIS_": "NXmx",
+            },
+        }
+
+        prefixes = reserved_prefixes.get(nx_type)
+        if not prefixes:
+            return True
+
+        name = key.rsplit("/", 1)[-1]
+
+        if not name.startswith(tuple(prefixes)):
+            return False  # Irrelevant prefix, no check needed
+
+        for prefix, allowed_context in prefixes.items():
+            if not name.startswith(prefix):
+                continue
+
+            if allowed_context is None:
+                # This prefix is disallowed entirely
+                collector.collect_and_log(
+                    prefix,
+                    ValidationProblem.ReservedPrefixInWrongContext,
+                    None,
+                    key,
+                )
+                return False
+            if allowed_context == "all":
+                # We can freely use this prefix everywhere.
+                return True
+
+            # Check that the prefix is used in the correct context.
+            match = re.match(r"(/ENTRY\[[^]]+])", key)
+            definition_value = None
+            if match:
+                definition_key = f"{match.group(1)}/definition"
+                definition_value = mapping.get(definition_key)
+
+            if definition_value != allowed_context:
+                collector.collect_and_log(
+                    prefix,
+                    ValidationProblem.ReservedPrefixInWrongContext,
+                    allowed_context,
+                    key,
+                )
+                return False
+
+        return True
+
     missing_type_err = {
         "field": ValidationProblem.MissingRequiredField,
         "group": ValidationProblem.MissingRequiredGroup,
@@ -937,6 +1089,16 @@ def startswith_with_variations(
                     keys_to_remove.append(not_visited_key)
                     continue
 
+        if "@" not in not_visited_key.rsplit("/", 1)[-1]:
+            check_reserved_suffix(not_visited_key, mapping)
+            check_reserved_prefix(not_visited_key, mapping, "field")
+
+        else:
+            associated_field = not_visited_key.rsplit("/", 1)[-2]
+            # Check the prefix both for this attribute and the field it belongs to
+            check_reserved_prefix(not_visited_key, mapping, "attribute")
+            check_reserved_prefix(associated_field, mapping, "field")
+
         if is_documented(not_visited_key, tree):
             continue
 
diff --git a/tests/dataconverter/test_validation.py b/tests/dataconverter/test_validation.py
@@ -1216,13 +1216,78 @@ def listify_template(data_dict: Template):
             [],
             id="nonvariadic-nxcollection",
         ),
+        pytest.param(
+            alter_dict(
+                alter_dict(
+                    TEMPLATE,
+                    "/ENTRY[my_entry]/OPTIONAL_group[my_group]/required_field_set",
+                    1,
+                ),
+                "/ENTRY[my_entry]/OPTIONAL_group[my_group]/some_field_set",
+                1,
+            ),
+            [
+                "Reserved suffix '_set' was used in /ENTRY[my_entry]/OPTIONAL_group[my_group]/some_field_set, "
+                "but there is no associated field some_field."
+            ],
+            id="reserved-suffix-from-appdef",
+        ),
+        pytest.param(
+            alter_dict(
+                alter_dict(
+                    TEMPLATE,
+                    "/ENTRY[my_entry]/OPTIONAL_group[my_group]/FIELDNAME_weights[required_field_weights]",
+                    0.1,
+                ),
+                "/ENTRY[my_entry]/OPTIONAL_group[my_group]/FIELDNAME_weights[some_random_field_weights]",
+                0.1,
+            ),
+            [
+                "Reserved suffix '_weights' was used in /ENTRY[my_entry]/OPTIONAL_group[my_group]/FIELDNAME_weights[some_random_field_weights], but there is no associated field some_random_field.",
+            ],
+            id="reserved-suffix-from-base-class",
+        ),
+        pytest.param(
+            alter_dict(
+                alter_dict(
+                    alter_dict(
+                        alter_dict(
+                            TEMPLATE,
+                            "/ENTRY[my_entry]/OPTIONAL_group[my_group]/@BLUESKY_attr",
+                            "some text",
+                        ),
+                        "/ENTRY[my_entry]/OPTIONAL_group[my_group]/@DECTRIS_attr",
+                        "some text",
+                    ),
+                    "/ENTRY[my_entry]/OPTIONAL_group[my_group]/DECTRIS_field",
+                    "some text",
+                ),
+                "/ENTRY[my_entry]/OPTIONAL_group[my_group]/@NX_attr",
+                "some text",
+            ),
+            [
+                "Reserved prefix @BLUESKY_ was used in key /ENTRY[my_entry]/OPTIONAL_group[my_group]/@BLUESKY_attr, but is not valid here.",
+                "Attribute /ENTRY[my_entry]/OPTIONAL_group[my_group]/@BLUESKY_attr written without documentation.",
+                "Reserved prefix @DECTRIS_ was used in key /ENTRY[my_entry]/OPTIONAL_group[my_group]/@DECTRIS_attr, but is not valid here. "
+                "It is only valid in the context of NXmx.",
+                "Attribute /ENTRY[my_entry]/OPTIONAL_group[my_group]/@DECTRIS_attr written without documentation.",
+                "Reserved prefix DECTRIS_ was used in key /ENTRY[my_entry]/OPTIONAL_group[my_group]/DECTRIS_field, but is not valid here. "
+                "It is only valid in the context of NXmx.",
+                "Field /ENTRY[my_entry]/OPTIONAL_group[my_group]/DECTRIS_field written without documentation.",
+                "Attribute /ENTRY[my_entry]/OPTIONAL_group[my_group]/@NX_attr written without documentation.",
+            ],
+            id="reserved-prefix",
+        ),
     ],
 )
 def test_validate_data_dict(caplog, data_dict, error_messages, request):
     """Unit test for the data validation routine."""
 
     def format_error_message(msg: str) -> str:
-        return msg[msg.rfind("G: ") + 3 :].rstrip("\n")
+        for prefix in ("ERROR:", "WARNING:"):
+            if msg.startswith(prefix):
+                return msg[len(prefix) :].lstrip()
+        return msg
 
     if not error_messages:
         with caplog.at_level(logging.WARNING):
