From 6a5c33bf75f976668905eaa13edd5c79dca6ab50 Mon Sep 17 00:00:00 2001 From: Andrey Mak Date: Sat, 12 Oct 2024 00:40:01 +0400 Subject: [PATCH 1/5] feat(router): add merge function Signed-off-by: Andrey Mak --- src/router.rs | 33 +++++++++++++++++++++++++++++++++ src/tree.rs | 27 +++++++++++++++++++++++++++ tests/merge.rs | 30 ++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 tests/merge.rs diff --git a/src/router.rs b/src/router.rs index 8256c4b..c93f6c5 100644 --- a/src/router.rs +++ b/src/router.rs @@ -133,6 +133,39 @@ impl Router { pub fn check_priorities(&self) -> Result { self.root.check_priorities() } + + + /// Merge a given router into current one. + /// + /// + /// # Examples + /// + /// ```rust + /// # use matchit::Router; + /// # fn main() -> Result<(), Box> { + /// let mut root = Router::new(); + /// root.insert("/home", "Welcome!")?; + /// + /// let mut child = Router::new(); + /// child.insert("/users/{id}", "A User")?; + /// + /// root.merge(child)?; + /// # Ok(()) + /// # } + /// ``` + pub fn merge(&mut self, other: Self) -> Result<(), InsertError> { + let mut result = Ok(()); + other.root.for_each(|path, value| { + match self.insert(path, value) { + Ok(..) => true, + Err(err) => { + result = Err(err); + false + } + } + }); + result + } } /// A successful match consisting of the registered value diff --git a/src/tree.rs b/src/tree.rs index 3965ac8..4589a6c 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -3,6 +3,7 @@ use crate::{InsertError, MatchError, Params}; use std::cell::UnsafeCell; use std::cmp::min; +use std::collections::VecDeque; use std::ops::Range; use std::{fmt, mem}; @@ -660,6 +661,32 @@ impl Node { } } +impl Node { + /// Iterates over the tree and calls the given visitor function + /// with fully resolved path and its value. + pub fn for_each bool >(self, mut visitor: V) { + let mut queue = VecDeque::new(); + queue.push_back(self); + while let Some(mut node) = queue.pop_front() { + if node.children.is_empty() { + denormalize_params(&mut node.prefix, &node.remapping); + let path = String::from_utf8(node.prefix.into_unescaped()).unwrap(); + let value = node.value.take().map(UnsafeCell::into_inner); + if !visitor(path, value.unwrap()) { + return; + } + } else { + for mut child in node.children { + let mut prefix = node.prefix.clone(); + prefix.append(&child.prefix); + child.prefix = prefix; + queue.push_front(child); + } + } + } + } +} + /// An ordered list of route parameters keys for a specific route. /// /// To support conflicting routes like `/{a}/foo` and `/{b}/bar`, route parameters diff --git a/tests/merge.rs b/tests/merge.rs new file mode 100644 index 0000000..02449c7 --- /dev/null +++ b/tests/merge.rs @@ -0,0 +1,30 @@ +use matchit::{InsertError, Router}; + +#[test] +fn merge_ok() { + let mut root = Router::new(); + assert!(root.insert("/foo", "foo").is_ok()); + assert!(root.insert("/bar/{id}", "bar").is_ok()); + + let mut child = Router::new(); + assert!(child.insert("/baz", "baz").is_ok()); + assert!(child.insert("/xyz/{id}", "xyz").is_ok()); + + 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")); + assert_eq!(root.at("/baz").map(|m| *m.value), Ok("baz")); + assert_eq!(root.at("/xyz/2").map(|m| *m.value), Ok("xyz")); +} + +#[test] +fn merge_conflict() { + let mut root = Router::new(); + assert!(root.insert("/foo", "foo").is_ok()); + + let mut child = Router::new(); + assert!(child.insert("/foo", "foo").is_ok()); + + assert_eq!(root.merge(child), Err(InsertError::Conflict {with: "/foo".into()})); +} From 124cee3ef2572f8ad6020d680fc9696677a8a92a Mon Sep 17 00:00:00 2001 From: Andrey Mak Date: Sat, 12 Oct 2024 03:29:52 +0400 Subject: [PATCH 2/5] feat(router): improve merge function - do not mutate child router - return list of insert errors - call visitor on each node with value Signed-off-by: Andrey Mak --- src/router.rs | 18 ++++++++---------- src/tree.rs | 26 ++++++++++++-------------- tests/merge.rs | 23 +++++++++++++++++++++-- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/src/router.rs b/src/router.rs index c93f6c5..e760908 100644 --- a/src/router.rs +++ b/src/router.rs @@ -149,22 +149,20 @@ impl Router { /// 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) -> Result<(), InsertError> { - let mut result = Ok(()); + pub fn merge(&mut self, other: Self) -> Vec { + let mut errors = vec![]; other.root.for_each(|path, value| { - match self.insert(path, value) { - Ok(..) => true, - Err(err) => { - result = Err(err); - false - } + if let Err(err) = self.insert(path, value) { + errors.push(err); } + true }); - result + errors } } diff --git a/src/tree.rs b/src/tree.rs index 4589a6c..c7cf5f4 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -664,24 +664,22 @@ impl Node { impl Node { /// Iterates over the tree and calls the given visitor function /// with fully resolved path and its value. - pub fn for_each bool >(self, mut visitor: V) { + pub fn for_each bool >(&self, mut visitor: V) { let mut queue = VecDeque::new(); - queue.push_back(self); - while let Some(mut node) = queue.pop_front() { - if node.children.is_empty() { - denormalize_params(&mut node.prefix, &node.remapping); - let path = String::from_utf8(node.prefix.into_unescaped()).unwrap(); - let value = node.value.take().map(UnsafeCell::into_inner); + queue.push_back((self.prefix.clone(), self)); + while let Some((mut prefix, node)) = queue.pop_front() { + denormalize_params(&mut prefix, &node.remapping); + if node.value.is_some() { + 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()) { return; } - } else { - for mut child in node.children { - let mut prefix = node.prefix.clone(); - prefix.append(&child.prefix); - child.prefix = prefix; - queue.push_front(child); - } + } + for child in node.children.iter() { + let mut prefix = prefix.clone(); + prefix.append(&child.prefix); + queue.push_front((prefix, child)); } } } diff --git a/tests/merge.rs b/tests/merge.rs index 02449c7..a1f1adc 100644 --- a/tests/merge.rs +++ b/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_ok()); + assert!(root.merge(child).is_empty()); assert_eq!(root.at("/foo").map(|m| *m.value), Ok("foo")); assert_eq!(root.at("/bar/1").map(|m| *m.value), Ok("bar")); @@ -22,9 +22,28 @@ fn merge_ok() { fn merge_conflict() { let mut root = Router::new(); assert!(root.insert("/foo", "foo").is_ok()); + assert!(root.insert("/bar", "bar").is_ok()); let mut child = Router::new(); assert!(child.insert("/foo", "foo").is_ok()); + assert!(child.insert("/bar", "bar").is_ok()); - assert_eq!(root.merge(child), Err(InsertError::Conflict {with: "/foo".into()})); + let errors = root.merge(child); + + assert_eq!(errors.get(0), Some(&InsertError::Conflict {with: "/bar".into()})); + assert_eq!(errors.get(1), Some(&InsertError::Conflict {with: "/foo".into()})); +} + +#[test] +fn merge_nested() { + let mut root = Router::new(); + assert!(root.insert("/foo", "foo").is_ok()); + + let mut child = Router::new(); + assert!(child.insert("/foo/bar", "bar").is_ok()); + + assert!(root.merge(child).is_empty()); + + assert_eq!(root.at("/foo").map(|m| *m.value), Ok("foo")); + assert_eq!(root.at("/foo/bar").map(|m| *m.value), Ok("bar")); } From fae9c25968b8243787b5fcf251f63dd44a056a4d Mon Sep 17 00:00:00 2001 From: Andrey Mak Date: Sat, 12 Oct 2024 11:19:29 +0400 Subject: [PATCH 3/5] feat(router): fix formatting errors Signed-off-by: Andrey Mak --- src/router.rs | 1 - src/tree.rs | 4 ++-- tests/merge.rs | 14 ++++++++++++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/router.rs b/src/router.rs index e760908..623a3ea 100644 --- a/src/router.rs +++ b/src/router.rs @@ -134,7 +134,6 @@ impl Router { self.root.check_priorities() } - /// Merge a given router into current one. /// /// diff --git a/src/tree.rs b/src/tree.rs index c7cf5f4..5c2e762 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -664,14 +664,14 @@ impl Node { impl Node { /// Iterates over the tree and calls the given visitor function /// with fully resolved path and its value. - pub fn for_each bool >(&self, mut visitor: V) { + pub fn for_each 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() { denormalize_params(&mut prefix, &node.remapping); if node.value.is_some() { let path = String::from_utf8(prefix.unescaped().to_vec()).unwrap(); - let value = unsafe {node.value.as_ref().map(|x| std::ptr::read(x.get()))}; + let value = unsafe { node.value.as_ref().map(|x| std::ptr::read(x.get())) }; if !visitor(path, value.unwrap()) { return; } diff --git a/tests/merge.rs b/tests/merge.rs index a1f1adc..72c5dfb 100644 --- a/tests/merge.rs +++ b/tests/merge.rs @@ -30,8 +30,18 @@ fn merge_conflict() { let errors = root.merge(child); - assert_eq!(errors.get(0), Some(&InsertError::Conflict {with: "/bar".into()})); - assert_eq!(errors.get(1), Some(&InsertError::Conflict {with: "/foo".into()})); + assert_eq!( + errors.get(0), + Some(&InsertError::Conflict { + with: "/bar".into() + }) + ); + assert_eq!( + errors.get(1), + Some(&InsertError::Conflict { + with: "/foo".into() + }) + ); } #[test] From 87ae11c4eaddbd1d4519100fd917caacaf618c34 Mon Sep 17 00:00:00 2001 From: Andrey Mak Date: Sat, 12 Oct 2024 19:55:11 +0400 Subject: [PATCH 4/5] refactor: improve merge function - Consume child node to take values - Add MergeError to satisfy error trait Signed-off-by: Andrey Mak --- src/error.rs | 24 ++++++++++++++++++++++++ src/router.rs | 14 +++++++++----- src/tree.rs | 11 +++++------ tests/merge.rs | 7 ++++--- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/error.rs b/src/error.rs index ab879f3..5f46f4b 100644 --- a/src/error.rs +++ b/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); + +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; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + /// A failed match attempt. /// /// ``` diff --git a/src/router.rs b/src/router.rs index 623a3ea..2ed4c50 100644 --- a/src/router.rs +++ b/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 Router { } /// Merge a given router into current one. - /// - /// + /// Returns a list of [`InsertError`] for every failed insertion. /// # Examples /// /// ```rust @@ -148,12 +148,12 @@ impl Router { /// 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 { + 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) { @@ -161,7 +161,11 @@ impl Router { } true }); - errors + if errors.is_empty() { + Ok(()) + } else { + Err(MergeError(errors)) + } } } diff --git a/src/tree.rs b/src/tree.rs index 5c2e762..ff26c37 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -664,19 +664,18 @@ impl Node { impl Node { /// Iterates over the tree and calls the given visitor function /// with fully resolved path and its value. - pub fn for_each bool>(&self, mut visitor: V) { + pub fn for_each 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)); diff --git a/tests/merge.rs b/tests/merge.rs index 72c5dfb..24dc41f 100644 --- a/tests/merge.rs +++ b/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,7 +28,7 @@ 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), @@ -36,6 +36,7 @@ fn merge_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")); From b615fa7f7e2e52ca42071cd919b132175e6be37d Mon Sep 17 00:00:00 2001 From: Andrey Mak Date: Sun, 13 Oct 2024 14:57:59 +0400 Subject: [PATCH 5/5] refactor: improve merge function - initialize queue from known-size set - push back childs to make algo true BFS - adjust test cases Signed-off-by: Andrey Mak --- src/tree.rs | 5 ++--- tests/merge.rs | 13 +++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/tree.rs b/src/tree.rs index ff26c37..3aa2027 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -665,8 +665,7 @@ impl Node { /// Iterates over the tree and calls the given visitor function /// with fully resolved path and its value. pub fn for_each bool>(self, mut visitor: V) { - let mut queue = VecDeque::new(); - queue.push_back((self.prefix.clone(), self)); + let mut queue = VecDeque::from([(self.prefix.clone(), self)]); while let Some((mut prefix, mut node)) = queue.pop_front() { denormalize_params(&mut prefix, &node.remapping); if let Some(value) = node.value.take() { @@ -678,7 +677,7 @@ impl Node { for child in node.children { let mut prefix = prefix.clone(); prefix.append(&child.prefix); - queue.push_front((prefix, child)); + queue.push_back((prefix, child)); } } } diff --git a/tests/merge.rs b/tests/merge.rs index 24dc41f..a4fa2bb 100644 --- a/tests/merge.rs +++ b/tests/merge.rs @@ -25,24 +25,29 @@ fn merge_conflict() { assert!(root.insert("/bar", "bar").is_ok()); let mut child = Router::new(); - assert!(child.insert("/foo", "foo").is_ok()); - assert!(child.insert("/bar", "bar").is_ok()); + assert!(child.insert("/foo", "changed").is_ok()); + assert!(child.insert("/bar", "changed").is_ok()); + assert!(child.insert("/baz", "baz").is_ok()); let errors = root.merge(child).unwrap_err(); assert_eq!( errors.get(0), Some(&InsertError::Conflict { - with: "/bar".into() + with: "/foo".into() }) ); assert_eq!( errors.get(1), Some(&InsertError::Conflict { - with: "/foo".into() + with: "/bar".into() }) ); + + assert_eq!(root.at("/foo").map(|m| *m.value), Ok("foo")); + assert_eq!(root.at("/bar").map(|m| *m.value), Ok("bar")); + assert_eq!(root.at("/baz").map(|m| *m.value), Ok("baz")); } #[test]