fix: use nac-yaml typ=safe to return plain dict/list (#788) (#796)

* fix: strip ruamel types at merge boundary to prevent Jinja2 attr collisions (#788)

Convert CommentedMap/CommentedSeq to plain dict/list via recursive
_to_builtin_types() in DataMerger.merge_data_files(), so Jinja2 templates
never see ruamel-internal attributes (tag, anchor, ca, etc.) via dot-notation.

This replaces the KeyFirstEnvironment approach from #789 with a simpler
solution — no blocklists, no custom Jinja2 Environment, no edge cases.

- Add _to_builtin_types() in data_merger.py (single-pass, no JSON overhead)
- Revert robot_writer.py to standard jinja2.Environment
- Add unit tests enforcing the no-ruamel-types contract
- Add integration test fixtures for attr collision regression

* fix: use nac-yaml typ=safe to return plain types, remove _to_builtin_types

Leverage nac-yaml's new typ='safe' parameter in load_yaml_files() to get
plain dict/list directly from the YAML parser, removing the need for
_to_builtin_types() post-processing in DataMerger.

- Pin nac-yaml to oboehmer/nac-yaml@load-typ branch with typ support
- Remove _to_builtin_types() and its unit tests from data_merger
- Keep contract tests and integration tests as regression guard

* build: allow direct references in hatch for nac-yaml git dep

* remove redundant method test

* change Any -> object typing

* update nac-yaml==2.0.0b2

Author: oboehmer

nac_test/data_merger.py

@@ -35,7 +35,7 @@ def merge_data_files(data_paths: list[Path]) -> dict[str, Any]:
         logger.info(
             "Loading yaml files from %s", ", ".join([str(path) for path in data_paths])
         )
-        data = yaml.load_yaml_files(data_paths)
+        data = yaml.load_yaml_files(data_paths, typ="safe")
         # Ensure we always return a dict, even if yaml returns None
         return data if isinstance(data, dict) else {}
 

nac_test/robot/robot_writer.py

@@ -9,7 +9,6 @@
 import re
 import shutil
 import sys
-from collections.abc import Mapping
 from pathlib import Path
 from typing import Any
 
@@ -31,14 +30,6 @@ class StrictChainableUndefined(ChainableUndefined):
     __contains__ = Undefined._fail_with_undefined_error
 
 
-class KeyFirstEnvironment(Environment):
-    # Prefer Mapping keys over attributes to avoid ruamel/Jinja dot-notation collisions (e.g. `tag`).
-    def getattr(self, obj: Any, attribute: str) -> Any:
-        if isinstance(obj, Mapping) and attribute in obj:
-            return obj[attribute]
-        return super().getattr(obj, attribute)
-
-
 class TestCollector(SuiteVisitor):  # type: ignore[misc]
     """Visitor to collect test or suite names to construct the pabot ordering file.
 
@@ -303,7 +294,7 @@ def write(
         self, templates_path: Path, output_path: Path, ordering_file: Path | None = None
     ) -> None:
         """Render Robot test suites."""
-        env = KeyFirstEnvironment(  # nosec B701
+        env = Environment(  # nosec B701
             loader=FileSystemLoader(templates_path),
             undefined=StrictChainableUndefined,
             lstrip_blocks=True,

pyproject.toml

@@ -32,7 +32,7 @@ dependencies = [
     "Jinja2>=3.1.6",
     "jmespath>=1.0.1",
     "nac-test-pyats-common>=0.3.0",
-    "nac-yaml==2.0.0a0",
+    "nac-yaml==2.0.0b2",
     "robotframework>=7.3.2",
     "robotframework-jmespath",
     "robotframework-jsonlibrary>=0.5",

tests/integration/fixtures/data_attr_collision/root.yaml

@@ -0,0 +1,26 @@
+---
+root:
+  children:
+    - name: abc
+      param: value
+      tag: 100
+      items: foo
+      keys: bar
+    - name: def
+      param: value
+      items: foo
+      keys: bar
+
+  # Nested structure with collision keys at different levels (also tests CommentedSeq)
+  fabric:
+    spine:
+      node_id: 101
+      interfaces:
+        - name: Ethernet1/1
+          tag: trunk
+        - name: Ethernet1/2
+          tag: access
+
+defaults:
+  tag: fallback
+  description: "default desc"

tests/integration/fixtures/data_attr_collision_dir/root.yaml

@@ -0,0 +1,14 @@
+*** Settings ***
+Documentation   Test collision key access via bracket notation and non-collision dot access
+
+*** Test Cases ***
+{% for child in root.children | default([]) %}
+
+Test {{ child.name }}
+    Should Be Equal   {{ child.param }}   value   msg=param_{{ child.name }}
+    Should Be Equal   {{ child.tag | default(defaults.tag) }}   {% if child.name == 'abc' %}100{% else %}fallback{% endif %}   msg=tag_{{ child.name }}
+    # 'items' and 'keys' collide with dict method names — dot access (child.items)
+    # would resolve to the dict method, not the YAML key value. Use bracket notation.
+    Should Be Equal   {{ child['items'] }}   foo   msg=items_key_{{ child.name }}
+    Should Be Equal   {{ child['keys'] }}   bar   msg=keys_key_{{ child.name }}
+{% endfor %}

tests/integration/fixtures/templates_attr_collision/test.robot

@@ -0,0 +1,17 @@
+*** Settings ***
+Documentation   Test selectattr/rejectattr/map(attribute=) with collision keys like 'tag'
+
+*** Test Cases ***
+Test selectattr with tag key
+    {% set trunk_ports = root.fabric.spine.interfaces | selectattr('tag', 'equalto', 'trunk') | list %}
+    Should Be Equal    {{ trunk_ports | length }}    1    msg=selectattr_count
+    Should Be Equal    {{ trunk_ports[0].name }}    Ethernet1/1    msg=selectattr_name
+
+Test rejectattr with tag key
+    {% set non_trunk = root.fabric.spine.interfaces | rejectattr('tag', 'equalto', 'trunk') | list %}
+    Should Be Equal    {{ non_trunk | length }}    1    msg=rejectattr_count
+    Should Be Equal    {{ non_trunk[0].name }}    Ethernet1/2    msg=rejectattr_name
+
+Test map attribute extraction with collision key
+    {% set all_tags = root.fabric.spine.interfaces | map(attribute='tag') | list %}
+    Should Be Equal    {{ all_tags | join(',') }}    trunk,access    msg=map_tags

tests/integration/fixtures/templates_attr_collision/test_selectattr.robot

@@ -5,8 +5,5 @@ Documentation   Test1
 {% for child in root.children | default([]) %}
 
 Test {{ child.name }}
-    Should Be Equal   {{ child.param }}   value
-    Log    tag={{ child.tag }}
-    Log    items={{ child.items }}
-    Log    keys={{ child.keys }}
+    Should Be Equal   {{ child.param is test1 child.param }}   True
 {% endfor %}

tests/integration/fixtures/templates_test/test1.robot

@@ -9,6 +9,7 @@
 """
 
 import filecmp
+import re
 from pathlib import Path
 
 import pytest
@@ -338,9 +339,16 @@ def test_render_only_without_controller_credentials(tmp_path: Path) -> None:
 
 
 def test_dict_key_attribute_collision_renders_key_values(tmp_path: Path) -> None:
+    """Comprehensive test for key-attribute collision handling.
+
+    Tests three categories with a single data dir + template dir:
+    1. test.robot — collision keys (items, keys) via bracket notation; non-collision (tag) via dot access
+    2. test_methods.robot — .items()/.get()/.keys()/.values() method calls on clean mappings
+    3. test_selectattr.robot — selectattr/rejectattr/map(attribute=) with collision key 'tag'
+    """
     runner = CliRunner()
-    data_path = "tests/integration/fixtures/data_attr_collision_dir"
-    templates_path = "tests/integration/fixtures/templates_test"
+    data_path = "tests/integration/fixtures/data_attr_collision"
+    templates_path = "tests/integration/fixtures/templates_attr_collision"
 
     result = runner.invoke(
         nac_test.cli.main.app,
@@ -351,12 +359,39 @@ def test_dict_key_attribute_collision_renders_key_values(tmp_path: Path) -> None
             templates_path,
             "-o",
             str(tmp_path),
-            "--render-only",
         ],
     )
-    assert result.exit_code == 0, result.output
+    assert result.exit_code == 0, f"CLI rendering failed:\n{result.output}"
 
-    output = (tmp_path / ROBOT_RESULTS_DIRNAME / "test1.robot").read_text()
-    assert "tag=100" in output
-    assert "items=foo" in output
-    assert "keys=bar" in output
+    output_dir = tmp_path / ROBOT_RESULTS_DIRNAME
+
+    # --- test.robot: collision keys via bracket notation ---
+    output_collision = (output_dir / "test.robot").read_text()
+    assert re.search(r"Should Be Equal\s+100\s+100\s+msg=tag_abc", output_collision), (
+        "tag key dot-access did not render '100' for child abc"
+    )
+    assert re.search(
+        r"Should Be Equal\s+foo\s+foo\s+msg=items_key_abc", output_collision
+    ), "items collision key did not render 'foo' via bracket notation"
+    assert re.search(
+        r"Should Be Equal\s+bar\s+bar\s+msg=keys_key_abc", output_collision
+    ), "keys collision key did not render 'bar' via bracket notation"
+    assert "{{" not in output_collision, "Unresolved Jinja2 expression in test.robot"
+
+    # --- test_selectattr.robot: selectattr/rejectattr/map ---
+    output_select = (output_dir / "test_selectattr.robot").read_text()
+    assert re.search(
+        r"Should Be Equal\s+Ethernet1/1\s+Ethernet1/1\s+msg=selectattr_name",
+        output_select,
+    ), "selectattr didn't find trunk port Ethernet1/1"
+    assert re.search(
+        r"Should Be Equal\s+Ethernet1/2\s+Ethernet1/2\s+msg=rejectattr_name",
+        output_select,
+    ), "rejectattr didn't find non-trunk port Ethernet1/2"
+    assert re.search(
+        r"Should Be Equal\s+trunk,access\s+trunk,access\s+msg=map_tags",
+        output_select,
+    ), "map(attribute='tag') didn't produce 'trunk,access'"
+    assert "{{" not in output_select, (
+        "Unresolved Jinja2 expression in test_selectattr.robot"
+    )

tests/integration/test_cli_rendering.py

@@ -97,3 +97,4 @@ def test_creates_output_file_and_parent_directories(
         )
         assert output.exists()
         assert output.parent.is_dir()
+        assert output.read_text().strip()  # non-empty content