refactor(tvix/castore): remove `name` from Nodes r/8504

Nodes only have names if they're contained inside a Directory, or if
they're a root node and have something else possibly giving them a name
externally.

This removes all `name` fields in the three different Nodes, and instead
maintains it inside a BTreeMap inside the Directory.

It also removes the NamedNode trait (they don't have a get_name()), as
well as Node::rename(self, name), and all [Partial]Ord implementations
for Node (as they don't have names to use for sorting).

The `nodes()`, `directories()`, `files()` iterators inside a `Directory`
now return a tuple of Name and Node, as does the RootNodesProvider.

The different {Directory,File,Symlink}Node struct constructors got
simpler, and the {Directory,File}Node ones became infallible - as
there's no more possibility to represent invalid state.

The proto structs stayed the same - there's now from_name_and_node and
into_name_and_node to convert back and forth between the two `Node`
structs.

Some further cleanups:

The error types for Node validation were renamed. Everything related to
names is now in the DirectoryError (not yet happy about the naming)

There's some leftover cleanups to do:
 - There should be a from_(sorted_)iter and into_iter in Directory, so
   we can construct and deconstruct in one go.
   That should also enable us to implement conversions from and to the
   proto representation that moves, rather than clones.

 - The BuildRequest and PathInfo structs are still proto-based, so we
   still do a bunch of conversions back and forth there (and have some
   ugly expect there). There's not much point for error handling here,
   this will be moved to stricter types in a followup CL.

Change-Id: I7369a8e3a426f44419c349077cb4fcab2044ebb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12205
Tested-by: BuildkiteCI
Reviewed-by: yuka <yuka@yuka.dev>
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: benjaminedwardwebb <benjaminedwardwebb@gmail.com>
Reviewed-by: Connor Brewster <cbrewster@hey.com>

3 files changed, 28 insertions, 23 deletions

diff --git a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
index e420801ce528..60da73012df7 100644
--- a/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
+++ b/tvix/store/src/proto/grpc_pathinfoservice_wrapper.rs
@@ -74,7 +74,7 @@ where
         &self,
         request: Request<castorepb::Node>,
     ) -> Result<Response<proto::CalculateNarResponse>> {
-        let root_node = (&request.into_inner()).try_into().map_err(|e| {
+        let (_, root_node) = request.into_inner().into_name_and_node().map_err(|e| {
             warn!(err = %e, "invalid root node");
             Status::invalid_argument("invalid root node")
         })?;
diff --git a/tvix/store/src/proto/mod.rs b/tvix/store/src/proto/mod.rs
index 375fb0dcadb8..13b24f3aaeb4 100644
--- a/tvix/store/src/proto/mod.rs
+++ b/tvix/store/src/proto/mod.rs
@@ -9,7 +9,7 @@ use nix_compat::{
     store_path::{self, StorePathRef},
 };
 use thiserror::Error;
-use tvix_castore::{NamedNode, ValidateNodeError};
+use tvix_castore::DirectoryError;
 
 mod grpc_pathinfoservice_wrapper;
 
@@ -39,7 +39,7 @@ pub enum ValidatePathInfoError {
 
     /// Node fails validation
     #[error("Invalid root node: {:?}", .0.to_string())]
-    InvalidRootNode(ValidateNodeError),
+    InvalidRootNode(DirectoryError),
 
     /// Invalid node name encountered. Root nodes in PathInfos have more strict name requirements
     #[error("Failed to parse {} as StorePath: {1}", .0.to_str_lossy())]
@@ -160,12 +160,13 @@ impl PathInfo {
         let root_nix_path = match &self.node {
             None => Err(ValidatePathInfoError::NoNodePresent)?,
             Some(node) => {
-                // TODO save result somewhere
-                let node: tvix_castore::Node = node
-                    .try_into()
+                let (name, _node) = node
+                    .clone()
+                    .into_name_and_node()
                     .map_err(ValidatePathInfoError::InvalidRootNode)?;
+
                 // parse the name of the node itself and return
-                parse_node_name_root(node.get_name(), ValidatePathInfoError::InvalidNodeName)?
+                parse_node_name_root(name.as_ref(), ValidatePathInfoError::InvalidNodeName)?
                     .to_owned()
             }
         };
diff --git a/tvix/store/src/proto/tests/pathinfo.rs b/tvix/store/src/proto/tests/pathinfo.rs
index 928bb8c8b185..ed8c64f6ccf8 100644
--- a/tvix/store/src/proto/tests/pathinfo.rs
+++ b/tvix/store/src/proto/tests/pathinfo.rs
@@ -6,11 +6,11 @@ use nix_compat::nixbase32;
 use nix_compat::store_path::{self, StorePath, StorePathRef};
 use rstest::rstest;
 use tvix_castore::proto as castorepb;
-use tvix_castore::ValidateNodeError;
+use tvix_castore::{DirectoryError, ValidateNodeError};
 
 #[rstest]
 #[case::no_node(None, Err(ValidatePathInfoError::NoNodePresent))]
-#[case::no_node_2(Some(castorepb::Node { node: None}), Err(ValidatePathInfoError::InvalidRootNode(ValidateNodeError::NoNodeSet)))]
+#[case::no_node_2(Some(castorepb::Node { node: None}), Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::NoNodeSet)))]
 
 fn validate_pathinfo(
     #[case] node: Option<castorepb::Node>,
@@ -35,7 +35,7 @@ fn validate_pathinfo(
         name: DUMMY_PATH.into(),
         digest: Bytes::new(),
         size: 0,
-}, Err(ValidatePathInfoError::InvalidRootNode(tvix_castore::ValidateNodeError::InvalidDigestLen(0))))]
+}, Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::InvalidNode(DUMMY_PATH.into(), ValidateNodeError::InvalidDigestLen(0)))))]
 #[case::invalid_node_name_no_storepath(castorepb::DirectoryNode {
         name: "invalid".into(),
         digest: DUMMY_DIGEST.clone().into(),
@@ -74,7 +74,7 @@ fn validate_directory(
         digest: Bytes::new(),
         ..Default::default()
     },
-    Err(ValidatePathInfoError::InvalidRootNode(tvix_castore::ValidateNodeError::InvalidDigestLen(0)))
+    Err(ValidatePathInfoError::InvalidRootNode(DirectoryError::InvalidNode(DUMMY_PATH.into(), ValidateNodeError::InvalidDigestLen(0))))
 )]
 #[case::invalid_node_name(
     castorepb::FileNode {
@@ -226,24 +226,28 @@ fn validate_inconsistent_narinfo_reference_name_digest() {
 /// Create a node with an empty symlink target, and ensure it fails validation.
 #[test]
 fn validate_symlink_empty_target_invalid() {
-    let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode {
-        name: "foo".into(),
-        target: "".into(),
-    });
-
-    tvix_castore::Node::try_from(&node).expect_err("must fail validation");
+    castorepb::Node {
+        node: Some(castorepb::node::Node::Symlink(castorepb::SymlinkNode {
+            name: "foo".into(),
+            target: "".into(),
+        })),
+    }
+    .into_name_and_node()
+    .expect_err("must fail validation");
 }
 
 /// Create a node with a symlink target including null bytes, and ensure it
 /// fails validation.
 #[test]
 fn validate_symlink_target_null_byte_invalid() {
-    let node = castorepb::node::Node::Symlink(castorepb::SymlinkNode {
-        name: "foo".into(),
-        target: "foo\0".into(),
-    });
-
-    tvix_castore::Node::try_from(&node).expect_err("must fail validation");
+    castorepb::Node {
+        node: Some(castorepb::node::Node::Symlink(castorepb::SymlinkNode {
+            name: "foo".into(),
+            target: "foo\0".into(),
+        })),
+    }
+    .into_name_and_node()
+    .expect_err("must fail validation");
 }
 
 /// Create a PathInfo with a correct deriver field and ensure it succeeds.