Skip to content

Commit

Permalink
Panic when there are duplicates in the routing table. #126
Browse files Browse the repository at this point in the history
  • Loading branch information
@sunli829
sunli829 committed Dec 18, 2021
1 parent beabc99 commit 2815148
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 44 deletions.
4 changes: 4 additions & 0 deletions poem/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
85 changes: 48 additions & 37 deletions poem/src/route/internal/radix_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]),
Expand Down Expand Up @@ -115,9 +122,6 @@ fn parse_path_segments(path: &[u8]) -> Result<Vec<RawSegment<'_>>, ()> {
}
}

if segments.is_empty() {
return Err(());
}
Ok(segments)
}

Expand Down Expand Up @@ -433,10 +437,10 @@ impl<T> Default for RadixTree<T> {
}

impl<T> RadixTree<T> {
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());
Expand All @@ -449,15 +453,22 @@ impl<T> RadixTree<T> {
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(),
});
}
}
};
segments.push(segment);
}
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<Matches<T>> {
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<I, K, V>(values: I) -> PathParams
Expand Down Expand Up @@ -1038,7 +1049,7 @@ mod tests {
];

for (path, id) in paths {
tree.add(path, id);
tree.add(path, id).unwrap();
}

let matches = vec![
Expand Down
71 changes: 64 additions & 7 deletions poem/src/route/router.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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<E>(mut self, path: impl AsRef<str>, 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<E>(self, path: impl AsRef<str>, ep: E) -> Self
where
Expand All @@ -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<E>(self, path: impl AsRef<str>, ep: E) -> Self
where
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 2815148

Please sign in to comment.