refactor: improve merge function

- Consume child node to take values
- Add MergeError to satisfy error trait

Signed-off-by: Andrey Mak <[email protected]>

src/error.rs

@@ -2,6 +2,7 @@ use crate::escape::{UnescapedRef, UnescapedRoute};
 use crate::tree::{denormalize_params, Node};
 
 use std::fmt;
+use std::ops::Deref;
 
 /// Represents errors that can occur when inserting a new route.
 #[non_exhaustive]
@@ -97,6 +98,29 @@ impl InsertError {
     }
 }
 
+/// A failed merge attempt.
+#[derive(Debug, Clone)]
+pub struct MergeError(pub(crate) Vec<InsertError>);
+
+impl fmt::Display for MergeError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        for error in self.0.iter() {
+            writeln!(f, "{}", error)?;
+        }
+        Ok(())
+    }
+}
+
+impl std::error::Error for MergeError {}
+
+impl Deref for MergeError {
+    type Target = Vec<InsertError>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
 /// A failed match attempt.
 ///
 /// ```

src/router.rs

@@ -1,3 +1,4 @@
+use crate::error::MergeError;
 use crate::tree::Node;
 use crate::{InsertError, MatchError, Params};
 
@@ -135,8 +136,7 @@ impl<T> Router<T> {
     }
 
     /// Merge a given router into current one.
-    ///
-    ///
+    /// Returns a list of [`InsertError`] for every failed insertion.
     /// # Examples
     ///
     /// ```rust
@@ -148,20 +148,24 @@ impl<T> Router<T> {
     /// let mut child = Router::new();
     /// child.insert("/users/{id}", "A User")?;
     ///
-    /// root.merge(child);
+    /// root.merge(child)?;
     /// assert!(root.at("/users/1").is_ok());
     /// # Ok(())
     /// # }
     /// ```
-    pub fn merge(&mut self, other: Self) -> Vec<InsertError> {
+    pub fn merge(&mut self, other: Self) -> Result<(), MergeError> {
         let mut errors = vec![];
         other.root.for_each(|path, value| {
             if let Err(err) = self.insert(path, value) {
                 errors.push(err);
             }
             true
         });
-        errors
+        if errors.is_empty() {
+            Ok(())
+        } else {
+            Err(MergeError(errors))
+        }
     }
 }
 

src/tree.rs

@@ -664,19 +664,18 @@ impl<T> Node<T> {
 impl<T> Node<T> {
     /// Iterates over the tree and calls the given visitor function
     /// with fully resolved path and its value.
-    pub fn for_each<V: FnMut(String, T) -> bool>(&self, mut visitor: V) {
+    pub fn for_each<V: FnMut(String, T) -> bool>(self, mut visitor: V) {
         let mut queue = VecDeque::new();
         queue.push_back((self.prefix.clone(), self));
-        while let Some((mut prefix, node)) = queue.pop_front() {
+        while let Some((mut prefix, mut node)) = queue.pop_front() {
             denormalize_params(&mut prefix, &node.remapping);
-            if node.value.is_some() {
+            if let Some(value) = node.value.take() {
                 let path = String::from_utf8(prefix.unescaped().to_vec()).unwrap();
-                let value = unsafe { node.value.as_ref().map(|x| std::ptr::read(x.get())) };
-                if !visitor(path, value.unwrap()) {
+                if !visitor(path, value.into_inner()) {
                     return;
                 }
             }
-            for child in node.children.iter() {
+            for child in node.children {
                 let mut prefix = prefix.clone();
                 prefix.append(&child.prefix);
                 queue.push_front((prefix, child));

tests/merge.rs

@@ -10,7 +10,7 @@ fn merge_ok() {
     assert!(child.insert("/baz", "baz").is_ok());
     assert!(child.insert("/xyz/{id}", "xyz").is_ok());
 
-    assert!(root.merge(child).is_empty());
+    assert!(root.merge(child).is_ok());
 
     assert_eq!(root.at("/foo").map(|m| *m.value), Ok("foo"));
     assert_eq!(root.at("/bar/1").map(|m| *m.value), Ok("bar"));
@@ -28,14 +28,15 @@ fn merge_conflict() {
     assert!(child.insert("/foo", "foo").is_ok());
     assert!(child.insert("/bar", "bar").is_ok());
 
-    let errors = root.merge(child);
+    let errors = root.merge(child).unwrap_err();
 
     assert_eq!(
         errors.get(0),
         Some(&InsertError::Conflict {
             with: "/bar".into()
         })
     );
+
     assert_eq!(
         errors.get(1),
         Some(&InsertError::Conflict {
@@ -52,7 +53,7 @@ fn merge_nested() {
     let mut child = Router::new();
     assert!(child.insert("/foo/bar", "bar").is_ok());
 
-    assert!(root.merge(child).is_empty());
+    assert!(root.merge(child).is_ok());
 
     assert_eq!(root.at("/foo").map(|m| *m.value), Ok("foo"));
     assert_eq!(root.at("/foo/bar").map(|m| *m.value), Ok("bar"));