From 4df7ce6bf5ff5885bfa8c423cbb3f00b4e09e683 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Fri, 18 Aug 2017 18:37:25 -0700 Subject: [PATCH] Propogate route names through codegen to runtime. This commit modifies `codegen` so that a route's name (the name of the route handler) is stored in the generated static route information structure and later propogated into the corresponding `Route` structure. The primary advantage of this change is an improvement to debug and error messages which now include route names. The collision error message, in particular, has been improved dramatically in this commit. Additionally, the `LaunchError::Collision` variant now contains a vector of the colliding routes. --- codegen/src/decorators/route.rs | 11 +++++++---- lib/src/codegen.rs | 1 + lib/src/error.rs | 18 +++++++++++++----- lib/src/rocket.rs | 12 ++++++++---- lib/src/router/mod.rs | 14 ++++++++++---- lib/src/router/route.rs | 11 +++++++++++ 6 files changed, 50 insertions(+), 17 deletions(-) diff --git a/codegen/src/decorators/route.rs b/codegen/src/decorators/route.rs index 61e63921..1fae4c78 100644 --- a/codegen/src/decorators/route.rs +++ b/codegen/src/decorators/route.rs @@ -12,6 +12,7 @@ use syntax::ast::{Arg, Ident, Stmt, Expr, MetaItem, Path}; use syntax::ext::base::{Annotatable, ExtCtxt}; use syntax::ext::build::AstBuilder; use syntax::parse::token; +use syntax::symbol::InternedString; use syntax::ptr::P; use rocket::http::{Method, MediaType}; @@ -46,7 +47,7 @@ trait RouteGenerateExt { fn generate_query_statement(&self, ecx: &ExtCtxt) -> Option; fn generate_param_statements(&self, ecx: &ExtCtxt) -> Vec; fn generate_fn_arguments(&self, ecx: &ExtCtxt) -> Vec; - fn explode(&self, ecx: &ExtCtxt) -> (&str, Path, P, P); + fn explode(&self, ecx: &ExtCtxt) -> (InternedString, &str, Path, P, P); } impl RouteGenerateExt for RouteParams { @@ -223,14 +224,15 @@ impl RouteGenerateExt for RouteParams { sep_by_tok(ecx, &args, token::Comma) } - fn explode(&self, ecx: &ExtCtxt) -> (&str, Path, P, P) { + fn explode(&self, ecx: &ExtCtxt) -> (InternedString, &str, Path, P, P) { + let name = self.annotated_fn.ident().name.as_str(); let path = &self.uri.node.as_str(); let method = method_to_path(ecx, self.method.node); let format = self.format.as_ref().map(|kv| kv.value().clone()); let media_type = option_as_expr(ecx, &media_type_to_expr(ecx, format)); let rank = option_as_expr(ecx, &self.rank); - (path, method, media_type, rank) + (name, path, method, media_type, rank) } } @@ -272,12 +274,13 @@ fn generic_route_decorator(known_method: Option>, // Generate and emit the static route info that uses the just generated // function as its handler. A proper Rocket route will be created from this. let struct_name = user_fn_name.prepend(ROUTE_STRUCT_PREFIX); - let (path, method, media_type, rank) = route.explode(ecx); + let (name, path, method, media_type, rank) = route.explode(ecx); let static_route_info_item = quote_item!(ecx, /// Rocket code generated static route information structure. #[allow(non_upper_case_globals)] pub static $struct_name: ::rocket::StaticRouteInfo = ::rocket::StaticRouteInfo { + name: $name, method: $method, path: $path, handler: $route_fn_name, diff --git a/lib/src/codegen.rs b/lib/src/codegen.rs index 7741584a..71aab265 100644 --- a/lib/src/codegen.rs +++ b/lib/src/codegen.rs @@ -2,6 +2,7 @@ use handler::{Handler, ErrorHandler}; use http::{Method, MediaType}; pub struct StaticRouteInfo { + pub name: &'static str, pub method: Method, pub path: &'static str, pub format: Option, diff --git a/lib/src/error.rs b/lib/src/error.rs index e8226894..297acdf6 100644 --- a/lib/src/error.rs +++ b/lib/src/error.rs @@ -3,7 +3,10 @@ use std::{io, fmt}; use std::sync::atomic::{Ordering, AtomicBool}; +use yansi::Paint; + use http::hyper; +use router::Route; /// [unstable] Error type for Rocket. Likely to change. #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] @@ -31,7 +34,7 @@ pub enum Error { #[derive(Debug)] pub enum LaunchErrorKind { Io(io::Error), - Collision, + Collision(Vec<(Route, Route)>), FailedFairing, Unknown(Box<::std::error::Error + Send + Sync>) } @@ -156,7 +159,7 @@ impl fmt::Display for LaunchErrorKind { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { LaunchErrorKind::Io(ref e) => write!(f, "I/O error: {}", e), - LaunchErrorKind::Collision => write!(f, "route collisions detected"), + LaunchErrorKind::Collision(_) => write!(f, "route collisions detected"), LaunchErrorKind::FailedFairing => write!(f, "a launch fairing failed"), LaunchErrorKind::Unknown(ref e) => write!(f, "unknown error: {}", e) } @@ -185,7 +188,7 @@ impl ::std::error::Error for LaunchError { self.mark_handled(); match *self.kind() { LaunchErrorKind::Io(_) => "an I/O error occured during launch", - LaunchErrorKind::Collision => "route collisions were detected", + LaunchErrorKind::Collision(_) => "route collisions were detected", LaunchErrorKind::FailedFairing => "a launch fairing reported an error", LaunchErrorKind::Unknown(_) => "an unknown error occured during launch" } @@ -203,8 +206,13 @@ impl Drop for LaunchError { error!("Rocket failed to launch due to an I/O error."); panic!("{}", e); } - LaunchErrorKind::Collision => { - error!("Rocket failed to launch due to routing collisions."); + LaunchErrorKind::Collision(ref collisions) => { + error!("Rocket failed to launch due to the following routing collisions:"); + for &(ref a, ref b) in collisions { + info_!("{} {} {}", a, Paint::red("collides with").italic(), b) + } + + info_!("Note: Collisions can usually be resolved by ranking routes."); panic!("route collisions detected"); } LaunchErrorKind::FailedFairing => { diff --git a/lib/src/rocket.rs b/lib/src/rocket.rs index 197182b4..43f85d86 100644 --- a/lib/src/rocket.rs +++ b/lib/src/rocket.rs @@ -409,7 +409,9 @@ impl Rocket { for (name, value) in config.extras() { info_!("{} {}: {}", - Paint::yellow("[extra]"), name, Paint::white(LoggedValue(value))); + Paint::yellow("[extra]"), + Paint::blue(name), + Paint::white(LoggedValue(value))); } Rocket { @@ -475,7 +477,7 @@ impl Rocket { /// ``` #[inline] pub fn mount(mut self, base: &str, routes: Vec) -> Self { - info!("🛰 {} '{}':", Paint::purple("Mounting"), base); + info!("🛰 {} '{}':", Paint::purple("Mounting"), Paint::blue(base)); if base.contains('<') { error_!("Bad mount point: '{}'.", base); @@ -622,8 +624,10 @@ impl Rocket { } pub(crate) fn prelaunch_check(&self) -> Option { - if self.router.has_collisions() { - Some(LaunchError::from(LaunchErrorKind::Collision)) + let collisions = self.router.collisions(); + if !collisions.is_empty() { + let owned = collisions.iter().map(|&(a, b)| (a.clone(), b.clone())); + Some(LaunchError::from(LaunchErrorKind::Collision(owned.collect()))) } else if self.fairings.had_failure() { Some(LaunchError::from(LaunchErrorKind::FailedFairing)) } else { diff --git a/lib/src/router/mod.rs b/lib/src/router/mod.rs index 7b3700ce..9498f05a 100644 --- a/lib/src/router/mod.rs +++ b/lib/src/router/mod.rs @@ -44,14 +44,13 @@ impl Router { matches } - pub fn has_collisions(&self) -> bool { - let mut result = false; + pub fn collisions(&self) -> Vec<(&Route, &Route)> { + let mut result = vec![]; for routes in self.routes.values() { for (i, a_route) in routes.iter().enumerate() { for b_route in routes.iter().skip(i + 1) { if a_route.collides_with(b_route) { - result = true; - error!("{} and {} collide!", a_route, b_route); + result.push((a_route, b_route)); } } } @@ -60,6 +59,13 @@ impl Router { result } + + // This is slow. Don't expose this publicly; only for tests. + #[cfg(test)] + fn has_collisions(&self) -> bool { + !self.collisions().is_empty() + } + #[inline] pub fn routes<'a>(&'a self) -> impl Iterator + 'a { self.routes.values().flat_map(|v| v.iter()) diff --git a/lib/src/router/route.rs b/lib/src/router/route.rs index 229ab3d9..0cce839f 100644 --- a/lib/src/router/route.rs +++ b/lib/src/router/route.rs @@ -10,6 +10,8 @@ use http::uri::URI; /// A route: a method, its handler, path, rank, and format/media type. pub struct Route { + /// The name of this route, if one was given. + pub name: Option<&'static str>, /// The method this route matches against. pub method: Method, /// The function that should be called when the route matches. @@ -80,6 +82,7 @@ impl Route { { let uri = URI::from(path.as_ref().to_string()); Route { + name: None, method: m, handler: handler, rank: default_rank(&uri), @@ -110,6 +113,7 @@ impl Route { where S: AsRef { Route { + name: None, method: m, handler: handler, base: URI::from("/"), @@ -220,6 +224,7 @@ impl Route { impl Clone for Route { fn clone(&self) -> Route { Route { + name: self.name, method: self.method, handler: self.handler, rank: self.rank, @@ -242,6 +247,11 @@ impl fmt::Display for Route { write!(f, " {}", Yellow.paint(format))?; } + if let Some(name) = self.name { + write!(f, " {}{}{}", + Cyan.paint("("), Purple.paint(name), Cyan.paint(")"))?; + } + Ok(()) } } @@ -257,6 +267,7 @@ impl<'a> From<&'a StaticRouteInfo> for Route { fn from(info: &'a StaticRouteInfo) -> Route { let mut route = Route::new(info.method, info.path, info.handler); route.format = info.format.clone(); + route.name = Some(info.name); if let Some(rank) = info.rank { route.rank = rank; }