diff --git a/libimagstore/src/file_abstraction.rs b/libimagstore/src/file_abstraction.rs index c4d7d51e..0d402538 100644 --- a/libimagstore/src/file_abstraction.rs +++ b/libimagstore/src/file_abstraction.rs @@ -17,112 +17,197 @@ // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA // -pub use self::fs::FileAbstraction; +//! The filesystem abstraction code +//! +//! # Problem +//! +//! First, we had a compiletime backend for the store. This means that the actual filesystem +//! operations were compiled into the store either as real filesystem operations (in a normal debug +//! or release build) but as a in-memory variant in the 'test' case. +//! So tests did not hit the filesystem when running. +//! This gave us us the possibility to run tests concurrently with multiple +//! stores that did not interfere with eachother. +//! +//! This approach worked perfectly well until we started to test not the +//! store itself but crates that depend on the store implementation. +//! When running tests in a crate that depends on the store, the store +//! itself was compiled with the filesystem-hitting-backend. +//! This was problematic, as tests could not be implemented without hitting +//! the filesystem. +//! +//! Hence we implemented this. +//! +//! # Implementation +//! +//! The filesystem is abstracted via a trait `FileAbstraction` which +//! contains the essential functions for working with the filesystem. +//! +//! Two implementations are provided in the code: +//! +//! * FSFileAbstraction +//! * InMemoryFileAbstraction +//! +//! whereas the first actually works with the filesystem and the latter +//! works with an in-memory HashMap that is used as filesystem. +//! +//! Further, the trait `FileAbstractionInstance` was introduced for +//! functions which are executed on actual instances of content from the +//! filesystem, which was previousely tied into the general abstraction +//! mechanism. +//! +//! So, the `FileAbstraction` trait is for working with the filesystem, the +//! `FileAbstractionInstance` trait is for working with instances of content +//! from the filesystem (speak: actual Files). +//! +//! In case of the `FSFileAbstractionInstance`, which is the implementation +//! of the `FileAbstractionInstance` for the actual filesystem-hitting code, +//! the underlying resource is managed like with the old code before. +//! The `InMemoryFileAbstractionInstance` implementation is corrosponding to +//! the `InMemoryFileAbstraction` implementation - for the in-memory +//! "filesystem". +//! +//! The implementation of the `get_file_content()` function had to be +//! changed to return a `String` rather than a `&mut Read` because of +//! lifetime issues. +//! This change is store-internally and the API of the store itself was not +//! affected. +//! + +use std::path::PathBuf; +use std::fmt::Debug; + +use error::StoreError as SE; + +pub use self::fs::FSFileAbstraction; +pub use self::fs::FSFileAbstractionInstance; +pub use self::inmemory::InMemoryFileAbstraction; +pub use self::inmemory::InMemoryFileAbstractionInstance; // TODO: // This whole thing can be written better with a trait based mechanism that is embedded into the // store. However it would mean rewriting most things to be generic which can be a pain in the ass. -#[cfg(test)] -mod fs { - use error::StoreError as SE; - use error::StoreErrorKind as SEK; - use std::io::Cursor; - use std::path::PathBuf; - use std::collections::HashMap; - use std::sync::Mutex; +/// An abstraction trait over filesystem actions +pub trait FileAbstraction : Debug { + fn remove_file(&self, path: &PathBuf) -> Result<(), SE>; + fn copy(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE>; + fn rename(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE>; + fn create_dir_all(&self, _: &PathBuf) -> Result<(), SE>; - use libimagerror::into::IntoError; - - use error::MapErrInto; - - lazy_static! { - static ref MAP: Mutex>>> = { - Mutex::new(HashMap::new()) - }; - } - - /// `FileAbstraction` type, this is the Test version! - /// - /// A lazy file is either absent, but a path to it is available, or it is present. - #[derive(Debug)] - pub enum FileAbstraction { - Absent(PathBuf), - } - - impl FileAbstraction { - - /** - * Get the mutable file behind a FileAbstraction object - */ - pub fn get_file_content(&mut self) -> Result>, SE> { - debug!("Getting lazy file: {:?}", self); - match *self { - FileAbstraction::Absent(ref f) => { - let map = try!(MAP.lock().map_err_into(SEK::LockPoisoned)); - return map.get(f).cloned().ok_or(SEK::FileNotFound.into_error()); - }, - }; - } - - pub fn write_file_content(&mut self, buf: &[u8]) -> Result<(), SE> { - match *self { - FileAbstraction::Absent(ref f) => { - let mut map = try!(MAP.lock().map_err_into(SEK::LockPoisoned)); - if let Some(ref mut cur) = map.get_mut(f) { - let mut vec = cur.get_mut(); - vec.clear(); - vec.extend_from_slice(buf); - return Ok(()); - } - let vec = Vec::from(buf); - map.insert(f.clone(), Cursor::new(vec)); - return Ok(()); - }, - }; - } - - pub fn remove_file(path: &PathBuf) -> Result<(), SE> { - try!(MAP.lock().map_err_into(SEK::LockPoisoned)) - .remove(path) - .map(|_| ()) - .ok_or(SEK::FileNotFound.into_error()) - } - - pub fn copy(from: &PathBuf, to: &PathBuf) -> Result<(), SE> { - let mut map = try!(MAP.lock().map_err_into(SEK::LockPoisoned)); - let a = try!(map.get(from).cloned().ok_or(SEK::FileNotFound.into_error())); - map.insert(to.clone(), a); - Ok(()) - } - - pub fn rename(from: &PathBuf, to: &PathBuf) -> Result<(), SE> { - let mut map = try!(MAP.lock().map_err_into(SEK::LockPoisoned)); - let a = try!(map.get(from).cloned().ok_or(SEK::FileNotFound.into_error())); - map.insert(to.clone(), a); - Ok(()) - } - - pub fn create_dir_all(_: &PathBuf) -> Result<(), SE> { - Ok(()) - } - } + fn new_instance(&self, p: PathBuf) -> Box; +} + +/// An abstraction trait over actions on files +pub trait FileAbstractionInstance : Debug { + fn get_file_content(&mut self) -> Result; + fn write_file_content(&mut self, buf: &[u8]) -> Result<(), SE>; } -#[cfg(not(test))] mod fs { - use error::{MapErrInto, StoreError as SE, StoreErrorKind as SEK}; + use std::fs::{File, OpenOptions, create_dir_all, remove_file, copy, rename}; use std::io::{Seek, SeekFrom, Read}; use std::path::{Path, PathBuf}; - use std::fs::{File, OpenOptions, create_dir_all, remove_file, copy, rename}; - /// `FileAbstraction` type + use error::{MapErrInto, StoreError as SE, StoreErrorKind as SEK}; + + use super::FileAbstraction; + use super::FileAbstractionInstance; + + #[derive(Debug)] + pub enum FSFileAbstractionInstance { + Absent(PathBuf), + File(File, PathBuf) + } + + impl FileAbstractionInstance for FSFileAbstractionInstance { + + /** + * Get the content behind this file + */ + fn get_file_content(&mut self) -> Result { + debug!("Getting lazy file: {:?}", self); + let (file, path) = match *self { + FSFileAbstractionInstance::File(ref mut f, _) => return { + // We seek to the beginning of the file since we expect each + // access to the file to be in a different context + try!(f.seek(SeekFrom::Start(0)) + .map_err_into(SEK::FileNotSeeked)); + + let mut s = String::new(); + f.read_to_string(&mut s); + Ok(s) + }, + FSFileAbstractionInstance::Absent(ref p) => + (try!(open_file(p).map_err_into(SEK::FileNotFound)), p.clone()), + }; + *self = FSFileAbstractionInstance::File(file, path); + if let FSFileAbstractionInstance::File(ref mut f, _) = *self { + let mut s = String::new(); + f.read_to_string(&mut s); + return Ok(s); + } + unreachable!() + } + + /** + * Write the content of this file + */ + fn write_file_content(&mut self, buf: &[u8]) -> Result<(), SE> { + use std::io::Write; + let (file, path) = match *self { + FSFileAbstractionInstance::File(ref mut f, _) => return { + // We seek to the beginning of the file since we expect each + // access to the file to be in a different context + try!(f.seek(SeekFrom::Start(0)) + .map_err_into(SEK::FileNotCreated)); + f.write_all(buf).map_err_into(SEK::FileNotWritten) + }, + FSFileAbstractionInstance::Absent(ref p) => + (try!(create_file(p).map_err_into(SEK::FileNotCreated)), p.clone()), + }; + *self = FSFileAbstractionInstance::File(file, path); + if let FSFileAbstractionInstance::File(ref mut f, _) = *self { + return f.write_all(buf).map_err_into(SEK::FileNotWritten); + } + unreachable!(); + } + + } + + /// `FSFileAbstraction` state type /// /// A lazy file is either absent, but a path to it is available, or it is present. #[derive(Debug)] - pub enum FileAbstraction { - Absent(PathBuf), - File(File, PathBuf) + pub struct FSFileAbstraction { + } + + impl FSFileAbstraction { + pub fn new() -> FSFileAbstraction { + FSFileAbstraction { } + } + } + + impl FileAbstraction for FSFileAbstraction { + + fn remove_file(&self, path: &PathBuf) -> Result<(), SE> { + remove_file(path).map_err_into(SEK::FileNotRemoved) + } + + fn copy(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE> { + copy(from, to).map_err_into(SEK::FileNotCopied).map(|_| ()) + } + + fn rename(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE> { + rename(from, to).map_err_into(SEK::FileNotRenamed) + } + + fn create_dir_all(&self, path: &PathBuf) -> Result<(), SE> { + create_dir_all(path).map_err_into(SEK::DirNotCreated) + } + + fn new_instance(&self, p: PathBuf) -> Box { + Box::new(FSFileAbstractionInstance::Absent(p)) + } } fn open_file>(p: A) -> ::std::io::Result { @@ -139,87 +224,173 @@ mod fs { OpenOptions::new().write(true).read(true).create(true).open(p) } - impl FileAbstraction { +} + +mod inmemory { + use error::StoreError as SE; + use error::StoreErrorKind as SEK; + use std::io::Read; + use std::io::Cursor; + use std::path::PathBuf; + use std::collections::HashMap; + use std::sync::Mutex; + use std::cell::RefCell; + use std::sync::Arc; + + use libimagerror::into::IntoError; + + use super::FileAbstraction; + use super::FileAbstractionInstance; + + type Backend = Arc>>>>>; + + /// `FileAbstraction` type, this is the Test version! + /// + /// A lazy file is either absent, but a path to it is available, or it is present. + #[derive(Debug)] + pub struct InMemoryFileAbstractionInstance { + fs_abstraction: Backend, + absent_path: PathBuf, + } + + impl InMemoryFileAbstractionInstance { + + pub fn new(fs: Backend, pb: PathBuf) -> InMemoryFileAbstractionInstance { + InMemoryFileAbstractionInstance { + fs_abstraction: fs, + absent_path: pb + } + } + + } + + impl FileAbstractionInstance for InMemoryFileAbstractionInstance { /** - * Get the content behind this file + * Get the mutable file behind a InMemoryFileAbstraction object */ - pub fn get_file_content(&mut self) -> Result<&mut Read, SE> { + fn get_file_content(&mut self) -> Result { debug!("Getting lazy file: {:?}", self); - let (file, path) = match *self { - FileAbstraction::File(ref mut f, _) => return { - // We seek to the beginning of the file since we expect each - // access to the file to be in a different context - try!(f.seek(SeekFrom::Start(0)) - .map_err_into(SEK::FileNotSeeked)); - Ok(f) - }, - FileAbstraction::Absent(ref p) => (try!(open_file(p).map_err_into(SEK::FileNotFound)), - p.clone()), - }; - *self = FileAbstraction::File(file, path); - if let FileAbstraction::File(ref mut f, _) = *self { - return Ok(f); + + let p = self.absent_path.clone(); + match self.fs_abstraction.lock() { + Ok(mut mtx) => { + mtx.get_mut() + .get_mut(&p) + .ok_or(SEK::FileNotFound.into_error()) + .map(|t| { + let mut s = String::new(); + t.read_to_string(&mut s); + s + }) + } + + Err(_) => Err(SEK::LockError.into_error()) } - unreachable!() } - /** - * Write the content of this file - */ - pub fn write_file_content(&mut self, buf: &[u8]) -> Result<(), SE> { - use std::io::Write; - let (file, path) = match *self { - FileAbstraction::File(ref mut f, _) => return { - // We seek to the beginning of the file since we expect each - // access to the file to be in a different context - try!(f.seek(SeekFrom::Start(0)) - .map_err_into(SEK::FileNotCreated)); - f.write_all(buf).map_err_into(SEK::FileNotWritten) + fn write_file_content(&mut self, buf: &[u8]) -> Result<(), SE> { + match *self { + InMemoryFileAbstractionInstance { ref absent_path, .. } => { + let mut mtx = self.fs_abstraction.lock().expect("Locking Mutex failed"); + let mut backend = mtx.get_mut(); + + if let Some(ref mut cur) = backend.get_mut(absent_path) { + let mut vec = cur.get_mut(); + vec.clear(); + vec.extend_from_slice(buf); + return Ok(()); + } + let vec = Vec::from(buf); + backend.insert(absent_path.clone(), Cursor::new(vec)); + return Ok(()); }, - FileAbstraction::Absent(ref p) => (try!(create_file(p).map_err_into(SEK::FileNotCreated)), - p.clone()), }; - *self = FileAbstraction::File(file, path); - if let FileAbstraction::File(ref mut f, _) = *self { - return f.write_all(buf).map_err_into(SEK::FileNotWritten); - } - unreachable!(); - } - - pub fn remove_file(path: &PathBuf) -> Result<(), SE> { - remove_file(path).map_err_into(SEK::FileNotRemoved) - } - - pub fn copy(from: &PathBuf, to: &PathBuf) -> Result<(), SE> { - copy(from, to).map_err_into(SEK::FileNotCopied).map(|_| ()) - } - - pub fn rename(from: &PathBuf, to: &PathBuf) -> Result<(), SE> { - rename(from, to).map_err_into(SEK::FileNotRenamed) - } - - pub fn create_dir_all(path: &PathBuf) -> Result<(), SE> { - create_dir_all(path).map_err_into(SEK::DirNotCreated) } } + + #[derive(Debug)] + pub struct InMemoryFileAbstraction { + virtual_filesystem: Backend, + } + + impl InMemoryFileAbstraction { + + pub fn new() -> InMemoryFileAbstraction { + InMemoryFileAbstraction { + virtual_filesystem: Arc::new(Mutex::new(RefCell::new(HashMap::new()))), + } + } + + pub fn backend(&self) -> &Backend { + &self.virtual_filesystem + } + + } + + impl FileAbstraction for InMemoryFileAbstraction { + + fn remove_file(&self, path: &PathBuf) -> Result<(), SE> { + debug!("Removing: {:?}", path); + self.backend() + .lock() + .expect("Locking Mutex failed") + .get_mut() + .remove(path) + .map(|_| ()) + .ok_or(SEK::FileNotFound.into_error()) + } + + fn copy(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE> { + debug!("Copying : {:?} -> {:?}", from, to); + let mut mtx = self.backend().lock().expect("Locking Mutex failed"); + let mut backend = mtx.get_mut(); + + let a = try!(backend.get(from).cloned().ok_or(SEK::FileNotFound.into_error())); + backend.insert(to.clone(), a); + debug!("Copying: {:?} -> {:?} worked", from, to); + Ok(()) + } + + fn rename(&self, from: &PathBuf, to: &PathBuf) -> Result<(), SE> { + debug!("Renaming: {:?} -> {:?}", from, to); + let mut mtx = self.backend().lock().expect("Locking Mutex failed"); + let mut backend = mtx.get_mut(); + + let a = try!(backend.get(from).cloned().ok_or(SEK::FileNotFound.into_error())); + backend.insert(to.clone(), a); + debug!("Renaming: {:?} -> {:?} worked", from, to); + Ok(()) + } + + fn create_dir_all(&self, _: &PathBuf) -> Result<(), SE> { + Ok(()) + } + + fn new_instance(&self, p: PathBuf) -> Box { + Box::new(InMemoryFileAbstractionInstance::new(self.backend().clone(), p)) + } + } + } #[cfg(test)] mod test { - use super::FileAbstraction; - use std::io::Read; + use super::FileAbstractionInstance; + use super::inmemory::InMemoryFileAbstraction; + use super::inmemory::InMemoryFileAbstractionInstance; use std::path::PathBuf; #[test] fn lazy_file() { + let fs = InMemoryFileAbstraction::new(); + let mut path = PathBuf::from("/tests"); path.set_file_name("test1"); - let mut lf = FileAbstraction::Absent(path); + let mut lf = InMemoryFileAbstractionInstance::new(fs.backend().clone(), path); lf.write_file_content(b"Hello World").unwrap(); - let mut bah = Vec::new(); - lf.get_file_content().unwrap().read_to_end(&mut bah).unwrap(); - assert_eq!(bah, b"Hello World"); + let bah = lf.get_file_content().unwrap(); + assert_eq!(bah, "Hello World"); } } diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 5b3ef6d2..75193789 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -42,6 +42,8 @@ use error::{StoreError as SE, StoreErrorKind as SEK}; use error::MapErrInto; use storeid::{IntoStoreId, StoreId, StoreIdIterator}; use file_abstraction::FileAbstraction; +use file_abstraction::FileAbstractionInstance; +use file_abstraction::FSFileAbstraction; use toml_ext::*; use libimagerror::into::IntoError; @@ -65,7 +67,7 @@ enum StoreEntryStatus { #[derive(Debug)] struct StoreEntry { id: StoreId, - file: FileAbstraction, + file: Box, status: StoreEntryStatus, } @@ -132,7 +134,7 @@ impl Iterator for Walk { impl StoreEntry { - fn new(id: StoreId) -> Result { + fn new(id: StoreId, backend: &Box) -> Result { let pb = try!(id.clone().into_pathbuf()); #[cfg(feature = "fs-lock")] @@ -144,7 +146,7 @@ impl StoreEntry { Ok(StoreEntry { id: id, - file: FileAbstraction::Absent(pb), + file: backend.new_instance(pb), status: StoreEntryStatus::Present, }) } @@ -160,7 +162,7 @@ impl StoreEntry { if !self.is_borrowed() { self.file .get_file_content() - .and_then(|mut file| Entry::from_reader(id.clone(), &mut file)) + .and_then(|content| Entry::from_str(id.clone(), &content)) .or_else(|err| if err.err_type() == SEK::FileNotFound { Ok(Entry::new(id.clone())) } else { @@ -213,6 +215,11 @@ pub struct Store { /// Could be optimized for a threadsafe HashMap /// entries: Arc>>, + + /// The backend to use + /// + /// This provides the filesystem-operation functions (or pretends to) + backend: Box, } impl Store { @@ -240,6 +247,17 @@ impl Store { /// - StorePathCreate(_) if creating the store directory failed /// - StorePathExists() if location exists but is a file pub fn new(location: PathBuf, store_config: Option) -> Result { + let backend = Box::new(FSFileAbstraction::new()); + Store::new_with_backend(location, store_config, backend) + } + + /// Create a Store object as descripbed in `Store::new()` documentation, but with an alternative + /// backend implementation. + /// + /// Do not use directly, only for testing purposes. + pub fn new_with_backend(location: PathBuf, + store_config: Option, + backend: Box) -> Result { use configuration::*; debug!("Validating Store configuration"); @@ -256,7 +274,7 @@ impl Store { .map_err_into(SEK::IoError); } - try!(FileAbstraction::create_dir_all(&location) + try!(backend.create_dir_all(&location) .map_err_into(SEK::StorePathCreate) .map_dbg_err_str("Failed")); } else if location.is_file() { @@ -268,6 +286,7 @@ impl Store { location: location.clone(), configuration: store_config, entries: Arc::new(RwLock::new(HashMap::new())), + backend: backend, }; debug!("Store building succeeded"); @@ -367,7 +386,7 @@ impl Store { } hsmap.insert(id.clone(), { debug!("Creating: '{}'", id); - let mut se = try!(StoreEntry::new(id.clone())); + let mut se = try!(StoreEntry::new(id.clone(), &self.backend)); se.status = StoreEntryStatus::Borrowed; se }); @@ -400,7 +419,7 @@ impl Store { .write() .map_err(|_| SE::new(SEK::LockPoisoned, None)) .and_then(|mut es| { - let new_se = try!(StoreEntry::new(id.clone())); + let new_se = try!(StoreEntry::new(id.clone(), &self.backend)); let mut se = es.entry(id.clone()).or_insert(new_se); let entry = se.get_entry(); se.status = StoreEntryStatus::Borrowed; @@ -557,7 +576,7 @@ impl Store { return Err(SE::new(SEK::IdLocked, None)).map_err_into(SEK::RetrieveCopyCallError); } - try!(StoreEntry::new(id)).get_entry() + try!(StoreEntry::new(id, &self.backend)).get_entry() } /// Delete an entry @@ -596,7 +615,7 @@ impl Store { // remove the entry first, then the file entries.remove(&id); let pb = try!(id.clone().with_base(self.path().clone()).into_pathbuf()); - if let Err(e) = FileAbstraction::remove_file(&pb) { + if let Err(e) = self.backend.remove_file(&pb) { return Err(SEK::FileError.into_error_with_cause(Box::new(e))) .map_err_into(SEK::DeleteCallError); } @@ -638,11 +657,11 @@ impl Store { let old_id_as_path = try!(old_id.clone().with_base(self.path().clone()).into_pathbuf()); let new_id_as_path = try!(new_id.clone().with_base(self.path().clone()).into_pathbuf()); - FileAbstraction::copy(&old_id_as_path, &new_id_as_path) + self.backend.copy(&old_id_as_path, &new_id_as_path) .and_then(|_| { if remove_old { debug!("Removing old '{:?}'", old_id_as_path); - FileAbstraction::remove_file(&old_id_as_path) + self.backend.remove_file(&old_id_as_path) } else { Ok(()) } @@ -710,7 +729,7 @@ impl Store { 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()); - match FileAbstraction::rename(&old_id_pb, &new_id_pb) { + match self.backend.rename(&old_id_pb, &new_id_pb) { Err(e) => return Err(SEK::EntryRenameError.into_error_with_cause(Box::new(e))), Ok(_) => { debug!("Rename worked on filesystem");