Merge pull request #144 from DuskSystems/143-reconsider-router-intern…

…al-structure Rearchitect router internals.
DuskSystems · Sep 7, 2024 · 77ad7a1 · 77ad7a1
2 parents 15dd3fe + 02687bf
commit 77ad7a1
Show file tree

Hide file tree

Showing 13 changed files with 904 additions and 867 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - Router display now uses different characters to represent root and matchable nodes.
+- Successful matches now return a flattened representation of node data.
 
 ## [0.2.1] - 2024-09-04
 

diff --git a/README.md b/README.md
@@ -47,15 +47,15 @@ fn main() -> Result<(), Box<dyn Error>> {
 
     let path = Path::new("/users/123")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 1);
-    assert_eq!(search.data.path, "/users/{id}".into());
+    assert_eq!(*search.data, 1);
+    assert_eq!(search.route, "/users/{id}".into());
     assert_eq!(search.parameters[0].key, "id");
     assert_eq!(search.parameters[0].value, "123");
 
     let path = Path::new("/users/123/files/my.document.pdf")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 2);
-    assert_eq!(search.data.path, "/users/{id}/files/{filename}.{extension}".into());
+    assert_eq!(*search.data, 2);
+    assert_eq!(search.route, "/users/{id}/files/{filename}.{extension}".into());
     assert_eq!(search.parameters[0].key, "id");
     assert_eq!(search.parameters[0].value, "123");
     assert_eq!(search.parameters[1].key, "filename");
@@ -88,15 +88,15 @@ fn main() -> Result<(), Box<dyn Error>> {
 
     let path = Path::new("/files/documents/reports/annual.pdf/delete")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 1);
-    assert_eq!(search.data.path, "/files/{*slug}/delete".into());
+    assert_eq!(*search.data, 1);
+    assert_eq!(search.route, "/files/{*slug}/delete".into());
     assert_eq!(search.parameters[0].key, "slug");
     assert_eq!(search.parameters[0].value, "documents/reports/annual.pdf");
 
     let path = Path::new("/any/other/path")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 2);
-    assert_eq!(search.data.path, "/{*catch_all}".into());
+    assert_eq!(*search.data, 2);
+    assert_eq!(search.route, "/{*catch_all}".into());
     assert_eq!(search.parameters[0].key, "catch_all");
     assert_eq!(search.parameters[0].value, "any/other/path");
 
@@ -179,13 +179,13 @@ fn main() -> Result<(), Box<dyn Error>> {
 
     let path = Path::new("/v2")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 1);
-    assert_eq!(search.data.path, "/v2".into());
+    assert_eq!(*search.data, 1);
+    assert_eq!(search.route, "/v2".into());
 
     let path = Path::new("/v2/my-org/my-repo/blobs/sha256:1234567890")?;
     let search = router.search(&path)?.unwrap();
-    assert_eq!(search.data.value, 2);
-    assert_eq!(search.data.path, "/v2/{*name:namespace}/blobs/{type}:{digest}".into());
+    assert_eq!(*search.data, 2);
+    assert_eq!(search.route, "/v2/{*name:namespace}/blobs/{type}:{digest}".into());
     assert_eq!(search.parameters[0].key, "name");
     assert_eq!(search.parameters[0].value, "my-org/my-repo");
     assert_eq!(search.parameters[1].key, "type");
@@ -347,29 +347,29 @@ In a router of 130 routes, benchmark matching 4 paths.
 
 | Library          | Time      | Alloc Count | Alloc Size | Dealloc Count | Dealloc Size |
 |:-----------------|----------:|------------:|-----------:|--------------:|-------------:|
-| matchit          | 462.33 ns | 4           | 416 B      | 4             | 448 B        |
-| wayfind          | 483.07 ns | 7           | 649 B      | 7             | 649 B        |
-| xitca-router     | 562.71 ns | 7           | 800 B      | 7             | 832 B        |
-| path-tree        | 572.69 ns | 4           | 416 B      | 4             | 448 B        |
-| ntex-router      | 1.7347 µs | 18          | 1.248 KB   | 18            | 1.28 KB      |
-| route-recognizer | 4.6183 µs | 160         | 8.515 KB   | 160           | 8.547 KB     |
-| routefinder      | 6.5185 µs | 67          | 5.024 KB   | 67            | 5.056 KB     |
-| actix-router     | 21.268 µs | 214         | 13.93 KB   | 214           | 13.96 KB     |
+| matchit          | 484.35 ns | 4           | 416 B      | 4             | 448 B        |
+| wayfind          | 513.07 ns | 7           | 649 B      | 7             | 649 B        |
+| xitca-router     | 556.80 ns | 7           | 800 B      | 7             | 832 B        |
+| path-tree        | 576.46 ns | 4           | 416 B      | 4             | 448 B        |
+| ntex-router      | 1.7798 µs | 18          | 1.248 KB   | 18            | 1.28 KB      |
+| route-recognizer | 4.7831 µs | 160         | 8.515 KB   | 160           | 8.547 KB     |
+| routefinder      | 6.4484 µs | 67          | 5.024 KB   | 67            | 5.056 KB     |
+| actix-router     | 22.061 µs | 214         | 13.93 KB   | 214           | 13.96 KB     |
 
 #### `path-tree` inspired benches
 
 In a router of 320 routes, benchmark matching 80 paths.
 
 | Library          | Time      | Alloc Count | Alloc Size | Dealloc Count | Dealloc Size |
 |:-----------------|----------:|------------:|-----------:|--------------:|-------------:|
-| wayfind          | 7.0411 µs | 117         | 9.991 KB   | 117           | 9.991 KB     |
-| matchit          | 8.8426 µs | 140         | 17.81 KB   | 140           | 17.83 KB     |
-| path-tree        | 9.2876 µs | 59          | 7.447 KB   | 59            | 7.47 KB      |
-| xitca-router     | 10.888 µs | 209         | 25.51 KB   | 209           | 25.53 KB     |
-| ntex-router      | 30.283 µs | 201         | 19.54 KB   | 201           | 19.56 KB     |
-| routefinder      | 99.873 µs | 525         | 48.4 KB    | 525           | 48.43 KB     |
-| route-recognizer | 107.16 µs | 2872        | 191.8 KB   | 2872          | 205 KB       |
-| actix-router     | 192.44 µs | 2201        | 128.8 KB   | 2201          | 128.8 KB     |
+| wayfind          | 7.4842 µs | 117         | 9.991 KB   | 117           | 9.991 KB     |
+| matchit          | 8.8485 µs | 140         | 17.81 KB   | 140           | 17.83 KB     |
+| path-tree        | 9.2987 µs | 59          | 7.447 KB   | 59            | 7.47 KB      |
+| xitca-router     | 10.813 µs | 209         | 25.51 KB   | 209           | 25.53 KB     |
+| ntex-router      | 29.732 µs | 201         | 19.54 KB   | 201           | 19.56 KB     |
+| route-recognizer | 91.251 µs | 2872        | 191.8 KB   | 2872          | 205 KB       |
+| routefinder      | 99.015 µs | 525         | 48.4 KB    | 525           | 48.43 KB     |
+| actix-router     | 180.71 µs | 2201        | 128.8 KB   | 2201          | 128.8 KB     |
 
 ## License
 

diff --git a/examples/oci/src/router.rs b/examples/oci/src/router.rs
@@ -94,9 +94,9 @@ impl AppRouter {
             return StatusCode::NOT_FOUND.into_response();
         };
 
-        let handler = &search.data.value;
+        let handler = &search.data;
 
-        let route = search.data.path.to_string();
+        let route = search.route.to_string();
         let route = RouteInner(route);
 
         let parameters: Vec<(String, String)> = search

diff --git a/src/node.rs b/src/node.rs
@@ -27,11 +27,19 @@ pub enum NodeKind {
 
 /// Holds data associated with a given node.
 #[derive(Clone, Debug, Eq, PartialEq)]
-pub struct NodeData<T> {
-    /// The full path from the root to this node.
-    pub path: Arc<str>,
-    /// The value associated with this node.
-    pub value: T,
+pub enum NodeData<T> {
+    /// Data is stored inline.
+    Inline {
+        /// The original route path.
+        route: Arc<str>,
+
+        /// The associated data.
+        value: T,
+    },
+
+    /// Data is stored at the router level, as it's shared between 2 or more nodes.
+    #[allow(dead_code)]
+    Reference(Arc<str>),
 }
 
 /// Represents a node in the tree structure.

diff --git a/src/node/search.rs b/src/node/search.rs
@@ -1,12 +1,15 @@
-use super::{Node, NodeData};
+use super::Node;
 use crate::{errors::SearchError, router::StoredConstraint};
-use std::collections::HashMap;
+use std::{collections::HashMap, sync::Arc};
 
 /// Stores data from a successful router match.
 #[derive(Debug, Eq, PartialEq)]
 pub struct Match<'router, 'path, T> {
-    /// A reference to the data stored at the end matching node.
-    pub data: &'router NodeData<T>,
+    /// The matching route.
+    pub route: Arc<str>,
+
+    /// A reference to the matching route data.
+    pub data: &'router T,
 
     /// Key-value pairs of parameters, extracted from the route.
     pub parameters: Vec<Parameter<'router, 'path>>,

diff --git a/src/router.rs b/src/router.rs
@@ -30,6 +30,10 @@ pub struct Router<T> {
 
     /// A map of constraint names to [`StoredConstraint`].
     constraints: HashMap<Vec<u8>, StoredConstraint>,
+
+    /// Shared route data.
+    /// Only used when multiple nodes require the same data storage.
+    data: HashMap<Arc<str>, T>,
 }
 
 impl<T> Router<T> {
@@ -56,6 +60,7 @@ impl<T> Router<T> {
                 quick_dynamic: false,
             },
             constraints: HashMap::new(),
+            data: HashMap::new(),
         };
 
         router.constraint::<u8>().unwrap();
@@ -138,15 +143,15 @@ impl<T> Router<T> {
     /// router.insert("/hello/{world}", 2).unwrap();
     /// ```
     pub fn insert(&mut self, route: &str, value: T) -> Result<(), InsertError> {
-        let path = Path::new(route)?;
-        if route.as_bytes() != path.decoded_bytes() {
+        let route_path = Path::new(route)?;
+        if route.as_bytes() != route_path.decoded_bytes() {
             return Err(InsertError::EncodedPath {
                 input: route.to_string(),
-                decoded: String::from_utf8_lossy(path.decoded_bytes()).to_string(),
+                decoded: String::from_utf8_lossy(route_path.decoded_bytes()).to_string(),
             });
         }
 
-        let path = Arc::from(route);
+        let route_arc = Arc::from(route);
         let mut parts = Parts::new(route.as_bytes())?;
 
         for part in &parts {
@@ -167,7 +172,13 @@ impl<T> Router<T> {
             }
         }
 
-        self.root.insert(&mut parts, NodeData { path, value })
+        // TODO: Using Parts, determine whether we need to store inline or not.
+        let node_data = NodeData::Inline {
+            route: Arc::clone(&route_arc),
+            value,
+        };
+
+        self.root.insert(&mut parts, node_data)
     }
 
     /// Deletes a route from the router.
@@ -224,11 +235,23 @@ impl<T> Router<T> {
             return Ok(None);
         };
 
-        let Some(data) = node.data.as_ref() else {
-            return Ok(None);
+        let (route, data) = match &node.data {
+            Some(NodeData::Inline { route, value }) => (Arc::clone(route), value),
+            Some(NodeData::Reference(key)) => {
+                let Some(data) = self.data.get(key) else {
+                    return Ok(None);
+                };
+
+                (Arc::clone(key), data)
+            }
+            None => return Ok(None),
         };
 
-        Ok(Some(Match { data, parameters }))
+        Ok(Some(Match {
+            route,
+            data,
+            parameters,
+        }))
     }
 }
 

diff --git a/tests/common.rs b/tests/common.rs
@@ -13,15 +13,15 @@ macro_rules! assert_router_matches {
     };
 
     (@parse_expected {
-        path: $path:expr,
-        value: $value:expr
+        route: $route:expr,
+        data: $data:expr
         $(, params: {
             $($param_key:expr => $param_value:expr),+
         })?
     }) => {
         Some($crate::common::ExpectedMatch {
-            path: std::sync::Arc::from($path),
-            value: $value,
+            route: std::sync::Arc::from($route),
+            data: $data,
             params: vec![
                 $(
                     $( wayfind::Parameter {
@@ -39,8 +39,8 @@ macro_rules! assert_router_matches {
 }
 
 pub struct ExpectedMatch<'k, 'v, T> {
-    pub path: Arc<str>,
-    pub value: T,
+    pub route: Arc<str>,
+    pub data: T,
     pub params: Vec<Parameter<'k, 'v>>,
 }
 
@@ -51,17 +51,19 @@ pub fn assert_router_match<'a, T: PartialEq + Debug>(
     expected: Option<ExpectedMatch<'_, 'a, T>>,
 ) {
     let path = Path::new(input).expect("Invalid path!");
-    let Ok(Some(Match { data, parameters })) = router.search(&path) else {
+    let Ok(Some(Match {
+        route,
+        data,
+        parameters,
+    })) = router.search(&path)
+    else {
         assert!(expected.is_none(), "No match found for input: {input}");
         return;
     };
 
     if let Some(expected) = expected {
-        assert_eq!(data.path, expected.path, "Path mismatch for input: {input}");
-        assert_eq!(
-            data.value, expected.value,
-            "Value mismatch for input: {input}"
-        );
+        assert_eq!(route, expected.route, "Path mismatch for input: {input}");
+        assert_eq!(*data, expected.data, "Value mismatch for input: {input}");
         assert_eq!(
             parameters, expected.params,
             "Parameters mismatch for input: {input}"

diff --git a/tests/constraints.rs b/tests/constraints.rs
@@ -95,16 +95,16 @@ fn test_multiple_constraints() -> Result<(), Box<dyn Error>> {
 
     assert_router_matches!(router, {
         "/user/john/1234" => {
-            path: "/user/{name:length_3_to_10}/{id:year_1000_to_10000}",
-            value: 1,
+            route: "/user/{name:length_3_to_10}/{id:year_1000_to_10000}",
+            data: 1,
             params: {
                 "name" => "john",
                 "id" => "1234"
             }
         }
         "/user/johndoe/10000" => {
-            path: "/user/{name:length_3_to_10}/{id:year_1000_to_10000}",
-            value: 1,
+            route: "/user/{name:length_3_to_10}/{id:year_1000_to_10000}",
+            data: 1,
             params: {
                 "name" => "johndoe",
                 "id" => "10000"
@@ -116,16 +116,16 @@ fn test_multiple_constraints() -> Result<(), Box<dyn Error>> {
         "/user/john/10001" => None
 
         "/profile/alice.png" => {
-            path: "/profile/{username:length_3_to_10}.{ext:png_or_jpg}",
-            value: 2,
+            route: "/profile/{username:length_3_to_10}.{ext:png_or_jpg}",
+            data: 2,
             params: {
                 "username" => "alice",
                 "ext" => "png"
             }
         }
         "/profile/bob.jpg" => {
-            path: "/profile/{username:length_3_to_10}.{ext:png_or_jpg}",
-            value: 2,
+            route: "/profile/{username:length_3_to_10}.{ext:png_or_jpg}",
+            data: 2,
             params: {
                 "username" => "bob",
                 "ext" => "jpg"
@@ -136,16 +136,16 @@ fn test_multiple_constraints() -> Result<(), Box<dyn Error>> {
         "/profile/alice.gif" => None
 
         "/posts/2022/hello" => {
-            path: "/posts/{year:even_year}/{slug:valid_slug}",
-            value: 3,
+            route: "/posts/{year:even_year}/{slug:valid_slug}",
+            data: 3,
             params: {
                 "year" => "2022",
                 "slug" => "hello"
             }
         }
         "/posts/2024/test-123" => {
-            path: "/posts/{year:even_year}/{slug:valid_slug}",
-            value: 3,
+            route: "/posts/{year:even_year}/{slug:valid_slug}",
+            data: 3,
             params: {
                 "year" => "2024",
                 "slug" => "test-123"