From d9f4898a3afb4bb515d7d420421e31e02779034f Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Tue, 20 Sep 2016 16:30:19 +0200 Subject: [PATCH 1/7] Abstract testing of hook execution in helper function --- libimagstore/src/store.rs | 72 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 07511e28..4a9a4524 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2582,16 +2582,80 @@ aspect = "test" "# } - #[test] - fn test_pre_create() { + fn test_hook_execution(hook_positions: &[HP]) { let mut store = get_store_with_config(); let pos = HP::PreCreate; let hook = TestHook::new(pos.clone(), true, false); - assert!(store.register_hook(pos, "test", Box::new(hook)).map_err(|e| println!("{:?}", e)).is_ok()); + println!("Registering hooks..."); + for pos in hook_positions { + let hook = TestHook::new(pos.clone(), true, false); + println!("\tRegistering: {:?}", pos); + assert!(store.register_hook(pos.clone(), "test", Box::new(hook)) + .map_err(|e| println!("{:?}", e)) + .is_ok() + ); + } + println!("... done."); - let pb = StoreId::new_baseless(PathBuf::from("test")).unwrap(); + let pb = StoreId::new_baseless(PathBuf::from("test")).unwrap(); + let pb_moved = StoreId::new_baseless(PathBuf::from("test-moved")).unwrap(); + + println!("Creating {:?}", pb); assert!(store.create(pb.clone()).is_ok()); + + { + println!("Getting {:?} -> Some?", pb); + assert!(match store.get(pb.clone()) { + Ok(Some(_)) => true, + _ => false, + }); + } + + { + println!("Getting {:?} -> None?", pb_moved); + assert!(match store.get(pb_moved.clone()) { + Ok(None) => true, + _ => false, + }); + } + + { + println!("Moving {:?} -> {:?}", pb, pb_moved); + assert!(store.move_by_id(pb.clone(), pb_moved.clone()).is_ok()); + } + + { + println!("Getting {:?} -> None", pb); + assert!(match store.get(pb.clone()) { + Ok(None) => true, + _ => false, + }); + } + + { + println!("Getting {:?} -> Some", pb_moved); + assert!(match store.get(pb_moved.clone()) { + Ok(Some(_)) => true, + _ => false, + }); + } + + { + println!("Getting {:?} -> Some -> updating", pb_moved); + assert!(match store.get(pb_moved.clone()).map_err(|e| println!("ERROR GETTING: {:?}", e)) { + Ok(Some(fle)) => store.update(fle).map_err(|e| println!("ERROR UPDATING: {:?}", e)).is_ok(), + _ => false, + }); + } + + println!("Deleting {:?}", pb_moved); + assert!(store.delete(pb_moved).is_ok()); + } + + #[test] + fn test_pre_create() { + test_hook_execution(&[HP::PreCreate]); } } From 1e83ad7bbd8834d555bb0e7387e824a2ea1fe503 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Wed, 21 Sep 2016 10:08:01 +0200 Subject: [PATCH 2/7] Add test for hook execution for each hook position --- libimagstore/src/store.rs | 57 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 4a9a4524..cbe7b54d 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2654,9 +2654,64 @@ aspect = "test" } #[test] - fn test_pre_create() { + fn test_storeunload() { + test_hook_execution(&[HP::StoreUnload]); + } + + #[test] + fn test_precreate() { test_hook_execution(&[HP::PreCreate]); } + #[test] + fn test_postcreate() { + test_hook_execution(&[HP::PostCreate]); + } + + #[test] + fn test_preretrieve() { + test_hook_execution(&[HP::PreRetrieve]); + } + + #[test] + fn test_postretrieve() { + test_hook_execution(&[HP::PostRetrieve]); + } + + #[test] + fn test_preupdate() { + test_hook_execution(&[HP::PreUpdate]); + } + + #[test] + fn test_postupdate() { + test_hook_execution(&[HP::PostUpdate]); + } + + #[test] + fn test_predelete() { + test_hook_execution(&[HP::PreDelete]); + } + + #[test] + fn test_postdelete() { + test_hook_execution(&[HP::PostDelete]); + } + + #[test] + fn test_multiple_same_position() { + let positions = [ HP::StoreUnload, HP::PreCreate, HP::PostCreate, HP::PreRetrieve, + HP::PostRetrieve, HP::PreUpdate, HP::PostUpdate, HP::PreDelete, HP::PostDelete ]; + + for position in positions.iter() { + for n in 2..10 { + let mut v = Vec::with_capacity(n); + for x in 0..n { v.push(position.clone()); } + + test_hook_execution(&v); + } + } + } + } From b189bf7b8cead0bd33190755d46e75f09584cd58 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Tue, 4 Oct 2016 16:36:40 +0200 Subject: [PATCH 3/7] Add check if entry is present If we try to rename an entry that is borrowed, we fail, as renaming an borrowed entry might result in some _really_ ugly bugs. --- libimagstore/src/store.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index cbe7b54d..1320c51d 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -670,6 +670,13 @@ impl Store { return Err(SEK::EntryAlreadyExists.into_error()); } + // if we do not have an entry here, we fail in `FileAbstraction::rename()` below. + // if we have one, but it is borrowed, we really should not rename it, as this might + // lead to strange errors + if hsmap.get(&old_id).map(|e| e.is_borrowed()).unwrap_or(false) { + return Err(SEK::EntryAlreadyBorrowed.into_error()); + } + let old_id_pb = try!(old_id.clone().with_base(self.path().clone()).into_pathbuf()); let new_id_pb = try!(new_id.clone().with_base(self.path().clone()).into_pathbuf()); From c72291159e5da3bd0e19f294dd1fb79a6f17eff0 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 17:47:49 +0200 Subject: [PATCH 4/7] Add comment/documentation for Store::move_by_id() --- libimagstore/src/store.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 1320c51d..35e42d17 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -649,6 +649,41 @@ impl Store { } /// Move an entry without loading + /// + /// This function moves an entry from one path to another. + /// + /// Generally, this function shouldn't be used by library authors, if they "just" want to move + /// something around. A library for moving entries while caring about meta-data and links. + /// + /// # Errors + /// + /// This function returns an error in certain cases: + /// + /// * If pre-move-hooks error (if they return an error which indicates that the action should be + /// aborted) + /// * If the about-to-be-moved entry is borrowed + /// * If the lock on the internal data structure cannot be aquired + /// * If the new path already exists + /// * If the about-to-be-moved entry does not exist + /// * If the FS-operation failed + /// * If the post-move-hooks error (though the operation has succeeded then). + /// + /// # Warnings + /// + /// This should be used with _great_ care, as moving an entry from `a` to `b` might result in + /// dangling links (see below). + /// + /// ## Moving linked entries + /// + /// If the entry which is moved is linked to another entry, these links get invalid (but we do + /// not detect this here). As links are always two-way-links, so `a` is not only linked to `b`, + /// but also the other way round, moving `b` to `c` results in the following scenario: + /// + /// * `a` links to `b`, which does not exist anymore. + /// * `c` links to `a`, which does exist. + /// + /// So the link is _partly dangling_, so to say. + /// pub fn move_by_id(&self, old_id: StoreId, new_id: StoreId) -> Result<()> { let new_id = new_id.with_base(self.path().clone()); let old_id = old_id.with_base(self.path().clone()); From 485d2802367338892ae4f774b7a39038ec0068b7 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 21:15:50 +0200 Subject: [PATCH 5/7] Bugfix: The StoreEntry should know the _new_ StoreId When moving a entry in the store, we also should tell the StoreEntry the new id. --- libimagstore/src/store.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 35e42d17..242fe10a 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -724,7 +724,10 @@ impl Store { // Should therefor never fail assert!(hsmap .remove(&old_id) - .and_then(|entry| hsmap.insert(new_id.clone(), entry)).is_none()) + .and_then(|mut entry| { + entry.id = new_id.clone(); + hsmap.insert(new_id.clone(), entry) + }).is_none()) } } From cf50ddae3380c1736b17f3e31c529d1e8d3361b8 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 18:46:50 +0200 Subject: [PATCH 6/7] [CHERRY-PICK] Add testing implementation for Drop for FileLockEntry --- libimagstore/src/store.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 242fe10a..5ce2bc6c 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -924,6 +924,7 @@ impl<'a> DerefMut for FileLockEntry<'a> { } } +#[cfg(not(test))] impl<'a> Drop for FileLockEntry<'a> { /// This will silently ignore errors, use `Store::update` if you want to catch the errors fn drop(&mut self) { @@ -931,6 +932,15 @@ impl<'a> Drop for FileLockEntry<'a> { } } +#[cfg(test)] +impl<'a> Drop for FileLockEntry<'a> { + /// This will not silently ignore errors but prints the result of the _update() call for testing + fn drop(&mut self) { + println!("Drop Result: {:?}", self.store._update(self)); + } +} + + /// `EntryContent` type pub type EntryContent = String; From 095ae194162dd58612bd772b3f842e0fe61249d1 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 7 Oct 2016 19:20:34 +0200 Subject: [PATCH 7/7] [CHERRY-PICK] Add flag for _update() whether precense should be modified This is a bugfix for an very particular issue. Here's what happens: If we create() an FileLockEntry and then update() it, we are running into a problem because update() calls _update() which changes the precense status of a FileLockEntry. Because update() is _consuming_, the FileLockEntry gets drop()ed afterwards. This _again_ causes _update() to be called, but with a new presence status, which is not true in this moment (as the FileLockEntry is still borrowed, but we already marked it as present). This patch is a short-term fix. The real problem is, that our Store interface is consuming. If the Store interface would be non-consuming, this issue wouldn't happen, as the drop() call would not happen. I'm rather sure that this patch will not be reverted in the process of rewriting the Store interface to be non-consuming. But we never know. --- libimagstore/src/store.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 5ce2bc6c..75ae0d37 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -507,7 +507,7 @@ impl Store { .map_err_into(SEK::UpdateCallError); } - if let Err(e) = self._update(&entry) { + if let Err(e) = self._update(&entry, false) { return Err(e).map_err_into(SEK::UpdateCallError); } @@ -522,7 +522,7 @@ impl Store { /// # Assumptions /// This method assumes that entry is dropped _right after_ the call, hence /// it is not public. - fn _update<'a>(&'a self, entry: &FileLockEntry<'a>) -> Result<()> { + fn _update<'a>(&'a self, entry: &FileLockEntry<'a>, modify_presence: bool) -> Result<()> { let mut hsmap = match self.entries.write() { Err(_) => return Err(SE::new(SEK::LockPoisoned, None)), Ok(e) => e, @@ -537,7 +537,9 @@ impl Store { debug!("Writing Entry"); try!(se.write_entry(&entry.entry)); - se.status = StoreEntryStatus::Present; + if modify_presence { + se.status = StoreEntryStatus::Present; + } Ok(()) } @@ -928,7 +930,7 @@ impl<'a> DerefMut for FileLockEntry<'a> { impl<'a> Drop for FileLockEntry<'a> { /// This will silently ignore errors, use `Store::update` if you want to catch the errors fn drop(&mut self) { - let _ = self.store._update(self); + let _ = self.store._update(self, true); } } @@ -936,7 +938,7 @@ impl<'a> Drop for FileLockEntry<'a> { impl<'a> Drop for FileLockEntry<'a> { /// This will not silently ignore errors but prints the result of the _update() call for testing fn drop(&mut self) { - println!("Drop Result: {:?}", self.store._update(self)); + println!("Drop Result: {:?}", self.store._update(self, true)); } }