refactor(tvix/castore): have PathComponent-specific errors r/8513

Don't use DirectoryError, but PathComponentError.

Also add checks for too long path components.

Change-Id: Ia9deb9dd0351138baadb2e9c9454c3e019d5a45e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12229
Tested-by: BuildkiteCI
Reviewed-by: Ilan Joselevich <personal@ilanjoselevich.com>
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: edef <edef@edef.eu>

2 files changed, 47 insertions, 43 deletions

diff --git a/tvix/castore/src/proto/mod.rs b/tvix/castore/src/proto/mod.rs
index a40a67f53cf3..67f7b2fa72a2 100644
--- a/tvix/castore/src/proto/mod.rs
+++ b/tvix/castore/src/proto/mod.rs
@@ -1,6 +1,5 @@
-use std::{cmp::Ordering, str};
-
 use prost::Message;
+use std::cmp::Ordering;
 
 mod grpc_blobservice_wrapper;
 mod grpc_directoryservice_wrapper;
@@ -80,31 +79,42 @@ impl TryFrom<Directory> for crate::Directory {
             .try_fold(&b""[..], |prev_name, e| {
                 match e.name.as_ref().cmp(prev_name) {
                     Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
-                    Ordering::Equal => {
-                        Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
-                    }
+                    Ordering::Equal => Err(DirectoryError::DuplicateName(
+                        e.name
+                            .to_owned()
+                            .try_into()
+                            .map_err(DirectoryError::InvalidName)?,
+                    )),
                     Ordering::Greater => Ok(e.name.as_ref()),
                 }
             })?;
         value.files.iter().try_fold(&b""[..], |prev_name, e| {
             match e.name.as_ref().cmp(prev_name) {
                 Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
-                Ordering::Equal => {
-                    Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
-                }
+                Ordering::Equal => Err(DirectoryError::DuplicateName(
+                    e.name
+                        .to_owned()
+                        .try_into()
+                        .map_err(DirectoryError::InvalidName)?,
+                )),
                 Ordering::Greater => Ok(e.name.as_ref()),
             }
         })?;
         value.symlinks.iter().try_fold(&b""[..], |prev_name, e| {
             match e.name.as_ref().cmp(prev_name) {
                 Ordering::Less => Err(DirectoryError::WrongSorting(e.name.to_owned())),
-                Ordering::Equal => {
-                    Err(DirectoryError::DuplicateName(e.name.to_owned().try_into()?))
-                }
+                Ordering::Equal => Err(DirectoryError::DuplicateName(
+                    e.name
+                        .to_owned()
+                        .try_into()
+                        .map_err(DirectoryError::InvalidName)?,
+                )),
                 Ordering::Greater => Ok(e.name.as_ref()),
             }
         })?;
 
+        // FUTUREWORK: use is_sorted() once stable, and/or implement the producer for
+        // [crate::Directory::try_from_iter] iterating over all three and doing all checks inline.
         let mut elems: Vec<(PathComponent, crate::Node)> =
             Vec::with_capacity(value.directories.len() + value.files.len() + value.symlinks.len());
 
@@ -184,7 +194,7 @@ impl Node {
     pub fn into_name_and_node(self) -> Result<(PathComponent, crate::Node), DirectoryError> {
         match self.node.ok_or_else(|| DirectoryError::NoNodeSet)? {
             node::Node::Directory(n) => {
-                let name: PathComponent = n.name.try_into()?;
+                let name: PathComponent = n.name.try_into().map_err(DirectoryError::InvalidName)?;
                 let digest = B3Digest::try_from(n.digest)
                     .map_err(|e| DirectoryError::InvalidNode(name.clone(), e.into()))?;
 
@@ -196,7 +206,7 @@ impl Node {
                 Ok((name, node))
             }
             node::Node::File(n) => {
-                let name: PathComponent = n.name.try_into()?;
+                let name: PathComponent = n.name.try_into().map_err(DirectoryError::InvalidName)?;
                 let digest = B3Digest::try_from(n.digest)
                     .map_err(|e| DirectoryError::InvalidNode(name.clone(), e.into()))?;
 
@@ -210,7 +220,8 @@ impl Node {
             }
 
             node::Node::Symlink(n) => {
-                let name: PathComponent = n.name.try_into()?;
+                let name: PathComponent = n.name.try_into().map_err(DirectoryError::InvalidName)?;
+
                 let node = crate::Node::Symlink {
                     target: n
                         .target
diff --git a/tvix/castore/src/proto/tests/directory.rs b/tvix/castore/src/proto/tests/directory.rs
index 26c434ab46c0..efbc4e9f2af1 100644
--- a/tvix/castore/src/proto/tests/directory.rs
+++ b/tvix/castore/src/proto/tests/directory.rs
@@ -161,12 +161,9 @@ fn validate_invalid_names() {
             }],
             ..Default::default()
         };
-        match crate::Directory::try_from(d).expect_err("must fail") {
-            DirectoryError::InvalidName(n) => {
-                assert_eq!(n.as_ref(), b"\0")
-            }
-            _ => panic!("unexpected error"),
-        };
+
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
     }
 
     {
@@ -178,12 +175,8 @@ fn validate_invalid_names() {
             }],
             ..Default::default()
         };
-        match crate::Directory::try_from(d).expect_err("must fail") {
-            DirectoryError::InvalidName(n) => {
-                assert_eq!(n.as_ref(), b".")
-            }
-            _ => panic!("unexpected error"),
-        };
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
     }
 
     {
@@ -196,12 +189,8 @@ fn validate_invalid_names() {
             }],
             ..Default::default()
         };
-        match crate::Directory::try_from(d).expect_err("must fail") {
-            DirectoryError::InvalidName(n) => {
-                assert_eq!(n.as_ref(), b"..")
-            }
-            _ => panic!("unexpected error"),
-        };
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
     }
 
     {
@@ -212,12 +201,8 @@ fn validate_invalid_names() {
             }],
             ..Default::default()
         };
-        match crate::Directory::try_from(d).expect_err("must fail") {
-            DirectoryError::InvalidName(n) => {
-                assert_eq!(n.as_ref(), b"\x00")
-            }
-            _ => panic!("unexpected error"),
-        };
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
     }
 
     {
@@ -228,12 +213,20 @@ fn validate_invalid_names() {
             }],
             ..Default::default()
         };
-        match crate::Directory::try_from(d).expect_err("must fail") {
-            DirectoryError::InvalidName(n) => {
-                assert_eq!(n.as_ref(), b"foo/bar")
-            }
-            _ => panic!("unexpected error"),
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
+    }
+
+    {
+        let d = Directory {
+            symlinks: vec![SymlinkNode {
+                name: bytes::Bytes::copy_from_slice("X".repeat(500).into_bytes().as_slice()),
+                target: "foo".into(),
+            }],
+            ..Default::default()
         };
+        let e = crate::Directory::try_from(d).expect_err("must fail");
+        assert!(matches!(e, DirectoryError::InvalidName(_)));
     }
 }