diff options
| author | Florian Klink <flokli@flokli.de> | 2024-08-14T19·00+0300 |
|---|---|---|
| committer | clbot <clbot@tvl.fyi> | 2024-08-17T09·45+0000 |
| commit | 49b173786cba575dbeb9d28fa7a62275d45ce41a (patch) | |
| tree | 722f6e45824c483572cf8d2cafedddbbe86c1bba /tvix/castore | |
| parent | 04e9531e65159309d5bb18bbfac0ff29c830f50a (diff) | |
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>
Diffstat (limited to 'tvix/castore')
24 files changed, 504 insertions, 699 deletions
diff --git a/tvix/castore/src/directoryservice/directory_graph.rs b/tvix/castore/src/directoryservice/directory_graph.rs index 0cf2f71adb06..ffb25de9a854 100644 --- a/tvix/castore/src/directoryservice/directory_graph.rs +++ b/tvix/castore/src/directoryservice/directory_graph.rs @@ -10,7 +10,7 @@ use petgraph::{ use tracing::instrument; use super::order_validator::{LeavesToRootValidator, OrderValidator, RootToLeavesValidator}; -use crate::{B3Digest, Directory, DirectoryNode, NamedNode}; +use crate::{B3Digest, Directory, DirectoryNode}; #[derive(thiserror::Error, Debug)] pub enum Error { @@ -69,12 +69,12 @@ pub struct ValidatedDirectoryGraph { root: Option<NodeIndex>, } -fn check_edge(dir: &DirectoryNode, child: &Directory) -> Result<(), Error> { +fn check_edge(dir: &DirectoryNode, dir_name: &[u8], child: &Directory) -> Result<(), Error> { // Ensure the size specified in the child node matches our records. if dir.size() != child.size() { return Err(Error::ValidationError(format!( "'{}' has wrong size, specified {}, recorded {}", - dir.get_name().as_bstr(), + dir_name.as_bstr(), dir.size(), child.size(), ))); @@ -141,19 +141,19 @@ impl<O: OrderValidator> DirectoryGraph<O> { } // set up edges to all child directories - for subdir in directory.directories() { + for (subdir_name, subdir_node) in directory.directories() { let child_ix = *self .digest_to_node_ix - .entry(subdir.digest().clone()) + .entry(subdir_node.digest().clone()) .or_insert_with(|| self.graph.add_node(None)); let pending_edge_check = match &self.graph[child_ix] { Some(child) => { // child is already available, validate the edge now - check_edge(subdir, child)?; + check_edge(subdir_node, subdir_name, child)?; None } - None => Some(subdir.clone()), // pending validation + None => Some(subdir_node.clone()), // pending validation }; self.graph.add_edge(ix, child_ix, pending_edge_check); } @@ -173,7 +173,9 @@ impl<O: OrderValidator> DirectoryGraph<O> { .expect("edge not found") .take() .expect("edge is already validated"); - check_edge(&edge_weight, &directory)?; + + // TODO: where's the name here? + check_edge(&edge_weight, b"??", &directory)?; } // finally, store the directory information in the node weight @@ -277,11 +279,12 @@ mod tests { lazy_static! { pub static ref BROKEN_PARENT_DIRECTORY: Directory = { let mut dir = Directory::new(); - dir.add(Node::Directory(DirectoryNode::new( + dir.add( "foo".into(), - DIRECTORY_A.digest(), - DIRECTORY_A.size() + 42, // wrong! - ).unwrap())).unwrap(); + Node::Directory(DirectoryNode::new( + DIRECTORY_A.digest(), + DIRECTORY_A.size() + 42, // wrong! + ))).unwrap(); dir }; } diff --git a/tvix/castore/src/directoryservice/grpc.rs b/tvix/castore/src/directoryservice/grpc.rs index 2079514d2b79..9696c5631949 100644 --- a/tvix/castore/src/directoryservice/grpc.rs +++ b/tvix/castore/src/directoryservice/grpc.rs @@ -3,8 +3,7 @@ use std::collections::HashSet; use super::{Directory, DirectoryPutter, DirectoryService}; use crate::composition::{CompositionContext, ServiceBuilder}; use crate::proto::{self, get_directory_request::ByWhat}; -use crate::ValidateDirectoryError; -use crate::{B3Digest, Error}; +use crate::{B3Digest, DirectoryError, Error}; use async_stream::try_stream; use futures::stream::BoxStream; use std::sync::Arc; @@ -154,7 +153,7 @@ where } let directory = directory.try_into() - .map_err(|e: ValidateDirectoryError| Error::StorageError(e.to_string()))?; + .map_err(|e: DirectoryError| Error::StorageError(e.to_string()))?; yield directory; }, diff --git a/tvix/castore/src/directoryservice/order_validator.rs b/tvix/castore/src/directoryservice/order_validator.rs index f03bb7ff9a86..6daf4a624d29 100644 --- a/tvix/castore/src/directoryservice/order_validator.rs +++ b/tvix/castore/src/directoryservice/order_validator.rs @@ -48,9 +48,9 @@ impl RootToLeavesValidator { self.expected_digests.insert(directory.digest()); } - for subdir in directory.directories() { + for (_, subdir_node) in directory.directories() { // Allow the children to appear next - self.expected_digests.insert(subdir.digest().clone()); + self.expected_digests.insert(subdir_node.digest().clone()); } } } @@ -79,11 +79,11 @@ impl OrderValidator for LeavesToRootValidator { fn add_directory(&mut self, directory: &Directory) -> bool { let digest = directory.digest(); - for subdir in directory.directories() { - if !self.allowed_references.contains(subdir.digest()) { + for (_, subdir_node) in directory.directories() { + if !self.allowed_references.contains(subdir_node.digest()) { warn!( directory.digest = %digest, - subdirectory.digest = %subdir.digest(), + subdirectory.digest = %subdir_node.digest(), "unexpected directory reference" ); return false; diff --git a/tvix/castore/src/directoryservice/tests/mod.rs b/tvix/castore/src/directoryservice/tests/mod.rs index 7a15f44a686e..c0a0f22c989f 100644 --- a/tvix/castore/src/directoryservice/tests/mod.rs +++ b/tvix/castore/src/directoryservice/tests/mod.rs @@ -218,14 +218,13 @@ async fn upload_reject_dangling_pointer(directory_service: impl DirectoryService async fn upload_reject_wrong_size(directory_service: impl DirectoryService) { let wrong_parent_directory = { let mut dir = Directory::new(); - dir.add(Node::Directory( - DirectoryNode::new( - "foo".into(), + dir.add( + "foo".into(), + Node::Directory(DirectoryNode::new( DIRECTORY_A.digest(), DIRECTORY_A.size() + 42, // wrong! - ) - .unwrap(), - )) + )), + ) .unwrap(); dir }; diff --git a/tvix/castore/src/directoryservice/traverse.rs b/tvix/castore/src/directoryservice/traverse.rs index ff667e0f65c0..de2ea33c7f9d 100644 --- a/tvix/castore/src/directoryservice/traverse.rs +++ b/tvix/castore/src/directoryservice/traverse.rs @@ -1,4 +1,4 @@ -use crate::{directoryservice::DirectoryService, Error, NamedNode, Node, Path}; +use crate::{directoryservice::DirectoryService, Error, Node, Path}; use tracing::{instrument, warn}; /// This descends from a (root) node to the given (sub)path, returning the Node @@ -37,9 +37,10 @@ where })?; // look for the component in the [Directory]. - // FUTUREWORK: as the nodes() iterator returns in a sorted fashion, we - // could stop as soon as e.name is larger than the search string. - if let Some(child_node) = directory.nodes().find(|n| n.get_name() == component) { + if let Some((_child_name, child_node)) = directory + .nodes() + .find(|(name, _node)| name.as_ref() == component) + { // child node found, update prev_node to that and continue. parent_node = child_node.clone(); } else { @@ -81,21 +82,23 @@ mod tests { handle.close().await.expect("must upload"); // construct the node for DIRECTORY_COMPLICATED - let node_directory_complicated = Node::Directory( - DirectoryNode::new( - "doesntmatter".into(), - DIRECTORY_COMPLICATED.digest(), - DIRECTORY_COMPLICATED.size(), - ) - .unwrap(), - ); + let node_directory_complicated = Node::Directory(DirectoryNode::new( + DIRECTORY_COMPLICATED.digest(), + DIRECTORY_COMPLICATED.size(), + )); // construct the node for DIRECTORY_COMPLICATED - let node_directory_with_keep = - Node::Directory(DIRECTORY_COMPLICATED.directories().next().unwrap().clone()); + let node_directory_with_keep = Node::Directory( + DIRECTORY_COMPLICATED + .directories() + .next() + .unwrap() + .1 + .clone(), + ); // construct the node for the .keep file - let node_file_keep = Node::File(DIRECTORY_WITH_KEEP.files().next().unwrap().clone()); + let node_file_keep = Node::File(DIRECTORY_WITH_KEEP.files().next().unwrap().1.clone()); // traversal to an empty subpath should return the root node. { diff --git a/tvix/castore/src/directoryservice/utils.rs b/tvix/castore/src/directoryservice/utils.rs index f1133663ede7..4b060707d4cc 100644 --- a/tvix/castore/src/directoryservice/utils.rs +++ b/tvix/castore/src/directoryservice/utils.rs @@ -57,16 +57,15 @@ pub fn traverse_directory<'a, DS: DirectoryService + 'static>( // enqueue all child directory digests to the work queue, as // long as they're not part of the worklist or already sent. // This panics if the digest looks invalid, it's supposed to be checked first. - for child_directory_node in current_directory.directories() { - // TODO: propagate error - let child_digest: B3Digest = child_directory_node.digest().clone(); + for (_, child_directory_node) in current_directory.directories() { + let child_digest = child_directory_node.digest(); - if worklist_directory_digests.contains(&child_digest) - || sent_directory_digests.contains(&child_digest) + if worklist_directory_digests.contains(child_digest) + || sent_directory_digests.contains(child_digest) { continue; } - worklist_directory_digests.push_back(child_digest); + worklist_directory_digests.push_back(child_digest.clone()); } yield current_directory; diff --git a/tvix/castore/src/errors.rs b/tvix/castore/src/errors.rs index 083f43036845..e03cfa5a5207 100644 --- a/tvix/castore/src/errors.rs +++ b/tvix/castore/src/errors.rs @@ -13,33 +13,12 @@ pub enum Error { StorageError(String), } -/// Errors that can occur during the validation of [Directory] messages. -#[derive(Debug, thiserror::Error, PartialEq)] -pub enum ValidateDirectoryError { - /// Elements are not in sorted order - #[error("{:?} is not sorted", .0.as_bstr())] - WrongSorting(Vec<u8>), - /// Multiple elements with the same name encountered - #[error("{:?} is a duplicate name", .0.as_bstr())] - DuplicateName(Vec<u8>), - /// Invalid node - #[error("invalid node with name {:?}: {:?}", .0.as_bstr(), .1.to_string())] - InvalidNode(Vec<u8>, ValidateNodeError), - #[error("Total size exceeds u32::MAX")] - SizeOverflow, -} - -/// Errors that occur during Node validation +/// Errors that occur during construction of [crate::Node] #[derive(Debug, thiserror::Error, PartialEq)] pub enum ValidateNodeError { - #[error("No node set")] - NoNodeSet, /// Invalid digest length encountered #[error("invalid digest length: {0}")] InvalidDigestLen(usize), - /// Invalid name encountered - #[error("Invalid name: {}", .0.as_bstr())] - InvalidName(bytes::Bytes), /// Invalid symlink target #[error("Invalid symlink target: {}", .0.as_bstr())] InvalidSymlinkTarget(bytes::Bytes), @@ -53,6 +32,29 @@ impl From<crate::digests::Error> for ValidateNodeError { } } +/// Errors that can occur when populating [crate::Directory] messages, +/// or parsing [crate::proto::Directory] +#[derive(Debug, thiserror::Error, PartialEq)] +pub enum DirectoryError { + /// Multiple elements with the same name encountered + #[error("{:?} is a duplicate name", .0.as_bstr())] + DuplicateName(Vec<u8>), + /// Node failed validation + #[error("invalid node with name {:?}: {:?}", .0.as_bstr(), .1.to_string())] + InvalidNode(bytes::Bytes, ValidateNodeError), + #[error("Total size exceeds u32::MAX")] + SizeOverflow, + /// Invalid name encountered + #[error("Invalid name: {}", .0.as_bstr())] + InvalidName(bytes::Bytes), + /// Elements are not in sorted order. Can only happen on protos + #[error("{:?} is not sorted", .0.as_bstr())] + WrongSorting(bytes::Bytes), + /// This can only happen if there's an unknown node type (on protos) + #[error("No node set")] + NoNodeSet, +} + impl From<JoinError> for Error { fn from(value: JoinError) -> Self { Error::StorageError(value.to_string()) diff --git a/tvix/castore/src/fixtures.rs b/tvix/castore/src/fixtures.rs index 06366f2057ab..b57540d177e2 100644 --- a/tvix/castore/src/fixtures.rs +++ b/tvix/castore/src/fixtures.rs @@ -35,70 +35,83 @@ lazy_static! { // Directories pub static ref DIRECTORY_WITH_KEEP: Directory = { let mut dir = Directory::new(); - dir.add(Node::File(FileNode::new( - b".keep".to_vec().into(), - EMPTY_BLOB_DIGEST.clone(), - 0, - false - ).unwrap())).unwrap(); + dir.add( + ".keep".into(), + Node::File(FileNode::new( + EMPTY_BLOB_DIGEST.clone(), + 0, + false + ))).unwrap(); + dir }; pub static ref DIRECTORY_COMPLICATED: Directory = { let mut dir = Directory::new(); - dir.add(Node::Directory(DirectoryNode::new( - b"keep".to_vec().into(), - DIRECTORY_WITH_KEEP.digest(), - DIRECTORY_WITH_KEEP.size() - ).unwrap())).unwrap(); - dir.add(Node::File(FileNode::new( - b".keep".to_vec().into(), + dir.add( + "keep".into(), + Node::Directory(DirectoryNode::new( + DIRECTORY_WITH_KEEP.digest(), + DIRECTORY_WITH_KEEP.size() + ))).unwrap(); + dir.add( + ".keep".into(), + Node::File(FileNode::new( EMPTY_BLOB_DIGEST.clone(), 0, false - ).unwrap())).unwrap(); - dir.add(Node::Symlink(SymlinkNode::new( - b"aa".to_vec().into(), - b"/nix/store/somewhereelse".to_vec().into() - ).unwrap())).unwrap(); + ))).unwrap(); + dir.add( + "aa".into(), + Node::Symlink(SymlinkNode::new( + b"/nix/store/somewhereelse".to_vec().into() + ).unwrap())).unwrap(); + dir }; pub static ref DIRECTORY_A: Directory = Directory::new(); pub static ref DIRECTORY_B: Directory = { let mut dir = Directory::new(); - dir.add(Node::Directory(DirectoryNode::new( - b"a".to_vec().into(), - DIRECTORY_A.digest(), - DIRECTORY_A.size(), - ).unwrap())).unwrap(); + dir.add( + "a".into(), + Node::Directory(DirectoryNode::new( + DIRECTORY_A.digest(), + DIRECTORY_A.size(), + ))).unwrap(); + dir }; pub static ref DIRECTORY_C: Directory = { let mut dir = Directory::new(); - dir.add(Node::Directory(DirectoryNode::new( - b"a".to_vec().into(), - DIRECTORY_A.digest(), - DIRECTORY_A.size(), - ).unwrap())).unwrap(); - dir.add(Node::Directory(DirectoryNode::new( - b"a'".to_vec().into(), + dir.add( + "a".into(), + Node::Directory(DirectoryNode::new( + DIRECTORY_A.digest(), + DIRECTORY_A.size(), + ))).unwrap(); + dir.add( + "a'".into(), + Node::Directory(DirectoryNode::new( DIRECTORY_A.digest(), DIRECTORY_A.size(), - ).unwrap())).unwrap(); + ))).unwrap(); + dir }; pub static ref DIRECTORY_D: Directory = { let mut dir = Directory::new(); - dir.add(Node::Directory(DirectoryNode::new( - b"a".to_vec().into(), - DIRECTORY_A.digest(), - DIRECTORY_A.size(), - ).unwrap())).unwrap(); - dir.add(Node::Directory(DirectoryNode::new( - b"b".to_vec().into(), - DIRECTORY_B.digest(), - DIRECTORY_B.size(), - ).unwrap())).unwrap(); - dir + dir.add( + "a".into(), + Node::Directory(DirectoryNode::new( + DIRECTORY_A.digest(), + DIRECTORY_A.size(), + ))).unwrap(); + dir.add( + "b".into(), + Node::Directory(DirectoryNode::new( + DIRECTORY_B.digest(), + DIRECTORY_B.size(), + ))).unwrap(); + dir }; } diff --git a/tvix/castore/src/fs/fuse/tests.rs b/tvix/castore/src/fs/fuse/tests.rs index 3dad7d83aedd..30bbfc46235d 100644 --- a/tvix/castore/src/fs/fuse/tests.rs +++ b/tvix/castore/src/fs/fuse/tests.rs @@ -68,15 +68,11 @@ async fn populate_blob_a( root_nodes.insert( BLOB_A_NAME.into(), - Node::File( - FileNode::new( - BLOB_A_NAME.into(), - fixtures::BLOB_A_DIGEST.clone(), - fixtures::BLOB_A.len() as u64, - false, - ) - .unwrap(), - ), + Node::File(FileNode::new( + fixtures::BLOB_A_DIGEST.clone(), + fixtures::BLOB_A.len() as u64, + false, + )), ); } @@ -92,15 +88,11 @@ async fn populate_blob_b( root_nodes.insert( BLOB_B_NAME.into(), - Node::File( - FileNode::new( - BLOB_B_NAME.into(), - fixtures::BLOB_B_DIGEST.clone(), - fixtures::BLOB_B.len() as u64, - false, - ) - .unwrap(), - ), + Node::File(FileNode::new( + fixtures::BLOB_B_DIGEST.clone(), + fixtures::BLOB_B.len() as u64, + false, + )), ); } @@ -120,22 +112,18 @@ async fn populate_blob_helloworld( root_nodes.insert( HELLOWORLD_BLOB_NAME.into(), - Node::File( - FileNode::new( - HELLOWORLD_BLOB_NAME.into(), - fixtures::HELLOWORLD_BLOB_DIGEST.clone(), - fixtures::HELLOWORLD_BLOB_CONTENTS.len() as u64, - true, - ) - .unwrap(), - ), + Node::File(FileNode::new( + fixtures::HELLOWORLD_BLOB_DIGEST.clone(), + fixtures::HELLOWORLD_BLOB_CONTENTS.len() as u64, + true, + )), ); } async fn populate_symlink(root_nodes: &mut BTreeMap<Bytes, Node>) { root_nodes.insert( SYMLINK_NAME.into(), - Node::Symlink(SymlinkNode::new(SYMLINK_NAME.into(), BLOB_A_NAME.into()).unwrap()), + Node::Symlink(SymlinkNode::new(BLOB_A_NAME.into()).unwrap()), ); } @@ -144,9 +132,7 @@ async fn populate_symlink(root_nodes: &mut BTreeMap<Bytes, Node>) { async fn populate_symlink2(root_nodes: &mut BTreeMap<Bytes, Node>) { root_nodes.insert( SYMLINK_NAME2.into(), - Node::Symlink( - SymlinkNode::new(SYMLINK_NAME2.into(), "/nix/store/somewhereelse".into()).unwrap(), - ), + Node::Symlink(SymlinkNode::new("/nix/store/somewhereelse".into()).unwrap()), ); } @@ -170,14 +156,10 @@ async fn populate_directory_with_keep( root_nodes.insert( DIRECTORY_WITH_KEEP_NAME.into(), - Node::Directory( - DirectoryNode::new( - DIRECTORY_WITH_KEEP_NAME.into(), - fixtures::DIRECTORY_WITH_KEEP.digest(), - fixtures::DIRECTORY_WITH_KEEP.size(), - ) - .unwrap(), - ), + Node::Directory(DirectoryNode::new( + fixtures::DIRECTORY_WITH_KEEP.digest(), + fixtures::DIRECTORY_WITH_KEEP.size(), + )), ); } @@ -186,14 +168,10 @@ async fn populate_directory_with_keep( async fn populate_directorynode_without_directory(root_nodes: &mut BTreeMap<Bytes, Node>) { root_nodes.insert( DIRECTORY_WITH_KEEP_NAME.into(), - Node::Directory( - DirectoryNode::new( - DIRECTORY_WITH_KEEP_NAME.into(), - fixtures::DIRECTORY_WITH_KEEP.digest(), - fixtures::DIRECTORY_WITH_KEEP.size(), - ) - .unwrap(), - ), + Node::Directory(DirectoryNode::new( + fixtures::DIRECTORY_WITH_KEEP.digest(), + fixtures::DIRECTORY_WITH_KEEP.size(), + )), ); } @@ -201,15 +179,11 @@ async fn populate_directorynode_without_directory(root_nodes: &mut BTreeMap<Byte async fn populate_filenode_without_blob(root_nodes: &mut BTreeMap<Bytes, Node>) { root_nodes.insert( BLOB_A_NAME.into(), - Node::File( - FileNode::new( - BLOB_A_NAME.into(), - fixtures::BLOB_A_DIGEST.clone(), - fixtures::BLOB_A.len() as u64, - false, - ) - .unwrap(), - ), + Node::File(FileNode::new( + fixtures::BLOB_A_DIGEST.clone(), + fixtures::BLOB_A.len() as u64, + false, + )), ); } @@ -239,14 +213,10 @@ async fn populate_directory_complicated( root_nodes.insert( DIRECTORY_COMPLICATED_NAME.into(), - Node::Directory( - DirectoryNode::new( - DIRECTORY_COMPLICATED_NAME.into(), - fixtures::DIRECTORY_COMPLICATED.digest(), - fixtures::DIRECTORY_COMPLICATED.size(), - ) - .unwrap(), - ), + Node::Directory(DirectoryNode::new( + fixtures::DIRECTORY_COMPLICATED.digest(), + fixtures::DIRECTORY_COMPLICATED.size(), + )), ); } diff --git a/tvix/castore/src/fs/inodes.rs b/tvix/castore/src/fs/inodes.rs index b04505c9fa98..e49d28f8e52e 100644 --- a/tvix/castore/src/fs/inodes.rs +++ b/tvix/castore/src/fs/inodes.rs @@ -2,9 +2,7 @@ //! about inodes, which present tvix-castore nodes in a filesystem. use std::time::Duration; -use bytes::Bytes; - -use crate::{B3Digest, NamedNode, Node}; +use crate::{B3Digest, Node}; #[derive(Clone, Debug)] pub enum InodeData { @@ -19,24 +17,19 @@ pub enum InodeData { /// lookup and did fetch the data. #[derive(Clone, Debug)] pub enum DirectoryInodeData { - Sparse(B3Digest, u64), // digest, size - Populated(B3Digest, Vec<(u64, Node)>), // [(child_inode, node)] + Sparse(B3Digest, u64), // digest, size + Populated(B3Digest, Vec<(u64, bytes::Bytes, Node)>), // [(child_inode, name, node)] } impl InodeData { /// Constructs a new InodeData by consuming a [Node]. - /// It splits off the orginal name, so it can be used later. - pub fn from_node(node: &Node) -> (Self, Bytes) { + pub fn from_node(node: &Node) -> Self { match node { - Node::Directory(n) => ( - Self::Directory(DirectoryInodeData::Sparse(n.digest().clone(), n.size())), - n.get_name().clone(), - ), - Node::File(n) => ( - Self::Regular(n.digest().clone(), n.size(), n.executable()), - n.get_name().clone(), - ), - Node::Symlink(n) => (Self::Symlink(n.target().clone()), n.get_name().clone()), + Node::Directory(n) => { + Self::Directory(DirectoryInodeData::Sparse(n.digest().clone(), n.size())) + } + Node::File(n) => Self::Regular(n.digest().clone(), n.size(), n.executable()), + Node::Symlink(n) => Self::Symlink(n.target().clone()), } } diff --git a/tvix/castore/src/fs/mod.rs b/tvix/castore/src/fs/mod.rs index 90c1f371b546..f819adb549f5 100644 --- a/tvix/castore/src/fs/mod.rs +++ b/tvix/castore/src/fs/mod.rs @@ -18,7 +18,7 @@ use self::{ use crate::{ blobservice::{BlobReader, BlobService}, directoryservice::DirectoryService, - {B3Digest, NamedNode, Node}, + {B3Digest, Node}, }; use bstr::ByteVec; use bytes::Bytes; @@ -103,7 +103,7 @@ pub struct TvixStoreFs<BS, DS, RN> { u64, ( Span, - Arc<Mutex<mpsc::Receiver<(usize, Result<Node, crate::Error>)>>>, + Arc<Mutex<mpsc::Receiver<(usize, Result<(Bytes, Node), crate::Error>)>>>, ), >, >, @@ -165,8 +165,12 @@ where /// It is ok if it's a [DirectoryInodeData::Sparse] - in that case, a lookup /// in self.directory_service is performed, and self.inode_tracker is updated with the /// [DirectoryInodeData::Populated]. + #[allow(clippy::type_complexity)] #[instrument(skip(self), err)] - fn get_directory_children(&self, ino: u64) -> io::Result<(B3Digest, Vec<(u64, Node)>)> { + fn get_directory_children( + &self, + ino: u64, + ) -> io::Result<(B3Digest, Vec<(u64, bytes::Bytes, Node)>)> { let data = self.inode_tracker.read().get(ino).unwrap(); match *data { // if it's populated already, return children. @@ -196,13 +200,13 @@ where let children = { let mut inode_tracker = self.inode_tracker.write(); - let children: Vec<(u64, Node)> = directory + let children: Vec<(u64, Bytes, Node)> = directory .nodes() - .map(|child_node| { - let (inode_data, _) = InodeData::from_node(child_node); + .map(|(child_name, child_node)| { + let inode_data = InodeData::from_node(child_node); let child_ino = inode_tracker.put(inode_data); - (child_ino, child_node.clone()) + (child_ino, child_name.to_owned(), child_node.clone()) }) .collect(); @@ -263,12 +267,6 @@ where Ok(None) => Err(io::Error::from_raw_os_error(libc::ENOENT)), // The root node does exist Ok(Some(root_node)) => { - // The name must match what's passed in the lookup, otherwise this is also a ENOENT. - if root_node.get_name() != name.to_bytes() { - debug!(root_node.name=?root_node.get_name(), found_node.name=%name.to_string_lossy(), "node name mismatch"); - return Err(io::Error::from_raw_os_error(libc::ENOENT)); - } - // Let's check if someone else beat us to updating the inode tracker and // root_nodes map. This avoids locking inode_tracker for writing. if let Some(ino) = self.root_nodes.read().get(name.to_bytes()) { @@ -285,9 +283,9 @@ where // insert the (sparse) inode data and register in // self.root_nodes. - let (inode_data, name) = InodeData::from_node(&root_node); + let inode_data = InodeData::from_node(&root_node); let ino = inode_tracker.put(inode_data.clone()); - root_nodes.insert(name, ino); + root_nodes.insert(bytes::Bytes::copy_from_slice(name.to_bytes()), ino); Ok((ino, Arc::new(inode_data))) } @@ -362,7 +360,7 @@ where // Search for that name in the list of children and return the FileAttrs. // in the children, find the one with the desired name. - if let Some((child_ino, _)) = children.iter().find(|e| e.1.get_name() == name.to_bytes()) { + if let Some((child_ino, _, _)) = children.iter().find(|(_, n, _)| n == name.to_bytes()) { // lookup the child [InodeData] in [self.inode_tracker]. // We know the inodes for children have already been allocated. let child_inode_data = self.inode_tracker.read().get(*child_ino).unwrap(); @@ -398,8 +396,8 @@ where self.tokio_handle.spawn( async move { let mut stream = root_nodes_provider.list().enumerate(); - while let Some(node) = stream.next().await { - if tx.send(node).await.is_err() { + while let Some(e) = stream.next().await { + if tx.send(e).await.is_err() { // If we get a send error, it means the sync code // doesn't want any more entries. break; @@ -461,12 +459,12 @@ where .map_err(|_| crate::Error::StorageError("mutex poisoned".into()))?; while let Some((i, n)) = rx.blocking_recv() { - let root_node = n.map_err(|e| { + let (name, node) = n.map_err(|e| { warn!("failed to retrieve root node: {}", e); io::Error::from_raw_os_error(libc::EIO) })?; - let (inode_data, name) = InodeData::from_node(&root_node); + let inode_data = InodeData::from_node(&node); // obtain the inode, or allocate a new one. let ino = self.get_inode_for_root_name(&name).unwrap_or_else(|| { @@ -495,15 +493,17 @@ where let (parent_digest, children) = self.get_directory_children(inode)?; Span::current().record("directory.digest", parent_digest.to_string()); - for (i, (ino, child_node)) in children.into_iter().skip(offset as usize).enumerate() { - let (inode_data, name) = InodeData::from_node(&child_node); + for (i, (ino, child_name, child_node)) in + children.into_iter().skip(offset as usize).enumerate() + { + let inode_data = InodeData::from_node(&child_node); // the second parameter will become the "offset" parameter on the next call. let written = add_entry(fuse_backend_rs::api::filesystem::DirEntry { ino, offset: offset + (i as u64) + 1, type_: inode_data.as_fuse_type(), - name: &name, + name: &child_name, })?; // If the buffer is full, add_entry will return `Ok(0)`. if written == 0 { @@ -548,12 +548,12 @@ where .map_err(|_| crate::Error::StorageError("mutex poisoned".into()))?; while let Some((i, n)) = rx.blocking_recv() { - let root_node = n.map_err(|e| { + let (name, node) = n.map_err(|e| { warn!("failed to retrieve root node: {}", e); io::Error::from_raw_os_error(libc::EPERM) })?; - let (inode_data, name) = InodeData::from_node(&root_node); + let inode_data = InodeData::from_node(&node); // obtain the inode, or allocate a new one. let ino = self.get_inode_for_root_name(&name).unwrap_or_else(|| { @@ -585,8 +585,8 @@ where let (parent_digest, children) = self.get_directory_children(inode)?; Span::current().record("directory.digest", parent_digest.to_string()); - for (i, (ino, child_node)) in children.into_iter().skip(offset as usize).enumerate() { - let (inode_data, name) = InodeData::from_node(&child_node); + for (i, (ino, name, child_node)) in children.into_iter().skip(offset as usize).enumerate() { + let inode_data = InodeData::from_node(&child_node); // the second parameter will become the "offset" parameter on the next call. let written = add_entry( diff --git a/tvix/castore/src/fs/root_nodes.rs b/tvix/castore/src/fs/root_nodes.rs index c237142c8d91..62f0b62196a6 100644 --- a/tvix/castore/src/fs/root_nodes.rs +++ b/tvix/castore/src/fs/root_nodes.rs @@ -12,9 +12,10 @@ pub trait RootNodes: Send + Sync { /// directory of the filesystem. async fn get_by_basename(&self, name: &[u8]) -> Result<Option<Node>, Error>; - /// Lists all root CA nodes in the filesystem. An error can be returned - /// in case listing is not allowed - fn list(&self) -> BoxStream<Result<Node, Error>>; + /// Lists all root CA nodes in the filesystem, as a tuple of (base)name + /// and Node. + /// An error can be returned in case listing is not allowed. + fn list(&self) -> BoxStream<Result<(bytes::Bytes, Node), Error>>; } #[async_trait] @@ -28,9 +29,11 @@ where Ok(self.as_ref().get(name).cloned()) } - fn list(&self) -> BoxStream<Result<Node, Error>> { + fn list(&self) -> BoxStream<Result<(bytes::Bytes, Node), Error>> { Box::pin(tokio_stream::iter( - self.as_ref().iter().map(|(_, v)| Ok(v.clone())), + self.as_ref() + .iter() + .map(|(name, node)| Ok((name.to_owned(), node.to_owned()))), )) } } diff --git a/tvix/castore/src/import/mod.rs b/tvix/castore/src/import/mod.rs index b40dfd74e9e4..408166beffb4 100644 --- a/tvix/castore/src/import/mod.rs +++ b/tvix/castore/src/import/mod.rs @@ -59,14 +59,6 @@ where // we break the loop manually. .expect("Tvix bug: unexpected end of stream")?; - let name = entry - .path() - .file_name() - // If this is the root node, it will have an empty name. - .unwrap_or_default() - .to_owned() - .into(); - let node = match &mut entry { IngestionEntry::Dir { .. } => { // If the entry is a directory, we traversed all its children (and @@ -92,36 +84,22 @@ where IngestionError::UploadDirectoryError(entry.path().to_owned(), e) })?; - Node::Directory( - DirectoryNode::new(name, directory_digest, directory_size).map_err(|e| { - IngestionError::UploadDirectoryError( - entry.path().to_owned(), - crate::Error::StorageError(e.to_string()), - ) - })?, - ) + Node::Directory(DirectoryNode::new(directory_digest, directory_size)) } - IngestionEntry::Symlink { ref target, .. } => Node::Symlink( - SymlinkNode::new(name, target.to_owned().into()).map_err(|e| { + IngestionEntry::Symlink { ref target, .. } => { + Node::Symlink(SymlinkNode::new(target.to_owned().into()).map_err(|e| { IngestionError::UploadDirectoryError( entry.path().to_owned(), crate::Error::StorageError(e.to_string()), ) - })?, - ), + })?) + } IngestionEntry::Regular { size, executable, digest, .. - } => Node::File( - FileNode::new(name, digest.clone(), *size, *executable).map_err(|e| { - IngestionError::UploadDirectoryError( - entry.path().to_owned(), - crate::Error::StorageError(e.to_string()), - ) - })?, - ), + } => Node::File(FileNode::new(digest.clone(), *size, *executable)), }; let parent = entry @@ -132,11 +110,19 @@ where if parent == crate::Path::ROOT { break node; } else { + let name = entry + .path() + .file_name() + // If this is the root node, it will have an empty name. + .unwrap_or_default() + .to_owned() + .into(); + // record node in parent directory, creating a new [Directory] if not there yet. directories .entry(parent.to_owned()) .or_default() - .add(node) + .add(name, node) .map_err(|e| { IngestionError::UploadDirectoryError( entry.path().to_owned(), @@ -227,18 +213,18 @@ mod test { executable: true, digest: DUMMY_DIGEST.clone(), }], - Node::File(FileNode::new("foo".into(), DUMMY_DIGEST.clone(), 42, true).unwrap()) + Node::File(FileNode::new(DUMMY_DIGEST.clone(), 42, true)) )] #[case::single_symlink(vec![IngestionEntry::Symlink { path: "foo".parse().unwrap(), target: b"blub".into(), }], - Node::Symlink(SymlinkNode::new("foo".into(), "blub".into()).unwrap()) + Node::Symlink(SymlinkNode::new("blub".into()).unwrap()) )] #[case::single_dir(vec![IngestionEntry::Dir { path: "foo".parse().unwrap(), }], - Node::Directory(DirectoryNode::new("foo".into(), Directory::default().digest(), Directory::default().size()).unwrap()) + Node::Directory(DirectoryNode::new(Directory::default().digest(), Directory::default().size())) )] #[case::dir_with_keep(vec![ IngestionEntry::Regular { @@ -251,7 +237,7 @@ mod test { path: "foo".parse().unwrap(), }, ], - Node::Directory(DirectoryNode::new("foo".into(), DIRECTORY_WITH_KEEP.digest(), DIRECTORY_WITH_KEEP.size()).unwrap()) + Node::Directory(DirectoryNode::new(DIRECTORY_WITH_KEEP.digest(), DIRECTORY_WITH_KEEP.size())) )] /// This is intentionally a bit unsorted, though it still satisfies all /// requirements we have on the order of elements in the stream. @@ -279,7 +265,7 @@ mod test { path: "blub".parse().unwrap(), }, ], - Node::Directory(DirectoryNode::new("blub".into(), DIRECTORY_COMPLICATED.digest(), DIRECTORY_COMPLICATED.size()).unwrap()) + Node::Directory(DirectoryNode::new(DIRECTORY_COMPLICATED.digest(), DIRECTORY_COMPLICATED.size())) )] #[tokio::test] async fn test_ingestion(#[case] entries: Vec<IngestionEntry>, #[case] exp_root_node: Node) { diff --git a/tvix/castore/src/lib.rs b/tvix/castore/src/lib.rs index e09926afe70d..f56ddf827722 100644 --- a/tvix/castore/src/lib.rs +++ b/tvix/castore/src/lib.rs @@ -21,7 +21,7 @@ pub mod proto; pub mod tonic; pub use digests::{B3Digest, B3_LEN}; -pub use errors::{Error, ValidateDirectoryError, ValidateNodeError}; +pub use errors::{DirectoryError, Error, ValidateNodeError}; pub use hashing_reader::{B3HashingReader, HashingReader}; #[cfg(test)] diff --git a/tvix/castore/src/nodes/directory.rs b/tvix/castore/src/nodes/directory.rs index 1752681c89c3..d58b7822a434 100644 --- a/tvix/castore/src/nodes/directory.rs +++ b/tvix/castore/src/nodes/directory.rs @@ -1,23 +1,25 @@ -use crate::{ - proto, B3Digest, DirectoryNode, FileNode, NamedNode, Node, SymlinkNode, ValidateDirectoryError, - ValidateNodeError, -}; - -/// A Directory can contain Directory, File or Symlink nodes. -/// Each of these nodes have a name attribute, which is the basename in that -/// directory and node type specific attributes. -/// While a Node by itself may have any name, the names of Directory entries: +use std::collections::BTreeMap; + +use crate::{errors::DirectoryError, proto, B3Digest, DirectoryNode, FileNode, Node, SymlinkNode}; + +/// 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. +/// These names: /// - MUST not contain slashes or null bytes /// - MUST not be '.' or '..' /// - MUST be unique across all three lists #[derive(Default, Debug, Clone, PartialEq, Eq)] pub struct Directory { - nodes: Vec<Node>, + nodes: BTreeMap<bytes::Bytes, Node>, } impl Directory { + /// Constructs a new, empty Directory. + /// FUTUREWORK: provide a constructor from an interator of (sorted) names and nodes. pub fn new() -> Self { - Directory { nodes: vec![] } + Directory { + nodes: BTreeMap::new(), + } } /// The size of a directory is the number of all regular and symlink elements, @@ -25,7 +27,7 @@ impl Directory { pub fn size(&self) -> u64 { // It's impossible to create a Directory where the size overflows, because we // check before every add() that the size won't overflow. - (self.nodes.len() as u64) + self.directories().map(|e| e.size()).sum::<u64>() + (self.nodes.len() as u64) + self.directories().map(|(_name, dn)| dn.size()).sum::<u64>() } /// Calculates the digest of a Directory, which is the blake3 hash of a @@ -35,62 +37,66 @@ impl Directory { } /// Allows iterating over all nodes (directories, files and symlinks) - /// ordered by their name. - pub fn nodes(&self) -> impl Iterator<Item = &Node> + Send + Sync + '_ { + /// For each, it returns a tuple of its name and node. + /// The elements are sorted by their names. + pub fn nodes(&self) -> impl Iterator<Item = (&bytes::Bytes, &Node)> + Send + Sync + '_ { self.nodes.iter() } - /// Allows iterating over the FileNode entries of this directory - /// ordered by their name - pub fn files(&self) -> impl Iterator<Item = &FileNode> + Send + Sync + '_ { - self.nodes.iter().filter_map(|node| match node { - Node::File(n) => Some(n), + /// Allows iterating over the FileNode entries of this directory. + /// For each, it returns a tuple of its name and node. + /// The elements are sorted by their names. + pub fn files(&self) -> impl Iterator<Item = (&bytes::Bytes, &FileNode)> + Send + Sync + '_ { + self.nodes.iter().filter_map(|(name, node)| match node { + Node::File(n) => Some((name, n)), _ => None, }) } - /// Allows iterating over the subdirectories of this directory - /// ordered by their name - pub fn directories(&self) -> impl Iterator<Item = &DirectoryNode> + Send + Sync + '_ { - self.nodes.iter().filter_map(|node| match node { - Node::Directory(n) => Some(n), + /// Allows iterating over the DirectoryNode entries (subdirectories) of this directory. + /// For each, it returns a tuple of its name and node. + /// The elements are sorted by their names. + pub fn directories( + &self, + ) -> impl Iterator<Item = (&bytes::Bytes, &DirectoryNode)> + Send + Sync + '_ { + self.nodes.iter().filter_map(|(name, node)| match node { + Node::Directory(n) => Some((name, n)), _ => None, }) } /// Allows iterating over the SymlinkNode entries of this directory - /// ordered by their name - pub fn symlinks(&self) -> impl Iterator<Item = &SymlinkNode> + Send + Sync + '_ { - self.nodes.iter().filter_map(|node| match node { - Node::Symlink(n) => Some(n), + /// For each, it returns a tuple of its name and node. + /// The elements are sorted by their names. + pub fn symlinks( + &self, + ) -> impl Iterator<Item = (&bytes::Bytes, &SymlinkNode)> + Send + Sync + '_ { + self.nodes.iter().filter_map(|(name, node)| match node { + Node::Symlink(n) => Some((name, n)), _ => None, }) } /// Checks a Node name for validity as a directory entry /// We disallow slashes, null bytes, '.', '..' and the empty string. - pub(crate) fn validate_node_name(name: &[u8]) -> Result<(), ValidateNodeError> { - if name.is_empty() + pub(crate) fn is_valid_name(name: &[u8]) -> bool { + !(name.is_empty() || name == b".." || name == b"." || name.contains(&0x00) - || name.contains(&b'/') - { - Err(ValidateNodeError::InvalidName(name.to_owned().into())) - } else { - Ok(()) - } + || name.contains(&b'/')) } - /// Adds the specified [Node] to the [Directory], preserving sorted entries. + /// 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 stricter requirements for - /// directory entries and yield an error if it is not. - pub fn add(&mut self, node: Node) -> Result<(), ValidateDirectoryError> { - Self::validate_node_name(node.get_name()) - .map_err(|e| ValidateDirectoryError::InvalidNode(node.get_name().clone().into(), e))?; + /// 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)); + } // Check that the even after adding this new directory entry, the size calculation will not // overflow @@ -104,24 +110,17 @@ impl Directory { _ => 0, }, ]) - .ok_or(ValidateDirectoryError::SizeOverflow)?; - - // This assumes the [Directory] is sorted, since we don't allow accessing the nodes list - // directly and all previous inserts should have been in-order - let pos = match self - .nodes - .binary_search_by_key(&node.get_name(), |n| n.get_name()) - { - Err(pos) => pos, // There is no node with this name; good! - Ok(_) => { - return Err(ValidateDirectoryError::DuplicateName( - node.get_name().to_vec(), - )) - } - }; + .ok_or(DirectoryError::SizeOverflow)?; - self.nodes.insert(pos, node); - Ok(()) + match self.nodes.entry(name) { + std::collections::btree_map::Entry::Vacant(e) => { + e.insert(node); + Ok(()) + } + std::collections::btree_map::Entry::Occupied(occupied) => { + Err(DirectoryError::DuplicateName(occupied.key().to_vec())) + } + } } } @@ -133,49 +132,58 @@ fn checked_sum(iter: impl IntoIterator<Item = u64>) -> Option<u64> { mod test { use super::{Directory, DirectoryNode, FileNode, Node, SymlinkNode}; use crate::fixtures::DUMMY_DIGEST; - use crate::ValidateDirectoryError; + use crate::DirectoryError; #[test] fn add_nodes_to_directory() { let mut d = Directory::new(); - d.add(Node::Directory( - DirectoryNode::new("b".into(), DUMMY_DIGEST.clone(), 1).unwrap(), - )) + d.add( + "b".into(), + Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), 1)), + ) .unwrap(); - d.add(Node::Directory( - DirectoryNode::new("a".into(), DUMMY_DIGEST.clone(), 1).unwrap(), - )) + d.add( + "a".into(), + Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), 1)), + ) .unwrap(); - d.add(Node::Directory( - DirectoryNode::new("z".into(), DUMMY_DIGEST.clone(), 1).unwrap(), - )) + d.add( + "z".into(), + Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), 1)), + ) .unwrap(); - d.add(Node::File( - FileNode::new("f".into(), DUMMY_DIGEST.clone(), 1, true).unwrap(), - )) + d.add( + "f".into(), + Node::File(FileNode::new(DUMMY_DIGEST.clone(), 1, true)), + ) .unwrap(); - d.add(Node::File( - FileNode::new("c".into(), DUMMY_DIGEST.clone(), 1, true).unwrap(), - )) + d.add( + "c".into(), + Node::File(FileNode::new(DUMMY_DIGEST.clone(), 1, true)), + ) .unwrap(); - d.add(Node::File( - FileNode::new("g".into(), DUMMY_DIGEST.clone(), 1, true).unwrap(), - )) + d.add( + "g".into(), + Node::File(FileNode::new(DUMMY_DIGEST.clone(), 1, true)), + ) .unwrap(); - d.add(Node::Symlink( - SymlinkNode::new("t".into(), "a".into()).unwrap(), - )) + d.add( + "t".into(), + Node::Symlink(SymlinkNode::new("a".into()).unwrap()), + ) .unwrap(); - d.add(Node::Symlink( - SymlinkNode::new("o".into(), "a".into()).unwrap(), - )) + d.add( + "o".into(), + Node::Symlink(SymlinkNode::new("a".into()).unwrap()), + ) .unwrap(); - d.add(Node::Symlink( - SymlinkNode::new("e".into(), "a".into()).unwrap(), - )) + d.add( + "e".into(), + Node::Symlink(SymlinkNode::new("a".into()).unwrap()), + ) .unwrap(); // Convert to proto struct and back to ensure we are not generating any invalid structures @@ -188,10 +196,11 @@ mod test { let mut d = Directory::new(); assert_eq!( - d.add(Node::Directory( - DirectoryNode::new("foo".into(), DUMMY_DIGEST.clone(), u64::MAX).unwrap(), - )), - Err(ValidateDirectoryError::SizeOverflow) + d.add( + "foo".into(), + Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), u64::MAX)) + ), + Err(DirectoryError::SizeOverflow) ); } @@ -199,16 +208,18 @@ mod test { fn add_duplicate_node_to_directory() { let mut d = Directory::new(); - d.add(Node::Directory( - DirectoryNode::new("a".into(), DUMMY_DIGEST.clone(), 1).unwrap(), - )) + d.add( + "a".into(), + Node::Directory(DirectoryNode::new(DUMMY_DIGEST.clone(), 1)), + ) .unwrap(); assert_eq!( format!( "{}", - d.add(Node::File( - FileNode::new("a".into(), DUMMY_DIGEST.clone(), 1, true).unwrap(), - )) + d.add( + "a".into(), + Node::File(FileNode::new(DUMMY_DIGEST.clone(), 1, true)) + ) .expect_err("adding duplicate dir entry must fail") ), "\"a\" is a duplicate name" @@ -220,13 +231,10 @@ mod test { async fn directory_reject_invalid_name() { let mut dir = Directory::new(); assert!( - dir.add(Node::Symlink( - SymlinkNode::new( - "".into(), // wrong! can not be added to directory - "doesntmatter".into(), - ) - .unwrap() - )) + dir.add( + "".into(), // wrong! can not be added to directory + Node::Symlink(SymlinkNode::new("doesntmatter".into(),).unwrap()) + ) .is_err(), "invalid symlink entry be rejected" ); diff --git a/tvix/castore/src/nodes/directory_node.rs b/tvix/castore/src/nodes/directory_node.rs index afbae1695c63..e6e6312a7a0c 100644 --- a/tvix/castore/src/nodes/directory_node.rs +++ b/tvix/castore/src/nodes/directory_node.rs @@ -1,13 +1,11 @@ -use crate::{B3Digest, NamedNode, ValidateNodeError}; +use crate::B3Digest; /// A DirectoryNode is a pointer to a [Directory], by its [Directory::digest]. -/// It also gives it a `name` and `size`. +/// It also records a`size`. /// Such a node is either an element in the [Directory] it itself is contained in, /// or a standalone root node./ #[derive(Debug, Clone, PartialEq, Eq)] pub struct DirectoryNode { - /// The (base)name of the directory - name: bytes::Bytes, /// The blake3 hash of a Directory message, serialized in protobuf canonical form. digest: B3Digest, /// Number of child elements in the Directory referred to by `digest`. @@ -23,8 +21,8 @@ pub struct DirectoryNode { } impl DirectoryNode { - pub fn new(name: bytes::Bytes, digest: B3Digest, size: u64) -> Result<Self, ValidateNodeError> { - Ok(Self { name, digest, size }) + pub fn new(digest: B3Digest, size: u64) -> Self { + Self { digest, size } } pub fn digest(&self) -> &B3Digest { @@ -34,31 +32,4 @@ impl DirectoryNode { pub fn size(&self) -> u64 { self.size } - - pub fn rename(self, name: bytes::Bytes) -> Self { - Self { name, ..self } - } -} - -impl PartialOrd for DirectoryNode { - fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { - Some(self.cmp(other)) - } -} - -impl Ord for DirectoryNode { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.get_name().cmp(other.get_name()) - } -} - -impl NamedNode for &DirectoryNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } -} -impl NamedNode for DirectoryNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } } diff --git a/tvix/castore/src/nodes/file_node.rs b/tvix/castore/src/nodes/file_node.rs index 5dd677c17b49..73abe21e59b4 100644 --- a/tvix/castore/src/nodes/file_node.rs +++ b/tvix/castore/src/nodes/file_node.rs @@ -1,11 +1,8 @@ -use crate::{B3Digest, NamedNode, ValidateNodeError}; +use crate::B3Digest; /// A FileNode represents a regular or executable file in a Directory or at the root. #[derive(Debug, Clone, PartialEq, Eq)] pub struct FileNode { - /// The (base)name of the file - name: bytes::Bytes, - /// The blake3 digest of the file contents digest: B3Digest, @@ -17,18 +14,12 @@ pub struct FileNode { } impl FileNode { - pub fn new( - name: bytes::Bytes, - digest: B3Digest, - size: u64, - executable: bool, - ) -> Result<Self, ValidateNodeError> { - Ok(Self { - name, + pub fn new(digest: B3Digest, size: u64, executable: bool) -> Self { + Self { digest, size, executable, - }) + } } pub fn digest(&self) -> &B3Digest { @@ -42,31 +33,4 @@ impl FileNode { pub fn executable(&self) -> bool { self.executable } - - pub fn rename(self, name: bytes::Bytes) -> Self { - Self { name, ..self } - } -} - -impl PartialOrd for FileNode { - fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { - Some(self.cmp(other)) - } -} - -impl Ord for FileNode { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.get_name().cmp(other.get_name()) - } -} - -impl NamedNode for &FileNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } -} -impl NamedNode for FileNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } } diff --git a/tvix/castore/src/nodes/mod.rs b/tvix/castore/src/nodes/mod.rs index bb8e14cf856e..47f39ae8f94a 100644 --- a/tvix/castore/src/nodes/mod.rs +++ b/tvix/castore/src/nodes/mod.rs @@ -4,66 +4,17 @@ mod directory_node; mod file_node; mod symlink_node; -use bytes::Bytes; pub use directory::Directory; pub use directory_node::DirectoryNode; pub use file_node::FileNode; pub use symlink_node::SymlinkNode; /// A Node is either a [DirectoryNode], [FileNode] or [SymlinkNode]. -/// While a Node by itself may have any name, only those matching specific requirements -/// can can be added as entries to a [Directory] (see the documentation on [Directory] for details). +/// Nodes themselves don't have names, what gives them names is either them +/// being inside a [Directory], or a root node with its own name attached to it. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Node { Directory(DirectoryNode), File(FileNode), Symlink(SymlinkNode), } - -impl Node { - /// Returns the node with a new name. - pub fn rename(self, name: Bytes) -> Self { - match self { - Node::Directory(n) => Node::Directory(n.rename(name)), - Node::File(n) => Node::File(n.rename(name)), - Node::Symlink(n) => Node::Symlink(n.rename(name)), - } - } -} - -impl PartialOrd for Node { - fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { - Some(self.cmp(other)) - } -} - -impl Ord for Node { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.get_name().cmp(other.get_name()) - } -} - -/// NamedNode is implemented for [FileNode], [DirectoryNode] and [SymlinkNode] -/// and [Node], so we can ask all of them for the name easily. -pub trait NamedNode { - fn get_name(&self) -> &Bytes; -} - -impl NamedNode for &Node { - fn get_name(&self) -> &Bytes { - match self { - Node::File(node_file) => node_file.get_name(), - Node::Directory(node_directory) => node_directory.get_name(), - Node::Symlink(node_symlink) => node_symlink.get_name(), - } - } -} -impl NamedNode for Node { - fn get_name(&self) -> &Bytes { - match self { - Node::File(node_file) => node_file.get_name(), - Node::Directory(node_directory) => node_directory.get_name(), - Node::Symlink(node_symlink) => node_symlink.get_name(), - } - } -} diff --git a/tvix/castore/src/nodes/symlink_node.rs b/tvix/castore/src/nodes/symlink_node.rs index 2fac27231a85..6b9df96a5dd3 100644 --- a/tvix/castore/src/nodes/symlink_node.rs +++ b/tvix/castore/src/nodes/symlink_node.rs @@ -1,50 +1,21 @@ -use crate::{NamedNode, ValidateNodeError}; +use crate::ValidateNodeError; /// A SymlinkNode represents a symbolic link in a Directory or at the root. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SymlinkNode { - /// The (base)name of the symlink - name: bytes::Bytes, /// The target of the symlink. target: bytes::Bytes, } impl SymlinkNode { - pub fn new(name: bytes::Bytes, target: bytes::Bytes) -> Result<Self, ValidateNodeError> { + pub fn new(target: bytes::Bytes) -> Result<Self, ValidateNodeError> { if target.is_empty() || target.contains(&b'\0') { return Err(ValidateNodeError::InvalidSymlinkTarget(target)); } - Ok(Self { name, target }) + Ok(Self { target }) } pub fn target(&self) -> &bytes::Bytes { &self.target } - - pub(crate) fn rename(self, name: bytes::Bytes) -> Self { - Self { name, ..self } - } -} - -impl PartialOrd for SymlinkNode { - fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { - Some(self.cmp(other)) - } -} - -impl Ord for SymlinkNode { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.get_name().cmp(other.get_name()) - } -} - -impl NamedNode for &SymlinkNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } -} -impl NamedNode for SymlinkNode { - fn get_name(&self) -> &bytes::Bytes { - &self.name - } } diff --git a/tvix/castore/src/path.rs b/tvix/castore/src/path.rs index fb340478c08c..8a55e9f5a1d3 100644 --- a/tvix/castore/src/path.rs +++ b/tvix/castore/src/path.rs @@ -38,7 +38,9 @@ impl Path { if !bytes.is_empty() { // Ensure all components are valid castore node names. for component in bytes.split_str(b"/") { - Directory::validate_node_name(component).ok()?; + if !Directory::is_valid_name(component) { + return None; + } } } @@ -211,7 +213,9 @@ impl PathBuf { /// Adjoins `name` to self. pub fn try_push(&mut self, name: &[u8]) -> Result<(), std::io::Error> { - Directory::validate_node_name(name).map_err(|_| std::io::ErrorKind::InvalidData)?; + if !Directory::is_valid_name(name) { + return Err(std::io::ErrorKind::InvalidData.into()); + } if !self.inner.is_empty() { self.inner.push(b'/'); diff --git a/tvix/castore/src/proto/grpc_directoryservice_wrapper.rs b/tvix/castore/src/proto/grpc_directoryservice_wrapper.rs index e65145509184..62fdb34a25a0 100644 --- a/tvix/castore/src/proto/grpc_directoryservice_wrapper.rs +++ b/tvix/castore/src/proto/grpc_directoryservice_wrapper.rs @@ -1,5 +1,5 @@ use crate::directoryservice::{DirectoryGraph, DirectoryService, LeavesToRootValidator}; -use crate::{proto, B3Digest, ValidateDirectoryError}; +use crate::{proto, B3Digest, DirectoryError}; use futures::stream::BoxStream; use futures::TryStreamExt; use std::ops::Deref; @@ -84,7 +84,7 @@ where let mut validator = DirectoryGraph::<LeavesToRootValidator>::default(); while let Some(directory) = req_inner.message().await? { validator - .add(directory.try_into().map_err(|e: ValidateDirectoryError| { + .add(directory.try_into().map_err(|e: DirectoryError| { tonic::Status::new(tonic::Code::Internal, e.to_string()) })?) .map_err(|e| tonic::Status::new(tonic::Code::Internal, e.to_string()))?; diff --git a/tvix/castore/src/proto/mod.rs b/tvix/castore/src/proto/mod.rs index 8f8f3c08347a..7cb1cecd27fa 100644 --- a/tvix/castore/src/proto/mod.rs +++ b/tvix/castore/src/proto/mod.rs @@ -5,12 +5,10 @@ use prost::Message; mod grpc_blobservice_wrapper; mod grpc_directoryservice_wrapper; +use crate::{B3Digest, DirectoryError}; pub use grpc_blobservice_wrapper::GRPCBlobServiceWrapper; pub use grpc_directoryservice_wrapper::GRPCDirectoryServiceWrapper; -use crate::NamedNode; -use crate::{B3Digest, ValidateDirectoryError, ValidateNodeError}; - tonic::include_proto!("tvix.castore.v1"); #[cfg(feature = "tonic-reflection")] @@ -71,168 +69,83 @@ impl Directory { /// Accepts a name, and a mutable reference to the previous name. /// If the passed name is larger than the previous one, the reference is updated. /// If it's not, an error is returned. -fn update_if_lt_prev<'n>( - prev_name: &mut &'n [u8], - name: &'n [u8], -) -> Result<(), ValidateDirectoryError> { +fn update_if_lt_prev<'n>(prev_name: &mut &'n [u8], name: &'n [u8]) -> Result<(), DirectoryError> { if *name < **prev_name { - return Err(ValidateDirectoryError::WrongSorting(name.to_vec())); + return Err(DirectoryError::WrongSorting(bytes::Bytes::copy_from_slice( + name, + ))); } *prev_name = name; Ok(()) } -impl TryFrom<&node::Node> for crate::Node { - type Error = ValidateNodeError; - - fn try_from(node: &node::Node) -> Result<crate::Node, ValidateNodeError> { - Ok(match node { - node::Node::Directory(n) => crate::Node::Directory(n.try_into()?), - node::Node::File(n) => crate::Node::File(n.try_into()?), - node::Node::Symlink(n) => crate::Node::Symlink(n.try_into()?), - }) - } -} - -impl TryFrom<&Node> for crate::Node { - type Error = ValidateNodeError; - - fn try_from(node: &Node) -> Result<crate::Node, ValidateNodeError> { - match node { - Node { node: None } => Err(ValidateNodeError::NoNodeSet), - Node { node: Some(node) } => node.try_into(), - } - } -} - -impl TryFrom<&DirectoryNode> for crate::DirectoryNode { - type Error = ValidateNodeError; - - fn try_from(node: &DirectoryNode) -> Result<crate::DirectoryNode, ValidateNodeError> { - crate::DirectoryNode::new( - node.name.clone(), - node.digest.clone().try_into()?, - node.size, - ) - } -} - -impl TryFrom<&SymlinkNode> for crate::SymlinkNode { - type Error = ValidateNodeError; - - fn try_from(node: &SymlinkNode) -> Result<crate::SymlinkNode, ValidateNodeError> { - crate::SymlinkNode::new(node.name.clone(), node.target.clone()) - } -} - -impl TryFrom<&FileNode> for crate::FileNode { - type Error = ValidateNodeError; - - fn try_from(node: &FileNode) -> Result<crate::FileNode, ValidateNodeError> { - crate::FileNode::new( - node.name.clone(), - node.digest.clone().try_into()?, - node.size, - node.executable, - ) - } -} - +// TODO: add a proper owned version here that moves various fields impl TryFrom<Directory> for crate::Directory { - type Error = ValidateDirectoryError; + type Error = DirectoryError; - fn try_from(directory: Directory) -> Result<crate::Directory, ValidateDirectoryError> { - (&directory).try_into() + fn try_from(value: Directory) -> Result<Self, Self::Error> { + (&value).try_into() } } impl TryFrom<&Directory> for crate::Directory { - type Error = ValidateDirectoryError; + type Error = DirectoryError; - fn try_from(directory: &Directory) -> Result<crate::Directory, ValidateDirectoryError> { + fn try_from(directory: &Directory) -> Result<crate::Directory, DirectoryError> { let mut dir = crate::Directory::new(); + let mut last_file_name: &[u8] = b""; + + // TODO: this currently loops over all three types separately, rather + // than peeking and picking from where would be the next. + for file in directory.files.iter().map(move |file| { update_if_lt_prev(&mut last_file_name, &file.name).map(|()| file.clone()) }) { let file = file?; - dir.add(crate::Node::File((&file).try_into().map_err(|e| { - ValidateDirectoryError::InvalidNode(file.name.into(), e) - })?))?; + + let (name, node) = Node { + node: Some(node::Node::File(file)), + } + .into_name_and_node()?; + + dir.add(name, node)?; } let mut last_directory_name: &[u8] = b""; for directory in directory.directories.iter().map(move |directory| { update_if_lt_prev(&mut last_directory_name, &directory.name).map(|()| directory.clone()) }) { let directory = directory?; - dir.add(crate::Node::Directory((&directory).try_into().map_err( - |e| ValidateDirectoryError::InvalidNode(directory.name.into(), e), - )?))?; + + let (name, node) = Node { + node: Some(node::Node::Directory(directory)), + } + .into_name_and_node()?; + + dir.add(name, node)?; } let mut last_symlink_name: &[u8] = b""; for symlink in directory.symlinks.iter().map(move |symlink| { update_if_lt_prev(&mut last_symlink_name, &symlink.name).map(|()| symlink.clone()) }) { let symlink = symlink?; - dir.add(crate::Node::Symlink((&symlink).try_into().map_err( - |e| ValidateDirectoryError::InvalidNode(symlink.name.into(), e), - )?))?; - } - Ok(dir) - } -} -impl From<&crate::Node> for node::Node { - fn from(node: &crate::Node) -> node::Node { - match node { - crate::Node::Directory(n) => node::Node::Directory(n.into()), - crate::Node::File(n) => node::Node::File(n.into()), - crate::Node::Symlink(n) => node::Node::Symlink(n.into()), - } - } -} - -impl From<&crate::Node> for Node { - fn from(node: &crate::Node) -> Node { - Node { - node: Some(node.into()), - } - } -} - -impl From<&crate::DirectoryNode> for DirectoryNode { - fn from(node: &crate::DirectoryNode) -> DirectoryNode { - DirectoryNode { - digest: node.digest().clone().into(), - size: node.size(), - name: node.get_name().clone(), - } - } -} + let (name, node) = Node { + node: Some(node::Node::Symlink(symlink)), + } + .into_name_and_node()?; -impl From<&crate::FileNode> for FileNode { - fn from(node: &crate::FileNode) -> FileNode { - FileNode { - digest: node.digest().clone().into(), - size: node.size(), - name: node.get_name().clone(), - executable: node.executable(), + dir.add(name, node)?; } - } -} -impl From<&crate::SymlinkNode> for SymlinkNode { - fn from(node: &crate::SymlinkNode) -> SymlinkNode { - SymlinkNode { - name: node.get_name().clone(), - target: node.target().clone(), - } + Ok(dir) } } +// TODO: add a proper owned version here that moves various fields impl From<crate::Directory> for Directory { - fn from(directory: crate::Directory) -> Directory { - (&directory).into() + fn from(value: crate::Directory) -> Self { + (&value).into() } } @@ -241,16 +154,25 @@ impl From<&crate::Directory> for Directory { let mut directories = vec![]; let mut files = vec![]; let mut symlinks = vec![]; - for node in directory.nodes() { + + for (name, node) in directory.nodes() { match node { - crate::Node::File(n) => { - files.push(n.into()); - } - crate::Node::Directory(n) => { - directories.push(n.into()); - } + crate::Node::File(n) => files.push(FileNode { + name: name.clone(), + digest: n.digest().to_owned().into(), + size: n.size(), + executable: n.executable(), + }), + crate::Node::Directory(n) => directories.push(DirectoryNode { + name: name.clone(), + digest: n.digest().to_owned().into(), + size: n.size(), + }), crate::Node::Symlink(n) => { - symlinks.push(n.into()); + symlinks.push(SymlinkNode { + name: name.clone(), + target: n.target().to_owned(), + }); } } } @@ -262,6 +184,67 @@ impl From<&crate::Directory> for Directory { } } +impl Node { + /// Converts a proto [Node] to a [crate::Node], and splits off the name. + pub fn into_name_and_node(self) -> Result<(bytes::Bytes, crate::Node), DirectoryError> { + match self.node.ok_or_else(|| DirectoryError::NoNodeSet)? { + node::Node::Directory(n) => { + let digest = B3Digest::try_from(n.digest) + .map_err(|e| DirectoryError::InvalidNode(n.name.to_owned(), e.into()))?; + + let node = crate::Node::Directory(crate::DirectoryNode::new(digest, n.size)); + + Ok((n.name, node)) + } + node::Node::File(n) => { + let digest = B3Digest::try_from(n.digest) + .map_err(|e| DirectoryError::InvalidNode(n.name.to_owned(), e.into()))?; + + let node = crate::Node::File(crate::FileNode::new(digest, n.size, n.executable)); + + Ok((n.name, node)) + } + + node::Node::Symlink(n) => { + let node = crate::Node::Symlink( + crate::SymlinkNode::new(n.target) + .map_err(|e| DirectoryError::InvalidNode(n.name.to_owned(), e))?, + ); + + Ok((n.name, node)) + } + } + } + + /// Construsts a [Node] from a name and [crate::Node]. + pub fn from_name_and_node(name: bytes::Bytes, n: crate::Node) -> Self { + // TODO: make these pub(crate) so we can avoid cloning? + match n { + crate::Node::Directory(directory_node) => Self { + node: Some(node::Node::Directory(DirectoryNode { + name, + digest: directory_node.digest().to_owned().into(), + size: directory_node.size(), + })), + }, + crate::Node::File(file_node) => Self { + node: Some(node::Node::File(FileNode { + name, + digest: file_node.digest().to_owned().into(), + size: file_node.size(), + executable: file_node.executable(), + })), + }, + crate::Node::Symlink(symlink_node) => Self { + node: Some(node::Node::Symlink(SymlinkNode { + name, + target: symlink_node.target().to_owned(), + })), + }, + } + } +} + impl StatBlobResponse { /// Validates a StatBlobResponse. All chunks must have valid blake3 digests. /// It is allowed to send an empty list, if no more granular chunking is diff --git a/tvix/castore/src/proto/tests/directory.rs b/tvix/castore/src/proto/tests/directory.rs index 7847b9c4c9cb..215bce7275dd 100644 --- a/tvix/castore/src/proto/tests/directory.rs +++ b/tvix/castore/src/proto/tests/directory.rs @@ -1,4 +1,4 @@ -use crate::proto::{Directory, DirectoryNode, FileNode, SymlinkNode, ValidateDirectoryError}; +use crate::proto::{Directory, DirectoryError, DirectoryNode, FileNode, SymlinkNode}; use crate::ValidateNodeError; use hex_literal::hex; @@ -162,8 +162,8 @@ fn validate_invalid_names() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(n, ValidateNodeError::InvalidName(_)) => { - assert_eq!(n, b"") + DirectoryError::InvalidName(n) => { + assert_eq!(n.as_ref(), b"") } _ => panic!("unexpected error"), }; @@ -179,8 +179,8 @@ fn validate_invalid_names() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(n, ValidateNodeError::InvalidName(_)) => { - assert_eq!(n, b".") + DirectoryError::InvalidName(n) => { + assert_eq!(n.as_ref(), b".") } _ => panic!("unexpected error"), }; @@ -197,8 +197,8 @@ fn validate_invalid_names() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(n, ValidateNodeError::InvalidName(_)) => { - assert_eq!(n, b"..") + DirectoryError::InvalidName(n) => { + assert_eq!(n.as_ref(), b"..") } _ => panic!("unexpected error"), }; @@ -213,8 +213,8 @@ fn validate_invalid_names() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(n, ValidateNodeError::InvalidName(_)) => { - assert_eq!(n, b"\x00") + DirectoryError::InvalidName(n) => { + assert_eq!(n.as_ref(), b"\x00") } _ => panic!("unexpected error"), }; @@ -229,8 +229,8 @@ fn validate_invalid_names() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(n, ValidateNodeError::InvalidName(_)) => { - assert_eq!(n, b"foo/bar") + DirectoryError::InvalidName(n) => { + assert_eq!(n.as_ref(), b"foo/bar") } _ => panic!("unexpected error"), }; @@ -248,7 +248,7 @@ fn validate_invalid_digest() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::InvalidNode(_, ValidateNodeError::InvalidDigestLen(n)) => { + DirectoryError::InvalidNode(_, ValidateNodeError::InvalidDigestLen(n)) => { assert_eq!(n, 2) } _ => panic!("unexpected error"), @@ -275,8 +275,8 @@ fn validate_sorting() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::WrongSorting(s) => { - assert_eq!(s, b"a"); + DirectoryError::WrongSorting(s) => { + assert_eq!(s.as_ref(), b"a"); } _ => panic!("unexpected error"), } @@ -300,7 +300,7 @@ fn validate_sorting() { ..Default::default() }; match crate::Directory::try_from(d).expect_err("must fail") { - ValidateDirectoryError::DuplicateName(s) => { + DirectoryError::DuplicateName(s) => { assert_eq!(s, b"a"); } _ => panic!("unexpected error"), diff --git a/tvix/castore/src/tests/import.rs b/tvix/castore/src/tests/import.rs index 5dff61782cf2..bbf28a2981bf 100644 --- a/tvix/castore/src/tests/import.rs +++ b/tvix/castore/src/tests/import.rs @@ -2,15 +2,11 @@ use crate::blobservice::{self, BlobService}; use crate::directoryservice; use crate::fixtures::*; use crate::import::fs::ingest_path; -use crate::proto; use crate::{DirectoryNode, Node, SymlinkNode}; use tempfile::TempDir; #[cfg(target_family = "unix")] -use std::os::unix::ffi::OsStrExt; - -#[cfg(target_family = "unix")] #[tokio::test] async fn symlink() { let blob_service = blobservice::from_addr("memory://").await.unwrap(); @@ -34,9 +30,7 @@ async fn symlink() { .expect("must succeed"); assert_eq!( - Node::Symlink( - SymlinkNode::new("doesntmatter".into(), "/nix/store/somewhereelse".into(),).unwrap() - ), + Node::Symlink(SymlinkNode::new("/nix/store/somewhereelse".into(),).unwrap()), root_node, ) } @@ -59,13 +53,12 @@ async fn single_file() { .expect("must succeed"); assert_eq!( - proto::node::Node::File(proto::FileNode { - name: "root".into(), - digest: HELLOWORLD_BLOB_DIGEST.clone().into(), - size: HELLOWORLD_BLOB_CONTENTS.len() as u64, - executable: false, - }), - (&root_node).into(), + Node::File(crate::FileNode::new( + HELLOWORLD_BLOB_DIGEST.clone(), + HELLOWORLD_BLOB_CONTENTS.len() as u64, + false, + )), + root_node, ); // ensure the blob has been uploaded @@ -95,20 +88,10 @@ async fn complicated() { // ensure root_node matched expectations assert_eq!( - Node::Directory( - DirectoryNode::new( - tmpdir - .path() - .file_name() - .unwrap() - .as_bytes() - .to_owned() - .into(), - DIRECTORY_COMPLICATED.digest().clone(), - DIRECTORY_COMPLICATED.size(), - ) - .unwrap() - ), + Node::Directory(DirectoryNode::new( + DIRECTORY_COMPLICATED.digest().clone(), + DIRECTORY_COMPLICATED.size(), + )), root_node, ); |