From 4e3789ea9508cfbcb8f12bc567ef37c90b675e98 Mon Sep 17 00:00:00 2001 From: mtt <> Date: Sun, 20 Aug 2023 17:34:36 +0200 Subject: [PATCH 1/6] feat: add catch all routes --- crates/router/src/lib.rs | 20 ++++++++--- crates/router/src/route.rs | 51 ++++++++++++++++------------ tests/data/params/sub/[...all].wasm | Bin 0 -> 8 bytes 3 files changed, 44 insertions(+), 27 deletions(-) create mode 100644 tests/data/params/sub/[...all].wasm diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs index e04dbbe9..fdd7a5c3 100644 --- a/crates/router/src/lib.rs +++ b/crates/router/src/lib.rs @@ -128,6 +128,7 @@ mod tests { let fixed_route = build_route("/fixed.wasm"); let param_folder_route = build_route("/[id]/fixed.wasm"); let param_sub_route = build_route("/sub/[id].wasm"); + let catch_all_sub_route = build_route("/sub/[...all].wasm"); let tests = [ (¶m_route, "/a", RouteAffinity::CanManage(1)), @@ -137,10 +138,15 @@ mod tests { (¶m_folder_route, "/a/fixed", RouteAffinity::CanManage(1)), (¶m_sub_route, "/a/b", RouteAffinity::CannotManage), (¶m_sub_route, "/sub/b", RouteAffinity::CanManage(2)), + ( + &catch_all_sub_route, + "/sub/catch/all/routes", + RouteAffinity::CanManage(i32::MAX), + ), ]; - for t in tests { - assert_eq!(t.0.affinity(t.1), t.2); + for (route, path, route_affinity) in tests { + assert_eq!(route.affinity(path), route_affinity); } } @@ -162,6 +168,7 @@ mod tests { let fixed_route = build_route("/fixed.wasm"); let param_folder_route = build_route("/[id]/fixed.wasm"); let param_sub_route = build_route("/sub/[id].wasm"); + let catch_all_sub_route = build_route("/sub/[...all].wasm"); // I'm gonna use this values for comparison as `routes` consumes // the Route elements. @@ -169,6 +176,7 @@ mod tests { let fixed_path = fixed_route.path.clone(); let param_folder_path = param_folder_route.path.clone(); let param_sub_path = param_sub_route.path.clone(); + let catch_all_sub_path: String = catch_all_sub_route.path.clone(); let routes = Routes { routes: vec![ @@ -176,6 +184,7 @@ mod tests { fixed_route, param_folder_route, param_sub_route, + catch_all_sub_route, ], prefix: String::from("/"), }; @@ -185,13 +194,14 @@ mod tests { ("/fixed", Some(fixed_path)), ("/a/fixed", Some(param_folder_path)), ("/sub/b", Some(param_sub_path)), + ("/sub/catch/all/routes", Some(catch_all_sub_path)), ("/donot/exist", None), ]; - for t in tests { - let route = routes.retrieve_best_route(t.0); + for (given_path, expected_path) in tests { + let route = routes.retrieve_best_route(given_path); - if let Some(path) = t.1 { + if let Some(path) = expected_path { assert!(route.is_some()); assert_eq!(route.unwrap().path, path); } else { diff --git a/crates/router/src/route.rs b/crates/router/src/route.rs index 24c83055..c4bbba10 100644 --- a/crates/router/src/route.rs +++ b/crates/router/src/route.rs @@ -11,8 +11,8 @@ use wws_config::Config as ProjectConfig; use wws_worker::Worker; lazy_static! { - static ref PARAMETER_REGEX: Regex = Regex::new(r"\[\w+\]").unwrap(); - static ref DYNAMIC_ROUTE_REGEX: Regex = Regex::new(r".*\[\w+\].*").unwrap(); + static ref PARAMETER_REGEX: Regex = + Regex::new(r"\[(?P\.{3})?(?P\w+)\]").unwrap(); } /// Identify if a route can manage a certain URL and generates @@ -118,6 +118,7 @@ impl Route { /// - /a/[id].js /// - /[id]/b.wasm /// - /[id]/[other].wasm + /// - /[id]/[..all].wasm /// /// We need to establish a priority. The lower of the returned number, /// the more priority it has. This number is calculated based on the number of used @@ -125,30 +126,32 @@ impl Route { /// /// To avoid collisions like `[id]/b.wasm` vs `/a/[id].js`. Every depth level will /// add an extra +1 to the score. So, in case of `[id]/b.wasm` vs `/a/[id].js`, - /// the /a/b path will be managed by `[id]/b.wasm` + /// the /a/b path will be managed by `/a/[id].js` /// /// In case it cannot manage it, it will return -1 pub fn affinity(&self, url_path: &str) -> RouteAffinity { let mut score: i32 = 0; let mut split_path = self.path.split('/').peekable(); - for (depth, portion) in url_path.split('/').enumerate() { + for (depth, segment) in url_path.split('/').enumerate() { match split_path.next() { - Some(el) if el == portion => continue, - Some(el) if PARAMETER_REGEX.is_match(el) => { - score += depth as i32; - continue; - } - _ => return RouteAffinity::CannotManage, + None => return RouteAffinity::CannotManage, + Some(el) if el == segment => continue, + Some(el) => match PARAMETER_REGEX.captures(el) { + None => return RouteAffinity::CannotManage, + Some(caps) => match (caps.name("ellipsis"), caps.name("segment")) { + (Some(_), Some(_)) => return RouteAffinity::CanManage(i32::MAX), + (_, Some(_)) => score += depth as i32, + _ => return RouteAffinity::CannotManage, + }, + }, } } - // I should check the other iterator to confirm is empty - if split_path.peek().is_none() { - RouteAffinity::CanManage(score) - } else { - // The split path iterator still have some entries. - RouteAffinity::CannotManage + // I should check the other iterator to confirm if it is empty + match split_path.peek() { + None => RouteAffinity::CanManage(score), + Some(_) => RouteAffinity::CannotManage, } } @@ -156,16 +159,20 @@ impl Route { /// we are using `[]` in the filenames. However, actix expects a `{}` /// format for parameters. pub fn actix_path(&self) -> String { - // Replace [] with {} for making the path compatible with - let mut formatted = self.path.replace('[', "{"); - formatted = formatted.replace(']', "}"); - - formatted + PARAMETER_REGEX + .replace_all(&self.path, |caps: ®ex::Captures| { + match (caps.name("ellipsis"), caps.name("segment")) { + (Some(_), Some(segment)) => format!("{{{}:.*}}", segment.as_str()), + (_, Some(segment)) => format!("{{{}}}", segment.as_str()), + _ => String::new(), + } + }) + .into() } /// Check if the current route is dynamic pub fn is_dynamic(&self) -> bool { - DYNAMIC_ROUTE_REGEX.is_match(&self.path) + PARAMETER_REGEX.is_match(&self.path) } } diff --git a/tests/data/params/sub/[...all].wasm b/tests/data/params/sub/[...all].wasm new file mode 100644 index 0000000000000000000000000000000000000000..d8fc92d022fbf4d1072da17bc8e0840054b51ddc GIT binary patch literal 8 PcmZQbEY4+QU|;|M2ZjMd literal 0 HcmV?d00001 From b6fce947ed71a9efe918380e5902bb64da67da76 Mon Sep 17 00:00:00 2001 From: mtt <> Date: Sun, 27 Aug 2023 17:26:11 +0200 Subject: [PATCH 2/6] fix: /sub/[id] is better than /[id]/sub --- crates/router/src/lib.rs | 128 ++++--------------- crates/router/src/route.rs | 159 ++++++++++++++++-------- crates/router/src/route/segment.rs | 58 +++++++++ tests/data/params/[id]/sub.wasm | Bin 0 -> 8 bytes tests/data/params/sub/sub/[...all].wasm | Bin 0 -> 8 bytes 5 files changed, 191 insertions(+), 154 deletions(-) create mode 100644 crates/router/src/route/segment.rs create mode 100644 tests/data/params/[id]/sub.wasm create mode 100644 tests/data/params/sub/sub/[...all].wasm diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs index fdd7a5c3..0cf4b55c 100644 --- a/crates/router/src/lib.rs +++ b/crates/router/src/lib.rs @@ -8,9 +8,7 @@ mod files; mod route; - use files::Files; -use route::RouteAffinity; use std::path::{Path, PathBuf}; use std::time::Instant; use wws_config::Config; @@ -49,28 +47,22 @@ impl Routes { for route_path in route_paths { routes.push(Route::new(path, route_path, &prefix, config)); } + routes.sort(); println!("✅ Workers loaded in {:?}.", start.elapsed()); Self { routes, prefix } } - /// Based on a set of routes and a given path, it provides the best - /// match based on the parametrized URL score. See the [`Route::can_manage_path`] - /// method to understand how to calculate the score. + /// Provides the **first** route that can handle the given path. + /// This only works because the routes are already sorted. + /// Because a '/a/b' route may be served by: + /// - /a/b.js + /// - /a/[id].js + /// - /[id]/b.wasm + /// - /[id]/[other].wasm + /// - /[id]/[..all].wasm pub fn retrieve_best_route<'a>(&'a self, path: &str) -> Option<&'a Route> { - // Keep it to avoid calculating the score twice when iterating - // to look for the best route - let mut best_score = -1; - - self.routes - .iter() - .fold(None, |acc, item| match item.affinity(path) { - RouteAffinity::CanManage(score) if best_score == -1 || score < best_score => { - best_score = score; - Some(item) - } - _ => acc, - }) + self.routes.iter().find(|r| r.can_manage(path)) } /// Defines a prefix in the context of the application. @@ -108,98 +100,30 @@ impl Routes { #[cfg(test)] mod tests { use super::*; - use std::path::PathBuf; - - #[test] - fn route_path_affinity() { - let build_route = |file: &str| -> Route { - let project_config = Config::default(); - Route::new( - Path::new("../../tests/data/params"), - PathBuf::from(format!("../../tests/data/params{file}")), - "", - &project_config, - ) - }; - - // Route initializes the Wasm module. We create these - // variables to avoid loading the same Wasm module multiple times - let param_route = build_route("/[id].wasm"); - let fixed_route = build_route("/fixed.wasm"); - let param_folder_route = build_route("/[id]/fixed.wasm"); - let param_sub_route = build_route("/sub/[id].wasm"); - let catch_all_sub_route = build_route("/sub/[...all].wasm"); - - let tests = [ - (¶m_route, "/a", RouteAffinity::CanManage(1)), - (&fixed_route, "/fixed", RouteAffinity::CanManage(0)), - (&fixed_route, "/a", RouteAffinity::CannotManage), - (¶m_folder_route, "/a", RouteAffinity::CannotManage), - (¶m_folder_route, "/a/fixed", RouteAffinity::CanManage(1)), - (¶m_sub_route, "/a/b", RouteAffinity::CannotManage), - (¶m_sub_route, "/sub/b", RouteAffinity::CanManage(2)), - ( - &catch_all_sub_route, - "/sub/catch/all/routes", - RouteAffinity::CanManage(i32::MAX), - ), - ]; - - for (route, path, route_affinity) in tests { - assert_eq!(route.affinity(path), route_affinity); - } - } #[test] - fn best_route_by_affinity() { - let build_route = |file: &str| -> Route { - let project_config = Config::default(); - Route::new( - Path::new("../../tests/data/params"), - PathBuf::from(format!("../../tests/data/params{file}")), - "", - &project_config, - ) - }; - - // Route initializes the Wasm module. We create these - // variables to avoid loading the same Wasm module multiple times - let param_route = build_route("/[id].wasm"); - let fixed_route = build_route("/fixed.wasm"); - let param_folder_route = build_route("/[id]/fixed.wasm"); - let param_sub_route = build_route("/sub/[id].wasm"); - let catch_all_sub_route = build_route("/sub/[...all].wasm"); - - // I'm gonna use this values for comparison as `routes` consumes - // the Route elements. - let param_path = param_route.path.clone(); - let fixed_path = fixed_route.path.clone(); - let param_folder_path = param_folder_route.path.clone(); - let param_sub_path = param_sub_route.path.clone(); - let catch_all_sub_path: String = catch_all_sub_route.path.clone(); - - let routes = Routes { - routes: vec![ - param_route, - fixed_route, - param_folder_route, - param_sub_route, - catch_all_sub_route, - ], - prefix: String::from("/"), - }; + fn retrieve_best_route() { + let project_config = Config::default(); + let router = Routes::new( + Path::new("../../tests/data/params"), + "", + Vec::new(), + &project_config, + ); let tests = [ - ("/a", Some(param_path)), - ("/fixed", Some(fixed_path)), - ("/a/fixed", Some(param_folder_path)), - ("/sub/b", Some(param_sub_path)), - ("/sub/catch/all/routes", Some(catch_all_sub_path)), + ("/any", Some("/[id]")), + ("/fixed", Some("/fixed")), + ("/any/fixed", Some("/[id]/fixed")), + ("/any/sub", Some("/[id]/sub")), + ("/sub/any", Some("/sub/[id]")), + ("/sub/any/catch/all/routes", Some("/sub/[...all]")), + ("/sub/sub/any/catch/all/routes", Some("/sub/sub/[...all]")), ("/donot/exist", None), ]; for (given_path, expected_path) in tests { - let route = routes.retrieve_best_route(given_path); + let route = router.retrieve_best_route(given_path); if let Some(path) = expected_path { assert!(route.is_some()); diff --git a/crates/router/src/route.rs b/crates/router/src/route.rs index c4bbba10..55eba219 100644 --- a/crates/router/src/route.rs +++ b/crates/router/src/route.rs @@ -1,9 +1,13 @@ // Copyright 2022 VMware, Inc. // SPDX-License-Identifier: Apache-2.0 +mod segment; use lazy_static::lazy_static; use regex::Regex; +use segment::Segment; use std::{ + cmp::Ordering, + cmp::Ordering::{Greater, Less}, ffi::OsStr, path::{Component, Path, PathBuf}, }; @@ -15,16 +19,15 @@ lazy_static! { Regex::new(r"\[(?P\.{3})?(?P\w+)\]").unwrap(); } -/// Identify if a route can manage a certain URL and generates -/// a score in that case. This is required by dynamic routes as -/// different files can manage the same route. For example: -/// `/test` may be managed by `test.js` and `[id].js`. Regarding -/// the score, routes with a lower value will have a higher priority. +/// Represents the type of a route. +/// +/// Each variant of this enum holds an associated `usize` value, +/// which represents the number of segments in the route's path. #[derive(PartialEq, Eq, Debug)] -pub enum RouteAffinity { - CannotManage, - // Score - CanManage(i32), +pub enum RouteType { + Satic(usize), + Dynamic(usize), + Tail(usize), } /// An existing route in the project. It contains a reference to the handler, the URL path, @@ -42,6 +45,10 @@ pub struct Route { pub handler: PathBuf, /// The URL path pub path: String, + /// The route type + pub route_type: RouteType, + /// The segments' URL path + pub segments: Vec, /// The associated worker pub worker: Worker, } @@ -58,10 +65,12 @@ impl Route { project_config: &ProjectConfig, ) -> Self { let worker = Worker::new(base_path, &filepath, project_config).unwrap(); - + let route_path = Self::retrieve_route(base_path, &filepath, prefix); Self { - path: Self::retrieve_route(base_path, &filepath, prefix), handler: filepath, + route_type: Self::get_route_type(&route_path), + segments: Self::get_segments(&route_path), + path: route_path, worker, } } @@ -109,49 +118,57 @@ impl Route { .collect() } - /// Check if the given path can be managed by this worker. This was introduced - /// to support parameters in the URLs. Note that this method returns an integer, - /// which means the priority for this route. - /// - /// Note that a /a/b route may be served by: - /// - /a/b.js - /// - /a/[id].js - /// - /[id]/b.wasm - /// - /[id]/[other].wasm - /// - /[id]/[..all].wasm - /// - /// We need to establish a priority. The lower of the returned number, - /// the more priority it has. This number is calculated based on the number of used - /// parameters, as fixed routes has more priority than parameted ones. - /// - /// To avoid collisions like `[id]/b.wasm` vs `/a/[id].js`. Every depth level will - /// add an extra +1 to the score. So, in case of `[id]/b.wasm` vs `/a/[id].js`, - /// the /a/b path will be managed by `/a/[id].js` + /// Determine the type of route based on the provided route path. + fn get_route_type(route_path: &str) -> RouteType { + let path_segments_count = route_path.chars().filter(|&c| c == '/').count(); + if route_path.contains("/[...") { + RouteType::Tail(path_segments_count) + } else if route_path.contains("/[") { + RouteType::Dynamic(path_segments_count) + } else { + RouteType::Satic(path_segments_count) + } + } + + /// Parse the route path into individual segments and determine their types. /// - /// In case it cannot manage it, it will return -1 - pub fn affinity(&self, url_path: &str) -> RouteAffinity { - let mut score: i32 = 0; - let mut split_path = self.path.split('/').peekable(); + /// This function parses the provided route path into individual segments and identifies + /// whether each segment is static, dynamic, or tail based on certain patterns. + fn get_segments(route_path: &str) -> Vec { + route_path + .split('/') + .skip(1) + .into_iter() + .map(|segment| { + if segment.starts_with("[...") { + Segment::Tail(segment.to_owned()) + } else if segment.contains("[") { + Segment::Dynamic(segment.to_owned()) + } else { + Segment::Satic(segment.to_owned()) + } + }) + .collect() + } - for (depth, segment) in url_path.split('/').enumerate() { - match split_path.next() { - None => return RouteAffinity::CannotManage, - Some(el) if el == segment => continue, - Some(el) => match PARAMETER_REGEX.captures(el) { - None => return RouteAffinity::CannotManage, - Some(caps) => match (caps.name("ellipsis"), caps.name("segment")) { - (Some(_), Some(_)) => return RouteAffinity::CanManage(i32::MAX), - (_, Some(_)) => score += depth as i32, - _ => return RouteAffinity::CannotManage, - }, - }, - } - } + /// Check if the given path can be managed by this worker. This was introduced + /// to support parameters in the URLs. + /// Dertermine the 'RouteType' allow to shortcut the comparaison. + pub fn can_manage(&self, path: &str) -> bool { + let path_segments_count = path.chars().filter(|&c| c == '/').count(); - // I should check the other iterator to confirm if it is empty - match split_path.peek() { - None => RouteAffinity::CanManage(score), - Some(_) => RouteAffinity::CannotManage, + match self.route_type { + RouteType::Satic(_) => self.path == path, + RouteType::Dynamic(segments_count) if segments_count != path_segments_count => false, + RouteType::Tail(segments_count) if segments_count > path_segments_count => false, + _ => path + .split("/") + .skip(1) + .zip(self.segments.iter()) + .all(|zip| match zip { + (sp, Segment::Satic(segment)) => sp == segment, + _ => true, + }), } } @@ -172,7 +189,45 @@ impl Route { /// Check if the current route is dynamic pub fn is_dynamic(&self) -> bool { - PARAMETER_REGEX.is_match(&self.path) + match self.route_type { + RouteType::Satic(_) => false, + RouteType::Dynamic(_) => true, + RouteType::Tail(_) => true, + } + } +} + +impl Ord for Route { + fn cmp(&self, other: &Self) -> Ordering { + match (&self.route_type, &other.route_type) { + (RouteType::Satic(a), RouteType::Satic(b)) => a.cmp(b), + (RouteType::Satic(_), _) => Less, + (_, RouteType::Satic(_)) => Greater, + (RouteType::Dynamic(a), RouteType::Dynamic(b)) if a == b => { + self.segments.cmp(&other.segments) + } + (RouteType::Dynamic(a), RouteType::Dynamic(b)) => a.cmp(b), + (RouteType::Dynamic(_), _) => Less, + (_, RouteType::Dynamic(_)) => Greater, + (RouteType::Tail(a), RouteType::Tail(b)) if a == b => { + self.segments.cmp(&other.segments) + } + (RouteType::Tail(a), RouteType::Tail(b)) => b.cmp(a), + } + } +} + +impl PartialOrd for Route { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Eq for Route {} + +impl PartialEq for Route { + fn eq(&self, other: &Self) -> bool { + self.path == other.path } } diff --git a/crates/router/src/route/segment.rs b/crates/router/src/route/segment.rs new file mode 100644 index 00000000..3100a929 --- /dev/null +++ b/crates/router/src/route/segment.rs @@ -0,0 +1,58 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +use std::cmp::Ordering; + +#[derive(PartialEq, Eq, Debug)] +pub enum Segment { + Satic(String), + Dynamic(String), + Tail(String), +} + +impl Ord for Segment { + fn cmp(&self, other: &Self) -> Ordering { + // Define your custom comparison logic here + match (self, other) { + (Segment::Satic(a), Segment::Satic(b)) => a.cmp(b), + (Segment::Satic(_), _) => Ordering::Less, + (_, Segment::Satic(_)) => Ordering::Greater, + (Segment::Dynamic(a), Segment::Dynamic(b)) => a.cmp(b), + (Segment::Dynamic(_), _) => Ordering::Less, + (_, Segment::Dynamic(_)) => Ordering::Greater, + (Segment::Tail(a), Segment::Tail(b)) => a.cmp(b), + } + } +} + +impl PartialOrd for Segment { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(test)] +mod segment_tests { + use super::*; + + #[test] + fn test_segment_sort() { + let mut segments = vec![ + Segment::Tail("[...all]".to_string()), + Segment::Satic("fixed".to_string()), + Segment::Dynamic("[id]".to_string()), + Segment::Satic("sub".to_string()), + ]; + + segments.sort(); + + let expected_order = vec![ + Segment::Satic("fixed".to_string()), + Segment::Satic("sub".to_string()), + Segment::Dynamic("[id]".to_string()), + Segment::Tail("[...all]".to_string()), + ]; + + assert_eq!(segments, expected_order); + } +} diff --git a/tests/data/params/[id]/sub.wasm b/tests/data/params/[id]/sub.wasm new file mode 100644 index 0000000000000000000000000000000000000000..d8fc92d022fbf4d1072da17bc8e0840054b51ddc GIT binary patch literal 8 PcmZQbEY4+QU|;|M2ZjMd literal 0 HcmV?d00001 diff --git a/tests/data/params/sub/sub/[...all].wasm b/tests/data/params/sub/sub/[...all].wasm new file mode 100644 index 0000000000000000000000000000000000000000..d8fc92d022fbf4d1072da17bc8e0840054b51ddc GIT binary patch literal 8 PcmZQbEY4+QU|;|M2ZjMd literal 0 HcmV?d00001 From 4cbceec8c175d51422464e94545e59e4ae24366b Mon Sep 17 00:00:00 2001 From: mtt <> Date: Tue, 29 Aug 2023 00:25:52 +0200 Subject: [PATCH 3/6] fix: pr advices --- crates/router/src/route.rs | 116 +++++++++++++------------- crates/router/src/route/route_type.rs | 84 +++++++++++++++++++ crates/router/src/route/segment.rs | 63 +++++++++++--- 3 files changed, 190 insertions(+), 73 deletions(-) create mode 100644 crates/router/src/route/route_type.rs diff --git a/crates/router/src/route.rs b/crates/router/src/route.rs index 55eba219..1a551f36 100644 --- a/crates/router/src/route.rs +++ b/crates/router/src/route.rs @@ -1,9 +1,14 @@ // Copyright 2022 VMware, Inc. // SPDX-License-Identifier: Apache-2.0 +mod route_type; mod segment; use lazy_static::lazy_static; use regex::Regex; +use route_type::{ + RouteType, + RouteType::{Dynamic, Satic, Tail}, +}; use segment::Segment; use std::{ cmp::Ordering, @@ -19,17 +24,6 @@ lazy_static! { Regex::new(r"\[(?P\.{3})?(?P\w+)\]").unwrap(); } -/// Represents the type of a route. -/// -/// Each variant of this enum holds an associated `usize` value, -/// which represents the number of segments in the route's path. -#[derive(PartialEq, Eq, Debug)] -pub enum RouteType { - Satic(usize), - Dynamic(usize), - Tail(usize), -} - /// An existing route in the project. It contains a reference to the handler, the URL path, /// the runner and configuration. Note that URL paths are calculated based on the file path. /// @@ -68,7 +62,7 @@ impl Route { let route_path = Self::retrieve_route(base_path, &filepath, prefix); Self { handler: filepath, - route_type: Self::get_route_type(&route_path), + route_type: RouteType::from(&route_path), segments: Self::get_segments(&route_path), path: route_path, worker, @@ -118,55 +112,34 @@ impl Route { .collect() } - /// Determine the type of route based on the provided route path. - fn get_route_type(route_path: &str) -> RouteType { - let path_segments_count = route_path.chars().filter(|&c| c == '/').count(); - if route_path.contains("/[...") { - RouteType::Tail(path_segments_count) - } else if route_path.contains("/[") { - RouteType::Dynamic(path_segments_count) - } else { - RouteType::Satic(path_segments_count) - } - } - /// Parse the route path into individual segments and determine their types. /// /// This function parses the provided route path into individual segments and identifies /// whether each segment is static, dynamic, or tail based on certain patterns. fn get_segments(route_path: &str) -> Vec { - route_path - .split('/') - .skip(1) - .into_iter() - .map(|segment| { - if segment.starts_with("[...") { - Segment::Tail(segment.to_owned()) - } else if segment.contains("[") { - Segment::Dynamic(segment.to_owned()) - } else { - Segment::Satic(segment.to_owned()) - } - }) - .collect() + route_path.split('/').skip(1).map(Segment::from).collect() } /// Check if the given path can be managed by this worker. This was introduced /// to support parameters in the URLs. /// Dertermine the 'RouteType' allow to shortcut the comparaison. pub fn can_manage(&self, path: &str) -> bool { - let path_segments_count = path.chars().filter(|&c| c == '/').count(); + let path_number_of_segments = path.chars().filter(|&c| c == '/').count(); match self.route_type { - RouteType::Satic(_) => self.path == path, - RouteType::Dynamic(segments_count) if segments_count != path_segments_count => false, - RouteType::Tail(segments_count) if segments_count > path_segments_count => false, + Satic { + number_of_segments: _, + } => self.path == path, + Dynamic { number_of_segments } if number_of_segments != path_number_of_segments => { + false + } + Tail { number_of_segments } if number_of_segments > path_number_of_segments => false, _ => path - .split("/") + .split('/') .skip(1) .zip(self.segments.iter()) .all(|zip| match zip { - (sp, Segment::Satic(segment)) => sp == segment, + (sp, Segment::Static(segment)) => sp == segment, _ => true, }), } @@ -190,9 +163,9 @@ impl Route { /// Check if the current route is dynamic pub fn is_dynamic(&self) -> bool { match self.route_type { - RouteType::Satic(_) => false, - RouteType::Dynamic(_) => true, - RouteType::Tail(_) => true, + Satic { .. } => false, + Dynamic { .. } => true, + Tail { .. } => true, } } } @@ -200,19 +173,42 @@ impl Route { impl Ord for Route { fn cmp(&self, other: &Self) -> Ordering { match (&self.route_type, &other.route_type) { - (RouteType::Satic(a), RouteType::Satic(b)) => a.cmp(b), - (RouteType::Satic(_), _) => Less, - (_, RouteType::Satic(_)) => Greater, - (RouteType::Dynamic(a), RouteType::Dynamic(b)) if a == b => { - self.segments.cmp(&other.segments) - } - (RouteType::Dynamic(a), RouteType::Dynamic(b)) => a.cmp(b), - (RouteType::Dynamic(_), _) => Less, - (_, RouteType::Dynamic(_)) => Greater, - (RouteType::Tail(a), RouteType::Tail(b)) if a == b => { - self.segments.cmp(&other.segments) - } - (RouteType::Tail(a), RouteType::Tail(b)) => b.cmp(a), + ( + Satic { + number_of_segments: a, + }, + Satic { + number_of_segments: b, + }, + ) => a.cmp(b), + (Satic { .. }, _) => Less, + (_, Satic { .. }) => Greater, + ( + Dynamic { + number_of_segments: a, + }, + Dynamic { + number_of_segments: b, + }, + ) if a == b => self.segments.cmp(&other.segments), + (Dynamic { .. }, _) => Less, + (_, Dynamic { .. }) => Greater, + ( + Tail { + number_of_segments: a, + }, + Tail { + number_of_segments: b, + }, + ) if a == b => self.segments.cmp(&other.segments), + ( + Tail { + number_of_segments: a, + }, + Tail { + number_of_segments: b, + }, + ) => b.cmp(a), } } } diff --git a/crates/router/src/route/route_type.rs b/crates/router/src/route/route_type.rs new file mode 100644 index 00000000..8cd5781e --- /dev/null +++ b/crates/router/src/route/route_type.rs @@ -0,0 +1,84 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +/// Represents the type of a route. +/// +/// - `Satic`: Represents a static route with a fixed number of segments. +/// - `Dynamic`: Represents a dynamic route with a fixed number of segments. +/// - `Tail`: Represents a tail route with a variable number of segments. It may also contain dynamic segments. +#[derive(PartialEq, Eq, Debug)] +pub enum RouteType { + Satic { number_of_segments: usize }, + Dynamic { number_of_segments: usize }, + Tail { number_of_segments: usize }, +} +impl From<&String> for RouteType { + fn from(route_path: &String) -> Self { + let number_of_segments = route_path.chars().filter(|&c| c == '/').count(); + if route_path.contains("/[...") { + RouteType::Tail { number_of_segments } + } else if route_path.contains("/[") { + RouteType::Dynamic { number_of_segments } + } else { + RouteType::Satic { number_of_segments } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn route_type_from_string_static() { + let route_path = String::from("/fixed"); + let route_type = RouteType::from(&route_path); + + assert_eq!( + route_type, + RouteType::Satic { + number_of_segments: 1 + } + ); + } + + #[test] + fn route_type_from_string_dynamic() { + let route_path = String::from("/[id]"); + let route_type = RouteType::from(&route_path); + + assert_eq!( + route_type, + RouteType::Dynamic { + number_of_segments: 1 + } + ); + } + + #[test] + fn route_type_from_string_tail() { + let route_path = String::from("/sub/[...all]"); + let route_type: RouteType = RouteType::from(&route_path); + + assert_eq!( + route_type, + RouteType::Tail { + number_of_segments: 2 + } + ); + } + + #[test] + fn route_type_from_string_unknown() { + let route_path = String::from("/unknown"); + let route_type: RouteType = RouteType::from(&route_path); + + // Default to static if the format is not recognized + assert_eq!( + route_type, + RouteType::Satic { + number_of_segments: 1 + } + ); + } +} diff --git a/crates/router/src/route/segment.rs b/crates/router/src/route/segment.rs index 3100a929..3b3f7f06 100644 --- a/crates/router/src/route/segment.rs +++ b/crates/router/src/route/segment.rs @@ -3,20 +3,30 @@ use std::cmp::Ordering; -#[derive(PartialEq, Eq, Debug)] +#[derive(PartialEq, Eq, PartialOrd, Debug)] pub enum Segment { - Satic(String), + /// A static segment in the URL path. + /// Static segments are fixed and don't contain any parameter. + /// Example: Segment::Static("fixed"). + Static(String), + + /// A dynamic segment in the URL path. + /// Dynamic segments can contain a parameter that can change. + /// Example: Segment::Dynamic("[id]"). Dynamic(String), + + /// A trailing segment in the URL path. + /// Trailing segments are used to match the remainder of the path after a certain point. + /// Example: Segment::Tail("[...all]"). Tail(String), } impl Ord for Segment { fn cmp(&self, other: &Self) -> Ordering { - // Define your custom comparison logic here match (self, other) { - (Segment::Satic(a), Segment::Satic(b)) => a.cmp(b), - (Segment::Satic(_), _) => Ordering::Less, - (_, Segment::Satic(_)) => Ordering::Greater, + (Segment::Static(a), Segment::Static(b)) => a.cmp(b), + (Segment::Static(_), _) => Ordering::Less, + (_, Segment::Static(_)) => Ordering::Greater, (Segment::Dynamic(a), Segment::Dynamic(b)) => a.cmp(b), (Segment::Dynamic(_), _) => Ordering::Less, (_, Segment::Dynamic(_)) => Ordering::Greater, @@ -25,9 +35,15 @@ impl Ord for Segment { } } -impl PartialOrd for Segment { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) +impl From<&str> for Segment { + fn from(segment: &str) -> Self { + if segment.starts_with("[...") { + Segment::Tail(segment.to_owned()) + } else if segment.contains('[') { + Segment::Dynamic(segment.to_owned()) + } else { + Segment::Static(segment.to_owned()) + } } } @@ -39,20 +55,41 @@ mod segment_tests { fn test_segment_sort() { let mut segments = vec![ Segment::Tail("[...all]".to_string()), - Segment::Satic("fixed".to_string()), + Segment::Static("fixed".to_string()), Segment::Dynamic("[id]".to_string()), - Segment::Satic("sub".to_string()), + Segment::Static("sub".to_string()), ]; segments.sort(); let expected_order = vec![ - Segment::Satic("fixed".to_string()), - Segment::Satic("sub".to_string()), + Segment::Static("fixed".to_string()), + Segment::Static("sub".to_string()), Segment::Dynamic("[id]".to_string()), Segment::Tail("[...all]".to_string()), ]; assert_eq!(segments, expected_order); } + + #[test] + fn test_segment_from_static() { + let static_segment_str = "fixed"; + let static_segment = Segment::from(static_segment_str); + assert_eq!(static_segment, Segment::Static("fixed".to_owned())); + } + + #[test] + fn test_segment_from_dynamic() { + let dynamic_segment_str = "[id]"; + let dynamic_segment = Segment::from(dynamic_segment_str); + assert_eq!(dynamic_segment, Segment::Dynamic("[id]".to_owned())); + } + + #[test] + fn test_segment_from_tail() { + let tail_segment_str = "[...all]"; + let tail_segment = Segment::from(tail_segment_str); + assert_eq!(tail_segment, Segment::Tail("[...all]".to_owned())); + } } From 6db4a366d4f45b1f60b9dcc11bde0fc5b091dd95 Mon Sep 17 00:00:00 2001 From: mtt <> Date: Sun, 3 Sep 2023 23:02:11 +0200 Subject: [PATCH 4/6] test: add routes_sorted_on_creation test --- crates/router/src/lib.rs | 26 ++++++++++++++++++++++++++ crates/router/src/route.rs | 8 ++++++++ 2 files changed, 34 insertions(+) diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs index 0cf4b55c..fad20526 100644 --- a/crates/router/src/lib.rs +++ b/crates/router/src/lib.rs @@ -101,6 +101,32 @@ impl Routes { mod tests { use super::*; + #[test] + fn routes_sorted_on_creation() { + let project_config = Config::default(); + let router = Routes::new( + Path::new("../../tests/data/params"), + "", + Vec::new(), + &project_config, + ); + + let mut sorted_router = Routes::new( + Path::new("../../tests/data/params"), + "", + Vec::new(), + &project_config, + ); + + sorted_router.routes.sort(); + + let router_paths: Vec<&String> = router.routes.iter().map(|r| &r.path).collect(); + let sorted_router_paths: Vec<&String> = + sorted_router.routes.iter().map(|r| &r.path).collect(); + + assert_eq!(router_paths, sorted_router_paths); + } + #[test] fn retrieve_best_route() { let project_config = Config::default(); diff --git a/crates/router/src/route.rs b/crates/router/src/route.rs index 1a551f36..ee9af9d1 100644 --- a/crates/router/src/route.rs +++ b/crates/router/src/route.rs @@ -191,6 +191,14 @@ impl Ord for Route { number_of_segments: b, }, ) if a == b => self.segments.cmp(&other.segments), + ( + Dynamic { + number_of_segments: a, + }, + Dynamic { + number_of_segments: b, + }, + ) => a.cmp(b), (Dynamic { .. }, _) => Less, (_, Dynamic { .. }) => Greater, ( From b0c821854487498d85693d4871c7cce8515f46ed Mon Sep 17 00:00:00 2001 From: mtt <> Date: Sun, 3 Sep 2023 23:55:50 +0200 Subject: [PATCH 5/6] fix: clean code --- crates/router/src/lib.rs | 12 ++---------- crates/router/src/route.rs | 16 ++++++++-------- crates/router/src/route/route_type.rs | 12 ++++++------ crates/router/src/route/segment.rs | 2 +- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/crates/router/src/lib.rs b/crates/router/src/lib.rs index de17509c..2095c4be 100644 --- a/crates/router/src/lib.rs +++ b/crates/router/src/lib.rs @@ -58,10 +58,6 @@ impl Routes { self.routes.iter() } - pub fn iter(&self) -> impl Iterator { - self.routes.iter() - } - /// Provides the **first** route that can handle the given path. /// This only works because the routes are already sorted. /// Because a '/a/b' route may be served by: @@ -71,7 +67,7 @@ impl Routes { /// - /[id]/[other].wasm /// - /[id]/[..all].wasm pub fn retrieve_best_route<'a>(&'a self, path: &str) -> Option<&'a Route> { - self.routes.iter().find(|r| r.can_manage(path)) + self.iter().find(|r| r.can_manage(path)) } /// Defines a prefix in the context of the application. @@ -129,11 +125,7 @@ mod tests { sorted_router.routes.sort(); - let router_paths: Vec<&String> = router.routes.iter().map(|r| &r.path).collect(); - let sorted_router_paths: Vec<&String> = - sorted_router.routes.iter().map(|r| &r.path).collect(); - - assert_eq!(router_paths, sorted_router_paths); + assert_eq!(router.routes, sorted_router.routes); } #[test] diff --git a/crates/router/src/route.rs b/crates/router/src/route.rs index a1ef43ff..55724621 100644 --- a/crates/router/src/route.rs +++ b/crates/router/src/route.rs @@ -7,7 +7,7 @@ use lazy_static::lazy_static; use regex::Regex; use route_type::{ RouteType, - RouteType::{Dynamic, Satic, Tail}, + RouteType::{Dynamic, Static, Tail}, }; use segment::Segment; use std::{ @@ -37,7 +37,7 @@ lazy_static! { /// api/index.wasm => /api /// api/v2/ping.wasm => /api/v2/ping /// ``` -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Route { /// The wasm module that will manage the route pub handler: PathBuf, @@ -160,7 +160,7 @@ impl Route { let path_number_of_segments = path.chars().filter(|&c| c == '/').count(); match self.route_type { - Satic { + Static { number_of_segments: _, } => self.path == path, Dynamic { number_of_segments } if number_of_segments != path_number_of_segments => { @@ -196,7 +196,7 @@ impl Route { /// Check if the current route is dynamic pub fn is_dynamic(&self) -> bool { match self.route_type { - Satic { .. } => false, + Static { .. } => false, Dynamic { .. } => true, Tail { .. } => true, } @@ -207,15 +207,15 @@ impl Ord for Route { fn cmp(&self, other: &Self) -> Ordering { match (&self.route_type, &other.route_type) { ( - Satic { + Static { number_of_segments: a, }, - Satic { + Static { number_of_segments: b, }, ) => a.cmp(b), - (Satic { .. }, _) => Less, - (_, Satic { .. }) => Greater, + (Static { .. }, _) => Less, + (_, Static { .. }) => Greater, ( Dynamic { number_of_segments: a, diff --git a/crates/router/src/route/route_type.rs b/crates/router/src/route/route_type.rs index 8cd5781e..ee849515 100644 --- a/crates/router/src/route/route_type.rs +++ b/crates/router/src/route/route_type.rs @@ -3,12 +3,12 @@ /// Represents the type of a route. /// -/// - `Satic`: Represents a static route with a fixed number of segments. +/// - `Static`: Represents a static route with a fixed number of segments. /// - `Dynamic`: Represents a dynamic route with a fixed number of segments. /// - `Tail`: Represents a tail route with a variable number of segments. It may also contain dynamic segments. -#[derive(PartialEq, Eq, Debug)] +#[derive(PartialEq, Eq, Debug, Clone)] pub enum RouteType { - Satic { number_of_segments: usize }, + Static { number_of_segments: usize }, Dynamic { number_of_segments: usize }, Tail { number_of_segments: usize }, } @@ -20,7 +20,7 @@ impl From<&String> for RouteType { } else if route_path.contains("/[") { RouteType::Dynamic { number_of_segments } } else { - RouteType::Satic { number_of_segments } + RouteType::Static { number_of_segments } } } } @@ -36,7 +36,7 @@ mod tests { assert_eq!( route_type, - RouteType::Satic { + RouteType::Static { number_of_segments: 1 } ); @@ -76,7 +76,7 @@ mod tests { // Default to static if the format is not recognized assert_eq!( route_type, - RouteType::Satic { + RouteType::Static { number_of_segments: 1 } ); diff --git a/crates/router/src/route/segment.rs b/crates/router/src/route/segment.rs index 3b3f7f06..18051116 100644 --- a/crates/router/src/route/segment.rs +++ b/crates/router/src/route/segment.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; -#[derive(PartialEq, Eq, PartialOrd, Debug)] +#[derive(PartialEq, Eq, PartialOrd, Debug, Clone)] pub enum Segment { /// A static segment in the URL path. /// Static segments are fixed and don't contain any parameter. From f5ff2d5b3f4269237e08c105805b1c475e7b5bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Fern=C3=A1ndez=20L=C3=B3pez?= Date: Tue, 5 Sep 2023 12:55:07 +0200 Subject: [PATCH 6/6] chore: implement PartialOrd --- crates/router/src/route/segment.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/router/src/route/segment.rs b/crates/router/src/route/segment.rs index 18051116..bbd7ad3e 100644 --- a/crates/router/src/route/segment.rs +++ b/crates/router/src/route/segment.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; -#[derive(PartialEq, Eq, PartialOrd, Debug, Clone)] +#[derive(PartialEq, Eq, Debug, Clone)] pub enum Segment { /// A static segment in the URL path. /// Static segments are fixed and don't contain any parameter. @@ -35,6 +35,12 @@ impl Ord for Segment { } } +impl PartialOrd for Segment { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + impl From<&str> for Segment { fn from(segment: &str) -> Self { if segment.starts_with("[...") {