From 5ec93b57e6a263eef91ee583aba9f04581e4a66b Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Fri, 16 Aug 2024 17:32:20 +0300 Subject: refactor(tvix/castore): add PathComponent type for checked components This encodes a verified component on the type level. Internally, it contains a bytes::Bytes. The castore Path/PathBuf component() and file_name() methods now return this type, the old ones returning bytes were renamed to component_bytes() and component_file_name() respectively. We can drop the directory_reject_invalid_name test - it's not possible anymore to pass an invalid name to Directories::add. Invalid names in the Directory proto are still being tested to be rejected in the validate_invalid_names tests. Change-Id: Ide4d16415dfd50b7e2d7e0c36d42a3bbeeb9b6c5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/12217 Autosubmit: flokli Reviewed-by: Connor Brewster Tested-by: BuildkiteCI --- tvix/castore/src/nodes/directory.rs | 64 ++++++++++--------------------------- 1 file changed, 17 insertions(+), 47 deletions(-) (limited to 'tvix/castore/src/nodes/directory.rs') diff --git a/tvix/castore/src/nodes/directory.rs b/tvix/castore/src/nodes/directory.rs index a2c520358929..03db30b52408 100644 --- a/tvix/castore/src/nodes/directory.rs +++ b/tvix/castore/src/nodes/directory.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; -use crate::{errors::DirectoryError, proto, B3Digest, Node}; +use crate::{errors::DirectoryError, path::PathComponent, proto, B3Digest, Node}; /// A Directory contains nodes, which can be Directory, File or Symlink nodes. /// It attached names to these nodes, which is the basename in that directory. @@ -10,7 +10,7 @@ use crate::{errors::DirectoryError, proto, B3Digest, Node}; /// - MUST be unique across all three lists #[derive(Default, Debug, Clone, PartialEq, Eq)] pub struct Directory { - nodes: BTreeMap, + nodes: BTreeMap, } impl Directory { @@ -46,31 +46,17 @@ impl Directory { /// Allows iterating over all nodes (directories, files and symlinks) /// For each, it returns a tuple of its name and node. /// The elements are sorted by their names. - pub fn nodes(&self) -> impl Iterator + Send + Sync + '_ { + pub fn nodes(&self) -> impl Iterator + Send + Sync + '_ { self.nodes.iter() } - /// Checks a Node name for validity as a directory entry - /// We disallow slashes, null bytes, '.', '..' and the empty string. - pub(crate) fn is_valid_name(name: &[u8]) -> bool { - !(name.is_empty() - || name == b".." - || name == b"." - || name.contains(&0x00) - || name.contains(&b'/')) - } - /// Adds the specified [Node] to the [Directory] with a given name. /// /// Inserting an element that already exists with the same name in the directory will yield an /// error. /// Inserting an element will validate that its name fulfills the /// requirements for directory entries and yield an error if it is not. - pub fn add(&mut self, name: bytes::Bytes, node: Node) -> Result<(), DirectoryError> { - if !Self::is_valid_name(&name) { - return Err(DirectoryError::InvalidName(name)); - } - + pub fn add(&mut self, name: PathComponent, node: Node) -> Result<(), DirectoryError> { // Check that the even after adding this new directory entry, the size calculation will not // overflow // FUTUREWORK: add some sort of batch add interface which only does this check once with @@ -91,7 +77,7 @@ impl Directory { Ok(()) } std::collections::btree_map::Entry::Occupied(occupied) => { - Err(DirectoryError::DuplicateName(occupied.key().to_vec())) + Err(DirectoryError::DuplicateName(occupied.key().to_owned())) } } } @@ -112,7 +98,7 @@ mod test { let mut d = Directory::new(); d.add( - "b".into(), + "b".try_into().unwrap(), Node::Directory { digest: DUMMY_DIGEST.clone(), size: 1, @@ -120,7 +106,7 @@ mod test { ) .unwrap(); d.add( - "a".into(), + "a".try_into().unwrap(), Node::Directory { digest: DUMMY_DIGEST.clone(), size: 1, @@ -128,7 +114,7 @@ mod test { ) .unwrap(); d.add( - "z".into(), + "z".try_into().unwrap(), Node::Directory { digest: DUMMY_DIGEST.clone(), size: 1, @@ -137,7 +123,7 @@ mod test { .unwrap(); d.add( - "f".into(), + "f".try_into().unwrap(), Node::File { digest: DUMMY_DIGEST.clone(), size: 1, @@ -146,7 +132,7 @@ mod test { ) .unwrap(); d.add( - "c".into(), + "c".try_into().unwrap(), Node::File { digest: DUMMY_DIGEST.clone(), size: 1, @@ -155,7 +141,7 @@ mod test { ) .unwrap(); d.add( - "g".into(), + "g".try_into().unwrap(), Node::File { digest: DUMMY_DIGEST.clone(), size: 1, @@ -165,21 +151,21 @@ mod test { .unwrap(); d.add( - "t".into(), + "t".try_into().unwrap(), Node::Symlink { target: "a".try_into().unwrap(), }, ) .unwrap(); d.add( - "o".into(), + "o".try_into().unwrap(), Node::Symlink { target: "a".try_into().unwrap(), }, ) .unwrap(); d.add( - "e".into(), + "e".try_into().unwrap(), Node::Symlink { target: "a".try_into().unwrap(), }, @@ -197,7 +183,7 @@ mod test { assert_eq!( d.add( - "foo".into(), + "foo".try_into().unwrap(), Node::Directory { digest: DUMMY_DIGEST.clone(), size: u64::MAX @@ -212,7 +198,7 @@ mod test { let mut d = Directory::new(); d.add( - "a".into(), + "a".try_into().unwrap(), Node::Directory { digest: DUMMY_DIGEST.clone(), size: 1, @@ -223,7 +209,7 @@ mod test { format!( "{}", d.add( - "a".into(), + "a".try_into().unwrap(), Node::File { digest: DUMMY_DIGEST.clone(), size: 1, @@ -235,20 +221,4 @@ mod test { "\"a\" is a duplicate name" ); } - - /// Attempt to add a directory entry with a name which should be rejected. - #[tokio::test] - async fn directory_reject_invalid_name() { - let mut dir = Directory::new(); - assert!( - dir.add( - "".into(), // wrong! can not be added to directory - Node::Symlink { - target: "doesntmatter".try_into().unwrap(), - }, - ) - .is_err(), - "invalid symlink entry be rejected" - ); - } } -- cgit 1.4.1