From a34374d913a4fa3278678b4df25899ac44878c24 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Fri, 26 Aug 2016 21:34:28 -0700 Subject: [PATCH] Output all matching routes, not just first ranked. --- Cargo.toml | 1 + examples/hello_ranks/Cargo.toml | 9 +++ examples/hello_ranks/src/main.rs | 20 +++++ lib/src/rocket.rs | 44 ++++++----- lib/src/router/collider.rs | 2 +- lib/src/router/mod.rs | 132 +++++++++++++++++++------------ lib/src/router/route.rs | 4 +- 7 files changed, 140 insertions(+), 72 deletions(-) create mode 100644 examples/hello_ranks/Cargo.toml create mode 100644 examples/hello_ranks/src/main.rs diff --git a/Cargo.toml b/Cargo.toml index ae030935..5de0ac08 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,4 +14,5 @@ members = [ "examples/static_files", "examples/todo", "examples/content_types", + "examples/hello_ranks", ] diff --git a/examples/hello_ranks/Cargo.toml b/examples/hello_ranks/Cargo.toml new file mode 100644 index 00000000..d0b126ca --- /dev/null +++ b/examples/hello_ranks/Cargo.toml @@ -0,0 +1,9 @@ +[package] +name = "hello_ranks" +version = "0.0.1" +authors = ["Sergio Benitez "] +workspace = "../../" + +[dependencies] +rocket = { path = "../../lib" } +rocket_macros = { path = "../../macros" } diff --git a/examples/hello_ranks/src/main.rs b/examples/hello_ranks/src/main.rs new file mode 100644 index 00000000..37e1824c --- /dev/null +++ b/examples/hello_ranks/src/main.rs @@ -0,0 +1,20 @@ +#![feature(plugin)] +#![plugin(rocket_macros)] + +extern crate rocket; +use rocket::Rocket; + +#[GET(path = "/hello//")] +fn hello(name: &str, age: i8) -> String { + format!("Hello, {} year old named {}!", age, name) +} + +// FIXME: Add 'rank = 2'. +#[GET(path = "/hello//")] +fn hi(name: &str, age: &str) -> String { + format!("Hi {}! You age ({}) is kind of funky.", name, age) +} + +fn main() { + Rocket::new("localhost", 8000).mount_and_launch("/", routes![hello, hi]); +} diff --git a/lib/src/rocket.rs b/lib/src/rocket.rs index a26a0de1..48745271 100644 --- a/lib/src/rocket.rs +++ b/lib/src/rocket.rs @@ -1,5 +1,5 @@ use super::*; -use response::FreshHyperResponse; +use response::{FreshHyperResponse, Outcome}; use request::HyperRequest; use catcher; @@ -29,7 +29,7 @@ impl HyperHandler for Rocket { impl Rocket { fn dispatch<'h, 'k>(&self, hyp_req: HyperRequest<'h, 'k>, - res: FreshHyperResponse<'h>) { + mut res: FreshHyperResponse<'h>) { // Get a copy of the URI for later use. let uri = hyp_req.uri.to_string(); @@ -43,27 +43,25 @@ impl Rocket { }; info!("{}:", request); - let route = self.router.route(&request); - if let Some(ref route) = route { + let matches = self.router.route(&request); + trace_!("Found {} matches.", matches.len()); + for route in matches { // Retrieve and set the requests parameters. + info_!("Matched: {}", route); request.set_params(route); // Here's the magic: dispatch the request to the handler. let outcome = (route.handler)(&request).respond(res); info_!("{} {}", White.paint("Outcome:"), outcome); - // TODO: keep trying lower ranked routes before dispatching a not - // found error. - outcome.map_forward(|res| { - error_!("No further matching routes."); - // TODO: Have some way to know why this was failed forward. Use that - // instead of always using an unchained error. - self.handle_not_found(&request, res); - }); - } else { - error_!("No matching routes."); - self.handle_not_found(&request, res); + res = match outcome { + Outcome::Complete => return, + Outcome::FailStop => return, + Outcome::FailForward(r) => r + }; } + + self.handle_not_found(&request, res); } // Call on internal server error. @@ -95,6 +93,7 @@ impl Rocket { pub fn mount(&mut self, base: &'static str, routes: Vec) -> &mut Self { + self.enable_normal_logging_if_disabled(); info!("🛰 {} '{}':", Magenta.paint("Mounting"), base); for mut route in routes { let path = format!("{}/{}", base, route.path.as_str()); @@ -108,12 +107,13 @@ impl Rocket { } pub fn catch(&mut self, catchers: Vec) -> &mut Self { + self.enable_normal_logging_if_disabled(); info!("👾 {}:", Magenta.paint("Catchers")); for c in catchers { if self.catchers.contains_key(&c.code) && !self.catchers.get(&c.code).unwrap().is_default() { let msg = format!("warning: overrides {} catcher!", c.code); - info_!("{} ({})", c, Yellow.paint(msg.as_str())); + warn!("{} ({})", c, Yellow.paint(msg.as_str())); } else { info_!("{}", c); } @@ -124,6 +124,13 @@ impl Rocket { self } + fn enable_normal_logging_if_disabled(&mut self) { + if !self.log_set { + logger::init(LoggingLevel::Normal); + self.log_set = true; + } + } + pub fn log(&mut self, level: LoggingLevel) { if self.log_set { warn!("Log level already set! Not overriding."); @@ -134,14 +141,11 @@ impl Rocket { } pub fn launch(mut self) { + self.enable_normal_logging_if_disabled(); if self.router.has_collisions() { warn!("Route collisions detected!"); } - if !self.log_set { - self.log(LoggingLevel::Normal) - } - let full_addr = format!("{}:{}", self.address, self.port); info!("🚀 {} {}...", White.paint("Rocket has launched from"), White.bold().paint(&full_addr)); diff --git a/lib/src/router/collider.rs b/lib/src/router/collider.rs index 3ba97027..faa66edb 100644 --- a/lib/src/router/collider.rs +++ b/lib/src/router/collider.rs @@ -45,7 +45,7 @@ mod tests { use Method; use Method::*; use {Request, Response}; - use content_type::{ContentType, TopLevel, SubLevel}; + use content_type::ContentType; use std::str::FromStr; type SimpleRoute = (Method, &'static str); diff --git a/lib/src/router/mod.rs b/lib/src/router/mod.rs index 0ecf435b..37075d18 100644 --- a/lib/src/router/mod.rs +++ b/lib/src/router/mod.rs @@ -32,27 +32,16 @@ impl Router { // `Route` structure is inflexible. Have it be an associated type. // FIXME: Figure out a way to get more than one route, i.e., to correctly // handle ranking. - // TODO: Should the Selector include the content-type? If it does, can't - // warn the user that a match was found for the wrong content-type. It - // doesn't, can, but this method is slower. - pub fn route<'b>(&'b self, req: &Request) -> Option<&'b Route> { + pub fn route<'b>(&'b self, req: &Request) -> Vec<&'b Route> { let num_segments = req.uri.segment_count(); + self.routes.get(&(req.method, num_segments)).map_or(vec![], |routes| { + let mut matches: Vec<&'b Route> = routes.iter().filter(|r| { + r.collides_with(req) + }).collect(); - let mut matched_route: Option<&Route> = None; - if let Some(routes) = self.routes.get(&(req.method, num_segments)) { - for route in routes.iter().filter(|r| r.collides_with(req)) { - info_!("Matched: {}", route); - if let Some(existing_route) = matched_route { - if route.rank < existing_route.rank { - matched_route = Some(route); - } - } else { - matched_route = Some(route); - } - } - } - - matched_route + matches.sort_by(|a, b| a.rank.cmp(&b.rank)); + matches + }) } pub fn has_collisions(&self) -> bool { @@ -94,6 +83,16 @@ mod test { router } + fn router_with_ranked_routes(routes: &[(isize, &'static str)]) -> Router { + let mut router = Router::new(); + for &(rank, route) in routes { + let route = Route::ranked(rank, Get, route.to_string(), dummy_handler); + router.add(route); + } + + router + } + fn router_with_unranked_routes(routes: &[&'static str]) -> Router { let mut router = Router::new(); for route in routes { @@ -104,42 +103,41 @@ mod test { router } + fn unranked_route_collisions(routes: &[&'static str]) -> bool { + let router = router_with_unranked_routes(routes); + router.has_collisions() + } + + fn default_rank_route_collisions(routes: &[&'static str]) -> bool { + let router = router_with_routes(routes); + router.has_collisions() + } + #[test] fn test_collisions() { - let router = router_with_unranked_routes(&["/hello", "/hello"]); - assert!(router.has_collisions()); - - let router = router_with_unranked_routes(&["/", "/hello"]); - assert!(router.has_collisions()); - - let router = router_with_unranked_routes(&["/", "/"]); - assert!(router.has_collisions()); - - let router = router_with_unranked_routes(&["/hello/bob", "/hello/"]); - assert!(router.has_collisions()); - - let router = router_with_routes(&["/a/b//d", "///c/d"]); - assert!(router.has_collisions()); + assert!(unranked_route_collisions(&["/hello", "/hello"])); + assert!(unranked_route_collisions(&["/", "/hello"])); + assert!(unranked_route_collisions(&["/", "/"])); + assert!(unranked_route_collisions(&["/hello/bob", "/hello/"])); + assert!(unranked_route_collisions(&["/a/b//d", "///c/d"])); } #[test] fn test_none_collisions_when_ranked() { - let router = router_with_routes(&["/", "/hello"]); - assert!(!router.has_collisions()); - - let router = router_with_routes(&["/hello/bob", "/hello/"]); - assert!(!router.has_collisions()); - - let router = router_with_routes(&["/a/b/c/d", "///c/d"]); - assert!(!router.has_collisions()); - - let router = router_with_routes(&["/hi", "/"]); - assert!(!router.has_collisions()); + assert!(!default_rank_route_collisions(&["/", "/hello"])); + assert!(!default_rank_route_collisions(&["/hello/bob", "/hello/"])); + assert!(!default_rank_route_collisions(&["/a/b/c/d", "///c/d"])); + assert!(!default_rank_route_collisions(&["/hi", "/"])); } fn route<'a>(router: &'a Router, method: Method, uri: &str) -> Option<&'a Route> { let request = Request::mock(method, uri); - router.route(&request) + let matches = router.route(&request); + if matches.len() > 0 { + Some(matches[0]) + } else { + None + } } #[test] @@ -215,11 +213,47 @@ mod test { assert_ranked_routes!(&["/hi/a", "/hi/"], "/hi/c", "/hi/"); } + fn ranked_collisions(routes: &[(isize, &'static str)]) -> bool { + let router = router_with_ranked_routes(routes); + router.has_collisions() + } + #[test] - fn test_ranking() { - let a = Route::ranked(1, Get, "a/", dummy_handler); - let b = Route::ranked(2, Get, "a/", dummy_handler); - // FIXME: Add tests for non-default ranks. + fn test_no_manual_ranked_collisions() { + assert!(!ranked_collisions(&[(1, "a/"), (2, "a/")])); + assert!(!ranked_collisions(&[(0, "a/"), (2, "a/")])); + assert!(!ranked_collisions(&[(5, "a/"), (2, "a/")])); + assert!(!ranked_collisions(&[(1, "a/"), (1, "b/")])); + } + + macro_rules! assert_ranked_routing { + (to: $to:expr, with: $routes:expr, expect: $want:expr) => ({ + let router = router_with_ranked_routes(&$routes); + let routed_to = route(&router, Get, $to).unwrap(); + assert_eq!(routed_to.path.as_str() as &str, $want.1); + assert_eq!(routed_to.rank, $want.0); + }) + } + + #[test] + fn test_ranked_routing() { + assert_ranked_routing!( + to: "a/b", + with: [(1, "a/"), (2, "a/")], + expect: (1, "a/") + ); + + assert_ranked_routing!( + to: "b/b", + with: [(1, "a/"), (2, "b/"), (3, "b/b")], + expect: (2, "b/") + ); + + assert_ranked_routing!( + to: "b/b", + with: [(1, "a/"), (2, "b/"), (0, "b/b")], + expect: (0, "b/b") + ); } fn match_params(router: &Router, path: &str, expected: &[&str]) -> bool { diff --git a/lib/src/router/route.rs b/lib/src/router/route.rs index c21c20e6..a6ca8720 100644 --- a/lib/src/router/route.rs +++ b/lib/src/router/route.rs @@ -69,10 +69,10 @@ impl Route { impl fmt::Display for Route { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{} {} ", Green.paint(&self.method), Blue.paint(&self.path))?; + write!(f, "{} {}", Green.paint(&self.method), Blue.paint(&self.path))?; if !self.content_type.is_any() { - write!(f, "{}", Yellow.paint(&self.content_type)) + write!(f, " {}", Yellow.paint(&self.content_type)) } else { Ok(()) }