Fix node discovery when compose-node-name is disabled

src/inventory.rs

@@ -352,4 +352,114 @@ mod inventory_tests {
             Some(&sequence(&["a.1"]))
         );
     }
+
+    #[test]
+    fn test_render_nested_nodes() {
+        let mut c =
+            crate::Config::new(Some("./tests/inventory-nested-nodes"), None, None, None).unwrap();
+        c.compose_node_name = false;
+        let r = Reclass::new_from_config(c).unwrap();
+
+        let inv = Inventory::render(&r).unwrap();
+
+        let invabsdir = std::fs::canonicalize("./tests/inventory-nested-nodes").unwrap();
+        let invabsdir = invabsdir.to_str().unwrap();
+
+        let mut nodes = inv.nodes.keys().collect::<Vec<_>>();
+        nodes.sort();
+        assert_eq!(nodes, vec!["a1", "b1", "c1", "d1"]);
+
+        for n in nodes {
+            let node = &inv.nodes[n];
+            assert_eq!(node.reclass.node, *n);
+            assert_eq!(node.reclass.name, *n);
+            let expected = node.parameters.get(&"nested".into()).unwrap();
+            let expected_full_name = expected.get(&"node_name".into()).unwrap();
+            let expected_short_name = expected.get(&"short_name".into()).unwrap();
+            let expected_path = expected.get(&"path".into()).unwrap();
+            let expected_parts = expected.get(&"parts".into()).unwrap();
+            let expected_uri_suffix = expected.get(&"uri_suffix".into()).unwrap();
+            assert_eq!(
+                node.reclass.uri,
+                format!(
+                    "yaml_fs://{invabsdir}/nodes/{}",
+                    expected_uri_suffix.as_str().unwrap()
+                )
+            );
+            let params_reclass_name = node
+                .parameters
+                .get(&"_reclass_".into())
+                .unwrap()
+                .get(&"name".into())
+                .unwrap();
+            assert_eq!(
+                params_reclass_name.get(&"full".into()),
+                Some(expected_full_name)
+            );
+            assert_eq!(
+                params_reclass_name.get(&"short".into()),
+                Some(expected_short_name)
+            );
+            assert_eq!(params_reclass_name.get(&"path".into()), Some(expected_path));
+            assert_eq!(
+                params_reclass_name.get(&"parts".into()),
+                Some(expected_parts)
+            )
+        }
+    }
+
+    #[test]
+    fn test_render_nested_nodes_composed() {
+        let mut c =
+            crate::Config::new(Some("./tests/inventory-nested-nodes"), None, None, None).unwrap();
+        c.compose_node_name = true;
+        let r = Reclass::new_from_config(c).unwrap();
+
+        let inv = Inventory::render(&r).unwrap();
+
+        let invabsdir = std::fs::canonicalize("./tests/inventory-nested-nodes").unwrap();
+        let invabsdir = invabsdir.to_str().unwrap();
+
+        let mut nodes = inv.nodes.keys().collect::<Vec<_>>();
+        nodes.sort();
+        assert_eq!(nodes, vec!["a.a1", "b.b1", "c.c1", "d1"]);
+
+        for n in nodes {
+            let node = &inv.nodes[n];
+            assert_eq!(node.reclass.node, *n);
+            assert_eq!(node.reclass.name, *n);
+            let expected = node.parameters.get(&"composed".into()).unwrap();
+            let expected_full_name = expected.get(&"node_name".into()).unwrap();
+            let expected_short_name = expected.get(&"short_name".into()).unwrap();
+            let expected_path = expected.get(&"path".into()).unwrap();
+            let expected_parts = expected.get(&"parts".into()).unwrap();
+            let expected_uri_suffix = expected.get(&"uri_suffix".into()).unwrap();
+            assert_eq!(
+                node.reclass.uri,
+                format!(
+                    "yaml_fs://{invabsdir}/nodes/{}",
+                    expected_uri_suffix.as_str().unwrap()
+                )
+            );
+            let params_reclass_name = node
+                .parameters
+                .get(&"_reclass_".into())
+                .unwrap()
+                .get(&"name".into())
+                .unwrap();
+            assert_eq!(
+                params_reclass_name.get(&"full".into()),
+                Some(expected_full_name)
+            );
+            assert_eq!(
+                params_reclass_name.get(&"short".into()),
+                Some(expected_short_name)
+            );
+            assert_eq!(params_reclass_name.get(&"path".into()), Some(expected_path));
+            assert_eq!(
+                params_reclass_name.get(&"parts".into()),
+                Some(expected_parts)
+            )
+        }
+    }
 }

src/lib.rs

@@ -122,13 +122,13 @@ fn walk_entity_dir(
     kind: &EntityKind,
     root: &str,
     entity_map: &mut HashMap<String, EntityInfo>,
-    max_depth: usize,
+    compose_node_name: bool,
 ) -> Result<()> {
     let entity_root = to_lexical_absolute(&PathBuf::from(root))?;
 
     // We need to follow symlinks when walking the root directory, so that inventories which
     // contain symlinked directories are loaded correctly.
-    for entry in WalkDir::new(root).max_depth(max_depth).follow_links(true) {
+    for entry in WalkDir::new(root).follow_links(true) {
         let entry = entry?;
         // We use `entry.path()` here to get the symlink name for symlinked files.
         let ext = if let Some(ext) = entry.path().extension() {
@@ -164,18 +164,21 @@ fn walk_entity_dir(
                 "Failed to normalize entity {}",
                 entry.path().display()
             ))?;
-            let (cls, loc) = if kind == &EntityKind::Node && max_depth > 1 && cls.starts_with('_') {
-                // special case node paths starting with _ for compose-node-name
-                (
-                    cls.split(MAIN_SEPARATOR).last().ok_or(anyhow!(
-                        "Can't shorten node name for {}",
-                        entry.path().display()
-                    ))?,
-                    Path::new(""),
-                )
-            } else {
-                (cls, loc)
-            };
+            let (cls, loc) =
+                if kind == &EntityKind::Node && (cls.starts_with('_') || !compose_node_name) {
+                    // special case node paths starting with _ for compose-node-name and return
+                    // only base name for all nodes regardless of depth if compose-node-name isn't
+                    // enabled.
+                    (
+                        cls.split(MAIN_SEPARATOR).last().ok_or(anyhow!(
+                            "Can't shorten node name for {}",
+                            entry.path().display()
+                        ))?,
+                        Path::new(""),
+                    )
+                } else {
+                    (cls, loc)
+                };
             let cls = cls.replace(MAIN_SEPARATOR, ".");
             if let Some(prev) = entity_map.get(&cls) {
                 return err_duplicate_entity(kind, root, relpath, &cls, &prev.path);
@@ -227,16 +230,11 @@ impl Reclass {
     /// exist. Currently the only case where this can happen is when an inventory defines a node as
     /// both `<name>.yml` and `<name>.yaml`.
     fn discover_nodes(&mut self) -> Result<()> {
-        let depth = if self.config.compose_node_name {
-            usize::MAX
-        } else {
-            1
-        };
         walk_entity_dir(
             &EntityKind::Node,
             &self.config.nodes_path,
             &mut self.nodes,
-            depth,
+            self.config.compose_node_name,
         )
     }
 
@@ -250,7 +248,7 @@ impl Reclass {
             &EntityKind::Class,
             &self.config.classes_path,
             &mut self.classes,
-            usize::MAX,
+            true,
         )
     }
 
@@ -471,4 +469,36 @@ mod tests {
         assert_eq!(r.nodes["d1"].path, PathBuf::from("_d/d1.yml"));
         assert_eq!(r.nodes["d2"].path, PathBuf::from("_d/d/d2.yml"));
     }
+
+    #[test]
+    fn test_reclass_discover_nodes_nested() {
+        let mut c = Config::new(Some("./tests/inventory-nested-nodes"), None, None, None).unwrap();
+        c.compose_node_name = false;
+        let r = Reclass::new_from_config(c).unwrap();
+        assert_eq!(r.nodes.len(), 4);
+        let mut nodes = r.nodes.keys().collect::<Vec<_>>();
+        nodes.sort();
+        assert_eq!(nodes, vec!["a1", "b1", "c1", "d1"]);
+
+        assert_eq!(r.nodes["a1"].path, PathBuf::from("a/a1.yml"));
+        assert_eq!(r.nodes["b1"].path, PathBuf::from("b/b1.yml"));
+        assert_eq!(r.nodes["c1"].path, PathBuf::from("c/c1.yml"));
+        assert_eq!(r.nodes["d1"].path, PathBuf::from("_d/d1.yml"));
+    }
+
+    #[test]
+    fn test_reclass_discover_nodes_nested_composed() {
+        let mut c = Config::new(Some("./tests/inventory-nested-nodes"), None, None, None).unwrap();
+        c.compose_node_name = true;
+        let r = Reclass::new_from_config(c).unwrap();
+        assert_eq!(r.nodes.len(), 4);
+        let mut nodes = r.nodes.keys().collect::<Vec<_>>();
+        nodes.sort();
+        assert_eq!(nodes, vec!["a.a1", "b.b1", "c.c1", "d1"]);
+
+        assert_eq!(r.nodes["a.a1"].path, PathBuf::from("a/a1.yml"));
+        assert_eq!(r.nodes["b.b1"].path, PathBuf::from("b/b1.yml"));
+        assert_eq!(r.nodes["c.c1"].path, PathBuf::from("c/c1.yml"));
+        assert_eq!(r.nodes["d1"].path, PathBuf::from("_d/d1.yml"));
+    }
 }

src/node/mod.rs

@@ -48,7 +48,13 @@ impl Node {
         let ncontents = std::fs::read_to_string(invpath.canonicalize()?)?;
 
         let uri = format!("yaml_fs://{}", to_lexical_absolute(&invpath)?.display());
-        let meta = NodeInfoMeta::new(name, name, &uri, nodeinfo.path.with_extension(""), "base");
+        // NOTE(sg): parts is only the name when compose-node-name isn't enabled
+        let meta_parts = if r.config.compose_node_name {
+            nodeinfo.path.with_extension("")
+        } else {
+            PathBuf::from(name)
+        };
+        let meta = NodeInfoMeta::new(name, name, &uri, meta_parts, "base");
         Node::from_str(meta, None, &ncontents)
     }
 

tests/inventory-nested-nodes/classes/cls1.yml

@@ -0,0 +1,3 @@
+parameters:
+  foo: foo
+  bar: bar

tests/inventory-nested-nodes/nodes/_d/d1.yml

@@ -0,0 +1,15 @@
+# compose-node-name treats nodes whose path in `nodes_path` starts with `_`
+# as top-level nodes.
+parameters:
+  composed:
+    node_name: d1
+    short_name: d1
+    path: d1
+    parts: ["d1"]
+    uri_suffix: _d/d1.yml
+  nested:
+    node_name: d1
+    short_name: d1
+    path: d1
+    parts: [d1]
+    uri_suffix: _d/d1.yml

tests/inventory-nested-nodes/nodes/a/a1.yml

@@ -0,0 +1,13 @@
+parameters:
+  composed:
+    node_name: a.a1
+    short_name: a1
+    path: a/a1
+    parts: ["a", "a1"]
+    uri_suffix: a/a1.yml
+  nested:
+    node_name: a1
+    short_name: a1
+    path: a1
+    parts: ["a1"]
+    uri_suffix: a/a1.yml

tests/inventory-nested-nodes/nodes/b/b1.yml

@@ -0,0 +1,13 @@
+parameters:
+  composed:
+    node_name: b.b1
+    short_name: b1
+    path: b/b1
+    parts: ["b", "b1"]
+    uri_suffix: b/b1.yml
+  nested:
+    node_name: b1
+    short_name: b1
+    path: b1
+    parts: ["b1"]
+    uri_suffix: b/b1.yml

tests/inventory-nested-nodes/nodes/c/c1.yml

@@ -0,0 +1,13 @@
+parameters:
+  composed:
+    node_name: c.c1
+    short_name: c1
+    path: c/c1
+    parts: ["c", "c1"]
+    uri_suffix: c/c1.yml
+  nested:
+    node_name: c1
+    short_name: c1
+    path: c1
+    parts: ["c1"]
+    uri_suffix: c/c1.yml

tests/test_compose_node_name.py

@@ -1,37 +1,50 @@
+import pytest
 import reclass_rs
 
 
+def test_no_compose_node_name_compat_collision():
+    config = reclass_rs.Config.from_dict(
+        inventory_path="./tests/inventory-compose-node-name",
+        config={"reclass_rs_compat_flags": ["compose-node-name-literal-dots"]},
+    )
+    assert config.compatflags == {reclass_rs.CompatFlag.ComposeNodeNameLiteralDots}
+    with pytest.raises(ValueError) as exc:
+        r = reclass_rs.Reclass.from_config(config)
+
+    assert "Error while discovering nodes: Definition of node '1'" in str(exc)
+
+
 def test_no_compose_node_name_compat():
-    r = reclass_rs.Reclass(inventory_path="./tests/inventory-compose-node-name")
+    r = reclass_rs.Reclass(inventory_path="./tests/inventory-nested-nodes")
     r.set_compat_flag(reclass_rs.CompatFlag.ComposeNodeNameLiteralDots)
     assert not r.config.compose_node_name
     assert r.config.compatflags == {reclass_rs.CompatFlag.ComposeNodeNameLiteralDots}
 
     inv = r.inventory()
 
-    assert set(inv.nodes.keys()) == {"a.1", "a", "d"}
+    assert set(inv.nodes.keys()) == {"a1", "b1", "c1", "d1"}
 
-    a1 = inv.nodes["a.1"].parameters["_reclass_"]["name"]
-    assert a1["full"] == "a.1"
-    assert a1["parts"] == ["a.1"]
-    assert a1["path"] == "a.1"
-    assert a1["short"] == "a.1"
+    a1 = inv.nodes["a1"].parameters["_reclass_"]["name"]
+    assert a1["full"] == "a1"
+    assert a1["parts"] == ["a1"]
+    assert a1["path"] == "a1"
+    assert a1["short"] == "a1"
 
 
 def test_no_compose_node_name():
-    r = reclass_rs.Reclass(inventory_path="./tests/inventory-compose-node-name")
+    r = reclass_rs.Reclass(inventory_path="./tests/inventory-nested-nodes")
     assert not r.config.compose_node_name
     assert r.config.compatflags == set()
 
     inv = r.inventory()
 
-    assert set(inv.nodes.keys()) == {"a.1", "a", "d"}
+    assert set(inv.nodes.keys()) == {"a1", "b1", "c1", "d1"}
 
-    a1 = inv.nodes["a.1"].parameters["_reclass_"]["name"]
-    assert a1["full"] == "a.1"
-    assert a1["parts"] == ["a.1"]
-    assert a1["path"] == "a.1"
-    assert a1["short"] == "a.1"
+    a1 = inv.nodes["a1"].parameters["_reclass_"]["name"]
+    assert a1["full"] == "a1"
+    assert a1["parts"] == ["a1"]
+    assert a1["path"] == "a1"
+    assert a1["short"] == "a1"
 
 
 def test_compose_node_name_compat():