Remove annotateable linking

This patch removes the annotateable linking feature from libimagentrylink.
It was not used anyways.

Also, this patch rewrites the complete linking code to use the
toml-query "typed" feature rather than constructing everything by
itself.

This removes a lot of unmaintainable boilerplate code.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>

lib/entry/libimagentrylink/Cargo.toml

@@ -27,14 +27,20 @@ url = "1.5"
 sha-1 = "0.7"
 hex = "0.3"
 is-match = "0.1"
-toml-query = "0.9"
 failure        = "0.1"
 failure_derive = "0.1"
+serde = "1"
+serde_derive = "1"
 
 libimagstore = { version = "0.10.0", path = "../../../lib/core/libimagstore" }
 libimagerror = { version = "0.10.0", path = "../../../lib/core/libimagerror" }
 libimagutil  = { version = "0.10.0", path = "../../../lib/etc/libimagutil" }
 
+[dependencies.toml-query]
+version = "0.9"
+default-features = false
+features = [ "typed" ]
+
 [dev-dependencies]
 env_logger = "0.5"
 

lib/entry/libimagentrylink/src/iter.rs

@@ -54,6 +54,8 @@ impl Iterator for LinkIter {
     }
 }
 
+
+
 pub trait IntoValues {
     fn into_values(self) -> Vec<Result<Value>>;
 }

lib/entry/libimagentrylink/src/lib.rs

@@ -44,6 +44,8 @@ extern crate toml_query;
 extern crate url;
 extern crate sha1;
 extern crate hex;
+extern crate serde;
+#[macro_use] extern crate serde_derive;
 #[macro_use] extern crate failure;
 #[macro_use] extern crate is_match;
 

lib/entry/libimagentrylink/src/link.rs

@@ -23,7 +23,6 @@ use libimagstore::store::Store;
 use libimagerror::errors::ErrorMsg as EM;
 
 use toml::Value;
-use toml::map::Map;
 use failure::ResultExt;
 use failure::Fallible as Result;
 use failure::Error;
@@ -31,7 +30,6 @@ use failure::Error;
 #[derive(Eq, PartialOrd, Ord, Hash, Debug, Clone)]
 pub enum Link {
     Id          { link: StoreId },
-    Annotated   { link: StoreId, annotation: String },
 }
 
 impl Link {
@@ -39,7 +37,6 @@ impl Link {
     pub fn exists(&self, store: &Store) -> Result<bool> {
         match *self {
             Link::Id { ref link }             => store.exists(link.clone()),
-            Link::Annotated { ref link, .. }  => store.exists(link.clone()),
         }
         .map_err(From::from)
     }
@@ -47,16 +44,14 @@ impl Link {
     pub fn to_str(&self) -> Result<String> {
         match *self {
             Link::Id { ref link }             => link.to_str(),
-            Link::Annotated { ref link, .. }  => link.to_str(),
         }
         .map_err(From::from)
     }
 
-
+    #[cfg(test)]
     pub(crate) fn eq_store_id(&self, id: &StoreId) -> bool {
         match self {
             &Link::Id { link: ref s }             => s.eq(id),
-            &Link::Annotated { link: ref s, .. }  => s.eq(id),
         }
     }
 
@@ -64,30 +59,16 @@ impl Link {
     pub fn get_store_id(&self) -> &StoreId {
         match self {
             &Link::Id { link: ref s }             => s,
-            &Link::Annotated { link: ref s, .. }  => s,
         }
     }
 
     pub(crate) fn to_value(&self) -> Result<Value> {
         match self {
-            &Link::Id { link: ref s } =>
+            Link::Id { ref link } => link
-                s.to_str()
+                .to_str()
                 .map(Value::String)
                 .context(EM::ConversionError)
                 .map_err(Error::from),
-            &Link::Annotated { ref link, annotation: ref anno } => {
-                link.to_str()
-                    .map(Value::String)
-                    .context(EM::ConversionError)
-                    .map_err(Error::from)
-                    .map(|link| {
-                        let mut tab = Map::new();
-
-                        tab.insert("link".to_owned(),       link);
-                        tab.insert("annotation".to_owned(), Value::String(anno.clone()));
-                        Value::Table(tab)
-                    })
-            }
         }
     }
 
@@ -97,10 +78,6 @@ impl ::std::cmp::PartialEq for Link {
     fn eq(&self, other: &Self) -> bool {
         match (self, other) {
             (&Link::Id { link: ref a }, &Link::Id { link: ref b }) => a.eq(&b),
-            (&Link::Annotated { link: ref a, annotation: ref ann1 },
-             &Link::Annotated { link: ref b, annotation: ref ann2 }) =>
-                (a, ann1).eq(&(b, ann2)),
-            _ => false,
         }
     }
 }
@@ -116,7 +93,6 @@ impl Into<StoreId> for Link {
     fn into(self) -> StoreId {
         match self {
             Link::Id { link }            => link,
-            Link::Annotated { link, .. } => link,
         }
     }
 }
@@ -125,7 +101,6 @@ impl IntoStoreId for Link {
     fn into_storeid(self) -> Result<StoreId> {
         match self {
             Link::Id { link }            => Ok(link),
-            Link::Annotated { link, .. } => Ok(link),
         }
     }
 }
@@ -134,7 +109,6 @@ impl AsRef<StoreId> for Link {
     fn as_ref(&self) -> &StoreId {
         match self {
             &Link::Id { ref link }            => &link,
-            &Link::Annotated { ref link, .. } => &link,
         }
     }
 }

lib/entry/libimagentrylink/src/linkable.rs

@@ -17,27 +17,25 @@
 // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
 //
 
+use std::path::PathBuf;
+
 use libimagstore::storeid::StoreId;
 use libimagstore::store::Entry;
 use libimagstore::store::Store;
-use libimagerror::errors::ErrorMsg as EM;
 
+use toml_query::read::Partial;
 use toml_query::read::TomlValueReadExt;
 use toml_query::insert::TomlValueInsertExt;
 use failure::ResultExt;
 use failure::Fallible as Result;
-use failure::Error;
 use failure::err_msg;
 
 use crate::iter::LinkIter;
-use crate::iter::IntoValues;
 use crate::link::Link;
 
-use toml::Value;
-
 pub trait Linkable {
 
-    /// Get the internal links from the implementor object
+    /// Get all links
     fn links(&self) -> Result<LinkIter>;
 
     /// Add an internal link to the implementor object
@@ -49,8 +47,24 @@ pub trait Linkable {
     /// Remove _all_ internal links
     fn unlink(&mut self, store: &Store) -> Result<()>;
 
-    /// Add internal annotated link
+}
-    fn add_annotated_link(&mut self, link: &mut Entry, annotation: String) -> Result<()>;
+
+#[derive(Serialize, Deserialize, Debug)]
+struct LinkPartial {
+    internal: Option<Vec<String>>,
+}
+
+impl Default for LinkPartial {
+    fn default() -> Self {
+        LinkPartial {
+            internal: None,
+        }
+    }
+}
+
+impl<'a> Partial<'a> for LinkPartial {
+    const LOCATION: &'static str = "links";
+    type Output = Self;
 }
 
 impl Linkable for Entry {
@@ -58,43 +72,58 @@ impl Linkable for Entry {
     fn links(&self) -> Result<LinkIter> {
         debug!("Getting internal links");
         trace!("Getting internal links from header of '{}' = {:?}", self.get_location(), self.get_header());
-        let res = self
+
+        let partial : LinkPartial = self
             .get_header()
-            .read("links.internal")
+            .read_partial::<LinkPartial>()?
-            .context(format_err!("Failed to read header 'links.internal' of '{}'", self.get_location()))
+            .unwrap_or_else(|| LinkPartial::default());
-            .context(EM::EntryHeaderReadError)
+
-            .context(EM::EntryHeaderError)
+        partial
-            .map_err(Error::from)
+            .internal
-            .map(|r| r.cloned());
+            .unwrap_or_else(|| vec![])
-        process_rw_result(res)
+            .into_iter()
+            .map(PathBuf::from)
+            .map(StoreId::new)
+            .map(|r| r.map(Link::from))
+            .collect::<Result<Vec<Link>>>()
+            .map(LinkIter::new)
     }
 
-    fn add_link(&mut self, link: &mut Entry) -> Result<()> {
+    fn add_link(&mut self, other: &mut Entry) -> Result<()> {
-        debug!("Adding internal link: {:?}", link);
+        debug!("Adding internal link: {:?}", other);
-        let location = link.get_location().clone().into();
+        let left_location  = self.get_location().to_str()?;
-        add_link_with_instance(self, link, location)
+        let right_location = other.get_location().to_str()?;
+
+        alter_linking(self, other, |mut left, mut right| {
+            let mut left_internal = left.internal.unwrap_or_else(|| vec![]);
+            left_internal.push(right_location);
+
+            let mut right_internal = right.internal.unwrap_or_else(|| vec![]);
+            right_internal.push(left_location);
+
+            left.internal = Some(left_internal);
+            right.internal = Some(right_internal);
+
+            Ok((left, right))
+        })
     }
 
-    fn remove_link(&mut self, link: &mut Entry) -> Result<()> {
+    fn remove_link(&mut self, other: &mut Entry) -> Result<()> {
-        debug!("Removing internal link: {:?}", link);
+        debug!("Remove internal link: {:?}", other);
+        let left_location  = self.get_location().to_str()?;
+        let right_location = other.get_location().to_str()?;
 
-        // Cloning because of borrowing
+        alter_linking(self, other, |mut left, mut right| {
-        let own_loc   = self.get_location().clone();
+            let mut left_internal = left.internal.unwrap_or_else(|| vec![]);
-        let other_loc = link.get_location().clone();
+            left_internal.retain(|l| *l != right_location);
 
-        debug!("Removing internal link from {:?} to {:?}", own_loc, other_loc);
+            let mut right_internal = right.internal.unwrap_or_else(|| vec![]);
+            right_internal.retain(|l| *l != left_location);
 
-        let links = link.links()?;
+            left.internal = Some(left_internal);
-        debug!("Rewriting own links for {:?}, without {:?}", other_loc, own_loc);
+            right.internal = Some(right_internal);
 
-        let links = links.filter(|l| !l.eq_store_id(&own_loc));
+            Ok((left, right))
-        let _     = rewrite_links(link.get_header_mut(), links)?;
-
-        self.links()
-            .and_then(|links| {
-                debug!("Rewriting own links for {:?}, without {:?}", own_loc, other_loc);
-                let links = links.filter(|l| !l.eq_store_id(&other_loc));
-                rewrite_links(self.get_header_mut(), links)
         })
     }
 
@@ -109,147 +138,33 @@ impl Linkable for Entry {
         Ok(())
     }
 
-    fn add_annotated_link(&mut self, link: &mut Entry, annotation: String) -> Result<()> {
+}
-        let new_link = Link::Annotated {
+
-            link: link.get_location().clone(),
+fn alter_linking<F>(left: &mut Entry, right: &mut Entry, f: F) -> Result<()>
-            annotation: annotation,
+    where F: FnOnce(LinkPartial, LinkPartial) -> Result<(LinkPartial, LinkPartial)>
+{
+    debug!("Altering linkage of {:?} and {:?}", left, right);
+
+    let get_partial = |entry: &mut Entry| -> Result<LinkPartial> {
+        Ok(entry.get_header().read_partial::<LinkPartial>()?.unwrap_or_else(|| LinkPartial::default()))
     };
 
-        add_link_with_instance(self, link, new_link)
+    let left_partial : LinkPartial = get_partial(left)?;
-    }
+    let right_partial : LinkPartial = get_partial(right)?;
 
-}
+    trace!("Partial left before: {:?}", left_partial);
+    trace!("Partial right before: {:?}", right_partial);
 
-fn add_link_with_instance(this: &mut Entry, link: &mut Entry, instance: Link) -> Result<()> {
+    let (left_partial, right_partial) = f(left_partial, right_partial)?;
-    debug!("Adding internal link from {:?} to {:?}", this.get_location(), instance);
 
-    add_foreign_link(link, this.get_location().clone())
+    trace!("Partial left after: {:?}", left_partial);
-        .and_then(|_| {
+    trace!("Partial right after: {:?}", right_partial);
-            this.links()
-                .and_then(|links| {
-                    let links = links.chain(LinkIter::new(vec![instance]));
-                    rewrite_links(this.get_header_mut(), links)
-                })
-        })
-}
 
-fn rewrite_links<I: Iterator<Item = Link>>(header: &mut Value, links: I) -> Result<()> {
+    left.get_header_mut().insert_serialized("links", left_partial)?;
-    let links = links.into_values()
+    right.get_header_mut().insert_serialized("links", right_partial)?;
-                     .into_iter()
-                     .fold(Ok(vec![]), |acc: Result<Vec<_>>, elem| {
-                        acc.and_then(move |mut v| {
-                            v.push(elem.context(EM::ConversionError)?);
-                            Ok(v)
-                        })
-                     })?;
 
-    debug!("Setting new link array: {:?}", links);
+    debug!("Finished altering linkage!");
-    let process = header
+    Ok(())
-        .insert("links.internal", Value::Array(links))
-        .context(format_err!("Failed to insert header 'links.internal'"))
-        .context(EM::EntryHeaderReadError)
-        .map_err(Error::from);
-    process_rw_result(process).map(|_| ())
-}
-
-/// When Linking A -> B, the specification wants us to link back B -> A.
-/// This is a helper function which does this.
-fn add_foreign_link(target: &mut Entry, from: StoreId) -> Result<()> {
-    debug!("Linking back from {:?} to {:?}", target.get_location(), from);
-    target.links()
-        .and_then(|links| {
-            let links = links
-                             .chain(LinkIter::new(vec![from.into()]))
-                             .into_values()
-                             .into_iter()
-                             .fold(Ok(vec![]), |acc: Result<Vec<_>>, elem| {
-                                acc.and_then(move |mut v| {
-                                    v.push(elem.context(EM::ConversionError)?);
-                                    Ok(v)
-                                })
-                             })?;
-            debug!("Setting links in {:?}: {:?}", target.get_location(), links);
-
-            let res = target
-                .get_header_mut()
-                .insert("links.internal", Value::Array(links))
-                .context(format_err!("Failed to insert header 'links.internal'"))
-                .context(EM::EntryHeaderReadError)
-                .map_err(Error::from);
-
-            process_rw_result(res).map(|_| ())
-        })
-}
-
-fn process_rw_result(links: Result<Option<Value>>) -> Result<LinkIter> {
-    use std::path::PathBuf;
-
-    let links = match links {
-        Err(e) => {
-            debug!("RW action on store failed. Generating LinkError");
-            return Err(e).context(EM::EntryHeaderReadError).map_err(Error::from)
-        },
-        Ok(None) => {
-            debug!("We got no value from the header!");
-            return Ok(LinkIter::new(vec![]))
-        },
-        Ok(Some(Value::Array(l))) => l,
-        Ok(Some(_)) => {
-            debug!("We expected an Array for the links, but there was a non-Array!");
-            return Err(err_msg("Link type error"));
-        }
-    };
-
-    if !links.iter().all(|l| is_match!(*l, Value::String(_)) || is_match!(*l, Value::Table(_))) {
-        debug!("At least one of the Values which were expected in the Array of links is not a String or a Table!");
-        debug!("Generating LinkError");
-        return Err(err_msg("Existing Link type error"));
-    }
-
-    let links : Vec<Link> = links.into_iter()
-        .map(|link| {
-            debug!("Matching the link: {:?}", link);
-            match link {
-                Value::String(s) => StoreId::new(PathBuf::from(s))
-                    .map(|s| Link::Id { link: s })
-                    .map_err(From::from)
-                    ,
-                Value::Table(mut tab) => {
-                    debug!("Destructuring table");
-                    if !tab.contains_key("link")
-                    || !tab.contains_key("annotation") {
-                        debug!("Things missing... returning Error instance");
-                        Err(err_msg("Link parser error"))
-                    } else {
-                        let link = tab.remove("link")
-                            .ok_or(err_msg("Link parser: field missing"))?;
-
-                        let anno = tab.remove("annotation")
-                            .ok_or(err_msg("Link parser: Field missing"))?;
-
-                        debug!("Ok, here we go with building a Link::Annotated");
-                        match (link, anno) {
-                            (Value::String(link), Value::String(anno)) => {
-                                StoreId::new(PathBuf::from(link))
-                                    .map_err(From::from)
-                                    .map(|link| {
-                                        Link::Annotated {
-                                            link: link,
-                                            annotation: anno,
-                                        }
-                                    })
-                            },
-                            _ => Err(err_msg("Link parser: Field type error")),
-                        }
-                    }
-                }
-                _ => unreachable!(),
-            }
-        })
-        .collect::<Result<Vec<Link>>>()?;
-
-    debug!("Ok, the RW action was successful, returning link vector now!");
-    Ok(LinkIter::new(links))
 }
 
 #[cfg(test)]
@@ -259,7 +174,6 @@ mod test {
     use libimagstore::store::Store;
 
     use super::Linkable;
-    use super::Link;
 
     fn setup_logging() {
         let _ = ::env_logger::try_init();
@@ -291,7 +205,9 @@ mod test {
         assert!(e2.links().is_ok());
 
         {
-            assert!(e1.add_link(&mut e2).is_ok());
+            let res = e1.add_link(&mut e2);
+            debug!("Result = {:?}", res);
+            assert!(res.is_ok());
 
             let e1_links = e1.links().unwrap().collect::<Vec<_>>();
             let e2_links = e2.links().unwrap().collect::<Vec<_>>();
@@ -464,33 +380,4 @@ mod test {
         assert_eq!(e3.links().unwrap().collect::<Vec<_>>().len(), 0);
     }
 
-    #[test]
-    fn test_link_annotating() {
-        setup_logging();
-        let store      = get_store();
-        let mut entry1 = store.create(PathBuf::from("test_link_annotating-1")).unwrap();
-        let mut entry2 = store.create(PathBuf::from("test_link_annotating-2")).unwrap();
-
-        let res = entry1.add_annotated_link(&mut entry2, String::from("annotation"));
-        assert!(res.is_ok());
-
-        {
-            for link in entry1.links().unwrap() {
-                match link  {
-                    Link::Annotated {annotation, ..} => assert_eq!(annotation, "annotation"),
-                    _ => assert!(false, "Non-annotated link found"),
-                }
-            }
-        }
-
-        {
-            for link in entry2.links().unwrap() {
-                match link  {
-                    Link::Id {..}        => {},
-                    Link::Annotated {..} => assert!(false, "Annotated link found"),
-                }
-            }
-        }
-    }
-
 }