diff --git a/poem/CHANGELOG.md b/poem/CHANGELOG.md index 94c2159327..7903d5c76a 100644 --- a/poem/CHANGELOG.md +++ b/poem/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +# Unreleased + +- Panic when there are duplicates in the routing table. [#126](https://github.com/poem-web/poem/issues/126) + # [1.2.4] 2021-12-17 - Rename `EndpointExt::inspect_error` to `EndpointExt::inspect_all_error`. diff --git a/poem/src/route/internal/radix_tree.rs b/poem/src/route/internal/radix_tree.rs index 1ba35bfb82..a1a26bae45 100644 --- a/poem/src/route/internal/radix_tree.rs +++ b/poem/src/route/internal/radix_tree.rs @@ -7,6 +7,13 @@ fn longest_common_prefix(a: &[u8], b: &[u8]) -> usize { a.iter().zip(b).take_while(|(a, b)| **a == **b).count() } +#[derive(Debug, Eq, PartialEq)] +pub(crate) enum RadixTreeError { + InvalidPath(String), + Duplicate(String), + InvalidRegex { path: String, regex: String }, +} + #[derive(Debug, Eq, PartialEq)] enum RawSegment<'a> { Static(&'a [u8]), @@ -115,9 +122,6 @@ fn parse_path_segments(path: &[u8]) -> Result>, ()> { } } - if segments.is_empty() { - return Err(()); - } Ok(segments) } @@ -433,10 +437,10 @@ impl Default for RadixTree { } impl RadixTree { - pub(crate) fn add(&mut self, path: &str, data: T) -> bool { + pub(crate) fn add(&mut self, path: &str, data: T) -> Result<(), RadixTreeError> { let raw_segments = match parse_path_segments(path.as_bytes()) { Ok(raw_segments) => raw_segments, - Err(_) => return false, + Err(_) => return Err(RadixTreeError::InvalidPath(path.to_string())), }; let mut segments = Vec::with_capacity(raw_segments.len()); @@ -449,7 +453,10 @@ impl RadixTree { if let Some(re) = PathRegex::new(re_bytes) { Segment::Regex(name, re) } else { - return false; + return Err(RadixTreeError::InvalidRegex { + path: path.to_string(), + regex: String::from_utf8(re_bytes.to_vec()).unwrap(), + }); } } }; @@ -457,7 +464,11 @@ impl RadixTree { } segments.reverse(); - self.root.insert_child(segments, data) + if self.root.insert_child(segments, data) { + Ok(()) + } else { + Err(RadixTreeError::Duplicate(path.to_string())) + } } pub(crate) fn matches(&self, path: &str) -> Option> { @@ -501,7 +512,7 @@ mod tests { Ok(vec![RawSegment::Static(b"/a/b")]) ); - assert_eq!(parse_path_segments(b""), Err(())); + assert_eq!(parse_path_segments(b""), Ok(vec![])); assert_eq!( parse_path_segments(b"/a/:v/b"), @@ -559,9 +570,9 @@ mod tests { #[test] fn test_insert_static_child_1() { let mut tree = RadixTree::default(); - tree.add("/abc", 1); - tree.add("/abcdef", 2); - tree.add("/abcdefgh", 3); + tree.add("/abc", 1).unwrap(); + tree.add("/abcdef", 2).unwrap(); + tree.add("/abcdefgh", 3).unwrap(); assert_eq!( tree, @@ -614,10 +625,10 @@ mod tests { #[test] fn test_insert_static_child_2() { let mut tree = RadixTree::default(); - tree.add("/abcd", 1); - tree.add("/ab1234", 2); - tree.add("/ab1256", 3); - tree.add("/ab125678", 4); + tree.add("/abcd", 1).unwrap(); + tree.add("/ab1234", 2).unwrap(); + tree.add("/ab1256", 3).unwrap(); + tree.add("/ab125678", 4).unwrap(); assert_eq!( tree, @@ -706,8 +717,8 @@ mod tests { #[test] fn test_insert_static_child_3() { let mut tree = RadixTree::default(); - tree.add("/abc", 1); - tree.add("/ab", 2); + tree.add("/abc", 1).unwrap(); + tree.add("/ab", 2).unwrap(); assert_eq!( tree, RadixTree { @@ -749,9 +760,9 @@ mod tests { #[test] fn test_insert_param_child() { let mut tree = RadixTree::default(); - tree.add("/abc/:p1", 1); - tree.add("/abc/:p1/p2", 2); - tree.add("/abc/:p1/:p3", 3); + tree.add("/abc/:p1", 1).unwrap(); + tree.add("/abc/:p1/p2", 2).unwrap(); + tree.add("/abc/:p1/:p3", 3).unwrap(); assert_eq!( tree, RadixTree { @@ -823,8 +834,8 @@ mod tests { #[test] fn test_catch_all_child_1() { let mut tree = RadixTree::default(); - tree.add("/abc/*p1", 1); - tree.add("/ab/de", 2); + tree.add("/abc/*p1", 1).unwrap(); + tree.add("/ab/de", 2).unwrap(); assert_eq!( tree, RadixTree { @@ -889,7 +900,7 @@ mod tests { #[test] fn test_catch_all_child_2() { let mut tree = RadixTree::default(); - tree.add("*p1", 1); + tree.add("*p1", 1).unwrap(); assert_eq!( tree, RadixTree { @@ -921,8 +932,8 @@ mod tests { #[test] fn test_insert_regex_child() { let mut tree = RadixTree::default(); - tree.add("/abc/<\\d+>/def", 1); - tree.add("/abc/def/:name<\\d+>", 2); + tree.add("/abc/<\\d+>/def", 1).unwrap(); + tree.add("/abc/def/:name<\\d+>", 2).unwrap(); assert_eq!( tree, @@ -995,17 +1006,17 @@ mod tests { #[test] fn test_add_result() { let mut tree = RadixTree::default(); - assert!(tree.add("/a/b", 1)); - assert!(!tree.add("/a/b", 2)); - assert!(tree.add("/a/b/:p/d", 1)); - assert!(tree.add("/a/b/c/d", 2)); - assert!(!tree.add("/a/b/:p2/d", 3)); - assert!(tree.add("/a/*p", 1)); - assert!(!tree.add("/a/*p", 2)); - assert!(tree.add("/a/b/*p", 1)); - assert!(!tree.add("/a/b/*p2", 2)); - assert!(tree.add("/k/h/<\\d>+", 1)); - assert!(!tree.add("/k/h/:name<\\d>+", 2)); + assert!(tree.add("/a/b", 1).is_ok()); + assert!(!tree.add("/a/b", 2).is_ok()); + assert!(tree.add("/a/b/:p/d", 1).is_ok()); + assert!(tree.add("/a/b/c/d", 2).is_ok()); + assert!(!tree.add("/a/b/:p2/d", 3).is_ok()); + assert!(tree.add("/a/*p", 1).is_ok()); + assert!(!tree.add("/a/*p", 2).is_ok()); + assert!(tree.add("/a/b/*p", 1).is_ok()); + assert!(!tree.add("/a/b/*p2", 2).is_ok()); + assert!(tree.add("/k/h/<\\d>+", 1).is_ok()); + assert!(!tree.add("/k/h/:name<\\d>+", 2).is_ok()); } fn create_url_params(values: I) -> PathParams @@ -1038,7 +1049,7 @@ mod tests { ]; for (path, id) in paths { - tree.add(path, id); + tree.add(path, id).unwrap(); } let matches = vec![ diff --git a/poem/src/route/router.rs b/poem/src/route/router.rs index 4723265d55..7af53ebd18 100644 --- a/poem/src/route/router.rs +++ b/poem/src/route/router.rs @@ -6,7 +6,7 @@ use crate::{ endpoint::BoxEndpoint, error::NotFoundError, http::{uri::PathAndQuery, Uri}, - route::internal::radix_tree::RadixTree, + route::internal::radix_tree::{RadixTree, RadixTreeError}, Endpoint, EndpointExt, IntoEndpoint, IntoResponse, Request, Response, Result, }; @@ -168,18 +168,28 @@ impl Route { } /// Add an [Endpoint] to the specified path. + /// + /// # Panics + /// + /// Panic when there are duplicates in the routing table. #[must_use] pub fn at(mut self, path: impl AsRef, ep: E) -> Self where E: IntoEndpoint, E::Endpoint: 'static, { - self.tree - .add(&normalize_path(path.as_ref()), ep.map_to_response().boxed()); + check_result( + self.tree + .add(&normalize_path(path.as_ref()), ep.map_to_response().boxed()), + ); self } /// Nest a `Endpoint` to the specified path and strip the prefix. + /// + /// # Panics + /// + /// Panic when there are duplicates in the routing table. #[must_use] pub fn nest(self, path: impl AsRef, ep: E) -> Self where @@ -190,6 +200,10 @@ impl Route { } /// Nest a `Endpoint` to the specified path, but do not strip the prefix. + /// + /// # Panics + /// + /// Panic when there are duplicates in the routing table. #[must_use] pub fn nest_no_strip(self, path: impl AsRef, ep: E) -> Self where @@ -254,22 +268,24 @@ impl Route { false => 0, true => path.len() - 1, }; - self.tree.add( + + check_result(self.tree.add( &format!("{}*--poem-rest", path), Box::new(Nest { inner: ep.clone(), root: false, prefix_len, }), - ); - self.tree.add( + )); + + check_result(self.tree.add( &path[..path.len() - 1], Box::new(Nest { inner: ep, root: true, prefix_len, }), - ); + )); self } @@ -300,6 +316,17 @@ fn normalize_path(path: &str) -> String { path } +fn check_result(res: Result<(), RadixTreeError>) { + match res { + Ok(()) => {} + Err(RadixTreeError::InvalidPath(path)) => panic!("invalid path: {}", path), + Err(RadixTreeError::Duplicate(path)) => panic!("duplicate path: {}", path), + Err(RadixTreeError::InvalidRegex { path, regex }) => { + panic!("invalid regex in path: {} `{}`", path, regex) + } + } +} + #[cfg(test)] mod tests { use http::Uri; @@ -411,4 +438,34 @@ mod tests { assert_eq!(get(&r, "/a").await, "/"); assert_eq!(get(&r, "/a?a=1").await, "/?a=1"); } + + #[test] + #[should_panic] + fn duplicate_1() { + let _ = Route::new().at("/", h).at("/", h); + } + + #[test] + #[should_panic] + fn duplicate_2() { + let _ = Route::new().at("/a", h).nest("/a", h); + } + + #[test] + #[should_panic] + fn duplicate_3() { + let _ = Route::new().nest("/a", h).nest("/a", h); + } + + #[test] + #[should_panic] + fn duplicate_4() { + let _ = Route::new().at("/a/:a", h).at("/a/:b", h); + } + + #[test] + #[should_panic] + fn duplicate_5() { + let _ = Route::new().at("/a/*:v", h).at("/a/*", h); + } }