From 22b582ec2bef8323dba994fb348609497e74d424 Mon Sep 17 00:00:00 2001 From: Elijah Date: Sat, 1 Aug 2026 14:54:23 +0000 Subject: [PATCH 01/16] Use a radix trie for route matching --- cot/src/router.rs | 741 +++++++++++++++++++++++++++++++++++++---- cot/src/router/path.rs | 307 +++++++++-------- 2 files changed, 842 insertions(+), 206 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index 2d49c26a..25c1dcab 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -36,7 +36,7 @@ use tracing::debug; use crate::error::NotFound; use crate::request::{PathParams, Request, RequestExt, RequestHead}; use crate::response::Response; -use crate::router::path::{CaptureResult, PathMatcher, ReverseParamMap}; +use crate::router::path::{PathMatcher, PathPart, ReverseParamMap}; use crate::{Error, ProjectContext, Result}; pub mod method; @@ -66,6 +66,7 @@ pub struct Router { app_name: Option, urls: Vec, names: HashMap>, + route_tree: RouteTree, } impl Router { @@ -102,6 +103,29 @@ impl Router { /// ``` #[must_use] pub fn with_urls>>(urls: T) -> Self { + match Self::try_with_urls(urls) { + Ok(router) => router, + Err(err) => panic!("{err}"), + } + } + + /// Create a router with the given routes. This is a fallible version + /// of [Self::with_urls] + /// + /// # Examples + /// + /// ``` + /// use cot::request::Request; + /// use cot::response::Response; + /// use cot::router::{Route, Router}; + /// + /// async fn home(request: Request) -> cot::Result { + /// unimplemented!() + /// } + /// + /// let router = Router::try_with_urls([Route::with_handler_and_name("/", home, "home")]).unwrap(); + /// ``` + pub fn try_with_urls>>(urls: T) -> Result { let urls = urls.into(); let mut names = HashMap::new(); @@ -110,12 +134,13 @@ impl Router { names.insert(name.clone(), url.url.clone()); } } - - Self { + let route_tree = RouteTree::from_routes(&urls)?; + Ok(Self { app_name: None, urls, names, - } + route_tree, + }) } pub(crate) fn set_app_name(&mut self, app_name: AppName) { @@ -145,60 +170,7 @@ impl Router { } fn get_handler(&self, request_path: &str) -> Option> { - for route in &self.urls { - if let Some(matches) = route.url.capture(request_path) { - let matches_fully = matches.matches_fully(); - - match &route.view { - RouteInner::Handler(handler) => { - if matches_fully { - return Some(HandlerFound { - handler: &**handler, - app_name: self.app_name.clone(), - name: route.name.clone(), - params: Self::matches_to_path_params(&matches, Vec::new()), - }); - } - } - RouteInner::Router(router) => { - if let Some(result) = router.get_handler(matches.remaining_path) { - return Some(HandlerFound { - handler: result.handler, - app_name: result.app_name.or_else(|| self.app_name.clone()), - name: result.name, - params: Self::matches_to_path_params(&matches, result.params), - }); - } - } - #[cfg(feature = "openapi")] - RouteInner::ApiHandler(handler) => { - if matches_fully { - let handler: &(dyn BoxRequestHandler + Send + Sync) = &**handler; - return Some(HandlerFound { - handler, - app_name: self.app_name.clone(), - name: route.name.clone(), - params: Self::matches_to_path_params(&matches, Vec::new()), - }); - } - } - } - } - } - - None - } - - fn matches_to_path_params( - matches: &CaptureResult<'_, '_>, - mut path_params: Vec<(String, String)>, - ) -> Vec<(String, String)> { - // Adding in reverse order, since we're doing this from the bottom up (we're - // going to reverse the order before running the handler) - for param in matches.params.iter().rev() { - path_params.push((param.name.to_owned(), param.value.clone())); - } - path_params + self.route_tree.find(self, request_path) } /// Handle a request. @@ -452,6 +424,381 @@ struct NoViewToReverse { } impl_into_cot_error!(NoViewToReverse); +type RouteNodeResult = std::result::Result; +const ERROR_PREFIX: &str = "route conflict error:"; +#[derive(Debug, thiserror::Error)] +enum RouteConflictError { + #[error( + "{ERROR_PREFIX} duplicate route: `{new}` conflicts with an already registered handler route `{existing}` \ + (both fully match the same path)" + )] + DuplicateHandler { existing: String, new: String }, + + #[error( + "{ERROR_PREFIX} duplicate nested router: `{new}` conflicts with an already registered \ + nested router mounted at `{existing}`" + )] + DuplicateRouter { existing: String, new: String }, + + #[error( + "{ERROR_PREFIX} conflicting route parameters: `{existing}` uses `{{{existing_name}}}` but `{new}` uses \ + `{{{new_name}}}` at the same position in the path -- both routes must bind the same \ + parameter name there, since only one value can be captured at that position" + )] + ConflictingParamName { + existing: String, + existing_name: String, + new: String, + new_name: String, + }, + + #[error( + "{ERROR_PREFIX} conflicting wildcard parameters: `{existing}` uses `{{*{existing_name}}}` but `{new}` \ + uses `{{*{new_name}}}` at the same position in the path" + )] + ConflictingWildcardName { + existing: String, + existing_name: String, + new: String, + new_name: String, + }, + + #[error( + "{ERROR_PREFIX} duplicate wildcard route: `{new}` conflicts with an already-registered \ + wildcard route `{existing}`" + )] + DuplicateWildcard { existing: String, new: String }, +} +impl_into_cot_error!(RouteConflictError); + +#[derive(Debug, Clone)] +struct RouteTree { + root: RouteNode, +} + +impl RouteTree { + fn from_routes(routes: &[Route]) -> Result { + let mut tree = Self { + root: RouteNode::default(), + }; + for (index, route) in routes.iter().enumerate() { + tree.root.insert(route.url.parts(), index, routes)?; + } + + Ok(tree) + } + + fn find<'a>(&'a self, router: &'a Router, path: &str) -> Option> { + let mut params = Vec::new(); + self.root.find(router, path, &mut params) + } +} + +#[derive(Debug, Clone, Default)] +struct RouteNode { + prefix: String, + static_children: Vec, + param_child: Option>, + wildcard_child: Option, + handler_route: Option, + router_route: Option, +} + +impl RouteNode { + fn insert( + &mut self, + parts: &[PathPart], + route_index: usize, + routes: &[Route], + ) -> RouteNodeResult<()> { + if let Some((part, rest)) = parts.split_first() { + match part { + PathPart::Literal(literal) => { + self.insert_static(literal, rest, route_index, routes) + } + PathPart::Param { name } => self.insert_param(name, rest, route_index, routes), + PathPart::Wildcard { name } => { + self.insert_wildcard(name, rest, route_index, routes) + } + } + } else { + self.insert_route(route_index, routes) + } + } + + fn insert_static( + &mut self, + literal: &str, + rest: &[PathPart], + route_index: usize, + routes: &[Route], + ) -> RouteNodeResult<()> { + if literal.is_empty() { + return self.insert(rest, route_index, routes); + } + + for child in &mut self.static_children { + let common = common_prefix_len(&child.prefix, literal); + if common == 0 { + continue; + } + if common < child.prefix.len() { + child.split_at(common); + } + return if common == literal.len() { + child.insert(rest, route_index, routes) + } else { + child.insert_static(&literal[common..], rest, route_index, routes) + }; + } + + let mut child = Self { + prefix: literal.to_string(), + ..Self::default() + }; + child.insert(rest, route_index, routes)?; + self.static_children.push(child); + Ok(()) + } + + fn insert_param( + &mut self, + name: &str, + rest: &[PathPart], + route_index: usize, + routes: &[Route], + ) -> RouteNodeResult<()> { + if let Some(param_child) = &mut self.param_child { + if param_child.name != name { + return Err(RouteConflictError::ConflictingParamName { + existing: routes[param_child.origin_route].url(), + existing_name: param_child.name.clone(), + new: routes[route_index].url(), + new_name: name.to_string(), + }); + } + return param_child.node.insert(rest, route_index, routes); + } + + let mut node = RouteNode::default(); + node.insert(rest, route_index, routes)?; + self.param_child = Some(Box::new(ParamRouteNode { + name: name.to_string(), + node, + origin_route: route_index, + })); + Ok(()) + } + + fn insert_wildcard( + &mut self, + name: &str, + rest: &[PathPart], + route_index: usize, + routes: &[Route], + ) -> RouteNodeResult<()> { + debug_assert!( + rest.is_empty(), + "wildcard should always be the final segment" + ); + + if let Some(wildcard_child) = &self.wildcard_child { + return Err(if wildcard_child.name != name { + RouteConflictError::ConflictingWildcardName { + existing: routes[wildcard_child.route_index].url(), + existing_name: wildcard_child.name.clone(), + new: routes[route_index].url(), + new_name: name.to_string(), + } + } else { + RouteConflictError::DuplicateWildcard { + existing: routes[wildcard_child.route_index].url(), + new: routes[route_index].url(), + } + }); + } + + self.wildcard_child = Some(WildcardRouteNode { + name: name.to_string(), + route_index, + }); + Ok(()) + } + + fn insert_route(&mut self, route_index: usize, routes: &[Route]) -> RouteNodeResult<()> { + match routes[route_index].kind() { + RouteKind::Handler => { + if let Some(existing) = self.handler_route { + return Err(RouteConflictError::DuplicateHandler { + existing: routes[existing].url(), + new: routes[route_index].url(), + }); + } + self.handler_route = Some(route_index); + } + RouteKind::Router => { + if let Some(existing) = self.router_route { + return Err(RouteConflictError::DuplicateRouter { + existing: routes[existing].url(), + new: routes[route_index].url(), + }); + } + self.router_route = Some(route_index); + } + } + Ok(()) + } + + fn split_at(&mut self, index: usize) { + let child = Self { + prefix: self.prefix[index..].to_string(), + static_children: std::mem::take(&mut self.static_children), + param_child: self.param_child.take(), + wildcard_child: self.wildcard_child.take(), + handler_route: self.handler_route.take(), + router_route: self.router_route.take(), + }; + + self.prefix.truncate(index); + self.static_children.push(child); + } + + fn find<'a>( + &'a self, + router: &'a Router, + path: &str, + params: &mut Vec<(String, String)>, + ) -> Option> { + if !path.starts_with(&self.prefix) { + return None; + } + + let remaining_path = &path[self.prefix.len()..]; + if remaining_path.is_empty() + && let Some(found) = self.find_handler_route(router, params) + { + return Some(found); + } + + for child in &self.static_children { + let checkpoint = params.len(); + if let Some(found) = child.find(router, remaining_path, params) { + return Some(found); + } + params.truncate(checkpoint); + } + + if let Some(param_child) = &self.param_child { + let segment_end = remaining_path.find('/').unwrap_or(remaining_path.len()); + if segment_end > 0 { + let (value, path_after_param) = remaining_path.split_at(segment_end); + params.push((param_child.name.clone(), value.to_string())); + if let Some(found) = param_child.node.find(router, path_after_param, params) { + return Some(found); + } + params.pop(); + } + } + + if let Some(wildcard_child) = &self.wildcard_child + && !remaining_path.is_empty() + { + params.push((wildcard_child.name.clone(), remaining_path.to_string())); + if let Some(found) = + Self::route_to_handler(router, wildcard_child.route_index, "", params) + { + return Some(found); + } + params.pop(); + } + + if let Some(found) = self.find_router_route(router, remaining_path, params) { + return Some(found); + } + + None + } + + fn find_handler_route<'a>( + &'a self, + router: &'a Router, + params: &[(String, String)], + ) -> Option> { + let route_index = self.handler_route?; + Self::route_to_handler(router, route_index, "", params) + } + + fn find_router_route<'a>( + &'a self, + router: &'a Router, + remaining_path: &str, + params: &[(String, String)], + ) -> Option> { + let route_index = self.router_route?; + Self::route_to_handler(router, route_index, remaining_path, params) + } + + fn route_to_handler<'a>( + router: &'a Router, + route_index: usize, + remaining_path: &str, + params: &[(String, String)], + ) -> Option> { + let route = &router.urls[route_index]; + + match &route.view { + RouteInner::Handler(handler) => Some(HandlerFound { + handler: &**handler, + app_name: router.app_name.clone(), + name: route.name.clone(), + params: params.iter().rev().cloned().collect(), + }), + RouteInner::Router(nested_router) => { + nested_router.get_handler(remaining_path).map(|mut result| { + result.app_name = result.app_name.or_else(|| router.app_name.clone()); + result.params.extend(params.iter().rev().cloned()); + result + }) + } + #[cfg(feature = "openapi")] + RouteInner::ApiHandler(handler) => { + let handler: &(dyn BoxRequestHandler + Send + Sync) = &**handler; + Some(HandlerFound { + handler, + app_name: router.app_name.clone(), + name: route.name.clone(), + params: params.iter().rev().cloned().collect(), + }) + } + } + } +} + +#[derive(Debug, Clone)] +struct ParamRouteNode { + name: String, + node: RouteNode, + origin_route: usize, +} + +#[derive(Debug, Clone)] +struct WildcardRouteNode { + name: String, + route_index: usize, +} + +fn common_prefix_len(a: &str, b: &str) -> usize { + let mut common = 0; + for ((a_index, a_char), (b_index, b_char)) in a.char_indices().zip(b.char_indices()) { + if a_char != b_char { + break; + } + debug_assert_eq!(a_index, b_index); + common = a_index + a_char.len_utf8(); + } + common +} + #[derive(Debug)] struct HandlerFound<'a> { #[debug("handler(...)")] @@ -1194,6 +1541,274 @@ mod tests { assert_eq!(url, "/test/123"); } + #[test] + fn router_no_param_route_matches_exact_path() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users", + MockHandler, + "users", + )]); + + let found = router.get_handler("/users").unwrap(); + + assert_eq!(found.name, Some(RouteName("users".to_string()))); + assert!(found.params.is_empty()); + } + + #[test] + fn router_no_param_route_rejects_different_path() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users", + MockHandler, + "users", + )]); + + assert!(router.get_handler("/test").is_none()); + } + + #[test] + fn router_param_route_captures_single_segment() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/{id}", + MockHandler, + "user_detail", + )]); + + let found = router.get_handler("/users/123").unwrap(); + + assert_eq!(found.name, Some(RouteName("user_detail".to_string()))); + assert_params(found.params, &[("id", "123")]); + } + + #[test] + fn router_param_route_rejects_empty_segment() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/{id}", + MockHandler, + "user_detail", + )]); + + assert!(router.get_handler("/users/").is_none()); + } + + #[test] + fn router_param_route_rejects_extra_path_for_handler() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/{id}", + MockHandler, + "user_detail", + )]); + + assert!(router.get_handler("/users/123/abc").is_none()); + } + + #[test] + fn router_multiple_param_route_captures_all_params() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/{id}/posts/{post_id}", + MockHandler, + "post_detail", + )]); + + let found = router.get_handler("/users/123/posts/456").unwrap(); + + assert_eq!(found.name, Some(RouteName("post_detail".to_string()))); + assert_params(found.params, &[("id", "123"), ("post_id", "456")]); + } + + #[test] + fn router_escaped_literal_route_matches() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/{{{{{{escaped}}}}}}", + MockHandler, + "escaped", + )]); + + let found = router.get_handler("/users/{{{escaped}}}").unwrap(); + + assert_eq!(found.name, Some(RouteName("escaped".to_string()))); + assert!(found.params.is_empty()); + } + + #[test] + fn router_non_ascii_literal_route_matches() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/café/{id}", + MockHandler, + "cafe", + )]); + + let found = router.get_handler("/café/123").unwrap(); + + assert_eq!(found.name, Some(RouteName("cafe".to_string()))); + assert_params(found.params, &[("id", "123")]); + } + + #[test] + fn router_routes_with_common_static_prefixes_match_independently() { + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/car", MockHandler, "car"), + Route::with_handler_and_name("/cart", MockHandler, "cart"), + Route::with_handler_and_name("/catalog", MockHandler, "catalog"), + ]); + + assert_eq!( + router.get_handler("/car").unwrap().name, + Some(RouteName("car".to_string())) + ); + assert_eq!( + router.get_handler("/cart").unwrap().name, + Some(RouteName("cart".to_string())) + ); + assert_eq!( + router.get_handler("/catalog").unwrap().name, + Some(RouteName("catalog".to_string())) + ); + assert!(router.get_handler("/cartographer").is_none()); + } + + #[test] + fn router_static_route_takes_priority_over_dynamic_route() { + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/users/{id}", MockHandler, "dynamic"), + Route::with_handler_and_name("/users/new", MockHandler, "static"), + ]); + + let found = router.get_handler("/users/new").unwrap(); + + assert_eq!(found.name, Some(RouteName("static".to_string()))); + } + + #[test] + fn router_dynamic_route_takes_priority_over_wildcard_route() { + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/files/{name}", MockHandler, "dynamic"), + Route::with_handler_and_name("/files/{*path}", MockHandler, "wildcard"), + ]); + + let found = router.get_handler("/files/readme").unwrap(); + + assert_eq!(found.name, Some(RouteName("dynamic".to_string()))); + assert_params(found.params, &[("name", "readme")]); + } + + #[test] + fn router_wildcard_route_captures_remaining_path() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/static/{*path}", + MockHandler, + "static_asset", + )]); + + let found = router.get_handler("/static/css/app.css").unwrap(); + + assert_eq!(found.name, Some(RouteName("static_asset".to_string()))); + assert_eq!( + found.params, + vec![("path".to_string(), "css/app.css".to_string())] + ); + } + + #[test] + fn router_wildcard_route_rejects_empty_remaining_path() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/static/{*path}", + MockHandler, + "static_asset", + )]); + + assert!(router.get_handler("/static/").is_none()); + } + + #[test] + fn router_wildcard_route_is_lower_priority_than_static_route() { + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/static/{*path}", MockHandler, "wildcard"), + Route::with_handler_and_name("/static/index.html", MockHandler, "static"), + ]); + + let found = router.get_handler("/static/index.html").unwrap(); + + assert_eq!(found.name, Some(RouteName("static".to_string()))); + } + + #[test] + fn router_nested_router_consumes_remaining_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/posts/{post_id}", + MockHandler, + "post_detail", + )]); + let router = Router::with_urls(vec![Route::with_router("/users/{id}", sub_router)]); + + let found = router.get_handler("/users/123/posts/456").unwrap(); + + assert_eq!(found.name, Some(RouteName("post_detail".to_string()))); + assert_params(found.params, &[("id", "123"), ("post_id", "456")]); + } + + #[test] + fn router_handler_takes_priority_over_nested_router_at_same_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "nested", + )]); + let router = Router::with_urls(vec![ + Route::with_router("/users", sub_router), + Route::with_handler_and_name("/users", MockHandler, "handler"), + ]); + + let found = router.get_handler("/users").unwrap(); + + assert_eq!(found.name, Some(RouteName("handler".to_string()))); + } + + #[test] + #[should_panic(expected = "Duplicate handler route at the same path")] + fn router_duplicate_handler_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/users", MockHandler), + Route::with_handler("/users", MockHandler), + ]); + } + + #[test] + #[should_panic(expected = "Duplicate nested router route at the same path")] + fn router_duplicate_nested_router_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_router("/users", Router::empty()), + Route::with_router("/users", Router::empty()), + ]); + } + + #[test] + #[should_panic(expected = "Conflicting route parameters")] + fn router_conflicting_param_names_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/foo/{bar}/", MockHandler), + Route::with_handler("/foo/{baz}", MockHandler), + ]); + } + + #[test] + #[should_panic(expected = "Duplicate wildcard route")] + fn router_duplicate_wildcard_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{*path}", MockHandler), + Route::with_handler("/static/{*path}", MockHandler), + ]); + } + + #[test] + #[should_panic(expected = "Conflicting wildcard route parameters")] + fn router_conflicting_wildcard_names_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{*path}", MockHandler), + Route::with_handler("/static/{*file_path}", MockHandler), + ]); + } + #[test] fn router_reverse_app_name() { let route = Route::with_handler_and_name("/test", MockHandler, "test"); @@ -1306,4 +1921,14 @@ mod tests { fn test_request() -> Request { TestRequestBuilder::get("/test").build() } + + fn assert_params(mut actual: Vec<(String, String)>, expected: &[(&str, &str)]) { + let mut expected = expected + .iter() + .map(|(key, value)| ((*key).to_string(), (*value).to_string())) + .collect::>(); + actual.sort(); + expected.sort(); + assert_eq!(actual, expected); + } } diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index cd250799..b25ec835 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -9,7 +9,56 @@ use std::fmt::Display; use cot_core::error::impl_into_cot_error; use thiserror::Error; -use tracing::debug; + +const PATH_MATCHER_ERROR_PREFIX: &str = "route conflict error:"; +/// An error produced when parsing a route path pattern fails. +#[derive(Debug, Error)] +#[non_exhaustive] +pub(super) enum PathMatcherError { + /// Two parameters appear consecutively with no literal text between them, + #[error("{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})")] + #[non_exhaustive] + ConsecutiveParams { pattern: String, position: usize }, + + /// A `{` was opened but never closed with a matching `}`. + #[error("{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`")] + #[non_exhaustive] + UnclosedParam { pattern: String, name: String }, + + /// A `}` appeared without a preceding `{` to open it. + #[error( + "{PATH_MATCHER_ERROR_PREFIX} closing brace `}}` without a matching opening `{{` in pattern `{pattern}` \ + (at position {position})" + )] + #[non_exhaustive] + UnmatchedClosingBrace { pattern: String, position: usize }, + + /// A parameter name is empty or contains characters other than + /// alphanumerics/underscore, or starts with a digit. + #[error( + "{PATH_MATCHER_ERROR_PREFIX} invalid parameter name `{name}` in pattern `{pattern}` -- parameter names must start \ + with a letter or underscore and contain only letters, digits, or underscores" + )] + #[non_exhaustive] + InvalidParamName { pattern: String, name: String }, + + /// Same as `InvalidParamName`, but for the name following a `*` in a wildcard segment. + #[error( + "{PATH_MATCHER_ERROR_PREFIX} invalid wildcard name `{name}` in pattern `{pattern}` -- wildcard names must start \ + with a letter or underscore and contain only letters, digits, or underscores" + )] + #[non_exhaustive] + InvalidWildcardName { pattern: String, name: String }, + + /// A wildcard segment (`{*name}`) was followed by more path segments, + #[error( + "{PATH_MATCHER_ERROR_PREFIX} wildcard parameter `{{*{name}}}` must be the last segment of pattern `{pattern}` -- \ + a wildcard consumes the rest of the path, so nothing can follow it" + )] + #[non_exhaustive] + WildcardNotAtEnd { pattern: String, name: String }, +} +impl_into_cot_error!(PathMatcherError); #[derive(Debug, Clone)] pub(super) struct PathMatcher { @@ -19,6 +68,13 @@ pub(super) struct PathMatcher { impl PathMatcher { #[must_use] pub(crate) fn new>(path_pattern: T) -> Self { + match Self::try_new(path_pattern) { + Ok(matcher) => matcher, + Err(err) => panic!("{err}"), + } + } + + pub(crate) fn try_new>(path_pattern: T) -> Result { #[derive(Debug, Copy, Clone)] enum State { Literal { start: usize }, @@ -43,10 +99,12 @@ impl PathMatcher { (Some('{') | None, State::Literal { start }) => { let literal = &path_pattern[start..index]; if literal.is_empty() { - assert!( - index == 0 || ch.is_none(), - "Consecutive parameters are not allowed" - ); + if index != 0 && ch.is_some() { + return Err(PathMatcherError::ConsecutiveParams { + pattern: path_pattern.clone(), + position: index, + }); + } } else { parts.push(PathPart::Literal(literal.to_string())); } @@ -57,7 +115,10 @@ impl PathMatcher { // escaped `{` state = State::Literal { start: index }; } else { - panic!("Unclosed parameter: `{}`", &path_pattern[start..index]); + return Err(PathMatcherError::UnclosedParam { + pattern: path_pattern.clone(), + name: path_pattern[start..index].to_string(), + }); } } (Some('}'), State::Literal { start }) => { @@ -71,29 +132,56 @@ impl PathMatcher { char_iter.next(); state = State::Literal { start: index + 2 }; } else { - panic!("Closing brace encountered without opening brace"); + return Err(PathMatcherError::UnmatchedClosingBrace { + pattern: path_pattern.clone(), + position: index, + }); } } (Some('}'), State::Param { start }) => { - let param_name = &path_pattern[start..index].trim(); - assert!( - Self::is_param_name_valid(param_name), - "Invalid parameter name: `{param_name}`" - ); - - parts.push(PathPart::Param { - name: (*param_name).to_string(), - }); + let param_name = path_pattern[start..index].trim(); + if let Some(wildcard_name) = param_name.strip_prefix('*') { + if !Self::is_param_name_valid(wildcard_name) { + return Err(PathMatcherError::InvalidWildcardName { + pattern: path_pattern.clone(), + name: wildcard_name.to_string(), + }); + } + if char_iter.peek().is_some_and(|(_, next_char)| next_char.is_some()) { + return Err(PathMatcherError::WildcardNotAtEnd { + pattern: path_pattern.clone(), + name: wildcard_name.to_string(), + }); + } + + parts.push(PathPart::Wildcard { + name: wildcard_name.to_string(), + }); + } else { + if !Self::is_param_name_valid(param_name) { + return Err(PathMatcherError::InvalidParamName { + pattern: path_pattern.clone(), + name: param_name.to_string(), + }); + } + + parts.push(PathPart::Param { + name: param_name.to_string(), + }); + } state = State::Literal { start: index + 1 }; } (Some('/') | None, State::Param { start }) => { - panic!("Unclosed parameter: `{}`", &path_pattern[start..index]); + return Err(PathMatcherError::UnclosedParam { + pattern: path_pattern.clone(), + name: path_pattern[start..index].to_string(), + }); } _ => {} } } - Self { parts } + Ok(Self { parts }) } fn is_param_name_valid(name: &str) -> bool { @@ -112,49 +200,13 @@ impl PathMatcher { true } - #[must_use] - pub(crate) fn capture<'matcher, 'path>( - &'matcher self, - path: &'path str, - ) -> Option> { - debug!("Matching path `{}` against pattern `{}`", path, self); - - let mut current_path = path; - let mut params = Vec::with_capacity(self.param_len()); - for part in &self.parts { - match part { - PathPart::Literal(s) => { - if !current_path.starts_with(s) { - return None; - } - current_path = ¤t_path[s.len()..]; - } - PathPart::Param { name } => { - let next_slash = current_path.find('/'); - let value = if let Some(next_slash) = next_slash { - ¤t_path[..next_slash] - } else { - current_path - }; - if value.is_empty() { - return None; - } - params.push(PathParam::new(name, value)); - current_path = ¤t_path[value.len()..]; - } - } - } - - Some(CaptureResult::new(params, current_path)) - } - pub(crate) fn reverse(&self, params: &ReverseParamMap) -> Result { let mut result = String::new(); for part in &self.parts { match part { PathPart::Literal(s) => result.push_str(s), - PathPart::Param { name } => { + PathPart::Param { name } | PathPart::Wildcard { name } => { let value = params .get(name) .ok_or_else(|| ReverseError::MissingParam(name.clone()))?; @@ -166,17 +218,17 @@ impl PathMatcher { Ok(result) } - #[must_use] - fn param_len(&self) -> usize { - self.param_names().count() - } - + #[allow(dead_code, reason = "used by OpenAPI route generation")] pub(super) fn param_names(&self) -> impl Iterator { self.parts.iter().filter_map(|part| match part { PathPart::Literal(..) => None, - PathPart::Param { name } => Some(name.as_str()), + PathPart::Param { name } | PathPart::Wildcard { name } => Some(name.as_str()), }) } + + pub(super) fn parts(&self) -> &[PathPart] { + &self.parts + } } impl Display for PathMatcher { @@ -265,43 +317,23 @@ macro_rules! reverse_param_map { }}; } -const ERROR_PREFIX: &str = "failed to reverse route:"; +const REVERSE_ERROR_PREFIX: &str = "failed to reverse route:"; /// An error that occurs when reversing a path with missing parameters. #[derive(Debug, Error)] #[non_exhaustive] pub enum ReverseError { /// A parameter is missing for the reverse operation. - #[error("{ERROR_PREFIX} missing parameter for reverse: `{0}`")] + #[error("{REVERSE_ERROR_PREFIX} missing parameter for reverse: `{0}`")] #[non_exhaustive] MissingParam(String), } impl_into_cot_error!(ReverseError); -#[derive(Debug, PartialEq, Eq)] -pub(super) struct CaptureResult<'matcher, 'path> { - pub(super) params: Vec>, - pub(super) remaining_path: &'path str, -} - -impl<'matcher, 'path> CaptureResult<'matcher, 'path> { - #[must_use] - fn new(params: Vec>, remaining_path: &'path str) -> Self { - Self { - params, - remaining_path, - } - } - - #[must_use] - pub(crate) fn matches_fully(&self) -> bool { - self.remaining_path.is_empty() - } -} - #[derive(Debug, Clone)] -enum PathPart { +pub(super) enum PathPart { Literal(String), Param { name: String }, + Wildcard { name: String }, } impl Display for PathPart { @@ -312,22 +344,7 @@ impl Display for PathPart { write!(f, "{s}") } PathPart::Param { name } => write!(f, "{{{name}}}"), - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq)] -pub(super) struct PathParam<'a> { - pub(super) name: &'a str, - pub(super) value: String, -} - -impl<'a> PathParam<'a> { - #[must_use] - pub(crate) fn new(name: &'a str, value: &str) -> Self { - Self { - name, - value: value.to_string(), + PathPart::Wildcard { name } => write!(f, "{{*{name}}}"), } } } @@ -345,11 +362,11 @@ mod tests { #[test] fn path_parser_no_params() { let path_parser = PathMatcher::new("/users"); + assert_eq!(path_parser.to_string(), "/users"); assert_eq!( - path_parser.capture("/users"), - Some(CaptureResult::new(vec![], "")) + path_parser.param_names().collect::>(), + Vec::<&str>::new() ); - assert_eq!(path_parser.capture("/test"), None); } #[test] @@ -358,79 +375,61 @@ mod tests { let mut params = ReverseParamMap::new(); params.insert("id", "123"); - assert_eq!( - path_parser.capture("/users/123"), - Some(CaptureResult::new(vec![PathParam::new("id", "123")], "")) - ); assert_eq!(path_parser.reverse(¶ms).unwrap(), "/users/123"); assert_eq!(path_parser.to_string(), "/users/{id}"); + assert_eq!(path_parser.param_names().collect::>(), vec!["id"]); } #[test] fn path_parser_escaped() { let path_parser = PathMatcher::new("/users/{{{{{{escaped}}}}}}"); + assert_eq!(path_parser.to_string(), "/users/{{{{{{escaped}}}}}}"); assert_eq!( - path_parser.capture("/users/{{{escaped}}}"), - Some(CaptureResult::new(vec![], "")) + path_parser.reverse(&ReverseParamMap::new()).unwrap(), + "/users/{{{escaped}}}" ); } #[test] fn path_parser_single_param() { let path_parser = PathMatcher::new("/users/{id}"); - assert_eq!( - path_parser.capture("/users/123"), - Some(CaptureResult::new(vec![PathParam::new("id", "123")], "")) - ); - assert_eq!( - path_parser.capture("/users/123/"), - Some(CaptureResult::new(vec![PathParam::new("id", "123")], "/")) - ); - assert_eq!( - path_parser.capture("/users/123/abc"), - Some(CaptureResult::new( - vec![PathParam::new("id", "123")], - "/abc" - )) - ); - assert_eq!(path_parser.capture("/users/"), None); + assert_eq!(path_parser.to_string(), "/users/{id}"); + assert_eq!(path_parser.param_names().collect::>(), vec!["id"]); } #[test] fn path_parser_param_whitespace() { let path_parser = PathMatcher::new("/users/{ id }"); - assert_eq!( - path_parser.capture("/users/123"), - Some(CaptureResult::new(vec![PathParam::new("id", "123")], "")) - ); + assert_eq!(path_parser.to_string(), "/users/{id}"); + assert_eq!(path_parser.param_names().collect::>(), vec!["id"]); } #[test] fn path_parser_multiple_params() { let path_parser = PathMatcher::new("/users/{id}/posts/{post_id}"); assert_eq!( - path_parser.capture("/users/123/posts/456"), - Some(CaptureResult::new( - vec![ - PathParam::new("id", "123"), - PathParam::new("post_id", "456"), - ], - "" - )) - ); - assert_eq!( - path_parser.capture("/users/123/posts/456/abc"), - Some(CaptureResult::new( - vec![ - PathParam::new("id", "123"), - PathParam::new("post_id", "456"), - ], - "/abc" - )) + path_parser.param_names().collect::>(), + vec!["id", "post_id"] ); } + #[test] + fn path_parser_wildcard() { + let path_parser = PathMatcher::new("/static/{*path}"); + assert_eq!(path_parser.to_string(), "/static/{*path}"); + assert_eq!(path_parser.param_names().collect::>(), vec!["path"]); + } + + #[test] + fn reverse_with_wildcard() { + let path_parser = PathMatcher::new("/static/{*path}"); + let mut params = ReverseParamMap::new(); + params.insert("path", "css/app.css"); + + assert_eq!(path_parser.reverse(¶ms).unwrap(), "/static/css/app.css"); + } + #[test] #[should_panic(expected = "Consecutive parameters are not allowed")] fn path_parser_consecutive_params() { @@ -455,6 +454,18 @@ mod tests { let _ = PathMatcher::new("/users/{abc#$%}"); } + #[test] + #[should_panic(expected = "Invalid wildcard name: ``")] + fn path_parser_invalid_wildcard_name_empty() { + let _ = PathMatcher::new("/users/{*}"); + } + + #[test] + #[should_panic(expected = "Wildcard parameters are only allowed at the end of a route")] + fn path_parser_wildcard_not_at_end() { + let _ = PathMatcher::new("/users/{*path}/edit"); + } + #[test] #[should_panic(expected = "Unclosed parameter: `foo`")] fn path_parser_unclosed() { From 0f587766b5538600b7ddaaeecf2371ede4ece577 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:27:59 +0000 Subject: [PATCH 02/16] chore(pre-commit.ci): auto fixes from pre-commit hooks --- cot/src/router/path.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index b25ec835..e00ddd13 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -16,12 +16,16 @@ const PATH_MATCHER_ERROR_PREFIX: &str = "route conflict error:"; #[non_exhaustive] pub(super) enum PathMatcherError { /// Two parameters appear consecutively with no literal text between them, - #[error("{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})")] + #[error( + "{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})" + )] #[non_exhaustive] ConsecutiveParams { pattern: String, position: usize }, /// A `{` was opened but never closed with a matching `}`. - #[error("{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`")] + #[error( + "{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`" + )] #[non_exhaustive] UnclosedParam { pattern: String, name: String }, @@ -147,7 +151,10 @@ impl PathMatcher { name: wildcard_name.to_string(), }); } - if char_iter.peek().is_some_and(|(_, next_char)| next_char.is_some()) { + if char_iter + .peek() + .is_some_and(|(_, next_char)| next_char.is_some()) + { return Err(PathMatcherError::WildcardNotAtEnd { pattern: path_pattern.clone(), name: wildcard_name.to_string(), From 6af73daf3aea76672674c25615b820fc52a61ef5 Mon Sep 17 00:00:00 2001 From: Elijah Date: Sat, 1 Aug 2026 15:39:09 +0000 Subject: [PATCH 03/16] common_prefix_len can use bytes since in practice routes are ascii based --- cot/src/router.rs | 14 +++----------- cot/src/router/path.rs | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index 25c1dcab..ce8b1c4c 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -538,7 +538,7 @@ impl RouteNode { } for child in &mut self.static_children { - let common = common_prefix_len(&child.prefix, literal); + let common = common_prefix_len(&child.prefix.as_bytes(), literal.as_bytes()); if common == 0 { continue; } @@ -787,16 +787,8 @@ struct WildcardRouteNode { route_index: usize, } -fn common_prefix_len(a: &str, b: &str) -> usize { - let mut common = 0; - for ((a_index, a_char), (b_index, b_char)) in a.char_indices().zip(b.char_indices()) { - if a_char != b_char { - break; - } - debug_assert_eq!(a_index, b_index); - common = a_index + a_char.len_utf8(); - } - common +fn common_prefix_len(a: &[u8], b: &[u8]) -> usize { + a.iter().zip(b).take_while(|(a, b)| a == b).count() } #[derive(Debug)] diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index b25ec835..14430a53 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -16,12 +16,16 @@ const PATH_MATCHER_ERROR_PREFIX: &str = "route conflict error:"; #[non_exhaustive] pub(super) enum PathMatcherError { /// Two parameters appear consecutively with no literal text between them, - #[error("{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})")] + #[error( + "{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})" + )] #[non_exhaustive] ConsecutiveParams { pattern: String, position: usize }, /// A `{` was opened but never closed with a matching `}`. - #[error("{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`")] + #[error( + "{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`" + )] #[non_exhaustive] UnclosedParam { pattern: String, name: String }, @@ -42,7 +46,8 @@ pub(super) enum PathMatcherError { #[non_exhaustive] InvalidParamName { pattern: String, name: String }, - /// Same as `InvalidParamName`, but for the name following a `*` in a wildcard segment. + /// Same as `InvalidParamName`, but for the name following a `*` in a + /// wildcard segment. #[error( "{PATH_MATCHER_ERROR_PREFIX} invalid wildcard name `{name}` in pattern `{pattern}` -- wildcard names must start \ with a letter or underscore and contain only letters, digits, or underscores" @@ -147,7 +152,10 @@ impl PathMatcher { name: wildcard_name.to_string(), }); } - if char_iter.peek().is_some_and(|(_, next_char)| next_char.is_some()) { + if char_iter + .peek() + .is_some_and(|(_, next_char)| next_char.is_some()) + { return Err(PathMatcherError::WildcardNotAtEnd { pattern: path_pattern.clone(), name: wildcard_name.to_string(), From 6b56ed1355ce4ebc89bd7d452c6eae04b662d0f3 Mon Sep 17 00:00:00 2001 From: Elijah Date: Mon, 17 Aug 2026 14:43:54 +0000 Subject: [PATCH 04/16] Use Matchit crate --- Cargo.lock | 19 +- Cargo.toml | 1 + cot/Cargo.toml | 1 + cot/src/error_page.rs | 8 +- cot/src/router.rs | 410 ++++++++--------------------------------- cot/src/router/path.rs | 35 +++- cot/src/router/tree.rs | 194 +++++++++++++++++++ 7 files changed, 326 insertions(+), 342 deletions(-) create mode 100644 cot/src/router/tree.rs diff --git a/Cargo.lock b/Cargo.lock index 9134f060..d86d2b83 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -450,7 +450,7 @@ dependencies = [ "hyper", "hyper-util", "itoa", - "matchit", + "matchit 0.8.4", "memchr", "mime", "percent-encoding", @@ -939,6 +939,7 @@ dependencies = [ "indexmap", "is_terminal_polyfill", "lettre", + "matchit 0.9.2", "mime", "mime_guess", "mockall", @@ -1522,7 +1523,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2662,6 +2663,12 @@ version = "0.8.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "47e1ffaa40ddd1f3ed91f717a33c8c0ee23fff369e3aa8772b9605cc1d22f4c3" +[[package]] +name = "matchit" +version = "0.9.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8863b587001c1b9a8a4e36008cebc6b3612cb1226fe2de94858e06092687b608" + [[package]] name = "md-5" version = "0.11.0" @@ -3489,7 +3496,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -3546,7 +3553,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -4177,7 +4184,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -4966,7 +4973,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index a7f41dde..b2bc6f31 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -115,6 +115,7 @@ insta-cmd = "0.7" is_terminal_polyfill = "1.70" lettre = { version = "0.11.22", default-features = false } libtest-mimic = "0.8" +matchit = "0.9.2" mime = "0.3" mime_guess = { version = "2", default-features = false } mockall = "0.15" diff --git a/cot/Cargo.toml b/cot/Cargo.toml index 8f50d158..9b29af38 100644 --- a/cot/Cargo.toml +++ b/cot/Cargo.toml @@ -43,6 +43,7 @@ idna = { workspace = true, optional = true } indexmap.workspace = true is_terminal_polyfill.workspace = true lettre = { workspace = true, features = ["builder", "sendmail-transport", "smtp-transport", "tokio1", "tokio1-rustls", "ring", "rustls-platform-verifier"], optional = true } +matchit.workspace = true mime.workspace = true mime_guess.workspace = true multer.workspace = true diff --git a/cot/src/error_page.rs b/cot/src/error_page.rs index 16e2eec4..03e3f6e8 100644 --- a/cot/src/error_page.rs +++ b/cot/src/error_page.rs @@ -133,7 +133,7 @@ impl ErrorPageTemplateBuilder { fn build_route_data( route_data: &mut Vec, - router: &Router, + router: &Arc, url_prefix: &str, index_prefix: &str, ) { @@ -156,7 +156,7 @@ impl ErrorPageTemplateBuilder { if let Some(inner_router) = route.router() { Self::build_route_data( route_data, - inner_router, + &inner_router, &format!("{}{}", url_prefix, route.url()), &format!("{index_prefix}{index}."), ); @@ -588,7 +588,9 @@ mod tests { let mut route_data = Vec::new(); let sub_sub_router = Router::with_urls(vec![]); let sub_router = Router::with_urls(vec![Route::with_router("/bar", sub_sub_router)]); - let router = Router::with_urls(vec![Route::with_router("/foo", sub_router)]); + let router = Arc::new(Router::with_urls(vec![Route::with_router( + "/foo", sub_router, + )])); ErrorPageTemplateBuilder::build_route_data(&mut route_data, &router, "", ""); diff --git a/cot/src/router.rs b/cot/src/router.rs index ce8b1c4c..d2156b0d 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -36,11 +36,13 @@ use tracing::debug; use crate::error::NotFound; use crate::request::{PathParams, Request, RequestExt, RequestHead}; use crate::response::Response; -use crate::router::path::{PathMatcher, PathPart, ReverseParamMap}; +use crate::router::path::{PathMatcher, ReverseParamMap}; +use crate::router::tree::{Entry, RouteTrie}; use crate::{Error, ProjectContext, Result}; pub mod method; pub mod path; +mod tree; /// A router that can be used to route requests to their respective views. /// @@ -66,7 +68,7 @@ pub struct Router { app_name: Option, urls: Vec, names: HashMap>, - route_tree: RouteTree, + route_tree: RouteTrie, } impl Router { @@ -101,6 +103,10 @@ impl Router { /// /// let router = Router::with_urls([Route::with_handler_and_name("/", home, "home")]); /// ``` + /// + /// # Panics + /// + /// Panics when a url string could not be parsed into a [`Route`] #[must_use] pub fn with_urls>>(urls: T) -> Self { match Self::try_with_urls(urls) { @@ -110,7 +116,7 @@ impl Router { } /// Create a router with the given routes. This is a fallible version - /// of [Self::with_urls] + /// of [`Self::with_urls`] /// /// # Examples /// @@ -125,6 +131,10 @@ impl Router { /// /// let router = Router::try_with_urls([Route::with_handler_and_name("/", home, "home")]).unwrap(); /// ``` + /// + /// # Errors + /// + /// This method fails when the underlying trie fails to build. pub fn try_with_urls>>(urls: T) -> Result { let urls = urls.into(); let mut names = HashMap::new(); @@ -134,7 +144,7 @@ impl Router { names.insert(name.clone(), url.url.clone()); } } - let route_tree = RouteTree::from_routes(&urls)?; + let route_tree = RouteTrie::build(&urls)?; Ok(Self { app_name: None, urls, @@ -170,7 +180,66 @@ impl Router { } fn get_handler(&self, request_path: &str) -> Option> { - self.route_tree.find(self, request_path) + let m = self.route_tree.at(request_path)?; + + let (route_index, remaining_path) = match m.value { + // For `Entry::Combined` (cases where a handler overlaps a router for the same + // route/path, the handler takes precedence. + Entry::Handler(idx) | Entry::Combined { handler: idx, .. } => (*idx, String::new()), + Entry::Router(idx) => { + let rest = m.params.get(tree::NESTED_ROUTER_PARAM).unwrap_or(""); + let remaining = if rest.is_empty() { + String::new() + } else { + format!("/{rest}") + }; + (*idx, remaining) + } + }; + + let params: Vec<(String, String)> = m + .params + .iter() + .filter(|(key, _)| *key != tree::NESTED_ROUTER_PARAM) + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect(); + + Self::route_to_handler(self, route_index, &remaining_path, ¶ms) + } + + fn route_to_handler<'a>( + router: &'a Router, + route_index: usize, + remaining_path: &str, + params: &[(String, String)], + ) -> Option> { + let route = &router.urls[route_index]; + + match &route.view { + RouteInner::Handler(handler) => Some(HandlerFound { + handler: &**handler, + app_name: router.app_name.clone(), + name: route.name.clone(), + params: params.to_vec(), + }), + RouteInner::Router(nested_router) => { + nested_router.get_handler(remaining_path).map(|mut found| { + found.app_name = found.app_name.or_else(|| router.app_name.clone()); + found.params.extend(params.iter().cloned()); + found + }) + } + #[cfg(feature = "openapi")] + RouteInner::ApiHandler(handler) => { + let handler: &(dyn BoxRequestHandler + Send + Sync) = &**handler; + Some(HandlerFound { + handler, + app_name: router.app_name.clone(), + name: route.name.clone(), + params: params.to_vec(), + }) + } + } } /// Handle a request. @@ -424,7 +493,6 @@ struct NoViewToReverse { } impl_into_cot_error!(NoViewToReverse); -type RouteNodeResult = std::result::Result; const ERROR_PREFIX: &str = "route conflict error:"; #[derive(Debug, thiserror::Error)] enum RouteConflictError { @@ -468,329 +536,11 @@ enum RouteConflictError { wildcard route `{existing}`" )] DuplicateWildcard { existing: String, new: String }, + #[error("{ERROR_PREFIX} error while inserting route")] + RouteInsert(#[from] matchit::InsertError), } impl_into_cot_error!(RouteConflictError); -#[derive(Debug, Clone)] -struct RouteTree { - root: RouteNode, -} - -impl RouteTree { - fn from_routes(routes: &[Route]) -> Result { - let mut tree = Self { - root: RouteNode::default(), - }; - for (index, route) in routes.iter().enumerate() { - tree.root.insert(route.url.parts(), index, routes)?; - } - - Ok(tree) - } - - fn find<'a>(&'a self, router: &'a Router, path: &str) -> Option> { - let mut params = Vec::new(); - self.root.find(router, path, &mut params) - } -} - -#[derive(Debug, Clone, Default)] -struct RouteNode { - prefix: String, - static_children: Vec, - param_child: Option>, - wildcard_child: Option, - handler_route: Option, - router_route: Option, -} - -impl RouteNode { - fn insert( - &mut self, - parts: &[PathPart], - route_index: usize, - routes: &[Route], - ) -> RouteNodeResult<()> { - if let Some((part, rest)) = parts.split_first() { - match part { - PathPart::Literal(literal) => { - self.insert_static(literal, rest, route_index, routes) - } - PathPart::Param { name } => self.insert_param(name, rest, route_index, routes), - PathPart::Wildcard { name } => { - self.insert_wildcard(name, rest, route_index, routes) - } - } - } else { - self.insert_route(route_index, routes) - } - } - - fn insert_static( - &mut self, - literal: &str, - rest: &[PathPart], - route_index: usize, - routes: &[Route], - ) -> RouteNodeResult<()> { - if literal.is_empty() { - return self.insert(rest, route_index, routes); - } - - for child in &mut self.static_children { - let common = common_prefix_len(&child.prefix.as_bytes(), literal.as_bytes()); - if common == 0 { - continue; - } - if common < child.prefix.len() { - child.split_at(common); - } - return if common == literal.len() { - child.insert(rest, route_index, routes) - } else { - child.insert_static(&literal[common..], rest, route_index, routes) - }; - } - - let mut child = Self { - prefix: literal.to_string(), - ..Self::default() - }; - child.insert(rest, route_index, routes)?; - self.static_children.push(child); - Ok(()) - } - - fn insert_param( - &mut self, - name: &str, - rest: &[PathPart], - route_index: usize, - routes: &[Route], - ) -> RouteNodeResult<()> { - if let Some(param_child) = &mut self.param_child { - if param_child.name != name { - return Err(RouteConflictError::ConflictingParamName { - existing: routes[param_child.origin_route].url(), - existing_name: param_child.name.clone(), - new: routes[route_index].url(), - new_name: name.to_string(), - }); - } - return param_child.node.insert(rest, route_index, routes); - } - - let mut node = RouteNode::default(); - node.insert(rest, route_index, routes)?; - self.param_child = Some(Box::new(ParamRouteNode { - name: name.to_string(), - node, - origin_route: route_index, - })); - Ok(()) - } - - fn insert_wildcard( - &mut self, - name: &str, - rest: &[PathPart], - route_index: usize, - routes: &[Route], - ) -> RouteNodeResult<()> { - debug_assert!( - rest.is_empty(), - "wildcard should always be the final segment" - ); - - if let Some(wildcard_child) = &self.wildcard_child { - return Err(if wildcard_child.name != name { - RouteConflictError::ConflictingWildcardName { - existing: routes[wildcard_child.route_index].url(), - existing_name: wildcard_child.name.clone(), - new: routes[route_index].url(), - new_name: name.to_string(), - } - } else { - RouteConflictError::DuplicateWildcard { - existing: routes[wildcard_child.route_index].url(), - new: routes[route_index].url(), - } - }); - } - - self.wildcard_child = Some(WildcardRouteNode { - name: name.to_string(), - route_index, - }); - Ok(()) - } - - fn insert_route(&mut self, route_index: usize, routes: &[Route]) -> RouteNodeResult<()> { - match routes[route_index].kind() { - RouteKind::Handler => { - if let Some(existing) = self.handler_route { - return Err(RouteConflictError::DuplicateHandler { - existing: routes[existing].url(), - new: routes[route_index].url(), - }); - } - self.handler_route = Some(route_index); - } - RouteKind::Router => { - if let Some(existing) = self.router_route { - return Err(RouteConflictError::DuplicateRouter { - existing: routes[existing].url(), - new: routes[route_index].url(), - }); - } - self.router_route = Some(route_index); - } - } - Ok(()) - } - - fn split_at(&mut self, index: usize) { - let child = Self { - prefix: self.prefix[index..].to_string(), - static_children: std::mem::take(&mut self.static_children), - param_child: self.param_child.take(), - wildcard_child: self.wildcard_child.take(), - handler_route: self.handler_route.take(), - router_route: self.router_route.take(), - }; - - self.prefix.truncate(index); - self.static_children.push(child); - } - - fn find<'a>( - &'a self, - router: &'a Router, - path: &str, - params: &mut Vec<(String, String)>, - ) -> Option> { - if !path.starts_with(&self.prefix) { - return None; - } - - let remaining_path = &path[self.prefix.len()..]; - if remaining_path.is_empty() - && let Some(found) = self.find_handler_route(router, params) - { - return Some(found); - } - - for child in &self.static_children { - let checkpoint = params.len(); - if let Some(found) = child.find(router, remaining_path, params) { - return Some(found); - } - params.truncate(checkpoint); - } - - if let Some(param_child) = &self.param_child { - let segment_end = remaining_path.find('/').unwrap_or(remaining_path.len()); - if segment_end > 0 { - let (value, path_after_param) = remaining_path.split_at(segment_end); - params.push((param_child.name.clone(), value.to_string())); - if let Some(found) = param_child.node.find(router, path_after_param, params) { - return Some(found); - } - params.pop(); - } - } - - if let Some(wildcard_child) = &self.wildcard_child - && !remaining_path.is_empty() - { - params.push((wildcard_child.name.clone(), remaining_path.to_string())); - if let Some(found) = - Self::route_to_handler(router, wildcard_child.route_index, "", params) - { - return Some(found); - } - params.pop(); - } - - if let Some(found) = self.find_router_route(router, remaining_path, params) { - return Some(found); - } - - None - } - - fn find_handler_route<'a>( - &'a self, - router: &'a Router, - params: &[(String, String)], - ) -> Option> { - let route_index = self.handler_route?; - Self::route_to_handler(router, route_index, "", params) - } - - fn find_router_route<'a>( - &'a self, - router: &'a Router, - remaining_path: &str, - params: &[(String, String)], - ) -> Option> { - let route_index = self.router_route?; - Self::route_to_handler(router, route_index, remaining_path, params) - } - - fn route_to_handler<'a>( - router: &'a Router, - route_index: usize, - remaining_path: &str, - params: &[(String, String)], - ) -> Option> { - let route = &router.urls[route_index]; - - match &route.view { - RouteInner::Handler(handler) => Some(HandlerFound { - handler: &**handler, - app_name: router.app_name.clone(), - name: route.name.clone(), - params: params.iter().rev().cloned().collect(), - }), - RouteInner::Router(nested_router) => { - nested_router.get_handler(remaining_path).map(|mut result| { - result.app_name = result.app_name.or_else(|| router.app_name.clone()); - result.params.extend(params.iter().rev().cloned()); - result - }) - } - #[cfg(feature = "openapi")] - RouteInner::ApiHandler(handler) => { - let handler: &(dyn BoxRequestHandler + Send + Sync) = &**handler; - Some(HandlerFound { - handler, - app_name: router.app_name.clone(), - name: route.name.clone(), - params: params.iter().rev().cloned().collect(), - }) - } - } - } -} - -#[derive(Debug, Clone)] -struct ParamRouteNode { - name: String, - node: RouteNode, - origin_route: usize, -} - -#[derive(Debug, Clone)] -struct WildcardRouteNode { - name: String, - route_index: usize, -} - -fn common_prefix_len(a: &[u8], b: &[u8]) -> usize { - a.iter().zip(b).take_while(|(a, b)| a == b).count() -} - #[derive(Debug)] struct HandlerFound<'a> { #[debug("handler(...)")] @@ -1046,7 +796,7 @@ impl Route { pub fn with_router(url: &str, router: Router) -> Self { Self { url: Arc::new(PathMatcher::new(url)), - view: RouteInner::Router(router), + view: RouteInner::Router(Arc::new(router)), name: None, } } @@ -1105,9 +855,9 @@ impl Route { } #[must_use] - pub(crate) fn router(&self) -> Option<&Router> { + pub(crate) fn router(&self) -> Option> { match &self.view { - RouteInner::Router(router) => Some(router), + RouteInner::Router(router) => Some(router.clone()), RouteInner::Handler(_) => None, #[cfg(feature = "openapi")] RouteInner::ApiHandler(_) => None, @@ -1124,7 +874,7 @@ pub(crate) enum RouteKind { #[derive(Clone)] enum RouteInner { Handler(Arc), - Router(Router), + Router(Arc), #[cfg(feature = "openapi")] ApiHandler(Arc), } diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index 14430a53..95bded23 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -3,10 +3,11 @@ //! This module provides a path matcher that can be used to match paths against //! a given pattern. It also provides a way to reverse paths to their original //! form given a set of parameters. - use std::collections::HashMap; -use std::fmt::Display; +use std::fmt::{Display, Write}; +use std::sync::Arc; +use cot::router::tree::MatchitPattern; use cot_core::error::impl_into_cot_error; use thiserror::Error; @@ -62,6 +63,8 @@ pub(super) enum PathMatcherError { )] #[non_exhaustive] WildcardNotAtEnd { pattern: String, name: String }, + #[error("{PATH_MATCHER_ERROR_PREFIX} unsupported brace")] + UnsupportedLiteralBrace { pattern: String }, } impl_into_cot_error!(PathMatcherError); @@ -226,7 +229,7 @@ impl PathMatcher { Ok(result) } - #[allow(dead_code, reason = "used by OpenAPI route generation")] + #[expect(dead_code, reason = "used by OpenAPI route generation")] pub(super) fn param_names(&self) -> impl Iterator { self.parts.iter().filter_map(|part| match part { PathPart::Literal(..) => None, @@ -239,6 +242,32 @@ impl PathMatcher { } } +impl TryFrom> for MatchitPattern { + type Error = PathMatcherError; + + fn try_from(value: Arc) -> Result { + let mut pattern = String::new(); + for part in &value.parts { + match part { + PathPart::Literal(s) if s.contains(['{', '}']) => { + return Err(PathMatcherError::UnsupportedLiteralBrace { + pattern: value.to_string(), + }); + } + PathPart::Literal(s) => pattern.push_str(s), + PathPart::Param { name } => { + let _ = write!(pattern, "{{{name}}}"); + } + PathPart::Wildcard { name } => { + let _ = write!(pattern, "{{*{name}}}"); + } + } + } + + Ok(MatchitPattern::new(pattern)) + } +} + impl Display for PathMatcher { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { for part in &self.parts { diff --git a/cot/src/router/tree.rs b/cot/src/router/tree.rs new file mode 100644 index 00000000..7385e688 --- /dev/null +++ b/cot/src/router/tree.rs @@ -0,0 +1,194 @@ +use std::collections::HashMap; + +use cot::router::{Route, RouteKind}; +use matchit::{Match, Router as MatchitRouter}; + +use crate::router::RouteConflictError; +use crate::router::path::PathPart; + +pub(super) const NESTED_ROUTER_PARAM: &str = "__cot_nested_router__"; + +#[derive(Debug, Clone, Hash, Eq, PartialEq)] +pub(super) struct MatchitPattern(String); + +impl MatchitPattern { + #[must_use] + pub(super) fn new>(pattern: T) -> Self { + Self(pattern.into()) + } + + #[must_use] + pub(super) fn as_str(&self) -> &str { + self.0.as_str() + } +} + +impl From for String { + fn from(value: MatchitPattern) -> Self { + value.0 + } +} + +#[derive(Debug, Clone, Copy)] +pub(super) enum Entry { + Handler(usize), + Router(usize), + Combined { handler: usize, _router: usize }, +} + +#[derive(Debug, Clone)] +pub(super) struct RouteTrie { + inner: MatchitRouter, +} + +impl RouteTrie { + pub(super) fn build(routes: &[Route]) -> super::Result { + let mut inner = MatchitRouter::new(); + + let mut pattern_map: HashMap, Option)> = + HashMap::new(); + for (i, route) in routes.iter().enumerate() { + let pattern = MatchitPattern::try_from(route.url.clone())?; + let entry = pattern_map.entry(pattern).or_default(); + match route.kind() { + RouteKind::Handler => { + if let Some(existing) = entry.0 { + return Err(RouteConflictError::DuplicateHandler { + existing: routes[existing].url(), + new: route.url(), + } + .into()); + } + entry.0 = Some(i); + } + RouteKind::Router => { + if let Some(existing) = entry.1 { + return Err(RouteConflictError::DuplicateRouter { + existing: routes[existing].url(), + new: route.url(), + } + .into()); + } + entry.1 = Some(i); + } + } + } + + for (pattern, (handler_idx, router_idx)) in pattern_map { + let value = match (handler_idx, router_idx) { + (Some(h), None) => Entry::Handler(h), + (None, Some(r)) => Entry::Router(r), + (Some(h), Some(r)) => Entry::Combined { + handler: h, + _router: r, + }, + (None, None) => unreachable!("there should always be a route or handler or both"), + }; + + let route_idx = handler_idx + .or(router_idx) + .expect("route index should exist"); + Self::insert_or_diagnose( + &mut inner, + pattern.clone(), + value, + &routes[route_idx], + routes, + )?; + + // when a nested router is provided, we treat it as a "false" wildcard segment + // and keep a sentinel there so we can use that to find what sub router to + // search at lookup time. + if let Some(r) = router_idx { + let wildcard = format!( + "{}/{{*{NESTED_ROUTER_PARAM}}}", + pattern.as_str().trim_end_matches('/') + ); + Self::insert_or_diagnose( + &mut inner, + MatchitPattern::new(wildcard), + Entry::Router(r), + &routes[r], + routes, + )?; + } + } + + Ok(Self { inner }) + } + + fn insert_or_diagnose( + trie: &mut MatchitRouter, + pattern: MatchitPattern, + value: Entry, + new_route: &Route, + routes: &[Route], + ) -> super::Result<()> { + trie.insert(pattern, value) + .map_err(|err| Self::diagnose(new_route, err, routes).into()) + } + + fn diagnose( + new_route: &Route, + err: matchit::InsertError, + routes: &[Route], + ) -> RouteConflictError { + match err { + matchit::InsertError::Conflict { with } => { + let existing_route = routes.iter().find(|r| { + MatchitPattern::try_from(r.url.clone()).is_ok_and(|p| p.as_str() == with) + }); + + if let Some(existing_route) = existing_route { + Self::classify(existing_route, new_route) + } else { + RouteConflictError::RouteInsert(matchit::InsertError::Conflict { with }) + } + } + + other => RouteConflictError::RouteInsert(other), + } + } + + fn classify(existing_route: &Route, new_route: &Route) -> RouteConflictError { + for (existing_part, new_part) in + existing_route.url.parts().iter().zip(new_route.url.parts()) + { + match (existing_part, new_part) { + (PathPart::Param { name: a }, PathPart::Param { name: b }) if a != b => { + return RouteConflictError::ConflictingParamName { + existing: existing_route.url(), + existing_name: a.clone(), + new: new_route.url(), + new_name: b.clone(), + }; + } + (PathPart::Wildcard { name: a }, PathPart::Wildcard { name: b }) if a != b => { + return RouteConflictError::ConflictingWildcardName { + existing: existing_route.url(), + existing_name: a.clone(), + new: new_route.url(), + new_name: b.clone(), + }; + } + (PathPart::Wildcard { .. }, PathPart::Wildcard { .. }) => { + return RouteConflictError::DuplicateWildcard { + existing: existing_route.url(), + new: new_route.url(), + }; + } + _ => continue, + } + } + + // Every segment matched so this is a duplicate + RouteConflictError::DuplicateHandler { + existing: existing_route.url(), + new: new_route.url(), + } + } + + pub(super) fn at<'a>(&'a self, path: &'a str) -> Option> { + self.inner.at(path).ok() + } +} From 7ee8813d0163fff6419b00d9de7c40d4a1da0c64 Mon Sep 17 00:00:00 2001 From: Elijah Date: Mon, 17 Aug 2026 23:54:54 +0000 Subject: [PATCH 05/16] Add tests with lots and lots of improvements! --- cot/src/error_page.rs | 77 +++++++++++- cot/src/router.rs | 263 ++++++++++++++++++++++++++++++----------- cot/src/router/path.rs | 151 ++++++++++++++++++----- cot/src/router/tree.rs | 206 ++++++++++++++++++++++++++++++-- cot/tests/project.rs | 6 +- cot/tests/router.rs | 106 ++++++++++++++++- 6 files changed, 687 insertions(+), 122 deletions(-) diff --git a/cot/src/error_page.rs b/cot/src/error_page.rs index 03e3f6e8..cb2444a5 100644 --- a/cot/src/error_page.rs +++ b/cot/src/error_page.rs @@ -8,6 +8,7 @@ use tracing::{Level, error, warn}; use crate::config::ProjectConfig; use crate::error::NotFound; use crate::router::Router; +use crate::router::path::AbsolutePath; use crate::{Error, Result, StatusCode, Template}; #[derive(Debug)] @@ -123,7 +124,13 @@ impl ErrorPageTemplateBuilder { fn diagnostics(&mut self, diagnostics: &Diagnostics) -> &mut Self { self.project_config = format!("{:#?}", diagnostics.project_config); self.route_data.clear(); - Self::build_route_data(&mut self.route_data, &diagnostics.router, "", ""); + Self::build_route_data( + &mut self.route_data, + &diagnostics.router, + &AbsolutePath::root(), + "", + ); + println!("\n\nroute_data: {:?}\n\n", self.route_data); self.request_data = diagnostics .request_head .as_ref() @@ -134,13 +141,15 @@ impl ErrorPageTemplateBuilder { fn build_route_data( route_data: &mut Vec, router: &Arc, - url_prefix: &str, + url_prefix: &AbsolutePath, index_prefix: &str, ) { for (index, route) in router.routes().iter().enumerate() { + let full_path = url_prefix.join(&AbsolutePath::new(route.url())); + route_data.push(RouteData { index: format!("{index_prefix}{index}"), - path: format!("{url_prefix}{}", route.url()), + path: full_path.to_string(), kind: match route.kind() { crate::router::RouteKind::Router => if route_data.is_empty() { "Root Router" @@ -157,7 +166,7 @@ impl ErrorPageTemplateBuilder { Self::build_route_data( route_data, &inner_router, - &format!("{}{}", url_prefix, route.url()), + &full_path, &format!("{index_prefix}{index}."), ); } @@ -453,6 +462,10 @@ mod tests { use std::panic; use std::sync::Arc; + use cot_core::handler::RequestHandler; + use cot_core::html::Html; + use cot_core::request::Request; + use cot_core::response::{IntoResponse, Response}; use tracing_test::traced_test; use super::*; @@ -468,6 +481,14 @@ mod tests { } } + struct MockHandler; + + impl RequestHandler for MockHandler { + async fn handle(&self, _request: Request) -> Result { + Html::new("OK").into_response() + } + } + #[test] #[traced_test] fn test_log_error() { @@ -592,7 +613,12 @@ mod tests { "/foo", sub_router, )])); - ErrorPageTemplateBuilder::build_route_data(&mut route_data, &router, "", ""); + ErrorPageTemplateBuilder::build_route_data( + &mut route_data, + &router, + &AbsolutePath::root(), + "", + ); assert_eq!( route_data, @@ -613,6 +639,47 @@ mod tests { ); } + #[test] + fn build_route_data_root_mount_no_double_slash() { + let mut route_data = Vec::new(); + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "index", + )]); + let router = Arc::new(Router::with_urls(vec![Route::with_router("/", sub_router)])); + + ErrorPageTemplateBuilder::build_route_data( + &mut route_data, + &router, + &AbsolutePath::root(), + "", + ); + + assert_eq!(route_data[0].path, "/"); + assert_eq!(route_data[1].path, "/"); + } + + #[test] + fn build_route_data_root_mount_with_nested_static_route() { + let mut route_data = Vec::new(); + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/nested", + MockHandler, + "nested", + )]); + let router = Arc::new(Router::with_urls(vec![Route::with_router("/", sub_router)])); + + ErrorPageTemplateBuilder::build_route_data( + &mut route_data, + &router, + &AbsolutePath::root(), + "", + ); + + assert_eq!(route_data[1].path, "/nested"); + } + #[test] fn test_build_cot_failure_page() { let response = build_cot_failure_page(); diff --git a/cot/src/router.rs b/cot/src/router.rs index d2156b0d..685bef9c 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -27,6 +27,7 @@ use std::pin::Pin; use std::sync::Arc; use std::task::{Context, Poll}; +use cot::router::path::AbsolutePath; use cot_core::error::impl_into_cot_error; use cot_core::handler::{BoxRequestHandler, RequestHandler, into_box_request_handler}; use cot_core::request::{AppName, RouteName}; @@ -183,17 +184,13 @@ impl Router { let m = self.route_tree.at(request_path)?; let (route_index, remaining_path) = match m.value { - // For `Entry::Combined` (cases where a handler overlaps a router for the same - // route/path, the handler takes precedence. - Entry::Handler(idx) | Entry::Combined { handler: idx, .. } => (*idx, String::new()), + Entry::Handler(idx) => (*idx, String::new()), Entry::Router(idx) => { - let rest = m.params.get(tree::NESTED_ROUTER_PARAM).unwrap_or(""); - let remaining = if rest.is_empty() { - String::new() - } else { - format!("/{rest}") + let remaining = match m.params.get(tree::NESTED_ROUTER_PARAM) { + Some(rest) => AbsolutePath::new(rest), + None => AbsolutePath::root(), }; - (*idx, remaining) + (*idx, remaining.into()) } }; @@ -331,7 +328,9 @@ impl Router { if let RouteInner::Router(router) = &route.view && let Some(url) = router.reverse_option(app_name, name, params)? { - return Ok(Some(route.url.reverse(params)? + &url)); + let prefix = AbsolutePath::new(route.url.reverse(params)?); + let suffix = AbsolutePath::new(url); + return Ok(Some(prefix.join(&suffix).into())); } } Ok(None) @@ -400,7 +399,12 @@ impl Router { let mut schema_generator = schemars::SchemaGenerator::new(schemars::generate::SchemaSettings::openapi3()); - self.as_openapi_impl("", &[], &mut paths, &mut schema_generator); + self.as_openapi_impl( + &AbsolutePath::root(), + &[], + &mut paths, + &mut schema_generator, + ); let component_schemas = schema_generator .take_definitions(true) @@ -431,7 +435,7 @@ impl Router { #[cfg(feature = "openapi")] fn as_openapi_impl( &self, - url: &str, + url: &AbsolutePath, param_names: &[&str], paths: &mut aide::openapi::Paths, schema_generator: &mut schemars::SchemaGenerator, @@ -447,14 +451,14 @@ impl Router { param_names: &[&str], paths: &mut aide::openapi::Paths, schema_generator: &mut schemars::SchemaGenerator, - url: &str, + url: &AbsolutePath, ) { match &route.view { RouteInner::Router(router) => { let mut params = Vec::from(param_names); params.extend(route.url.param_names()); - let url = format!("{url}{}", route.url); + let url = url.join(&AbsolutePath::new(route.url())); router.as_openapi_impl(&url, ¶ms, paths, schema_generator); } @@ -462,13 +466,13 @@ impl Router { let mut params = Vec::from(param_names); params.extend(route.url.param_names()); - let url = format!("{url}{}", route.url); + let url = url.join(&AbsolutePath::new(route.url())); let mut route_context = crate::openapi::RouteContext::new(); route_context.param_names = ¶ms; paths.paths.insert( - url, + url.into(), aide::openapi::ReferenceOr::Item( handler.as_api_route(&route_context, schema_generator), ), @@ -510,7 +514,7 @@ enum RouteConflictError { #[error( "{ERROR_PREFIX} conflicting route parameters: `{existing}` uses `{{{existing_name}}}` but `{new}` uses \ - `{{{new_name}}}` at the same position in the path -- both routes must bind the same \ + `{{{new_name}}}` at the same position in the path; both routes must bind the same \ parameter name there, since only one value can be captured at that position" )] ConflictingParamName { @@ -1169,7 +1173,6 @@ mod tests { } #[test] - #[cfg(feature = "openapi")] fn route_inner_debug() { let route = Route::with_handler("/test", MockHandler); assert!(format!("{route:?}").contains("Handler(\"handler(...)\")")); @@ -1177,12 +1180,14 @@ mod tests { let route = Route::with_router("/test", Router::empty()); assert!(format!("{route:?}").contains("Router(Router {")); - let route = Route::with_api_handler("/test", MockHandler); - assert!(format!("{route:?}").contains("ApiHandler(\"handler(...)\")")); + #[cfg(feature = "openapi")] + { + let route = Route::with_api_handler("/test", MockHandler); + assert!(format!("{route:?}").contains("ApiHandler(\"handler(...)\")")); + } } #[test] - #[cfg(feature = "openapi")] fn route_kind() { let handler_route = Route::with_handler("/test", MockHandler); assert_eq!(handler_route.kind(), RouteKind::Handler); @@ -1190,12 +1195,14 @@ mod tests { let router_route = Route::with_router("/test", Router::empty()); assert_eq!(router_route.kind(), RouteKind::Router); - let api_route = Route::with_api_handler("/test", MockHandler); - assert_eq!(api_route.kind(), RouteKind::Handler); + #[cfg(feature = "openapi")] + { + let api_route = Route::with_api_handler("/test", MockHandler); + assert_eq!(api_route.kind(), RouteKind::Handler); + } } #[test] - #[cfg(feature = "openapi")] fn route_router() { let router = Router::empty(); let route = Route::with_router("/test", router.clone()); @@ -1204,8 +1211,11 @@ mod tests { let route = Route::with_handler("/test", MockHandler); assert!(route.router().is_none()); - let route = Route::with_api_handler("/test", MockHandler); - assert!(route.router().is_none()); + #[cfg(feature = "openapi")] + { + let route = Route::with_api_handler("/test", MockHandler); + assert!(route.router().is_none()); + } } #[test] @@ -1358,34 +1368,6 @@ mod tests { assert_params(found.params, &[("id", "123"), ("post_id", "456")]); } - #[test] - fn router_escaped_literal_route_matches() { - let router = Router::with_urls(vec![Route::with_handler_and_name( - "/users/{{{{{{escaped}}}}}}", - MockHandler, - "escaped", - )]); - - let found = router.get_handler("/users/{{{escaped}}}").unwrap(); - - assert_eq!(found.name, Some(RouteName("escaped".to_string()))); - assert!(found.params.is_empty()); - } - - #[test] - fn router_non_ascii_literal_route_matches() { - let router = Router::with_urls(vec![Route::with_handler_and_name( - "/café/{id}", - MockHandler, - "cafe", - )]); - - let found = router.get_handler("/café/123").unwrap(); - - assert_eq!(found.name, Some(RouteName("cafe".to_string()))); - assert_params(found.params, &[("id", "123")]); - } - #[test] fn router_routes_with_common_static_prefixes_match_independently() { let router = Router::with_urls(vec![ @@ -1421,19 +1403,6 @@ mod tests { assert_eq!(found.name, Some(RouteName("static".to_string()))); } - #[test] - fn router_dynamic_route_takes_priority_over_wildcard_route() { - let router = Router::with_urls(vec![ - Route::with_handler_and_name("/files/{name}", MockHandler, "dynamic"), - Route::with_handler_and_name("/files/{*path}", MockHandler, "wildcard"), - ]); - - let found = router.get_handler("/files/readme").unwrap(); - - assert_eq!(found.name, Some(RouteName("dynamic".to_string()))); - assert_params(found.params, &[("name", "readme")]); - } - #[test] fn router_wildcard_route_captures_remaining_path() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1507,7 +1476,9 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate handler route at the same path")] + #[should_panic( + expected = "route conflict error: duplicate route: `/users` conflicts with an already registered handler route `/users` (both fully match the same path)" + )] fn router_duplicate_handler_routes_panic() { let _ = Router::with_urls(vec![ Route::with_handler("/users", MockHandler), @@ -1516,7 +1487,9 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate nested router route at the same path")] + #[should_panic( + expected = "route conflict error: duplicate nested router: `/users` conflicts with an already registered nested router mounted at `/users`" + )] fn router_duplicate_nested_router_routes_panic() { let _ = Router::with_urls(vec![ Route::with_router("/users", Router::empty()), @@ -1525,8 +1498,19 @@ mod tests { } #[test] - #[should_panic(expected = "Conflicting route parameters")] + #[should_panic( + expected = "route conflict error: conflicting route parameters: `/foo/{bar}` uses `{bar}` but `/foo/{baz}` uses `{baz}` at the same position in the path; both routes must bind the same parameter name there, since only one value can be captured at that position" + )] fn router_conflicting_param_names_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/foo/{bar}", MockHandler), + Route::with_handler("/foo/{baz}", MockHandler), + ]); + } + + #[test] + fn router_same_path_with_trailing_lash_diff() { + // this should not fail let _ = Router::with_urls(vec![ Route::with_handler("/foo/{bar}/", MockHandler), Route::with_handler("/foo/{baz}", MockHandler), @@ -1534,7 +1518,9 @@ mod tests { } #[test] - #[should_panic(expected = "Duplicate wildcard route")] + #[should_panic( + expected = "route conflict error: duplicate route: `/static/{*path}` conflicts with an already registered handler route `/static/{*path}` (both fully match the same path)" + )] fn router_duplicate_wildcard_routes_panic() { let _ = Router::with_urls(vec![ Route::with_handler("/static/{*path}", MockHandler), @@ -1543,7 +1529,9 @@ mod tests { } #[test] - #[should_panic(expected = "Conflicting wildcard route parameters")] + #[should_panic( + expected = "route conflict error: conflicting wildcard parameters: `/static/{*path}` uses `{*path}` but `/static/{*file_path}` uses `{*file_path}` at the same position in the path" + )] fn router_conflicting_wildcard_names_panic() { let _ = Router::with_urls(vec![ Route::with_handler("/static/{*path}", MockHandler), @@ -1551,6 +1539,137 @@ mod tests { ]); } + #[test] + #[should_panic( + expected = "route conflict error: duplicate route: `/static/{*file_path}` conflicts with an already registered handler route `/static/{path}` (both fully match the same path)" + )] + fn router_wildcard_and_param_at_same_segment_conflict() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{path}", MockHandler), + Route::with_handler("/static/{*file_path}", MockHandler), + ]); + } + + #[test] + fn router_empty_returns_no_handler() { + let router = Router::empty(); + assert!(router.get_handler("/").is_none()); + } + + #[test] + fn router_root_mounted_nested_router() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/inner", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); + + let found = router.get_handler("/inner").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + } + + #[test] + fn router_nested_router_trailing_slash_prefix() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/inner", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("/api/", sub_router)]); + + let found = router.get_handler("/api/inner").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + } + + #[test] + fn router_reverse_option_wrong_app_name_returns_none() { + let route = Route::with_handler_and_name("/test", MockHandler, "test"); + let mut router = Router::with_urls(vec![route]); + router.set_app_name(AppName("app_1".to_string())); + + let result = router + .reverse_option(Some("app_2"), "test", &ReverseParamMap::new()) + .unwrap(); + + assert!(result.is_none()); + } + + #[test] + fn router_reverse_missing_view_returns_error() { + let router = Router::empty(); + let result = router.reverse(None, "missing", &ReverseParamMap::new()); + assert!(result.is_err()); + } + + #[test] + fn router_root_mount_matches_root_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "index", + )]); + let router = Router::with_urls(vec![Route::with_router("", sub_router)]); + + let found = router.get_handler("/").unwrap(); + + assert_eq!(found.name, Some(RouteName("index".to_string()))); + } + + #[test] + fn router_exact_mount_match_routes_to_nested_root_not_empty_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "sub_index", + )]); + let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); + + let found = router.get_handler("/api").unwrap(); + + assert_eq!(found.name, Some(RouteName("sub_index".to_string()))); + } + + #[test] + fn router_reverse_root_mount_no_double_slash() { + let route = Route::with_handler_and_name("/", MockHandler, "index"); + let sub_router = Router::with_urls(vec![route]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); + + let url = router + .reverse(None, "index", &ReverseParamMap::new()) + .unwrap(); + + assert_eq!(url, "/"); + } + + #[test] + fn router_reverse_nested_under_root_mount_no_double_slash() { + let route = Route::with_handler_and_name("/inner", MockHandler, "inner"); + let sub_router = Router::with_urls(vec![route]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + + assert_eq!(url, "/inner"); + } + + #[test] + fn router_reverse_deeply_nested_root_mounts_no_double_slash() { + let route = Route::with_handler_and_name("/leaf", MockHandler, "leaf"); + let inner_router = Router::with_urls(vec![route]); + let mid_router = Router::with_urls(vec![Route::with_router("/", inner_router)]); + let router = Router::with_urls(vec![Route::with_router("/", mid_router)]); + + let url = router + .reverse(None, "leaf", &ReverseParamMap::new()) + .unwrap(); + + assert_eq!(url, "/leaf"); + } + #[test] fn router_reverse_app_name() { let route = Route::with_handler_and_name("/test", MockHandler, "test"); diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index 95bded23..ac0fd0c5 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -18,56 +18,96 @@ const PATH_MATCHER_ERROR_PREFIX: &str = "route conflict error:"; pub(super) enum PathMatcherError { /// Two parameters appear consecutively with no literal text between them, #[error( - "{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}` (at position {position})" + "{PATH_MATCHER_ERROR_PREFIX} consecutive parameters are not allowed in pattern `{pattern}`" )] #[non_exhaustive] - ConsecutiveParams { pattern: String, position: usize }, - + ConsecutiveParams { pattern: String }, /// A `{` was opened but never closed with a matching `}`. #[error( - "{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}` -- expected a closing `}}`" + "{PATH_MATCHER_ERROR_PREFIX} unclosed parameter `{{{name}` in pattern `{pattern}`; expected a closing `}}`" )] #[non_exhaustive] UnclosedParam { pattern: String, name: String }, - /// A `}` appeared without a preceding `{` to open it. #[error( - "{PATH_MATCHER_ERROR_PREFIX} closing brace `}}` without a matching opening `{{` in pattern `{pattern}` \ - (at position {position})" + "{PATH_MATCHER_ERROR_PREFIX} closing brace `}}` without a matching opening `{{` in pattern `{pattern}`" )] #[non_exhaustive] - UnmatchedClosingBrace { pattern: String, position: usize }, - + UnmatchedClosingBrace { pattern: String }, /// A parameter name is empty or contains characters other than /// alphanumerics/underscore, or starts with a digit. #[error( - "{PATH_MATCHER_ERROR_PREFIX} invalid parameter name `{name}` in pattern `{pattern}` -- parameter names must start \ + "{PATH_MATCHER_ERROR_PREFIX} invalid parameter name `{name}` in pattern `{pattern}`; parameter names must start \ with a letter or underscore and contain only letters, digits, or underscores" )] #[non_exhaustive] InvalidParamName { pattern: String, name: String }, - /// Same as `InvalidParamName`, but for the name following a `*` in a /// wildcard segment. #[error( - "{PATH_MATCHER_ERROR_PREFIX} invalid wildcard name `{name}` in pattern `{pattern}` -- wildcard names must start \ + "{PATH_MATCHER_ERROR_PREFIX} invalid wildcard name `{name}` in pattern `{pattern}`; wildcard names must start \ with a letter or underscore and contain only letters, digits, or underscores" )] #[non_exhaustive] InvalidWildcardName { pattern: String, name: String }, - /// A wildcard segment (`{*name}`) was followed by more path segments, #[error( - "{PATH_MATCHER_ERROR_PREFIX} wildcard parameter `{{*{name}}}` must be the last segment of pattern `{pattern}` -- \ + "{PATH_MATCHER_ERROR_PREFIX} wildcard parameter `{{*{name}}}` must be the last segment of pattern `{pattern}`; \ a wildcard consumes the rest of the path, so nothing can follow it" )] #[non_exhaustive] WildcardNotAtEnd { pattern: String, name: String }, - #[error("{PATH_MATCHER_ERROR_PREFIX} unsupported brace")] + #[error("{PATH_MATCHER_ERROR_PREFIX} unsupported brace in {pattern}")] UnsupportedLiteralBrace { pattern: String }, } impl_into_cot_error!(PathMatcherError); +/// An absolute route path. +/// +/// The path is normalized to always begin with `/` and allows paths to be +/// joined without introducing duplicate `/` separators. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct AbsolutePath(String); + +impl AbsolutePath { + #[must_use] + pub(crate) fn new>(s: S) -> Self { + let mut s = s.into(); + if !s.starts_with('/') { + s.insert(0, '/'); + } + Self(s) + } + + #[must_use] + pub(crate) fn root() -> Self { + Self(String::from("/")) + } + + #[must_use] + pub(crate) fn as_str(&self) -> &str { + self.0.as_str() + } + + #[must_use] + pub(crate) fn join(&self, suffix: &AbsolutePath) -> AbsolutePath { + let trimmed = self.0.strip_suffix('/').unwrap_or(&self.0); + AbsolutePath(format!("{trimmed}{}", suffix.0)) + } +} + +impl Display for AbsolutePath { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl From for String { + fn from(value: AbsolutePath) -> Self { + value.0 + } +} + #[derive(Debug, Clone)] pub(super) struct PathMatcher { parts: Vec, @@ -90,7 +130,7 @@ impl PathMatcher { } let mut path_pattern = path_pattern.into(); - if !path_pattern.is_empty() && !path_pattern.starts_with('/') { + if !path_pattern.starts_with('/') { path_pattern.insert(0, '/'); } @@ -110,7 +150,6 @@ impl PathMatcher { if index != 0 && ch.is_some() { return Err(PathMatcherError::ConsecutiveParams { pattern: path_pattern.clone(), - position: index, }); } } else { @@ -142,7 +181,6 @@ impl PathMatcher { } else { return Err(PathMatcherError::UnmatchedClosingBrace { pattern: path_pattern.clone(), - position: index, }); } } @@ -468,67 +506,89 @@ mod tests { } #[test] - #[should_panic(expected = "Consecutive parameters are not allowed")] + #[should_panic( + expected = "route conflict error: consecutive parameters are not allowed in pattern `/users/{id}{post_id}`" + )] fn path_parser_consecutive_params() { let _ = PathMatcher::new("/users/{id}{post_id}"); } #[test] - #[should_panic(expected = "Invalid parameter name: ``")] + #[should_panic( + expected = "route conflict error: invalid parameter name `` in pattern `/users/{}`; parameter names must start with a letter or underscore and contain only letters, digits, or underscores" + )] fn path_parser_invalid_name_empty() { let _ = PathMatcher::new("/users/{}"); } #[test] - #[should_panic(expected = "Invalid parameter name: `123`")] + #[should_panic( + expected = "route conflict error: invalid parameter name `123` in pattern `/users/{123}`; parameter names must start with a letter or underscore and contain only letters, digits, or underscores" + )] fn path_parser_invalid_name_numeric() { let _ = PathMatcher::new("/users/{123}"); } #[test] - #[should_panic(expected = "Invalid parameter name: `abc#$%`")] + #[should_panic( + expected = "route conflict error: invalid parameter name `abc#$%` in pattern `/users/{abc#$%}`; parameter names must start with a letter or underscore and contain only letters, digits, or underscores" + )] fn path_parser_invalid_name_non_alphanumeric() { let _ = PathMatcher::new("/users/{abc#$%}"); } #[test] - #[should_panic(expected = "Invalid wildcard name: ``")] + #[should_panic( + expected = "route conflict error: invalid wildcard name `` in pattern `/users/{*}`; wildcard names must start with a letter or underscore and contain only letters, digits, or underscores" + )] fn path_parser_invalid_wildcard_name_empty() { let _ = PathMatcher::new("/users/{*}"); } #[test] - #[should_panic(expected = "Wildcard parameters are only allowed at the end of a route")] + #[should_panic( + expected = "route conflict error: wildcard parameter `{*path}` must be the last segment of pattern `/users/{*path}/edit`; a wildcard consumes the rest of the path, so nothing can follow it" + )] fn path_parser_wildcard_not_at_end() { let _ = PathMatcher::new("/users/{*path}/edit"); } #[test] - #[should_panic(expected = "Unclosed parameter: `foo`")] + #[should_panic( + expected = "route conflict error: unclosed parameter `{foo` in pattern `/users/{foo`; expected a closing `}`" + )] fn path_parser_unclosed() { let _ = PathMatcher::new("/users/{foo"); } #[test] - #[should_panic(expected = "Closing brace encountered without opening brace")] + #[should_panic( + expected = "route conflict error: closing brace `}` without a matching opening `{` in pattern `/users/foo}`" + )] fn path_parser_missing_opening_brace() { let _ = PathMatcher::new("/users/foo}"); } #[test] - #[should_panic(expected = "Unclosed parameter: `foo`")] + #[should_panic( + expected = "route conflict error: unclosed parameter `{foo` in pattern `/users/{foo/bar`; expected a closing `}`" + )] fn path_parser_unclosed_slash() { let _ = PathMatcher::new("/users/{foo/bar"); } #[test] - #[should_panic(expected = "Unclosed parameter: `foo`")] + #[should_panic( + expected = "route conflict error: unclosed parameter `{foo` in pattern `/users/{foo{bar`; expected a closing `}`" + )] fn path_parser_unclosed_double() { let _ = PathMatcher::new("/users/{foo{bar"); } #[test] - #[should_panic(expected = "Closing brace encountered without opening brace")] + #[should_panic( + expected = "route conflict error: closing brace `}` without a matching opening `{` in pattern `/users/{{{foo}}/bar`" + )] fn path_parser_escaping_unclosed() { let _ = PathMatcher::new("/users/{{{foo}}/bar"); } @@ -591,4 +651,37 @@ mod tests { let params = ReverseParamMap::new(); assert_eq!(path_parser.reverse(¶ms).unwrap(), "/café/test"); } + + #[test] + fn absolute_path_join_root_with_root() { + assert_eq!( + AbsolutePath::root().join(&AbsolutePath::root()).as_str(), + "/" + ); + } + + #[test] + fn absolute_path_join_root_is_identity() { + let x = AbsolutePath::new("/foo/bar"); + assert_eq!(AbsolutePath::root().join(&x), x); + } + + #[test] + fn absolute_path_join_trims_doubled_slash() { + let prefix = AbsolutePath::new("/api/"); + let suffix = AbsolutePath::new("/inner"); + assert_eq!(prefix.join(&suffix).as_str(), "/api/inner"); + } + + #[test] + fn absolute_path_join_no_trailing_slash_on_prefix() { + let prefix = AbsolutePath::new("/api"); + let suffix = AbsolutePath::new("/inner"); + assert_eq!(prefix.join(&suffix).as_str(), "/api/inner"); + } + + #[test] + fn absolute_path_new_normalizes_missing_leading_slash() { + assert_eq!(AbsolutePath::new("foo").as_str(), "/foo"); + } } diff --git a/cot/src/router/tree.rs b/cot/src/router/tree.rs index 7385e688..27d5f681 100644 --- a/cot/src/router/tree.rs +++ b/cot/src/router/tree.rs @@ -4,7 +4,7 @@ use cot::router::{Route, RouteKind}; use matchit::{Match, Router as MatchitRouter}; use crate::router::RouteConflictError; -use crate::router::path::PathPart; +use crate::router::path::{AbsolutePath, PathPart}; pub(super) const NESTED_ROUTER_PARAM: &str = "__cot_nested_router__"; @@ -33,7 +33,6 @@ impl From for String { pub(super) enum Entry { Handler(usize), Router(usize), - Combined { handler: usize, _router: usize }, } #[derive(Debug, Clone)] @@ -74,14 +73,21 @@ impl RouteTrie { } } - for (pattern, (handler_idx, router_idx)) in pattern_map { + let mut entries: Vec<_> = pattern_map.into_iter().collect(); + // sort for deterministic insertion behavior + entries.sort_by_key(|(_, (handler_idx, router_idx))| { + handler_idx + .or(*router_idx) + .expect("route index should exist") + }); + + for (pattern, (handler_idx, router_idx)) in entries { let value = match (handler_idx, router_idx) { (Some(h), None) => Entry::Handler(h), (None, Some(r)) => Entry::Router(r), - (Some(h), Some(r)) => Entry::Combined { - handler: h, - _router: r, - }, + // for cases where a handler overlaps a router for the same route/path, the handler + // takes precedence. + (Some(h), Some(_r)) => Entry::Handler(h), (None, None) => unreachable!("there should always be a route or handler or both"), }; @@ -100,13 +106,13 @@ impl RouteTrie { // and keep a sentinel there so we can use that to find what sub router to // search at lookup time. if let Some(r) = router_idx { - let wildcard = format!( - "{}/{{*{NESTED_ROUTER_PARAM}}}", - pattern.as_str().trim_end_matches('/') - ); + let prefix = AbsolutePath::new(pattern.as_str()); + let wildcard_suffix = AbsolutePath::new(format!("{{*{NESTED_ROUTER_PARAM}}}")); + let wildcard = prefix.join(&wildcard_suffix); + Self::insert_or_diagnose( &mut inner, - MatchitPattern::new(wildcard), + MatchitPattern::new(wildcard.as_str()), Entry::Router(r), &routes[r], routes, @@ -192,3 +198,179 @@ impl RouteTrie { self.inner.at(path).ok() } } + +#[cfg(test)] +mod tests { + use cot::router::Route; + + use super::*; + use crate::html::Html; + use crate::router::Router; + + async fn handler() -> Html { + Html::new("ok") + } + + fn route(url: &str) -> Route { + Route::with_handler(url, handler) + } + + #[test] + fn build_single_handler_route() { + let routes = vec![route("/users")]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/users").unwrap(); + assert!(matches!(m.value, Entry::Handler(0))); + } + + #[test] + fn build_no_match_returns_none() { + let routes = vec![route("/users")]; + let trie = RouteTrie::build(&routes).unwrap(); + + assert!(trie.at("/other").is_none()); + } + + #[test] + fn build_root_path_matches() { + let routes = vec![route("/")]; + let trie = RouteTrie::build(&routes).unwrap(); + + assert!(matches!(trie.at("/").unwrap().value, Entry::Handler(0))); + } + + #[test] + fn build_param_route_captures_value() { + let routes = vec![route("/users/{id}")]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/users/42").unwrap(); + assert!(matches!(m.value, Entry::Handler(0))); + assert_eq!(m.params.get("id"), Some("42")); + } + + #[test] + fn build_wildcard_route_captures_remaining_path() { + let routes = vec![route("/static/{*path}")]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/static/css/app.css").unwrap(); + assert!(matches!(m.value, Entry::Handler(0))); + assert_eq!(m.params.get("path"), Some("css/app.css")); + } + + #[test] + fn build_router_route_inserts_wildcard_sentinel() { + let sub_router = Router::with_urls(vec![route("/inner")]); + let routes = vec![Route::with_router("/api", sub_router)]; + let trie = RouteTrie::build(&routes).unwrap(); + + assert!(matches!(trie.at("/api").unwrap().value, Entry::Router(0))); + + let m = trie.at("/api/inner").unwrap(); + assert!(matches!(m.value, Entry::Router(0))); + assert_eq!(m.params.get(NESTED_ROUTER_PARAM), Some("inner")); + } + + #[test] + fn build_router_trailing_slash_prefix_does_not_double_slash() { + let sub_router = Router::with_urls(vec![route("/inner")]); + let routes = vec![Route::with_router("/api/", sub_router)]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/api/inner").unwrap(); + assert_eq!(m.params.get(NESTED_ROUTER_PARAM), Some("inner")); + } + + #[test] + fn build_combined_handler_and_router_same_path() { + let sub_router = Router::with_urls(vec![route("/inner")]); + let routes = vec![Route::with_router("/api", sub_router), route("/api")]; + let trie = RouteTrie::build(&routes).unwrap(); + + assert!(matches!(trie.at("/api").unwrap().value, Entry::Handler(1))); + } + + #[test] + fn static_route_priority_over_param_route() { + let routes = vec![route("/users/{id}"), route("/users/new")]; + let trie = RouteTrie::build(&routes).unwrap(); + + assert!(matches!( + trie.at("/users/new").unwrap().value, + Entry::Handler(1) + )); + } + + #[test] + fn build_duplicate_handler_errors() { + let routes = vec![route("/users"), route("/users")]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("duplicate route")); + } + + #[test] + fn build_duplicate_router_errors() { + let routes = vec![ + Route::with_router("/users", Router::empty()), + Route::with_router("/users", Router::empty()), + ]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("duplicate nested router")); + } + + #[test] + fn build_conflicting_param_names_errors() { + let routes = vec![route("/foo/{bar}/"), route("/foo/{baz}/")]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("conflicting route parameters")); + } + + #[test] + fn build_conflicting_wildcard_names_errors() { + let routes = vec![route("/static/{*path}"), route("/static/{*file_path}")]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("conflicting wildcard parameters")); + } + + #[test] + fn build_duplicate_wildcard_errors() { + let routes = vec![route("/static/{*path}"), route("/static/{*path}")]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("duplicate route")); + } + + #[test] + fn build_root_mounted_router_matches_root_path() { + let sub_router = Router::with_urls(vec![route("/")]); + let routes = vec![Route::with_router("", sub_router)]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/").unwrap(); + assert!(matches!(m.value, Entry::Router(0))); + } + + #[test] + fn build_root_mounted_router_exact_match_has_no_wildcard_capture() { + let sub_router = Router::with_urls(vec![route("/")]); + let routes = vec![Route::with_router("/", sub_router)]; + let trie = RouteTrie::build(&routes).unwrap(); + + let m = trie.at("/").unwrap(); + assert!(m.params.get(NESTED_ROUTER_PARAM).is_none()); + } + + #[test] + fn matchit_pattern_new_and_as_str() { + let pattern = MatchitPattern::new("/users/{id}"); + assert_eq!(pattern.as_str(), "/users/{id}"); + } + + #[test] + fn matchit_pattern_into_string() { + let pattern = MatchitPattern::new("/users"); + let s: String = pattern.into(); + assert_eq!(s, "/users"); + } +} diff --git a/cot/tests/project.rs b/cot/tests/project.rs index ba96670c..70911c6d 100644 --- a/cot/tests/project.rs +++ b/cot/tests/project.rs @@ -102,7 +102,7 @@ async fn cot_router_reverse_local() { fn register_apps(&self, apps: &mut AppBuilder, _context: &RegisterAppsContext) { apps.register_with_views(App1, ""); - apps.register_with_views(App2, ""); + apps.register_with_views(App2, "/foo"); } } @@ -114,9 +114,9 @@ async fn cot_router_reverse_local() { Bytes::from("/index1") ); - let response = client.get("/index2").await.unwrap(); + let response = client.get("/foo/index2").await.unwrap(); assert_eq!( response.into_body().into_bytes().await.unwrap(), - Bytes::from("/index2") + Bytes::from("/foo/index2") ); } diff --git a/cot/tests/router.rs b/cot/tests/router.rs index cd598cf4..49653a10 100644 --- a/cot/tests/router.rs +++ b/cot/tests/router.rs @@ -13,10 +13,25 @@ async fn index() -> Html { async fn parameterized(request: Request) -> Html { let name = request.path_params().get("name").unwrap().to_owned(); - Html::new(name) } +async fn multi_param(request: Request) -> Html { + let id = request.path_params().get("id").unwrap().to_owned(); + let post_id = request.path_params().get("post_id").unwrap().to_owned(); + Html::new(format!("{id}/{post_id}")) +} + +async fn catch_all(request: Request) -> Html { + let path = request.path_params().get("path").unwrap().to_owned(); + Html::new(path) +} + +async fn nested(request: Request) -> Html { + let id = request.path_params().get("id").unwrap().to_owned(); + Html::new(format!("nested/{id}")) +} + #[cot::test] #[cfg_attr( miri, @@ -49,6 +64,82 @@ async fn path_params() { ); } +#[cot::test] +#[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" +)] +async fn multi_path_params() { + let client = Client::new(project()); + + let response = client.await.get("/multi/1/posts/2").await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.into_body().into_bytes().await.unwrap(), + Bytes::from("1/2") + ); +} + +#[cot::test] +#[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" +)] +async fn wildcard_catch_all() { + let client = Client::new(project()); + + let response = client.await.get("/static/css/app.css").await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.into_body().into_bytes().await.unwrap(), + Bytes::from("css/app.css") + ); +} + +#[cot::test] +#[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" +)] +async fn nested_router() { + let client = Client::new(project()); + + let response = client.await.get("/nested/inner/42").await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.into_body().into_bytes().await.unwrap(), + Bytes::from("nested/42") + ); +} + +#[cot::test] +#[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" +)] +async fn unmatched_path_returns_404() { + let client = Client::new(project()); + + let response = client.await.get("/does-not-exist").await.unwrap(); + assert_eq!(response.status(), StatusCode::NOT_FOUND); +} + +#[cot::test] +#[cfg_attr( + miri, + ignore = "unsupported operation: can't call foreign function `sqlite3_open_v2`" +)] +async fn static_route_priority_over_dynamic() { + let client = Client::new(project()); + + let response = client.await.get("/get/new").await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + assert_eq!( + response.into_body().into_bytes().await.unwrap(), + Bytes::from("new") + ); +} + #[must_use] fn project() -> impl Project { struct RouterApp; @@ -58,9 +149,22 @@ fn project() -> impl Project { } fn router(&self) -> Router { + let nested_router = Router::with_urls([Route::with_handler_and_name( + "/inner/{id}", + nested, + "nested", + )]); + Router::with_urls([ Route::with_handler_and_name("/", index, "index"), Route::with_handler_and_name("/get/{name}", parameterized, "parameterized"), + Route::with_handler_and_name( + "/multi/{id}/posts/{post_id}", + multi_param, + "multi_param", + ), + Route::with_handler_and_name("/static/{*path}", catch_all, "catch_all"), + Route::with_router("/nested", nested_router), ]) } } From fb47870450d38dd52b89c8128d37ce15f21de8d8 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 18 Aug 2026 00:18:48 +0000 Subject: [PATCH 06/16] make clippy happy --- cot/src/router/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index ac0fd0c5..70659f7b 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -267,7 +267,7 @@ impl PathMatcher { Ok(result) } - #[expect(dead_code, reason = "used by OpenAPI route generation")] + #[cfg(feature = "openapi")] pub(super) fn param_names(&self) -> impl Iterator { self.parts.iter().filter_map(|part| match part { PathPart::Literal(..) => None, From 0ba361be4472346254da4fdbec1873b50c373d9f Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 18 Aug 2026 02:17:50 +0000 Subject: [PATCH 07/16] doctest fix --- cot/src/openapi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cot/src/openapi.rs b/cot/src/openapi.rs index f2b41c6b..97de8edc 100644 --- a/cot/src/openapi.rs +++ b/cot/src/openapi.rs @@ -95,7 +95,7 @@ //! # async fn main() -> cot::Result<()> { //! # let mut client = cot::test::Client::new(ApiProject).await; //! # -//! # let response = client.get("/swagger/").await?; +//! # let response = client.get("/swagger").await?; //! # assert_eq!(response.status(), StatusCode::OK); //! # //! # Ok(()) From 89dc1ee57a0725d686ec1ecc89c4ac3bace43d08 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 18 Aug 2026 02:40:20 +0000 Subject: [PATCH 08/16] remove print --- cot/src/error_page.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/cot/src/error_page.rs b/cot/src/error_page.rs index cb2444a5..4f3beca2 100644 --- a/cot/src/error_page.rs +++ b/cot/src/error_page.rs @@ -130,7 +130,6 @@ impl ErrorPageTemplateBuilder { &AbsolutePath::root(), "", ); - println!("\n\nroute_data: {:?}\n\n", self.route_data); self.request_data = diagnostics .request_head .as_ref() From eefac7c3a60bf8966b978e881f9f28cc8c440c7b Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 20 Aug 2026 21:53:56 +0000 Subject: [PATCH 09/16] fix tests after merge conflict resolution --- cot/src/router.rs | 49 ++++++++--- cot/src/router/path.rs | 182 +++++++---------------------------------- 2 files changed, 68 insertions(+), 163 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index 51585891..7156c1ff 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -1418,31 +1418,60 @@ mod tests { } #[test] - fn router_wildcard_route_captures_remaining_path() { + fn router_wildcard_root() { let router = Router::with_urls(vec![Route::with_handler_and_name( - "/static/{*path}", + "/{*path}", MockHandler, - "static_asset", + "users", + )]); + + let found = router.get_handler("/foo/bar").unwrap(); + + assert_eq!(found.name, Some(RouteName("users".to_string()))); + assert_eq!( + found.params, + vec![("path".to_string(), "foo/bar".to_string())] + ); + } + #[test] + fn router_wildcard_single_segment() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/rand/{*path}", + MockHandler, + "users", + )]); + + let found = router.get_handler("/users/rand/foo").unwrap(); + + assert_eq!(found.name, Some(RouteName("users".to_string()))); + assert_eq!(found.params, vec![("path".to_string(), "foo".to_string())]); + } + #[test] + fn router_wildcard_multi_segment() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/users/rand/{*path}", + MockHandler, + "users", )]); - let found = router.get_handler("/static/css/app.css").unwrap(); + let found = router.get_handler("/users/rand/foo/bar").unwrap(); - assert_eq!(found.name, Some(RouteName("static_asset".to_string()))); + assert_eq!(found.name, Some(RouteName("users".to_string()))); assert_eq!( found.params, - vec![("path".to_string(), "css/app.css".to_string())] + vec![("path".to_string(), "foo/bar".to_string())] ); } #[test] - fn router_wildcard_route_rejects_empty_remaining_path() { + fn router_wildcard_empty_not_allowed() { let router = Router::with_urls(vec![Route::with_handler_and_name( - "/static/{*path}", + "/users/rand/{*path}", MockHandler, - "static_asset", + "users", )]); - assert!(router.get_handler("/static/").is_none()); + assert!(router.get_handler("/users/rand").is_none()); } #[test] diff --git a/cot/src/router/path.rs b/cot/src/router/path.rs index 0087c707..963d1610 100644 --- a/cot/src/router/path.rs +++ b/cot/src/router/path.rs @@ -42,15 +42,15 @@ pub(super) enum PathMatcherError { )] #[non_exhaustive] InvalidParamName { pattern: String, name: String }, - /// Same as `InvalidParamName`, but for the name following a `*` in a - /// wildcard segment. + /// Same as [`PathMatcherError::InvalidParamName`], but for the name + /// following a `*` in a wildcard segment. #[error( "{PATH_MATCHER_ERROR_PREFIX} invalid wildcard name `{name}` in pattern `{pattern}`; wildcard names must start \ with a letter or underscore and contain only letters, digits, or underscores" )] #[non_exhaustive] InvalidWildcardName { pattern: String, name: String }, - /// A wildcard segment (`{*name}`) was followed by more path segments, + /// A wildcard segment was followed by more path segments, #[error( "{PATH_MATCHER_ERROR_PREFIX} wildcard parameter `{{*{name}}}` must be the last segment of pattern `{pattern}`; \ a wildcard consumes the rest of the path, so nothing can follow it" @@ -196,13 +196,12 @@ impl PathMatcher { } let next_char = char_iter.peek().map(|(_, ch)| *ch).unwrap_or_default(); - if next_char.is_some(){ + if next_char.is_some() { return Err(PathMatcherError::WildcardNotAtEnd { pattern: path_pattern.clone(), name: wildcard_name.to_string(), }); } - } parts.push(PathPart::Wildcard { name: wildcard_name.to_string(), @@ -213,7 +212,7 @@ impl PathMatcher { pattern: path_pattern.clone(), name: param_name.to_string(), }); - } + } parts.push(PathPart::Param { name: param_name.to_string(), @@ -250,49 +249,6 @@ impl PathMatcher { true } - #[must_use] - pub(crate) fn capture<'matcher, 'path>( - &'matcher self, - path: &'path str, - ) -> Option> { - debug!("Matching path `{}` against pattern `{}`", path, self); - - let mut current_path = path; - let mut params = Vec::with_capacity(self.param_len()); - for part in &self.parts { - match part { - PathPart::Literal(s) => { - if !current_path.starts_with(s) { - return None; - } - current_path = ¤t_path[s.len()..]; - } - PathPart::Wildcard { name } => { - if current_path.is_empty() { - return None; - } - params.push(PathParam::new(name, current_path)); - current_path = ""; - } - PathPart::Param { name } => { - let next_slash = current_path.find('/'); - let value = if let Some(next_slash) = next_slash { - ¤t_path[..next_slash] - } else { - current_path - }; - if value.is_empty() { - return None; - } - params.push(PathParam::new(name, value)); - current_path = ¤t_path[value.len()..]; - } - } - } - - Some(CaptureResult::new(params, current_path)) - } - pub(crate) fn reverse(&self, params: &ReverseParamMap) -> Result { let mut result = String::new(); @@ -448,27 +404,6 @@ pub enum ReverseError { } impl_into_cot_error!(ReverseError); -#[derive(Debug, PartialEq, Eq)] -pub(super) struct CaptureResult<'matcher, 'path> { - pub(super) params: Vec>, - pub(super) remaining_path: &'path str, -} - -impl<'matcher, 'path> CaptureResult<'matcher, 'path> { - #[must_use] - fn new(params: Vec>, remaining_path: &'path str) -> Self { - Self { - params, - remaining_path, - } - } - - #[must_use] - pub(crate) fn matches_fully(&self) -> bool { - self.remaining_path.is_empty() - } -} - #[derive(Debug, Clone)] pub(super) enum PathPart { Literal(String), @@ -489,22 +424,6 @@ impl Display for PathPart { } } -#[derive(Debug, Clone, PartialEq, Eq)] -pub(super) struct PathParam<'a> { - pub(super) name: &'a str, - pub(super) value: String, -} - -impl<'a> PathParam<'a> { - #[must_use] - pub(crate) fn new(name: &'a str, value: &str) -> Self { - Self { - name, - value: value.to_string(), - } - } -} - #[cfg(test)] mod tests { use super::*; @@ -570,22 +489,6 @@ mod tests { ); } - #[test] - fn path_parser_wildcard() { - let path_parser = PathMatcher::new("/static/{*path}"); - assert_eq!(path_parser.to_string(), "/static/{*path}"); - assert_eq!(path_parser.param_names().collect::>(), vec!["path"]); - } - - #[test] - fn reverse_with_wildcard() { - let path_parser = PathMatcher::new("/static/{*path}"); - let mut params = ReverseParamMap::new(); - params.insert("path", "css/app.css"); - - assert_eq!(path_parser.reverse(¶ms).unwrap(), "/static/css/app.css"); - } - #[test] #[should_panic( expected = "route conflict error: consecutive parameters are not allowed in pattern `/users/{id}{post_id}`" @@ -618,22 +521,6 @@ mod tests { let _ = PathMatcher::new("/users/{abc#$%}"); } - #[test] - #[should_panic( - expected = "route conflict error: invalid wildcard name `` in pattern `/users/{*}`; wildcard names must start with a letter or underscore and contain only letters, digits, or underscores" - )] - fn path_parser_invalid_wildcard_name_empty() { - let _ = PathMatcher::new("/users/{*}"); - } - - #[test] - #[should_panic( - expected = "route conflict error: wildcard parameter `{*path}` must be the last segment of pattern `/users/{*path}/edit`; a wildcard consumes the rest of the path, so nothing can follow it" - )] - fn path_parser_wildcard_not_at_end() { - let _ = PathMatcher::new("/users/{*path}/edit"); - } - #[test] #[should_panic( expected = "route conflict error: unclosed parameter `{foo` in pattern `/users/{foo`; expected a closing `}`" @@ -734,54 +621,43 @@ mod tests { } #[test] - fn path_parser_wildcard_root() { - let path_parser = PathMatcher::new("/{*path}"); - assert_eq!( - path_parser.capture("/foo/bar"), - Some(CaptureResult::new( - vec![PathParam::new("path", "foo/bar")], - "" - )) - ); + fn path_parser_wildcard() { + let path_parser = PathMatcher::new("/static/{*path}"); + assert_eq!(path_parser.to_string(), "/static/{*path}"); + assert_eq!(path_parser.param_names().collect::>(), vec!["path"]); } #[test] - fn path_parser_wildcard_single_segment() { - let path_parser = PathMatcher::new("/users/rand/{*path}"); - assert_eq!( - path_parser.capture("/users/rand/foo"), - Some(CaptureResult::new(vec![PathParam::new("path", "foo")], "")) - ); - } + fn reverse_with_wildcard() { + let path_parser = PathMatcher::new("/static/{*path}"); + let mut params = ReverseParamMap::new(); + params.insert("path", "css/app.css"); - #[test] - fn path_parser_wildcard_multi_segment() { - let path_parser = PathMatcher::new("/users/rand/{*path}"); - assert_eq!( - path_parser.capture("/users/rand/foo/bar"), - Some(CaptureResult::new( - vec![PathParam::new("path", "foo/bar")], - "" - )) - ); + assert_eq!(path_parser.reverse(¶ms).unwrap(), "/static/css/app.css"); } #[test] - fn path_parser_wildcard_no_match() { - let path_parser = PathMatcher::new("/prefix/{*path}"); - assert_eq!(path_parser.capture("/other/foo"), None); + #[should_panic( + expected = "route conflict error: wildcard parameter `{*rest}` must be the last segment of pattern `/users/{*rest}/edit`; a wildcard consumes the rest of the path, so nothing can follow it" + )] + fn path_parser_no_path_allowed_after_wildcard() { + let _ = PathMatcher::new("/users/{*rest}/edit"); } #[test] - fn path_parser_wildcard_empty_not_allowed() { - let path_parser = PathMatcher::new("/users/rand/{*path}"); - assert_eq!(path_parser.capture("/users/rand/"), None); + #[should_panic( + expected = "route conflict error: wildcard parameter `{*rest}` must be the last segment of pattern `/users/{*rest}/`; a wildcard consumes the rest of the path, so nothing can follow it" + )] + fn path_parser_trail_slash_not_allowed_after_wildcard() { + let _ = PathMatcher::new("/users/{*rest}/"); } #[test] - #[should_panic(expected = "Wildcard must be the last part of the path: `/users/{*rest}/`")] - fn path_parser_no_path_allowed_after_wildcard() { - let _ = PathMatcher::new("/users/{*rest}/"); + #[should_panic( + expected = "route conflict error: invalid wildcard name `` in pattern `/users/{*}`; wildcard names must start with a letter or underscore and contain only letters, digits, or underscores" + )] + fn path_parser_invalid_wildcard_name_empty() { + let _ = PathMatcher::new("/users/{*}"); } #[test] From 994cbe67987b62b16d73ad348875dea62be100eb Mon Sep 17 00:00:00 2001 From: Elijah Date: Sun, 23 Aug 2026 01:42:45 +0000 Subject: [PATCH 10/16] fix UI tests, more improvs --- cot/src/router.rs | 144 +++++++++++++++++- cot/src/router/tree.rs | 46 +++++- cot/tests/admin.rs | 2 +- .../ui/unimplemented_request_handler.stderr | 66 ++++---- 4 files changed, 223 insertions(+), 35 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index 7156c1ff..c722209c 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -163,7 +163,7 @@ impl Router { if let Some(result) = self.get_handler(request_path) { let mut path_params = PathParams::new(); - for (key, value) in result.params.iter().rev() { + for (key, value) in &result.params { path_params.insert(key.clone(), value.clone()); } request.extensions_mut().insert(path_params); @@ -222,7 +222,10 @@ impl Router { RouteInner::Router(nested_router) => { nested_router.get_handler(remaining_path).map(|mut found| { found.app_name = found.app_name.or_else(|| router.app_name.clone()); - found.params.extend(params.iter().cloned()); + + let mut combined = params.to_vec(); + combined.extend(found.params); + found.params = combined; found }) } @@ -334,7 +337,17 @@ impl Router { { let prefix = AbsolutePath::new(route.url.reverse(params)?); let suffix = AbsolutePath::new(url); - return Ok(Some(prefix.join(&suffix).into())); + + // we are in a sub-router, and if its parent does not end in a trailing slash + // (eg. `foo`) and the found route is the sub-router's root + // (`/`), then we can safely assume that the trailing-slash + // version (eg. `foo/`) does not exist. We return its parent and must not join + let combined = if !prefix.as_str().ends_with('/') && suffix.as_str() == "/" { + prefix + } else { + prefix.join(&suffix) + }; + return Ok(Some(combined.into())); } } Ok(None) @@ -1822,6 +1835,131 @@ mod tests { assert_eq!(response.headers().get("location").unwrap(), "/test/123"); } + #[test] + fn router_reverse_of_nested_index_uses_bare_mount_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "index", + )]); + let router = Router::with_urls(vec![Route::with_router("/admin", sub_router)]); + + let url = router + .reverse(None, "index", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/admin"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_reverse_slash_mounted_root_route_keeps_slash() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "index", + )]); + let router = Router::with_urls(vec![Route::with_router("/admin/", sub_router)]); + + let url = router + .reverse(None, "index", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/admin/"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_reverse_slash_mounted_non_root_route_unaffected() { + let nested_sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/bar/{buz}", + MockHandler, + "bar", + )]); + let sub_router = Router::with_urls(vec![ + Route::with_handler_and_name("/foo", MockHandler, "foo"), + Route::with_router("/fab", nested_sub_router), + ]); + let router = Router::with_urls(vec![Route::with_router("/admin/", sub_router)]); + + let url = router + .reverse(None, "foo", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/admin/foo"); + assert!(router.has_route(&url)); + + let mut params = ReverseParamMap::new(); + params.insert("buz", "random"); + let url = router.reverse(None, "bar", ¶ms).unwrap(); + assert_eq!(url, "/admin/fab/bar/random"); + } + + #[test] + fn router_single_pattern_multi_param_order_preserved() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/{model_name}/{pk}/edit/", + MockHandler, + "edit", + )]); + + let found = router.get_handler("/database_user/1/edit/").unwrap(); + + assert_eq!( + found.params, + vec![ + ("model_name".to_string(), "database_user".to_string()), + ("pk".to_string(), "1".to_string()), + ] + ); + } + + #[test] + fn router_nested_mount_and_leaf_param_order_preserved() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{model_name}/{pk}/edit/", + MockHandler, + "edit", + )]); + let router = Router::with_urls(vec![Route::with_router("/admin", sub_router)]); + + let found = router.get_handler("/admin/database_user/1/edit/").unwrap(); + + assert_eq!( + found.params, + vec![ + ("model_name".to_string(), "database_user".to_string()), + ("pk".to_string(), "1".to_string()), + ] + ); + } + + #[test] + fn router_very_nested_mount_and_leaf_param_order_preserved() { + let nested_sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/foo/{bar}/{*baz}", + MockHandler, + "edit", + )]); + + let sub_router = Router::with_urls(vec![Route::with_router( + "/{model_name}/{pk}/edit/", + nested_sub_router, + )]); + let router = Router::with_urls(vec![Route::with_router("/admin", sub_router)]); + + let found = router + .get_handler("/admin/database_user/1/edit/foo/jon/2/doe") + .unwrap(); + + assert_eq!( + found.params, + vec![ + ("model_name".to_string(), "database_user".to_string()), + ("pk".to_string(), "1".to_string()), + ("bar".to_string(), "jon".to_string()), + ("baz".to_string(), "2/doe".to_string()) + ] + ); + } + fn test_request() -> Request { TestRequestBuilder::get("/test").build() } diff --git a/cot/src/router/tree.rs b/cot/src/router/tree.rs index 27d5f681..313ab095 100644 --- a/cot/src/router/tree.rs +++ b/cot/src/router/tree.rs @@ -47,7 +47,20 @@ impl RouteTrie { let mut pattern_map: HashMap, Option)> = HashMap::new(); for (i, route) in routes.iter().enumerate() { - let pattern = MatchitPattern::try_from(route.url.clone())?; + let pattern = if route.kind() == RouteKind::Router { + // normalize path of sub-routers since we will attach an internal wildcard + // sentinel. This should also allow us reject routes for + // routers(sub-routers) who's version without a trailing slash + // already exist. (eg. `foo` and `foo/`cannot overlap as sub-routers) + let url = route.url(); + let trimmed = url + .strip_suffix('/') + .filter(|s| !s.is_empty()) + .unwrap_or(&url); + MatchitPattern::new(trimmed) + } else { + MatchitPattern::try_from(route.url.clone())? + }; let entry = pattern_map.entry(pattern).or_default(); match route.kind() { RouteKind::Handler => { @@ -81,7 +94,7 @@ impl RouteTrie { .expect("route index should exist") }); - for (pattern, (handler_idx, router_idx)) in entries { + for (_, (handler_idx, router_idx)) in entries { let value = match (handler_idx, router_idx) { (Some(h), None) => Entry::Handler(h), (None, Some(r)) => Entry::Router(r), @@ -94,9 +107,14 @@ impl RouteTrie { let route_idx = handler_idx .or(router_idx) .expect("route index should exist"); + + // we insert the original path, not the (possibly trimmed) deduped route so that + // routers(sub-routers) that were mounted/declared with trailing slashes still + // match. + let insertion_pattern = MatchitPattern::try_from(routes[route_idx].url.clone())?; Self::insert_or_diagnose( &mut inner, - pattern.clone(), + insertion_pattern, value, &routes[route_idx], routes, @@ -106,7 +124,7 @@ impl RouteTrie { // and keep a sentinel there so we can use that to find what sub router to // search at lookup time. if let Some(r) = router_idx { - let prefix = AbsolutePath::new(pattern.as_str()); + let prefix = AbsolutePath::new(routes[route_idx].url()); let wildcard_suffix = AbsolutePath::new(format!("{{*{NESTED_ROUTER_PARAM}}}")); let wildcard = prefix.join(&wildcard_suffix); @@ -373,4 +391,24 @@ mod tests { let s: String = pattern.into(); assert_eq!(s, "/users"); } + + #[test] + fn build_root_mounted_router_pattern_not_trimmed_to_empty() { + let sub_router = Router::with_urls(vec![route("/inner")]); + let routes = vec![Route::with_router("/", sub_router)]; + let trie = RouteTrie::build(&routes).unwrap(); + assert!(trie.at("/inner").is_some()); + } + + #[test] + fn build_router_mount_slash_and_no_slash_variants_conflict_with_clear_error() { + let router1 = Router::with_urls(vec![route("/foo")]); + let router2 = Router::with_urls(vec![route("/bar")]); + let routes = vec![ + Route::with_router("/admin", router1), + Route::with_router("/admin/", router2), + ]; + let err = RouteTrie::build(&routes).unwrap_err(); + assert!(err.to_string().contains("duplicate nested router")); + } } diff --git a/cot/tests/admin.rs b/cot/tests/admin.rs index 46867a0a..556548d9 100644 --- a/cot/tests/admin.rs +++ b/cot/tests/admin.rs @@ -146,7 +146,7 @@ async fn login_with( username: &str, password: &str, ) -> Result<(), Box> { - driver.goto(&format!("{}/admin/", server.url())).await?; + driver.goto(&format!("{}/admin", server.url())).await?; let username_form = driver.find(Locator::Id("username")).await?; username_form.send_keys(username).await?; diff --git a/cot/tests/ui/unimplemented_request_handler.stderr b/cot/tests/ui/unimplemented_request_handler.stderr index e926b9e4..8d248b60 100644 --- a/cot/tests/ui/unimplemented_request_handler.stderr +++ b/cot/tests/ui/unimplemented_request_handler.stderr @@ -1,27 +1,39 @@ -error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler - --> tests/ui/unimplemented_request_handler.rs:8:57 - | -8 | let _ = Router::with_urls([Route::with_handler("/", test)]); - | ------------------- ^^^^ not a valid request handler - | | - | required by a bound introduced by this call - | - = help: the trait `RequestHandler<_>` is not implemented for fn item `fn(()) -> impl Future, cot::Error>> {test}` - = note: make sure the function is marked `async` - = note: make sure all parameters implement `FromRequest` or `FromRequestHead` - = note: make sure there is at most one parameter implementing `FromRequest` - = note: make sure the function takes no more than 10 parameters - = note: make sure the function returns a type that implements `IntoResponse` -help: the trait `RequestHandler` is implemented for `MethodRouter` - --> src/router/method.rs - | - | impl RequestHandler for MethodRouter { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -note: required by a bound in `Route::with_handler` - --> src/router.rs - | - | pub fn with_handler(url: &str, handler: H) -> Self - | ------------ required by a bound in this associated function -... - | H: RequestHandler + Send + Sync + 'static, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` + error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler + --> tests/ui/unimplemented_request_handler.rs:8:57 + | + 8 | let _ = Router::with_urls([Route::with_handler("/", test)]); + | ------------------- ^^^^ not a valid request handler + | | + | required by a bound introduced by this call + | + = help: the trait `RequestHandler<_>` is not implemented for fn item `fn(()) -> impl Future, cot::Error>> {test}` + = note: make sure the function is marked `async` + = note: make sure all parameters implement `FromRequest` or `FromRequestHead` + = note: make sure there is at most one parameter implementing `FromRequest` + = note: make sure the function takes no more than 10 parameters + = note: make sure the function returns a type that implements `IntoResponse` + help: the following other types implement trait `RequestHandler` + --> src/router/method/openapi.rs + | + | impl RequestHandler for ApiMethodRouter { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ApiMethodRouter` implements `RequestHandler` + | + ::: src/router/method.rs + | + | impl RequestHandler for MethodRouter { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `MethodRouter` implements `RequestHandler` + | + ::: src/openapi.rs + | + | / impl RequestHandler for NoApi + | | where + | | H: RequestHandler, + | |_____________________________________^ `cot::openapi::NoApi` implements `RequestHandler` + note: required by a bound in `Route::with_handler` + --> src/router.rs + | + | pub fn with_handler(url: &str, handler: H) -> Self + | ------------ required by a bound in this associated function + ... + | H: RequestHandler + Send + Sync + 'static, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` \ No newline at end of file From a1765343156de7caf73fcff2fc69e34705280974 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 01:43:00 +0000 Subject: [PATCH 11/16] chore(pre-commit.ci): auto fixes from pre-commit hooks --- .../ui/unimplemented_request_handler.stderr | 58 +++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/cot/tests/ui/unimplemented_request_handler.stderr b/cot/tests/ui/unimplemented_request_handler.stderr index 8d248b60..1cac8a15 100644 --- a/cot/tests/ui/unimplemented_request_handler.stderr +++ b/cot/tests/ui/unimplemented_request_handler.stderr @@ -1,11 +1,11 @@ - error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler - --> tests/ui/unimplemented_request_handler.rs:8:57 - | - 8 | let _ = Router::with_urls([Route::with_handler("/", test)]); - | ------------------- ^^^^ not a valid request handler - | | - | required by a bound introduced by this call - | + error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler + --> tests/ui/unimplemented_request_handler.rs:8:57 + | + 8 | let _ = Router::with_urls([Route::with_handler("/", test)]); + | ------------------- ^^^^ not a valid request handler + | | + | required by a bound introduced by this call + | = help: the trait `RequestHandler<_>` is not implemented for fn item `fn(()) -> impl Future, cot::Error>> {test}` = note: make sure the function is marked `async` = note: make sure all parameters implement `FromRequest` or `FromRequestHead` @@ -16,24 +16,24 @@ --> src/router/method/openapi.rs | | impl RequestHandler for ApiMethodRouter { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ApiMethodRouter` implements `RequestHandler` - | - ::: src/router/method.rs - | - | impl RequestHandler for MethodRouter { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `MethodRouter` implements `RequestHandler` - | - ::: src/openapi.rs - | - | / impl RequestHandler for NoApi - | | where - | | H: RequestHandler, - | |_____________________________________^ `cot::openapi::NoApi` implements `RequestHandler` - note: required by a bound in `Route::with_handler` - --> src/router.rs - | - | pub fn with_handler(url: &str, handler: H) -> Self - | ------------ required by a bound in this associated function - ... - | H: RequestHandler + Send + Sync + 'static, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` \ No newline at end of file + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ApiMethodRouter` implements `RequestHandler` + | + ::: src/router/method.rs + | + | impl RequestHandler for MethodRouter { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `MethodRouter` implements `RequestHandler` + | + ::: src/openapi.rs + | + | / impl RequestHandler for NoApi + | | where + | | H: RequestHandler, + | |_____________________________________^ `cot::openapi::NoApi` implements `RequestHandler` + note: required by a bound in `Route::with_handler` + --> src/router.rs + | + | pub fn with_handler(url: &str, handler: H) -> Self + | ------------ required by a bound in this associated function + ... + | H: RequestHandler + Send + Sync + 'static, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` From 0a257a778150ba609f192f85c59d6b7fdb8a31c2 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 25 Aug 2026 04:10:47 +0000 Subject: [PATCH 12/16] add more tests --- cot/src/router.rs | 1066 ++++++++++++++++++++++++++++++++------------- 1 file changed, 773 insertions(+), 293 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index c722209c..b83b2ba5 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -1246,12 +1246,51 @@ mod tests { } #[test] - fn router_with_urls() { + fn route_with_handler() { + let route = Route::with_handler("/test", MockHandler); + assert_eq!(route.url.to_string(), "/test"); + } + + #[test] + fn route_with_handler_and_params() { + let route = Route::with_handler("/test/{id}", MockHandler); + assert_eq!(route.url.to_string(), "/test/{id}"); + } + + #[test] + fn route_with_handler_and_name() { + let route = Route::with_handler_and_name("/test", MockHandler, "test"); + assert_eq!(route.url.to_string(), "/test"); + assert_eq!(route.name, Some(RouteName("test".to_string()))); + } + + #[test] + fn route_with_router() { + let sub_route = Route::with_handler("/sub", MockHandler); + let sub_router = Router::with_urls(vec![sub_route]); + let route = Route::with_router("/test", sub_router); + assert_eq!(route.url.to_string(), "/test"); + } + + #[test] + fn router_is_empty() { + let router = Router::with_urls(vec![]); + assert!(router.is_empty()); + } + + #[test] + fn router_routes() { let route = Route::with_handler("/test", MockHandler); let router = Router::with_urls(vec![route.clone()]); assert_eq!(router.routes().len(), 1); } + #[test] + fn router_empty_returns_no_handler() { + let router = Router::empty(); + assert!(router.get_handler("/").is_none()); + } + #[cot::test] async fn router_route() { let route = Route::with_handler("/test", MockHandler); @@ -1301,25 +1340,6 @@ mod tests { assert_eq!(response.status(), StatusCode::OK); } - #[test] - fn router_reverse() { - let route = Route::with_handler_and_name("/test", MockHandler, "test"); - let router = Router::with_urls(vec![route.clone()]); - let params = ReverseParamMap::new(); - let url = router.reverse(None, "test", ¶ms).unwrap(); - assert_eq!(url, "/test"); - } - - #[test] - fn router_reverse_with_param() { - let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); - let router = Router::with_urls(vec![route.clone()]); - let mut params = ReverseParamMap::new(); - params.insert("id", "123"); - let url = router.reverse(None, "test", ¶ms).unwrap(); - assert_eq!(url, "/test/123"); - } - #[test] fn router_no_param_route_matches_exact_path() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1345,6 +1365,29 @@ mod tests { assert!(router.get_handler("/test").is_none()); } + #[test] + fn router_routes_with_common_static_prefixes_match_independently() { + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/car", MockHandler, "car"), + Route::with_handler_and_name("/cart", MockHandler, "cart"), + Route::with_handler_and_name("/catalog", MockHandler, "catalog"), + ]); + + assert_eq!( + router.get_handler("/car").unwrap().name, + Some(RouteName("car".to_string())) + ); + assert_eq!( + router.get_handler("/cart").unwrap().name, + Some(RouteName("cart".to_string())) + ); + assert_eq!( + router.get_handler("/catalog").unwrap().name, + Some(RouteName("catalog".to_string())) + ); + assert!(router.get_handler("/cartographer").is_none()); + } + #[test] fn router_param_route_captures_single_segment() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1395,29 +1438,6 @@ mod tests { assert_params(found.params, &[("id", "123"), ("post_id", "456")]); } - #[test] - fn router_routes_with_common_static_prefixes_match_independently() { - let router = Router::with_urls(vec![ - Route::with_handler_and_name("/car", MockHandler, "car"), - Route::with_handler_and_name("/cart", MockHandler, "cart"), - Route::with_handler_and_name("/catalog", MockHandler, "catalog"), - ]); - - assert_eq!( - router.get_handler("/car").unwrap().name, - Some(RouteName("car".to_string())) - ); - assert_eq!( - router.get_handler("/cart").unwrap().name, - Some(RouteName("cart".to_string())) - ); - assert_eq!( - router.get_handler("/catalog").unwrap().name, - Some(RouteName("catalog".to_string())) - ); - assert!(router.get_handler("/cartographer").is_none()); - } - #[test] fn router_static_route_takes_priority_over_dynamic_route() { let router = Router::with_urls(vec![ @@ -1430,6 +1450,25 @@ mod tests { assert_eq!(found.name, Some(RouteName("static".to_string()))); } + #[test] + fn router_single_pattern_multi_param_order_preserved() { + let router = Router::with_urls(vec![Route::with_handler_and_name( + "/{model_name}/{pk}/edit/", + MockHandler, + "edit", + )]); + + let found = router.get_handler("/database_user/1/edit/").unwrap(); + + assert_eq!( + found.params, + vec![ + ("model_name".to_string(), "database_user".to_string()), + ("pk".to_string(), "1".to_string()), + ] + ); + } + #[test] fn router_wildcard_root() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1446,6 +1485,7 @@ mod tests { vec![("path".to_string(), "foo/bar".to_string())] ); } + #[test] fn router_wildcard_single_segment() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1459,6 +1499,7 @@ mod tests { assert_eq!(found.name, Some(RouteName("users".to_string()))); assert_eq!(found.params, vec![("path".to_string(), "foo".to_string())]); } + #[test] fn router_wildcard_multi_segment() { let router = Router::with_urls(vec![Route::with_handler_and_name( @@ -1500,116 +1541,31 @@ mod tests { } #[test] - fn router_nested_router_consumes_remaining_path() { + fn router_root_mount_matches_root_path() { let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "/posts/{post_id}", + "/", MockHandler, - "post_detail", + "index", )]); - let router = Router::with_urls(vec![Route::with_router("/users/{id}", sub_router)]); + let router = Router::with_urls(vec![Route::with_router("", sub_router)]); - let found = router.get_handler("/users/123/posts/456").unwrap(); + let found = router.get_handler("/").unwrap(); - assert_eq!(found.name, Some(RouteName("post_detail".to_string()))); - assert_params(found.params, &[("id", "123"), ("post_id", "456")]); + assert_eq!(found.name, Some(RouteName("index".to_string()))); } #[test] - fn router_handler_takes_priority_over_nested_router_at_same_path() { + fn router_exact_mount_match_routes_to_nested_root_not_empty_path() { let sub_router = Router::with_urls(vec![Route::with_handler_and_name( "/", MockHandler, - "nested", + "sub_index", )]); - let router = Router::with_urls(vec![ - Route::with_router("/users", sub_router), - Route::with_handler_and_name("/users", MockHandler, "handler"), - ]); - - let found = router.get_handler("/users").unwrap(); - - assert_eq!(found.name, Some(RouteName("handler".to_string()))); - } - - #[test] - #[should_panic( - expected = "route conflict error: duplicate route: `/users` conflicts with an already registered handler route `/users` (both fully match the same path)" - )] - fn router_duplicate_handler_routes_panic() { - let _ = Router::with_urls(vec![ - Route::with_handler("/users", MockHandler), - Route::with_handler("/users", MockHandler), - ]); - } - - #[test] - #[should_panic( - expected = "route conflict error: duplicate nested router: `/users` conflicts with an already registered nested router mounted at `/users`" - )] - fn router_duplicate_nested_router_routes_panic() { - let _ = Router::with_urls(vec![ - Route::with_router("/users", Router::empty()), - Route::with_router("/users", Router::empty()), - ]); - } - - #[test] - #[should_panic( - expected = "route conflict error: conflicting route parameters: `/foo/{bar}` uses `{bar}` but `/foo/{baz}` uses `{baz}` at the same position in the path; both routes must bind the same parameter name there, since only one value can be captured at that position" - )] - fn router_conflicting_param_names_panic() { - let _ = Router::with_urls(vec![ - Route::with_handler("/foo/{bar}", MockHandler), - Route::with_handler("/foo/{baz}", MockHandler), - ]); - } - - #[test] - fn router_same_path_with_trailing_lash_diff() { - // this should not fail - let _ = Router::with_urls(vec![ - Route::with_handler("/foo/{bar}/", MockHandler), - Route::with_handler("/foo/{baz}", MockHandler), - ]); - } - - #[test] - #[should_panic( - expected = "route conflict error: duplicate route: `/static/{*path}` conflicts with an already registered handler route `/static/{*path}` (both fully match the same path)" - )] - fn router_duplicate_wildcard_routes_panic() { - let _ = Router::with_urls(vec![ - Route::with_handler("/static/{*path}", MockHandler), - Route::with_handler("/static/{*path}", MockHandler), - ]); - } - - #[test] - #[should_panic( - expected = "route conflict error: conflicting wildcard parameters: `/static/{*path}` uses `{*path}` but `/static/{*file_path}` uses `{*file_path}` at the same position in the path" - )] - fn router_conflicting_wildcard_names_panic() { - let _ = Router::with_urls(vec![ - Route::with_handler("/static/{*path}", MockHandler), - Route::with_handler("/static/{*file_path}", MockHandler), - ]); - } + let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); - #[test] - #[should_panic( - expected = "route conflict error: duplicate route: `/static/{*file_path}` conflicts with an already registered handler route `/static/{path}` (both fully match the same path)" - )] - fn router_wildcard_and_param_at_same_segment_conflict() { - let _ = Router::with_urls(vec![ - Route::with_handler("/static/{path}", MockHandler), - Route::with_handler("/static/{*file_path}", MockHandler), - ]); - } + let found = router.get_handler("/api").unwrap(); - #[test] - fn router_empty_returns_no_handler() { - let router = Router::empty(); - assert!(router.get_handler("/").is_none()); + assert_eq!(found.name, Some(RouteName("sub_index".to_string()))); } #[test] @@ -1626,138 +1582,619 @@ mod tests { } #[test] - fn router_nested_router_trailing_slash_prefix() { + fn router_root_mounted_nested_router_empty() { let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "/inner", + "", MockHandler, "inner", )]); - let router = Router::with_urls(vec![Route::with_router("/api/", sub_router)]); + let router = Router::with_urls(vec![Route::with_router("/outer", sub_router)]); - let found = router.get_handler("/api/inner").unwrap(); + let found = router.get_handler("/outer").unwrap(); assert_eq!(found.name, Some(RouteName("inner".to_string()))); - } - #[test] - fn router_reverse_option_wrong_app_name_returns_none() { - let route = Route::with_handler_and_name("/test", MockHandler, "test"); - let mut router = Router::with_urls(vec![route]); - router.set_app_name(AppName("app_1".to_string())); + assert!(router.get_handler("/outer/").is_none()); + assert!(router.get_handler("outer/").is_none()); + assert!(router.get_handler("outer").is_none()); - let result = router - .reverse_option(Some("app_2"), "test", &ReverseParamMap::new()) + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) .unwrap(); - - assert!(result.is_none()); + assert_eq!(url, "/outer"); + assert!(router.has_route(&url)); } #[test] - fn router_reverse_missing_view_returns_error() { - let router = Router::empty(); - let result = router.reverse(None, "missing", &ReverseParamMap::new()); - assert!(result.is_err()); - } - + fn router_root_mounted_nested_router_empty_and_root_without_slash() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "inner", + )]); + // this should normalize to `/outer` + let router = Router::with_urls(vec![Route::with_router("outer", sub_router)]); + + let found = router.get_handler("/outer").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + assert!(router.get_handler("/outer/").is_none()); + assert!(router.get_handler("outer/").is_none()); + assert!(router.get_handler("outer").is_none()); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/outer"); + assert!(router.has_route(&url)); + } + #[test] - fn router_root_mount_matches_root_path() { + fn router_root_mounted_nested_router_empty_root() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "inner", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("", sub_router)]); + + let found = router.get_handler("/inner").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + // remaining path becomes "/inner/", sub-router only registered "/inner" + assert!(router.get_handler("/inner/").is_none()); + assert!(router.get_handler("inner").is_none()); + assert!(router.get_handler("inner/").is_none()); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/inner"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_root_mounted_nested_router_empty_root_empty_nested() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("", sub_router)]); + + // exact match at the mount point, remaining defaults to root "/" + let found = router.get_handler("/").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + // wildcard sentinel capturing a literal "/" (non-empty, so legal). remaining "/" + let found = router.get_handler("//").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_root_mounted_nested_router_slash_root_slash_nested() { let sub_router = Router::with_urls(vec![Route::with_handler_and_name( "/", MockHandler, - "index", + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); + + let found = router.get_handler("/").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let found = router.get_handler("//").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_root_mounted_nested_router_slash_root_empty_nested() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); + + let found = router.get_handler("/").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let found = router.get_handler("//").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_root_mounted_nested_router_empty_root_slash_nested() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "inner", )]); let router = Router::with_urls(vec![Route::with_router("", sub_router)]); let found = router.get_handler("/").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); - assert_eq!(found.name, Some(RouteName("index".to_string()))); + let found = router.get_handler("//").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/"); + assert!(router.has_route(&url)); + } + + #[test] + fn router_nested_router_trailing_slash_prefix() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/inner", + MockHandler, + "inner", + )]); + let router = Router::with_urls(vec![Route::with_router("/api/", sub_router)]); + + let found = router.get_handler("/api/inner").unwrap(); + assert_eq!(found.name, Some(RouteName("inner".to_string()))); + } + + #[test] + fn router_nested_router_consumes_remaining_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/posts/{post_id}", + MockHandler, + "post_detail", + )]); + let router = Router::with_urls(vec![Route::with_router("/users/{id}", sub_router)]); + + let found = router.get_handler("/users/123/posts/456").unwrap(); + + assert_eq!(found.name, Some(RouteName("post_detail".to_string()))); + assert_params(found.params, &[("id", "123"), ("post_id", "456")]); + } + + #[test] + fn router_param_mount_param_nested_captures_both_in_order() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{sub_id}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/{id}", sub_router)]); + + let found = router.get_handler("/123/456").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_eq!( + found.params, + vec![ + ("id".to_string(), "123".to_string()), + ("sub_id".to_string(), "456".to_string()), + ] + ); + } + + #[test] + fn router_param_mount_wildcard_nested_exact_match_fails_deep_match_works() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/{id}", sub_router)]); + + assert!(router.get_handler("/123").is_none()); + + let found = router.get_handler("/123/a/b").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_params(found.params, &[("id", "123"), ("rest", "a/b")]); + } + + #[test] + fn router_param_mount_trailing_slash_empty_nested_captures_param() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/{id}/", sub_router)]); + + let found = router.get_handler("/123/").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_eq!(found.params, vec![("id".to_string(), "123".to_string())]); + } + + #[test] + fn router_param_mount_trailing_slash_bare_path_fails() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/{id}/", sub_router)]); + assert!(router.get_handler("/123").is_none()); + } + + #[test] + fn router_duplicate_param_name_across_nesting_levels_allowed() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{id}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/{id}", sub_router)]); + + let found = router.get_handler("/1/2").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_eq!( + found.params, + vec![ + ("id".to_string(), "1".to_string()), + ("id".to_string(), "2".to_string()), + ] + ); + } + + #[test] + fn router_bare_mount_match_fails_when_nested_is_wildcard_only() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); + + // exact mount match -> remaining defaults to "/" -> nested catch-all can't match it. + assert!(router.get_handler("/api").is_none()); + + let found = router.get_handler("/api/x/y").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_params(found.params, &[("rest", "x/y")]); + } + + #[test] + fn router_multi_segment_mount_with_wildcard_nested() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/a/b", sub_router)]); + + assert!(router.get_handler("/a/b").is_none()); + let found = router.get_handler("/a/b/c/d").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_params(found.params, &[("rest", "c/d")]); + } + + #[test] + fn router_slash_mount_wildcard_nested_bare_slash_fails() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*path}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/files/", sub_router)]); + + assert!(router.get_handler("/files/").is_none()); // remaining "/" vs catch-all + assert!(router.get_handler("/files").is_none()); // no trailing slash, no match at all + + let found = router.get_handler("/files/x").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_params(found.params, &[("path", "x")]); + } + + #[test] + fn router_multi_segment_slash_mount_param_nested() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{id}", + MockHandler, + "leaf", + )]); + let router = Router::with_urls(vec![Route::with_router("/a/b/", sub_router)]); + + assert!(router.get_handler("/a/b").is_none()); + assert!(router.get_handler("/a/b/").is_none()); + + let found = router.get_handler("/a/b/42").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_params(found.params, &[("id", "42")]); + } + + #[test] + fn router_handler_takes_priority_over_nested_router_at_same_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "nested", + )]); + let router = Router::with_urls(vec![ + Route::with_router("/users", sub_router), + Route::with_handler_and_name("/users", MockHandler, "handler"), + ]); + + let found = router.get_handler("/users").unwrap(); + + assert_eq!(found.name, Some(RouteName("handler".to_string()))); + } + + #[test] + fn router_static_nested_mount_priority_over_sibling_wildcard_mount() { + let generic_sub = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "generic", + )]); + let specific_sub = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "specific", + )]); + let router = Router::with_urls(vec![ + Route::with_router("/admin", generic_sub), + Route::with_router("/admin/extra", specific_sub), + ]); + + let found = router.get_handler("/admin/extra/more").unwrap(); + assert_eq!(found.name, Some(RouteName("specific".to_string()))); + assert_params(found.params, &[("rest", "more")]); + + let found = router.get_handler("/admin/other/thing").unwrap(); + assert_eq!(found.name, Some(RouteName("generic".to_string()))); + assert_params(found.params, &[("rest", "other/thing")]); + } + + #[test] + fn router_handler_priority_swallows_routers_own_trailing_slash_exact_entry() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "sub_catch_all", + )]); + let router = Router::with_urls(vec![ + Route::with_handler_and_name("/users", MockHandler, "handler"), + Route::with_router("/users/", sub_router), + ]); + + let found = router.get_handler("/users").unwrap(); + assert_eq!(found.name, Some(RouteName("handler".to_string()))); + assert!(router.get_handler("/users/").is_none()); + + let found = router.get_handler("/users/anything").unwrap(); + assert_eq!(found.name, Some(RouteName("sub_catch_all".to_string()))); + assert_params(found.params, &[("rest", "anything")]); + } + + #[test] + fn router_triple_nested_all_empty_mounts_reachable_via_single_slash() { + let leaf = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "leaf", + )]); + let mid = Router::with_urls(vec![Route::with_router("", leaf)]); + let router = Router::with_urls(vec![Route::with_router("", mid)]); + + let found = router.get_handler("/").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + } + + #[test] + fn router_triple_nested_param_then_empty_then_param() { + let leaf = Router::with_urls(vec![Route::with_handler_and_name( + "/{b}", + MockHandler, + "leaf", + )]); + let mid = Router::with_urls(vec![Route::with_router("", leaf)]); + let router = Router::with_urls(vec![Route::with_router("/{a}", mid)]); + + let found = router.get_handler("/1/2").unwrap(); + assert_eq!(found.name, Some(RouteName("leaf".to_string()))); + assert_eq!( + found.params, + vec![ + ("a".to_string(), "1".to_string()), + ("b".to_string(), "2".to_string()), + ] + ); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate route: `/users` conflicts with an already registered handler route `/users` (both fully match the same path)" + )] + fn router_duplicate_handler_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/users", MockHandler), + Route::with_handler("/users", MockHandler), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate route: `/users/` conflicts with an already registered handler route `/users/` (both fully match the same path)" + )] + fn router_duplicate_handler_routes_with_trailing_slash_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/users/", MockHandler), + Route::with_handler("/users/", MockHandler), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate nested router: `/users` conflicts with an already registered nested router mounted at `/users`" + )] + fn router_duplicate_nested_router_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_router("/users", Router::empty()), + Route::with_router("/users", Router::empty()), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate nested router: `/users/` conflicts with an already registered nested router mounted at `/users/`" + )] + fn router_duplicate_nested_router_routes_trailing_slash_panic() { + let _ = Router::with_urls(vec![ + Route::with_router("/users/", Router::empty()), + Route::with_router("/users/", Router::empty()), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate nested router: `/users` conflicts with an already registered nested router mounted at `/users/`" + )] + fn router_duplicate_with_trailing_slash_diff_panic() { + let _ = Router::with_urls(vec![ + Route::with_router("/users/", Router::empty()), + Route::with_router("/users", Router::empty()), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: conflicting route parameters: `/foo/{bar}` uses `{bar}` but `/foo/{baz}` uses `{baz}` at the same position in the path; both routes must bind the same parameter name there, since only one value can be captured at that position" + )] + fn router_conflicting_param_names_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/foo/{bar}", MockHandler), + Route::with_handler("/foo/{baz}", MockHandler), + ]); + } + + #[test] + fn router_same_path_with_trailing_lash_diff() { + // this should not fail + let _ = Router::with_urls(vec![ + Route::with_handler("/foo/{bar}/", MockHandler), + Route::with_handler("/foo/{baz}", MockHandler), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate route: `/static/{*path}` conflicts with an already registered handler route `/static/{*path}` (both fully match the same path)" + )] + fn router_duplicate_wildcard_routes_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{*path}", MockHandler), + Route::with_handler("/static/{*path}", MockHandler), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: conflicting wildcard parameters: `/static/{*path}` uses `{*path}` but `/static/{*file_path}` uses `{*file_path}` at the same position in the path" + )] + fn router_conflicting_wildcard_names_panic() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{*path}", MockHandler), + Route::with_handler("/static/{*file_path}", MockHandler), + ]); + } + + #[test] + #[should_panic( + expected = "route conflict error: duplicate route: `/static/{*file_path}` conflicts with an already registered handler route `/static/{path}` (both fully match the same path)" + )] + fn router_wildcard_and_param_at_same_segment_conflict() { + let _ = Router::with_urls(vec![ + Route::with_handler("/static/{path}", MockHandler), + Route::with_handler("/static/{*file_path}", MockHandler), + ]); + } + + #[test] + #[should_panic(expected = "route conflict error")] + fn router_wildcard_mount_with_static_nested_errors() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/leaf", + MockHandler, + "leaf", + )]); + let _ = Router::with_urls(vec![Route::with_router("/{*rest}", sub_router)]); } #[test] - fn router_exact_mount_match_routes_to_nested_root_not_empty_path() { + #[should_panic(expected = "route conflict error")] + fn router_wildcard_mount_with_param_nested_errors() { let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "/", + "/{id}", MockHandler, - "sub_index", + "leaf", )]); - let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); - - let found = router.get_handler("/api").unwrap(); - - assert_eq!(found.name, Some(RouteName("sub_index".to_string()))); + let _ = Router::with_urls(vec![Route::with_router("/{*rest}", sub_router)]); } #[test] - fn router_reverse_root_mount_no_double_slash() { - let route = Route::with_handler_and_name("/", MockHandler, "index"); - let sub_router = Router::with_urls(vec![route]); - let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); - - let url = router - .reverse(None, "index", &ReverseParamMap::new()) - .unwrap(); - - assert_eq!(url, "/"); + #[should_panic(expected = "route conflict error")] + fn router_wildcard_mount_with_empty_nested_errors() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "leaf", + )]); + let _ = Router::with_urls(vec![Route::with_router("/{*rest}", sub_router)]); } #[test] - fn router_reverse_nested_under_root_mount_no_double_slash() { - let route = Route::with_handler_and_name("/inner", MockHandler, "inner"); - let sub_router = Router::with_urls(vec![route]); - let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); - - let url = router - .reverse(None, "inner", &ReverseParamMap::new()) - .unwrap(); - - assert_eq!(url, "/inner"); + #[should_panic(expected = "route conflict error")] + fn router_wildcard_mount_with_wildcard_nested_errors() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/{*rest}", + MockHandler, + "leaf", + )]); + let _ = Router::with_urls(vec![Route::with_router("/{*outer}", sub_router)]); } #[test] - fn router_reverse_deeply_nested_root_mounts_no_double_slash() { - let route = Route::with_handler_and_name("/leaf", MockHandler, "leaf"); - let inner_router = Router::with_urls(vec![route]); - let mid_router = Router::with_urls(vec![Route::with_router("/", inner_router)]); - let router = Router::with_urls(vec![Route::with_router("/", mid_router)]); - - let url = router - .reverse(None, "leaf", &ReverseParamMap::new()) - .unwrap(); - - assert_eq!(url, "/leaf"); + #[should_panic(expected = "route conflict error")] + fn router_prefixed_wildcard_mount_errors() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "", + MockHandler, + "leaf", + )]); + let _ = Router::with_urls(vec![Route::with_router("/files/{*path}", sub_router)]); } #[test] - fn router_reverse_app_name() { + fn router_reverse() { let route = Route::with_handler_and_name("/test", MockHandler, "test"); - let mut router_1 = Router::with_urls(vec![route.clone()]); - router_1.set_app_name(AppName("app_1".to_string())); - let mut router_2 = Router::with_urls(vec![route.clone()]); - router_2.set_app_name(AppName("app_2".to_string())); - let root_router = Router::with_urls(vec![ - Route::with_router("/", router_1), - Route::with_router("/sub", router_2), - ]); - + let router = Router::with_urls(vec![route.clone()]); let params = ReverseParamMap::new(); - let url = root_router.reverse(Some("app_2"), "test", ¶ms).unwrap(); - - assert_eq!(url, "/sub/test"); + let url = router.reverse(None, "test", ¶ms).unwrap(); + assert_eq!(url, "/test"); } #[test] - fn router_reverse_app_name_nested() { - let route = Route::with_handler_and_name("/test", MockHandler, "test"); + fn router_reverse_with_param() { + let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); let router = Router::with_urls(vec![route.clone()]); - let sub_router = Router::with_urls(vec![Route::with_router("/sub", router)]); - let mut root_router = Router::with_urls(vec![Route::with_router("/subsub", sub_router)]); - root_router.set_app_name(AppName("app_root".to_string())); - - let params = ReverseParamMap::new(); - let url = root_router - .reverse(Some("app_root"), "test", ¶ms) - .unwrap(); - - assert_eq!(url, "/subsub/sub/test"); + let mut params = ReverseParamMap::new(); + params.insert("id", "123"); + let url = router.reverse(None, "test", ¶ms).unwrap(); + assert_eq!(url, "/test/123"); } #[test] @@ -1773,82 +2210,80 @@ mod tests { } #[test] - fn router_routes() { - let route = Route::with_handler("/test", MockHandler); - let router = Router::with_urls(vec![route.clone()]); - assert_eq!(router.routes().len(), 1); - } + fn router_reverse_option_wrong_app_name_returns_none() { + let route = Route::with_handler_and_name("/test", MockHandler, "test"); + let mut router = Router::with_urls(vec![route]); + router.set_app_name(AppName("app_1".to_string())); - #[test] - fn router_is_empty() { - let router = Router::with_urls(vec![]); - assert!(router.is_empty()); - } + let result = router + .reverse_option(Some("app_2"), "test", &ReverseParamMap::new()) + .unwrap(); - #[test] - fn route_with_handler() { - let route = Route::with_handler("/test", MockHandler); - assert_eq!(route.url.to_string(), "/test"); + assert!(result.is_none()); } #[test] - fn route_with_handler_and_params() { - let route = Route::with_handler("/test/{id}", MockHandler); - assert_eq!(route.url.to_string(), "/test/{id}"); - } + fn router_reverse_missing_view_returns_error() { + let router = Router::empty(); - #[test] - fn route_with_handler_and_name() { - let route = Route::with_handler_and_name("/test", MockHandler, "test"); - assert_eq!(route.url.to_string(), "/test"); - assert_eq!(route.name, Some(RouteName("test".to_string()))); + let result = router.reverse(None, "missing", &ReverseParamMap::new()); + assert!(result.is_err()); } #[test] - fn route_with_router() { - let sub_route = Route::with_handler("/sub", MockHandler); - let sub_router = Router::with_urls(vec![sub_route]); - let route = Route::with_router("/test", sub_router); - assert_eq!(route.url.to_string(), "/test"); + fn router_reverse_of_nested_index_uses_bare_mount_path() { + let sub_router = Router::with_urls(vec![Route::with_handler_and_name( + "/", + MockHandler, + "index", + )]); + let router = Router::with_urls(vec![Route::with_router("/admin", sub_router)]); + + let url = router + .reverse(None, "index", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/admin"); + assert!(router.has_route(&url)); } #[test] - fn test_reverse_macro() { - let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); - let router = Router::with_urls(vec![route]); + fn router_reverse_root_mount_no_double_slash() { + let route = Route::with_handler_and_name("/", MockHandler, "index"); + let sub_router = Router::with_urls(vec![route]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); - let request = TestRequestBuilder::get("/").router(router).build(); - let url = reverse!(request, "test", id = 123).unwrap(); + let url = router + .reverse(None, "index", &ReverseParamMap::new()) + .unwrap(); - assert_eq!(url, "/test/123"); + assert_eq!(url, "/"); } #[test] - fn test_reverse_redirect_macro() { - let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); - let router = Router::with_urls(vec![route]); + fn router_reverse_nested_under_root_mount_no_double_slash() { + let route = Route::with_handler_and_name("/inner", MockHandler, "inner"); + let sub_router = Router::with_urls(vec![route]); + let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); - let request = TestRequestBuilder::get("/").router(router).build(); - let response = cot::reverse_redirect!(request, "test", id = 123).unwrap(); + let url = router + .reverse(None, "inner", &ReverseParamMap::new()) + .unwrap(); - assert_eq!(response.status(), StatusCode::SEE_OTHER); - assert_eq!(response.headers().get("location").unwrap(), "/test/123"); + assert_eq!(url, "/inner"); } #[test] - fn router_reverse_of_nested_index_uses_bare_mount_path() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "/", - MockHandler, - "index", - )]); - let router = Router::with_urls(vec![Route::with_router("/admin", sub_router)]); + fn router_reverse_deeply_nested_root_mounts_no_double_slash() { + let route = Route::with_handler_and_name("/leaf", MockHandler, "leaf"); + let inner_router = Router::with_urls(vec![route]); + let mid_router = Router::with_urls(vec![Route::with_router("/", inner_router)]); + let router = Router::with_urls(vec![Route::with_router("/", mid_router)]); let url = router - .reverse(None, "index", &ReverseParamMap::new()) + .reverse(None, "leaf", &ReverseParamMap::new()) .unwrap(); - assert_eq!(url, "/admin"); - assert!(router.has_route(&url)); + + assert_eq!(url, "/leaf"); } #[test] @@ -1872,11 +2307,12 @@ mod tests { let nested_sub_router = Router::with_urls(vec![Route::with_handler_and_name( "/bar/{buz}", MockHandler, - "bar", + "biz", )]); let sub_router = Router::with_urls(vec![ Route::with_handler_and_name("/foo", MockHandler, "foo"), Route::with_router("/fab", nested_sub_router), + Route::with_handler_and_name("/bar/", MockHandler, "bar") ]); let router = Router::with_urls(vec![Route::with_router("/admin/", sub_router)]); @@ -1888,27 +2324,48 @@ mod tests { let mut params = ReverseParamMap::new(); params.insert("buz", "random"); - let url = router.reverse(None, "bar", ¶ms).unwrap(); + let url = router.reverse(None, "biz", ¶ms).unwrap(); assert_eq!(url, "/admin/fab/bar/random"); + + let url = router + .reverse(None, "bar", &ReverseParamMap::new()) + .unwrap(); + assert_eq!(url, "/admin/bar/"); + assert!(router.has_route(&url)); } #[test] - fn router_single_pattern_multi_param_order_preserved() { - let router = Router::with_urls(vec![Route::with_handler_and_name( - "/{model_name}/{pk}/edit/", - MockHandler, - "edit", - )]); + fn router_reverse_app_name() { + let route = Route::with_handler_and_name("/test", MockHandler, "test"); + let mut router_1 = Router::with_urls(vec![route.clone()]); + router_1.set_app_name(AppName("app_1".to_string())); + let mut router_2 = Router::with_urls(vec![route.clone()]); + router_2.set_app_name(AppName("app_2".to_string())); + let root_router = Router::with_urls(vec![ + Route::with_router("/", router_1), + Route::with_router("/sub", router_2), + ]); - let found = router.get_handler("/database_user/1/edit/").unwrap(); + let params = ReverseParamMap::new(); + let url = root_router.reverse(Some("app_2"), "test", ¶ms).unwrap(); - assert_eq!( - found.params, - vec![ - ("model_name".to_string(), "database_user".to_string()), - ("pk".to_string(), "1".to_string()), - ] - ); + assert_eq!(url, "/sub/test"); + } + + #[test] + fn router_reverse_app_name_nested() { + let route = Route::with_handler_and_name("/test", MockHandler, "test"); + let router = Router::with_urls(vec![route.clone()]); + let sub_router = Router::with_urls(vec![Route::with_router("/sub", router)]); + let mut root_router = Router::with_urls(vec![Route::with_router("/subsub", sub_router)]); + root_router.set_app_name(AppName("app_root".to_string())); + + let params = ReverseParamMap::new(); + let url = root_router + .reverse(Some("app_root"), "test", ¶ms) + .unwrap(); + + assert_eq!(url, "/subsub/sub/test"); } #[test] @@ -1960,6 +2417,29 @@ mod tests { ); } + #[test] + fn test_reverse_macro() { + let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); + let router = Router::with_urls(vec![route]); + + let request = TestRequestBuilder::get("/").router(router).build(); + let url = reverse!(request, "test", id = 123).unwrap(); + + assert_eq!(url, "/test/123"); + } + + #[test] + fn test_reverse_redirect_macro() { + let route = Route::with_handler_and_name("/test/{id}", MockHandler, "test"); + let router = Router::with_urls(vec![route]); + + let request = TestRequestBuilder::get("/").router(router).build(); + let response = cot::reverse_redirect!(request, "test", id = 123).unwrap(); + + assert_eq!(response.status(), StatusCode::SEE_OTHER); + assert_eq!(response.headers().get("location").unwrap(), "/test/123"); + } + fn test_request() -> Request { TestRequestBuilder::get("/test").build() } From f5d32140042bfb380da678393a8d8ec25bdfc038 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 25 Aug 2026 04:11:42 +0000 Subject: [PATCH 13/16] lint --- cot/src/router.rs | 70 +++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 48 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index b83b2ba5..7973282a 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -1583,11 +1583,8 @@ mod tests { #[test] fn router_root_mounted_nested_router_empty() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "inner", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "inner")]); let router = Router::with_urls(vec![Route::with_router("/outer", sub_router)]); let found = router.get_handler("/outer").unwrap(); @@ -1606,11 +1603,8 @@ mod tests { #[test] fn router_root_mounted_nested_router_empty_and_root_without_slash() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "inner", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "inner")]); // this should normalize to `/outer` let router = Router::with_urls(vec![Route::with_router("outer", sub_router)]); @@ -1653,18 +1647,16 @@ mod tests { #[test] fn router_root_mounted_nested_router_empty_root_empty_nested() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "inner", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "inner")]); let router = Router::with_urls(vec![Route::with_router("", sub_router)]); // exact match at the mount point, remaining defaults to root "/" let found = router.get_handler("/").unwrap(); assert_eq!(found.name, Some(RouteName("inner".to_string()))); - // wildcard sentinel capturing a literal "/" (non-empty, so legal). remaining "/" + // wildcard sentinel capturing a literal "/" (non-empty, so legal). remaining + // "/" let found = router.get_handler("//").unwrap(); assert_eq!(found.name, Some(RouteName("inner".to_string()))); @@ -1699,11 +1691,8 @@ mod tests { #[test] fn router_root_mounted_nested_router_slash_root_empty_nested() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "inner", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "inner")]); let router = Router::with_urls(vec![Route::with_router("/", sub_router)]); let found = router.get_handler("/").unwrap(); @@ -1807,11 +1796,8 @@ mod tests { #[test] fn router_param_mount_trailing_slash_empty_nested_captures_param() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "leaf", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "leaf")]); let router = Router::with_urls(vec![Route::with_router("/{id}/", sub_router)]); let found = router.get_handler("/123/").unwrap(); @@ -1821,11 +1807,8 @@ mod tests { #[test] fn router_param_mount_trailing_slash_bare_path_fails() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "leaf", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "leaf")]); let router = Router::with_urls(vec![Route::with_router("/{id}/", sub_router)]); assert!(router.get_handler("/123").is_none()); } @@ -1859,7 +1842,8 @@ mod tests { )]); let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); - // exact mount match -> remaining defaults to "/" -> nested catch-all can't match it. + // exact mount match -> remaining defaults to "/" -> nested catch-all can't + // match it. assert!(router.get_handler("/api").is_none()); let found = router.get_handler("/api/x/y").unwrap(); @@ -1982,11 +1966,7 @@ mod tests { #[test] fn router_triple_nested_all_empty_mounts_reachable_via_single_slash() { - let leaf = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "leaf", - )]); + let leaf = Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "leaf")]); let mid = Router::with_urls(vec![Route::with_router("", leaf)]); let router = Router::with_urls(vec![Route::with_router("", mid)]); @@ -2148,11 +2128,8 @@ mod tests { #[test] #[should_panic(expected = "route conflict error")] fn router_wildcard_mount_with_empty_nested_errors() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "leaf", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "leaf")]); let _ = Router::with_urls(vec![Route::with_router("/{*rest}", sub_router)]); } @@ -2170,11 +2147,8 @@ mod tests { #[test] #[should_panic(expected = "route conflict error")] fn router_prefixed_wildcard_mount_errors() { - let sub_router = Router::with_urls(vec![Route::with_handler_and_name( - "", - MockHandler, - "leaf", - )]); + let sub_router = + Router::with_urls(vec![Route::with_handler_and_name("", MockHandler, "leaf")]); let _ = Router::with_urls(vec![Route::with_router("/files/{*path}", sub_router)]); } @@ -2312,7 +2286,7 @@ mod tests { let sub_router = Router::with_urls(vec![ Route::with_handler_and_name("/foo", MockHandler, "foo"), Route::with_router("/fab", nested_sub_router), - Route::with_handler_and_name("/bar/", MockHandler, "bar") + Route::with_handler_and_name("/bar/", MockHandler, "bar"), ]); let router = Router::with_urls(vec![Route::with_router("/admin/", sub_router)]); From 189751b1598213fffc821026c191c8297ef39789 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 25 Aug 2026 04:21:20 +0000 Subject: [PATCH 14/16] comments --- cot/src/router.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cot/src/router.rs b/cot/src/router.rs index 7973282a..73079641 100644 --- a/cot/src/router.rs +++ b/cot/src/router.rs @@ -1655,8 +1655,7 @@ mod tests { let found = router.get_handler("/").unwrap(); assert_eq!(found.name, Some(RouteName("inner".to_string()))); - // wildcard sentinel capturing a literal "/" (non-empty, so legal). remaining - // "/" + // wildcard sentinel capturing a literal "/", so this is legal let found = router.get_handler("//").unwrap(); assert_eq!(found.name, Some(RouteName("inner".to_string()))); @@ -1842,8 +1841,6 @@ mod tests { )]); let router = Router::with_urls(vec![Route::with_router("/api", sub_router)]); - // exact mount match -> remaining defaults to "/" -> nested catch-all can't - // match it. assert!(router.get_handler("/api").is_none()); let found = router.get_handler("/api/x/y").unwrap(); @@ -1875,8 +1872,8 @@ mod tests { )]); let router = Router::with_urls(vec![Route::with_router("/files/", sub_router)]); - assert!(router.get_handler("/files/").is_none()); // remaining "/" vs catch-all - assert!(router.get_handler("/files").is_none()); // no trailing slash, no match at all + assert!(router.get_handler("/files/").is_none()); + assert!(router.get_handler("/files").is_none()); let found = router.get_handler("/files/x").unwrap(); assert_eq!(found.name, Some(RouteName("leaf".to_string()))); From 020cc77d302edc9d054330cbcc15dc71ed2f4fab Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 25 Aug 2026 18:13:29 +0000 Subject: [PATCH 15/16] revert snapshot --- .../ui/unimplemented_request_handler.stderr | 66 ++++++++----------- 1 file changed, 27 insertions(+), 39 deletions(-) diff --git a/cot/tests/ui/unimplemented_request_handler.stderr b/cot/tests/ui/unimplemented_request_handler.stderr index 1cac8a15..e926b9e4 100644 --- a/cot/tests/ui/unimplemented_request_handler.stderr +++ b/cot/tests/ui/unimplemented_request_handler.stderr @@ -1,39 +1,27 @@ - error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler - --> tests/ui/unimplemented_request_handler.rs:8:57 - | - 8 | let _ = Router::with_urls([Route::with_handler("/", test)]); - | ------------------- ^^^^ not a valid request handler - | | - | required by a bound introduced by this call - | - = help: the trait `RequestHandler<_>` is not implemented for fn item `fn(()) -> impl Future, cot::Error>> {test}` - = note: make sure the function is marked `async` - = note: make sure all parameters implement `FromRequest` or `FromRequestHead` - = note: make sure there is at most one parameter implementing `FromRequest` - = note: make sure the function takes no more than 10 parameters - = note: make sure the function returns a type that implements `IntoResponse` - help: the following other types implement trait `RequestHandler` - --> src/router/method/openapi.rs - | - | impl RequestHandler for ApiMethodRouter { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `ApiMethodRouter` implements `RequestHandler` - | - ::: src/router/method.rs - | - | impl RequestHandler for MethodRouter { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `MethodRouter` implements `RequestHandler` - | - ::: src/openapi.rs - | - | / impl RequestHandler for NoApi - | | where - | | H: RequestHandler, - | |_____________________________________^ `cot::openapi::NoApi` implements `RequestHandler` - note: required by a bound in `Route::with_handler` - --> src/router.rs - | - | pub fn with_handler(url: &str, handler: H) -> Self - | ------------ required by a bound in this associated function - ... - | H: RequestHandler + Send + Sync + 'static, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` +error[E0277]: `fn(()) -> impl Future, cot::Error>> {test}` is not a valid request handler + --> tests/ui/unimplemented_request_handler.rs:8:57 + | +8 | let _ = Router::with_urls([Route::with_handler("/", test)]); + | ------------------- ^^^^ not a valid request handler + | | + | required by a bound introduced by this call + | + = help: the trait `RequestHandler<_>` is not implemented for fn item `fn(()) -> impl Future, cot::Error>> {test}` + = note: make sure the function is marked `async` + = note: make sure all parameters implement `FromRequest` or `FromRequestHead` + = note: make sure there is at most one parameter implementing `FromRequest` + = note: make sure the function takes no more than 10 parameters + = note: make sure the function returns a type that implements `IntoResponse` +help: the trait `RequestHandler` is implemented for `MethodRouter` + --> src/router/method.rs + | + | impl RequestHandler for MethodRouter { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: required by a bound in `Route::with_handler` + --> src/router.rs + | + | pub fn with_handler(url: &str, handler: H) -> Self + | ------------ required by a bound in this associated function +... + | H: RequestHandler + Send + Sync + 'static, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Route::with_handler` From 3d85d8b7458b7251f73c09fb5847c1d2002f6693 Mon Sep 17 00:00:00 2001 From: Elijah Date: Tue, 25 Aug 2026 19:22:44 +0000 Subject: [PATCH 16/16] clippy fix --- cot/src/error_page.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cot/src/error_page.rs b/cot/src/error_page.rs index 4f3beca2..67ad6c9d 100644 --- a/cot/src/error_page.rs +++ b/cot/src/error_page.rs @@ -483,6 +483,7 @@ mod tests { struct MockHandler; impl RequestHandler for MockHandler { + #[expect(clippy::unused_async_trait_impl)] async fn handle(&self, _request: Request) -> Result { Html::new("OK").into_response() }