The great tree shaking, part 3: add a builder.

This started out as a fix for structure corruption corner cases, grew
into simplifying tree construction for callers, and turned into a
full-blown rewrite. In a way, we've come full circle: the new tree
stores a fully consistent structure, and relies on the new builder
to resolve inconsistencies and flag diverging structure.

PR #19 added lots of complexity to the original tree. This came from
storing two different sets of structure, one that's resolved at
`insert` time, and the other when we actually walk the tree. This
separation exists because we `insert` items one at a time, in the
order based on the `children`, so the tree doesn't have a complete
picture of all items in it.

However, we _do_ have that picture, in the mirror database. By the
time we build the tree, we know exactly which `children` and
`parentid`s exist, if an item has multiple parents, or no parents.
Those entries might not be in the tree yet, but that's because our
implementation requires the tree to always be valid.

This requirement also forces callers to go through unnecessary
contortions. On Desktop, `Store::fetch_remote_tree` first buffers all
items into a pseudo-tree, then walks it recursively to inflate the
Dogear tree. That's a lot of busy-work to query and normalize the
data, and assumes we have a complete structure, which might not be
the case.

Instead, what if we built the tree in two passes? One to add all
items, and one to set their parent-child relationships. The queries
for these are simpler, more correct, and let us defer resolving
inconsistencies until we're ready to build the tree. We can add
optimizations for valid trees, and still handle every kind of
diverging structure.

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

@@ -28,4 +28,4 @@ pub use crate::error::{Error, ErrorKind, Result};
 pub use crate::guid::{Guid, ROOT_GUID, USER_CONTENT_ROOTS};
 pub use crate::merge::{Deletion, Merger, StructureCounts};
 pub use crate::store::Store;
-pub use crate::tree::{Child, Content, Item, Kind, Validity, MergeState, MergedNode, ParentGuidFrom, Tree, Descendant};
+pub use crate::tree::{Content, IntoTree, Item, Kind, Validity, MergeState, MergedNode, Tree, Descendant};

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,36 @@ 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())?;
+            match b.item(node.item) {
+                Ok(_) => Ok(()),
+                Err(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()
     }
 }
 
@@ -1940,7 +1951,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)
@@ -1949,21 +1960,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_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_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();
@@ -2092,19 +2110,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::default();