Replant the tree! 🌲

This started out as a fix for corruption corner cases, grew into
simplifying how we build the remote tree, and turned into a rewrite
(again >.>). Instead of managing two different sets of structure at
insert and merge time, we now store items and parent-child
relationships in a tree builder, then build a consistent tree that
flags divergent items.

The old tree was complicated because it had to maintain a valid
structure before and after every insert, but with references to
potentially unknown parents and children. Since we couldn't insert
an item without knowing its parent, we had to pick a canonical
structure (parents by `children`), insert children in level
order, and note divergent structure (by `parentid`) separately.

This meant Desktop's `Store::fetch_remote_tree()` had to left-join
`mirror.items` to `mirror.structure`, store the items in a
pseudo-tree, then recursively walk children to inflate the complete
tree. This was complicated enough with a valid tree, let alone
orphans and multiple parents.

With the new approach, we build the tree in three steps:

1. First, we add all items, without structure.
2. Next, we set parent-child relationships. Parents by `children`
   require the parent to exist, but not the child; this handles the
   case where a folder mentions nonexistent or deleted GUIDs in
   its `children`. Parents by `parentid` require the item to exist,
   but not its parent; this handles orphans that reference missing
   or non-folder parents.
3. Finally, once we've added all entries to the tree, we have a
   complete view of the structure, so we can resolve missing, multiple,
   and conflicting parents.

For cases where we know the structure is valid and in level order,
like Desktop's `Store::fetch_local_tree()`, we handle steps 1 and 2
at the same time: `builder.item(item)?.by_structure(&parent_guid)?`.

For the remote tree, we insert all items and their `parentid`s
first (`builder.item(item)?.by_parent_guid(&parent_guid)?`, where
`parent_guid` might not be in the tree), then add structure from
children later:
`builder.parent_for(&child_guid)?.by_children(&parent_guid)?`,
where `child_guid` might not be in the tree.

Closes #22.

src/error.rs

@@ -65,6 +65,9 @@ impl fmt::Display for Error {
             ErrorKind::DuplicateItem(guid) => {
                 write!(f, "Item {} already exists in tree", guid)
             },
+            ErrorKind::MissingItem(guid) => {
+                write!(f, "Item {} doesn't exist in tree", guid)
+            },
             ErrorKind::InvalidParent(child_guid, parent_guid) => {
                 write!(f, "Can't insert item {} into non-folder {}",
                        child_guid, parent_guid)
@@ -99,6 +102,7 @@ pub enum ErrorKind {
     DuplicateItem(Guid),
     InvalidParent(Guid, Guid),
     MissingParent(Guid, Guid),
+    MissingItem(Guid),
     MergeConflict,
     UnmergedLocalItems,
     UnmergedRemoteItems,

src/lib.rs

@@ -26,7 +26,7 @@ mod tests;
 
 pub use crate::driver::{DefaultDriver, Driver, LogLevel};
 pub use crate::error::{Error, ErrorKind, Result};
-pub use crate::guid::{Guid, ROOT_GUID, USER_CONTENT_ROOTS};
+pub use crate::guid::{Guid, ROOT_GUID, UNFILED_GUID, USER_CONTENT_ROOTS};
 pub use crate::merge::{Deletion, Merger, StructureCounts};
 pub use crate::store::{MergeTimings, Stats, Store};
-pub use crate::tree::{Child, Content, Item, Kind, Validity, MergedDescendant, MergeState, MergedNode, ParentGuidFrom, Tree};
+pub use crate::tree::{Content, IntoTree, Item, Kind, Validity, MergedDescendant, MergeState, MergedNode, Tree};

src/tests.rs

@@ -18,7 +18,7 @@ use crate::driver::Driver;
 use crate::error::{ErrorKind, Result};
 use crate::guid::{Guid, ROOT_GUID, UNFILED_GUID};
 use crate::merge::{Merger, StructureCounts};
-use crate::tree::{Content, Item, Kind, ParentGuidFrom, Tree, Validity};
+use crate::tree::{Content, IntoTree, Item, Kind, Tree, Builder, Validity};
 
 #[derive(Debug)]
 struct Node {
@@ -31,25 +31,35 @@ impl Node {
         Node { item, children: Vec::new() }
     }
 
-    fn into_tree(self) -> Result<Tree> {
-        fn inflate(tree: &mut Tree, parent_guid: &Guid, node: Node) -> Result<()> {
+    fn into_builder(self) -> Result<Builder> {
+        fn inflate(b: &mut Builder, parent_guid: &Guid, node: Node) -> Result<()> {
             let guid = node.item.guid.clone();
-            tree.insert(ParentGuidFrom::default()
-                            .children(&parent_guid)
-                            .item(&parent_guid),
-                        node.item.into())?;
+            b.item(node.item)
+                .map(|_| ())
+                .or_else(|err| match err.kind() {
+                    ErrorKind::DuplicateItem(_) => Ok(()),
+                    _ => Err(err),
+                })?;
+            b.parent_for(&guid).by_structure(&parent_guid)?;
             for child in node.children {
-                inflate(tree, &guid, *child)?;
+                inflate(b, &guid, *child)?;
             }
             Ok(())
         }
 
         let guid = self.item.guid.clone();
-        let mut tree = Tree::with_reparenting(self.item, &UNFILED_GUID);
+        let mut builder = Tree::with_root(self.item);
+        builder.reparent_orphans_to(&UNFILED_GUID);
         for child in self.children {
-            inflate(&mut tree, &guid, *child)?;
+            inflate(&mut builder, &guid, *child)?;
         }
-        Ok(tree)
+        Ok(builder)
+    }
+}
+
+impl IntoTree for Node {
+    fn into_tree(self) -> Result<Tree> {
+        self.into_builder()?.into_tree()
     }
 }
 
@@ -2009,7 +2019,7 @@ fn reparent_orphans() {
         })
     }).into_tree().unwrap();
 
-    let mut remote_tree = nodes!({
+    let mut remote_tree_builder = nodes!({
         ("toolbar_____", Folder[needs_merge = true], {
             ("bookmarkBBBB", Bookmark),
             ("bookmarkAAAA", Bookmark)
@@ -2018,21 +2028,28 @@ fn reparent_orphans() {
             ("bookmarkDDDD", Bookmark[needs_merge = true]),
             ("bookmarkCCCC", Bookmark)
         })
-    }).into_tree().unwrap();
-    remote_tree.insert(ParentGuidFrom::default().item(&"toolbar_____".into()), Item {
-        guid: "bookmarkEEEE".into(),
-        kind: Kind::Bookmark,
-        age: 0,
-        needs_merge: true,
-        validity: Validity::Valid,
-    }.into()).expect("Should insert orphan E");
-    remote_tree.insert(ParentGuidFrom::default().item(&"nonexistent".into()), Item {
-        guid: "bookmarkFFFF".into(),
-        kind: Kind::Bookmark,
-        age: 0,
-        needs_merge: true,
-        validity: Validity::Valid,
-    }.into()).expect("Should insert orphan F");
+    }).into_builder().unwrap();
+    remote_tree_builder
+        .item(Item {
+            guid: "bookmarkEEEE".into(),
+            kind: Kind::Bookmark,
+            age: 0,
+            needs_merge: true,
+            validity: Validity::Valid,
+        })
+        .and_then(|p| p.by_parent_guid("toolbar_____".into()))
+        .expect("Should insert orphan E");
+    remote_tree_builder
+        .item(Item {
+            guid: "bookmarkFFFF".into(),
+            kind: Kind::Bookmark,
+            age: 0,
+            needs_merge: true,
+            validity: Validity::Valid,
+        })
+        .and_then(|p| p.by_parent_guid("nonexistent".into()))
+        .expect("Should insert orphan F");
+    let remote_tree = remote_tree_builder.into_tree().unwrap();
 
     let mut merger = Merger::new(&local_tree, &remote_tree);
     let merged_root = merger.merge().unwrap();
@@ -2166,19 +2183,19 @@ fn moved_user_content_roots() {
             ("bookmarkIIII", Bookmark),
             ("bookmarkAAAA", Bookmark[needs_merge = true])
         }),
-         ("menu________", Folder[needs_merge = true], {
-            ("bookmarkHHHH", Bookmark),
-            ("bookmarkBBBB", Bookmark[needs_merge = true]),
-            ("folderCCCCCC", Folder[needs_merge = true], {
-                ("bookmarkDDDD", Bookmark[needs_merge = true])
-            })
+        ("mobile______", Folder[needs_merge = true], {
+            ("bookmarkFFFF", Bookmark[needs_merge = true])
+        }),
+        ("menu________", Folder[needs_merge = true], {
+           ("bookmarkHHHH", Bookmark),
+           ("bookmarkBBBB", Bookmark[needs_merge = true]),
+           ("folderCCCCCC", Folder[needs_merge = true], {
+               ("bookmarkDDDD", Bookmark[needs_merge = true])
+           })
         }),
         ("toolbar_____", Folder[needs_merge = true], {
             ("bookmarkGGGG", Bookmark),
             ("bookmarkEEEE", Bookmark)
-        }),
-        ("mobile______", Folder[needs_merge = true], {
-            ("bookmarkFFFF", Bookmark[needs_merge = true])
         })
     }).into_tree().unwrap();
     let expected_telem = StructureCounts {