From 70b4baa7187b5b20379a38e9fbf75815384d25c3 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Wed, 15 Jan 2025 18:11:31 +0100 Subject: [PATCH 1/2] Fix node discovery when compose-node-name is disabled Python reclass will still discover nested nodes if compose-node-name is disabled, but will strip the prefix path from the resulting node names (and the `parts` metadata). This commit adjusts reclass-rs to behave identically to Python reclass for inventories which contain nodes in subfolders of `inventory/nodes` when compose-node-name isn't enabled. Fixes #148 --- src/inventory.rs | 110 ++++++++++++++++++ src/lib.rs | 72 ++++++++---- src/node/mod.rs | 8 +- tests/inventory-nested-nodes/classes/cls1.yml | 3 + tests/inventory-nested-nodes/nodes/_d/d1.yml | 15 +++ tests/inventory-nested-nodes/nodes/a/a1.yml | 13 +++ tests/inventory-nested-nodes/nodes/b/b1.yml | 13 +++ tests/inventory-nested-nodes/nodes/c/c1.yml | 13 +++ 8 files changed, 225 insertions(+), 22 deletions(-) create mode 100644 tests/inventory-nested-nodes/classes/cls1.yml create mode 100644 tests/inventory-nested-nodes/nodes/_d/d1.yml create mode 100644 tests/inventory-nested-nodes/nodes/a/a1.yml create mode 100644 tests/inventory-nested-nodes/nodes/b/b1.yml create mode 100644 tests/inventory-nested-nodes/nodes/c/c1.yml diff --git a/src/inventory.rs b/src/inventory.rs index e9c8aa2..61cca61 100644 --- a/src/inventory.rs +++ b/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::>(); + 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::>(); + 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) + ) + } + } } diff --git a/src/lib.rs b/src/lib.rs index 2b1b5b5..53ec444 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -122,13 +122,13 @@ fn walk_entity_dir( kind: &EntityKind, root: &str, entity_map: &mut HashMap, - 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 `.yml` and `.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::>(); + 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::>(); + 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")); + } } diff --git a/src/node/mod.rs b/src/node/mod.rs index a937832..25b76f5 100644 --- a/src/node/mod.rs +++ b/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) } diff --git a/tests/inventory-nested-nodes/classes/cls1.yml b/tests/inventory-nested-nodes/classes/cls1.yml new file mode 100644 index 0000000..d5298b8 --- /dev/null +++ b/tests/inventory-nested-nodes/classes/cls1.yml @@ -0,0 +1,3 @@ +parameters: + foo: foo + bar: bar diff --git a/tests/inventory-nested-nodes/nodes/_d/d1.yml b/tests/inventory-nested-nodes/nodes/_d/d1.yml new file mode 100644 index 0000000..d89953d --- /dev/null +++ b/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 diff --git a/tests/inventory-nested-nodes/nodes/a/a1.yml b/tests/inventory-nested-nodes/nodes/a/a1.yml new file mode 100644 index 0000000..91512ac --- /dev/null +++ b/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 diff --git a/tests/inventory-nested-nodes/nodes/b/b1.yml b/tests/inventory-nested-nodes/nodes/b/b1.yml new file mode 100644 index 0000000..7a8419a --- /dev/null +++ b/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 diff --git a/tests/inventory-nested-nodes/nodes/c/c1.yml b/tests/inventory-nested-nodes/nodes/c/c1.yml new file mode 100644 index 0000000..f65a659 --- /dev/null +++ b/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 From 4e7ccc8f9929e086a624b9176cbf5c8c6a0e6393 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Wed, 15 Jan 2025 18:48:10 +0100 Subject: [PATCH 2/2] Update Python tests to match fixed behavior --- tests/test_compose_node_name.py | 41 ++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/tests/test_compose_node_name.py b/tests/test_compose_node_name.py index d336eca..14b79f9 100644 --- a/tests/test_compose_node_name.py +++ b/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():