fix(foundationdb): layer verification

PierreZ · PierreZ · commit 9281bd3cf767 · 2021-04-07T22:36:31.000+02:00
diff --git a/foundationdb/src/directory/directory_layer.rs b/foundationdb/src/directory/directory_layer.rs
@@ -6,7 +6,7 @@ use crate::tuple::{Subspace, TuplePack};
 use crate::{FdbResult, Transaction};
 
 use crate::directory::DirectorySubspace;
-use crate::directory::{compare_slice_string, Directory};
+use crate::directory::{compare_slice, Directory};
 use async_trait::async_trait;
 use byteorder::{LittleEndian, WriteBytesExt};
 use std::cmp::Ordering;
@@ -120,7 +120,7 @@ impl Directory for DirectoryLayer {
         }
 
         match self
-            .find_or_create_node(trx, path.to_owned(), false, None)
+            .find_or_create_node(trx, path.to_owned(), false, None, None)
             .await
         {
             Ok(_node) => Ok(true),
@@ -157,15 +157,15 @@ impl Directory for DirectoryLayer {
             slice_end = new_path.len();
         }
 
-        if compare_slice_string(&old_path[..], &new_path[..slice_end]) == Ordering::Equal
+        if compare_slice(&old_path[..], &new_path[..slice_end]) == Ordering::Equal
             || old_path.is_empty()
             || new_path.is_empty()
         {
             return Err(DirectoryError::CannotMoveBetweenSubdirectory);
         }
 
         let old_node = self
-            .find_or_create_node(&trx, old_path.to_owned(), false, None)
+            .find_or_create_node(&trx, old_path.to_owned(), false, None, None)
             .await?;
 
         if self.exists(&trx, new_path.to_owned()).await? {
@@ -178,6 +178,7 @@ impl Directory for DirectoryLayer {
                 Vec::from(&new_path.to_owned()[..new_path.len() - 1]),
                 true,
                 None,
+                None,
             )
             .await?;
 
@@ -190,7 +191,7 @@ impl Directory for DirectoryLayer {
         }
 
         // create new node
-        self.find_or_create_node(&trx, new_path.to_owned(), true, Some(prefix))
+        self.find_or_create_node(&trx, new_path.to_owned(), true, Some(prefix), None)
             .await?;
 
         let child_name = old_path.last().unwrap().to_owned();
@@ -205,7 +206,7 @@ impl Directory for DirectoryLayer {
     /// as well as all of their contents.
     async fn remove(&self, trx: &Transaction, path: Vec<String>) -> Result<bool, DirectoryError> {
         let node = self
-            .find_or_create_node(trx, path.to_owned(), false, None)
+            .find_or_create_node(trx, path.to_owned(), false, None, None)
             .await?;
         node.remove_all(trx).await?;
         Ok(true)
@@ -219,7 +220,7 @@ impl Directory for DirectoryLayer {
         path: Vec<String>,
     ) -> Result<Vec<String>, DirectoryError> {
         let node = self
-            .find_or_create_node(trx, path.to_owned(), false, None)
+            .find_or_create_node(trx, path.to_owned(), false, None, None)
             .await?;
         node.list(&trx).await
     }
@@ -259,7 +260,13 @@ impl DirectoryLayer {
         }
 
         match self
-            .find_or_create_node(&trx, path.to_owned(), allow_create, prefix.to_owned())
+            .find_or_create_node(
+                &trx,
+                path.to_owned(),
+                allow_create,
+                prefix.to_owned(),
+                layer.to_owned(),
+            )
             .await
         {
             Ok(node) => {
@@ -384,6 +391,7 @@ impl DirectoryLayer {
         path: Vec<String>,
         allow_creation: bool,
         prefix: Option<Vec<u8>>,
+        layer: Option<Vec<u8>>,
     ) -> Result<Node, DirectoryError> {
         let mut node = Node {
             layer: None,
@@ -449,6 +457,10 @@ impl DirectoryLayer {
             if new_node {
                 trx.set(key.bytes(), prefix.as_slice());
             }
+
+            if i == last_path_index && layer.is_some() && new_node {
+                node.store_layer(&trx, layer.to_owned().unwrap().to_owned())?;
+            }
         }
 
         Ok(node)
diff --git a/foundationdb/src/directory/directory_subspace.rs b/foundationdb/src/directory/directory_subspace.rs
@@ -1,6 +1,6 @@
 use crate::directory::directory_layer::DirectoryLayer;
 use crate::directory::error::DirectoryError;
-use crate::directory::{compare_slice_string, Directory};
+use crate::directory::{compare_slice, Directory};
 use crate::tuple::{PackResult, Subspace, TuplePack, TupleUnpack};
 use crate::Transaction;
 use async_trait::async_trait;
@@ -112,7 +112,7 @@ impl Directory for DirectorySubspace {
             return Err(DirectoryError::CannotMoveBetweenPartition);
         }
 
-        if compare_slice_string(&new_path[..partition_length], &self.path) != Ordering::Equal {
+        if compare_slice(&new_path[..partition_length], &self.path) != Ordering::Equal {
             Err(DirectoryError::CannotMoveBetweenPartition)
         } else {
             self.move_to(
diff --git a/foundationdb/src/directory/mod.rs b/foundationdb/src/directory/mod.rs
@@ -70,7 +70,7 @@ pub trait Directory {
     fn get_layer(&self) -> Vec<u8>;
 }
 
-pub fn compare_slice_string(a: &[String], b: &[String]) -> cmp::Ordering {
+pub fn compare_slice<T: Ord>(a: &[T], b: &[T]) -> cmp::Ordering {
     for (ai, bi) in a.iter().zip(b.iter()) {
         match ai.cmp(&bi) {
             Ordering::Equal => continue,
diff --git a/foundationdb/src/directory/node.rs b/foundationdb/src/directory/node.rs
@@ -7,10 +7,12 @@
 // copied, modified, or distributed except according to those terms.
 
 use crate::directory::directory_layer::DEFAULT_SUB_DIRS;
-use crate::directory::DirectoryError;
+use crate::directory::{compare_slice, DirectoryError};
 use crate::tuple::{Subspace, TuplePack};
 use crate::{FdbError, RangeOption, Transaction};
 
+use std::cmp::Ordering;
+
 /// Node are used to represent the paths generated by a Directory.
 /// They are stored in the `Directory.node_subspace``.
 #[derive(Debug)]
@@ -47,15 +49,25 @@ impl Node {
                     unimplemented!("partition is not yet supported")
                 }
 
-                if layer_bytes.len() != layer.len() {
-                    Err(DirectoryError::IncompatibleLayer)
-                } else {
-                    Ok(())
+                match compare_slice(&layer_bytes[..], &layer[..]) {
+                    Ordering::Less | Ordering::Greater => Err(DirectoryError::IncompatibleLayer),
+                    Ordering::Equal => Ok(()),
                 }
             }
         }
     }
 
+    pub(crate) fn store_layer(
+        &mut self,
+        trx: &Transaction,
+        layer: Vec<u8>,
+    ) -> Result<(), FdbError> {
+        let key = self.node_subspace.subspace(&b"layer".to_vec());
+        trx.set(key.bytes(), layer.as_slice());
+        self.layer = Some(layer);
+        Ok(())
+    }
+
     /// retrieve the layer used for this node
     pub(crate) async fn retrieve_layer(&mut self, trx: &Transaction) -> Result<(), FdbError> {
         if self.layer == None {
diff --git a/foundationdb/tests/directory.rs b/foundationdb/tests/directory.rs
@@ -29,7 +29,7 @@ fn test_create_or_open_directory() {
     futures::executor::block_on(test_create_or_open_async_then_delete(
         &db,
         &directory,
-        vec![String::from("application")],
+        vec![String::from("a")],
     ))
     .expect("failed to run");
 
@@ -43,15 +43,117 @@ fn test_create_or_open_directory() {
     futures::executor::block_on(test_list(&db, &directory, vec![String::from("a")], 10))
         .expect("failed to run");
 
-    // TODO: fix prefix
-    // futures::executor::block_on(test_prefix(
-    //     &db,
-    //     vec![String::from("prefix")],
-    //     vec![0x01, 0x02],
-    // ))
-    // .expect("failed to run");
+    futures::executor::block_on(test_children_content_subspace(
+        &db,
+        &directory,
+        vec![String::from("c")],
+    ))
+    .expect("failed to run");
+
+    futures::executor::block_on(test_bad_layer(&db)).expect("failed to run");
+
+    eprintln!("clearing all keys");
+    let trx = db.create_trx().expect("cannot create txn");
+    trx.clear_range(b"", b"\xff");
+    futures::executor::block_on(trx.commit()).expect("could not clear keys");
+
+    // test deletions, first we need to create it
+    futures::executor::block_on(test_create_or_open_async_then_delete(
+        &db,
+        &directory,
+        vec![String::from("deletion")],
+    ))
+    .expect("failed to run");
+
+    futures::executor::block_on(test_create_then_delete(
+        &db,
+        &directory,
+        vec![String::from("n0")],
+        1,
+    ))
+    .expect("failed to run");
+
+    futures::executor::block_on(test_prefix(
+        &db,
+        vec![String::from("prefix")],
+        vec![0xFC, 0xFC],
+    ))
+    .expect("failed to run");
+    futures::executor::block_on(test_not_allowed_prefix(&db, vec![0xFC, 0xFC]))
+        .expect_err("should have failed");
+
+    // moves
+    eprintln!("clearing all keys");
+    let trx = db.create_trx().expect("cannot create txn");
+    trx.clear_range(b"", b"\xff");
+    futures::executor::block_on(trx.commit()).expect("could not clear keys");
+
+    futures::executor::block_on(test_create_then_move_to(
+        &db,
+        &directory,
+        vec![String::from("d"), String::from("e")],
+        vec![String::from("a")],
+    ))
+    .expect("failed to run");
+
+    // trying to move on empty path
+    match futures::executor::block_on(test_move_to(
+        &db,
+        &directory,
+        vec![String::from("dsa")],
+        vec![],
+    )) {
+        Err(DirectoryError::NoPathProvided) => {}
+        _ => panic!("should have failed"),
+    }
+
+    // trying to move on empty path
+    match futures::executor::block_on(test_move_to(
+        &db,
+        &directory,
+        vec![],
+        vec![String::from("dsa")],
+    )) {
+        Err(DirectoryError::NoPathProvided) => {}
+        Err(err) => panic!("should have NoPathProvided, got {:?}", err),
+        Ok(()) => panic!("should not be fine"),
+    }
+
+    // source path does not exists
+    match futures::executor::block_on(test_move_to(
+        &db,
+        &directory,
+        vec![String::from("e")],
+        vec![String::from("f")],
+    )) {
+        Err(DirectoryError::PathDoesNotExists) => {}
+        Err(err) => panic!("should have NoPathProvided, got {:?}", err),
+        Ok(()) => panic!("should not be fine"),
+    }
 
-    futures::executor::block_on(test_bad_layer(&db)).expect_err("should have failed");
+    // destination's parent does not exists
+    match futures::executor::block_on(test_create_then_move_to(
+        &db,
+        &directory,
+        vec![String::from("a"), String::from("g")],
+        vec![String::from("i-do-not-exists-yet"), String::from("z")],
+    )) {
+        Err(DirectoryError::ParentDirDoesNotExists) => {}
+        Err(err) => panic!("should have ParentDirDoesNotExists, got {:?}", err),
+        Ok(()) => panic!("should not be fine"),
+    }
+
+    // destination not empty
+    match futures::executor::block_on(test_create_then_move_to(
+        &db,
+        &directory,
+        vec![String::from("a"), String::from("g")],
+        vec![String::from("a"), String::from("g")],
+    )) {
+        Err(DirectoryError::BadDestinationDirectory) => {}
+        Err(err) => panic!("should have BadDestinationDirectory, got {:?}", err),
+        Ok(()) => panic!("should not be fine"),
+    }
 }
 
 async fn test_prefix(
@@ -89,7 +191,7 @@ async fn test_not_allowed_prefix(db: &Database, prefix: Vec<u8>) -> Result<(), D
     directory
         .create_or_open(
             &trx,
-            vec![String::from("bad_layer")],
+            vec![String::from("prefix_not_allowed")],
             Some(prefix.to_owned()),
             None,
         )
@@ -288,23 +390,27 @@ async fn test_create_or_open_async_then_delete(
 }
 
 /// testing that we throwing Err(DirectoryError::IncompatibleLayer)
-async fn test_bad_layer(db: &Database) -> Result<DirectorySubspace, DirectoryError> {
+async fn test_bad_layer(db: &Database) -> Result<(), DirectoryError> {
     let directory = DirectoryLayer {
         ..Default::default()
     };
     let trx = db.create_trx()?;
 
+    eprintln!("creating directory with one layer");
     directory
         .create_or_open(&trx, vec![String::from("bad_layer")], None, Some(vec![0u8]))
         .await?;
 
-    let directory = DirectoryLayer {
-        ..Default::default()
-    };
+    trx.commit().await.expect("cannot commit");
+    let trx = db.create_trx()?;
 
-    return directory
+    eprintln!("opening directory with another");
+    let result = directory
         .create_or_open(&trx, vec![String::from("bad_layer")], None, Some(vec![1u8]))
         .await;
+
+    assert!(result.is_err(), "expected an error, got {:?}", result);
+    Ok(())
 }
 
 /// testing list functionality. Will open paths and create n sub-folders.
@@ -352,3 +458,35 @@ async fn test_list(
 
     Ok(())
 }
+
+/// checks that the content_subspace of the children is inside the parent
+async fn test_children_content_subspace(
+    db: &Database,
+    directory: &DirectoryLayer,
+    paths: Vec<String>,
+) -> Result<(), DirectoryError> {
+    let trx = db.create_trx()?;
+
+    eprintln!("parent = {:?}", paths.to_owned());
+
+    let _root_subspace = directory
+        .create_or_open(&trx, paths.to_owned(), None, None)
+        .await?;
+
+    let mut children_path = paths.clone();
+    children_path.push(String::from("nested"));
+    eprintln!("children = {:?}", children_path.to_owned());
+
+    let children_subspace = directory
+        .create_or_open(&trx, children_path.to_owned(), None, None)
+        .await?;
+
+    trx.commit().await.expect("could not commit");
+    let trx = db.create_trx()?;
+
+    let open_children_subspace = directory.open(&trx, children_path.to_owned(), None).await?;
+
+    assert_eq!(children_subspace.bytes(), open_children_subspace.bytes());
+
+    Ok(())
+}

Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,7 @@ pub trait Directory {`
`70`	`70`	`fn get_layer(&self) -> Vec<u8>;`
`71`	`71`	`}`
`72`	`72`
`73`		`-pub fn compare_slice_string(a: &[String], b: &[String]) -> cmp::Ordering {`
	`73`	`+pub fn compare_slice<T: Ord>(a: &[T], b: &[T]) -> cmp::Ordering {`
`74`	`74`	`for (ai, bi) in a.iter().zip(b.iter()) {`
`75`	`75`	`match ai.cmp(&bi) {`
`76`	`76`	`Ordering::Equal => continue,`