From 6ea894547db17c3c5e90f764f57226a9aea244b9 Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Fri, 29 Dec 2017 14:04:13 -0800 Subject: [PATCH] better application handling, fix url_for method for routes with prefix --- guide/src/qs_3.md | 16 ++++++--- guide/src/qs_5.md | 3 +- src/application.rs | 38 ++++++++++++++++++--- src/httprequest.rs | 23 ++++++++++++- src/router.rs | 85 ++++++++++++++++++++++++++++++---------------- 5 files changed, 123 insertions(+), 42 deletions(-) diff --git a/guide/src/qs_3.md b/guide/src/qs_3.md index 38383084..c970b3ef 100644 --- a/guide/src/qs_3.md +++ b/guide/src/qs_3.md @@ -10,7 +10,11 @@ Also it stores application specific state that is shared across all handlers within same application. Application acts as namespace for all routes, i.e all routes for specific application -has same url path prefix: +has same url path prefix. Application prefix always contains laading "/" slash. +If supplied prefix does not contain leading slash, it get inserted. +Prefix should consists of valud path segments. i.e for application with prefix `/app` +any request with following paths `/app`, `/app/` or `/app/test` would match, +but path `/application` would not match. ```rust,ignore # extern crate actix_web; @@ -21,14 +25,14 @@ has same url path prefix: # } # fn main() { let app = Application::new() - .prefix("/prefix") + .prefix("/app") .resource("/index.html", |r| r.method(Method::GET).f(index)) .finish() # } ``` -In this example application with `/prefix` prefix and `index.html` resource -get created. This resource is available as on `/prefix/index.html` url. +In this example application with `/app` prefix and `index.html` resource +get created. This resource is available as on `/app/index.html` url. For more information check [*URL Matching*](./qs_5.html#using-a-application-prefix-to-compose-applications) section. @@ -56,6 +60,10 @@ fn main() { ``` All `/app1` requests route to first application, `/app2` to second and then all other to third. +Applications get matched based on registration order, if application with more general +prefix is registered before less generic, that would effectively block less generic +application to get matched. For example if *application* with prefix "/" get registered +as first application, it would match all incoming requests. ## State diff --git a/guide/src/qs_5.md b/guide/src/qs_5.md index dba0b637..6a3a452c 100644 --- a/guide/src/qs_5.md +++ b/guide/src/qs_5.md @@ -445,7 +445,6 @@ fn main() { ## Using a Application Prefix to Compose Applications The `Applicaiton::prefix()`" method allows to set specific application prefix. -If route_prefix is supplied to the include method, it must be a string. This prefix represents a resource prefix that will be prepended to all resource patterns added by the resource configuration. This can be used to help mount a set of routes at a different location than the included callable's author intended while still maintaining the same @@ -471,7 +470,7 @@ fn main() { In the above example, the *show_users* route will have an effective route pattern of */users/show* instead of */show* because the application's prefix argument will be prepended -to the pattern. The route will then only match if the URL path is /users/show, +to the pattern. The route will then only match if the URL path is */users/show*, and when the `HttpRequest.url_for()` function is called with the route name show_users, it will generate a URL with that same path. diff --git a/src/application.rs b/src/application.rs index 04c15052..0fb44bed 100644 --- a/src/application.rs +++ b/src/application.rs @@ -50,7 +50,13 @@ impl HttpApplication { impl HttpHandler for HttpApplication { fn handle(&mut self, req: HttpRequest) -> Result, HttpRequest> { - if req.path().starts_with(&self.prefix) { + let m = { + let path = req.path(); + path.starts_with(&self.prefix) && ( + path.len() == self.prefix.len() || + path.split_at(self.prefix.len()).1.starts_with('/')) + }; + if m { let inner = Rc::clone(&self.inner); let req = req.with_state(Rc::clone(&self.state), self.router.clone()); @@ -126,11 +132,14 @@ impl Application where S: 'static { /// /// Only requests that matches application's prefix get processed by this application. /// Application prefix always contains laading "/" slash. If supplied prefix - /// does not contain leading slash, it get inserted. + /// does not contain leading slash, it get inserted. Prefix should + /// consists of valud path segments. i.e for application with + /// prefix `/app` any request with following paths `/app`, `/app/` or `/app/test` + /// would match, but path `/application` would not match. /// - /// Inthe following example only requests with "/app/" path prefix - /// get handled. Request with path "/app/test/" will be handled, - /// but request with path "/other/..." will return *NOT FOUND* + /// In the following example only requests with "/app/" path prefix + /// get handled. Request with path "/app/test/" would be handled, + /// but request with path "/application" or "/other/..." would return *NOT FOUND* /// /// ```rust /// # extern crate actix_web; @@ -387,4 +396,23 @@ mod tests { let resp = app.run(req); assert_eq!(resp.as_response().unwrap().status(), StatusCode::OK); } + + #[test] + fn test_prefix() { + let mut app = Application::new() + .prefix("/test") + .resource("/blah", |r| r.h(httpcodes::HTTPOk)) + .finish(); + let req = TestRequest::with_uri("/test").finish(); + let resp = app.handle(req); + assert!(resp.is_ok()); + + let req = TestRequest::with_uri("/test/").finish(); + let resp = app.handle(req); + assert!(resp.is_ok()); + + let req = TestRequest::with_uri("/testing").finish(); + let resp = app.handle(req); + assert!(resp.is_err()); + } } diff --git a/src/httprequest.rs b/src/httprequest.rs index ce0667ba..96c36dbb 100644 --- a/src/httprequest.rs +++ b/src/httprequest.rs @@ -823,7 +823,7 @@ mod tests { resource.name("index"); let mut map = HashMap::new(); map.insert(Pattern::new("index", "/user/{name}.{ext}"), Some(resource)); - let (router, _) = Router::new("", ServerSettings::default(), map); + let (router, _) = Router::new("/", ServerSettings::default(), map); assert!(router.has_route("/user/test.html")); assert!(!router.has_route("/test/unknown")); @@ -840,6 +840,27 @@ mod tests { assert_eq!(url.ok().unwrap().as_str(), "http://www.rust-lang.org/user/test.html"); } + #[test] + fn test_url_for_with_prefix() { + let mut headers = HeaderMap::new(); + headers.insert(header::HOST, + header::HeaderValue::from_static("www.rust-lang.org")); + let req = HttpRequest::new( + Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, headers, None); + + let mut resource = Resource::<()>::default(); + resource.name("index"); + let mut map = HashMap::new(); + map.insert(Pattern::new("index", "/user/{name}.{ext}"), Some(resource)); + let (router, _) = Router::new("/prefix/", ServerSettings::default(), map); + assert!(router.has_route("/user/test.html")); + assert!(!router.has_route("/prefix/user/test.html")); + + let req = req.with_state(Rc::new(()), router); + let url = req.url_for("index", &["test", "html"]); + assert_eq!(url.ok().unwrap().as_str(), "http://www.rust-lang.org/prefix/user/test.html"); + } + #[test] fn test_url_for_external() { let req = HttpRequest::new( diff --git a/src/router.rs b/src/router.rs index 92eb01c3..eb1027fb 100644 --- a/src/router.rs +++ b/src/router.rs @@ -16,6 +16,7 @@ pub struct Router(Rc); struct Inner { prefix: String, + prefix_len: usize, regset: RegexSet, named: HashMap, patterns: Vec, @@ -47,8 +48,10 @@ impl Router { } } + let len = prefix.len(); (Router(Rc::new( Inner{ prefix: prefix, + prefix_len: len, regset: RegexSet::new(&paths).unwrap(), named: named, patterns: patterns, @@ -71,7 +74,10 @@ impl Router { pub fn recognize(&self, req: &mut HttpRequest) -> Option { let mut idx = None; { - let path = &req.path()[self.0.prefix.len()..]; + if self.0.prefix_len > req.path().len() { + return None + } + let path = &req.path()[self.0.prefix_len..]; if path.is_empty() { if let Some(i) = self.0.regset.matches("/").into_iter().next() { idx = Some(i); @@ -82,7 +88,7 @@ impl Router { } if let Some(idx) = idx { - let path: &str = unsafe{ mem::transmute(&req.path()[self.0.prefix.len()..]) }; + let path: &str = unsafe{ mem::transmute(&req.path()[self.0.prefix_len..]) }; self.0.patterns[idx].update_match_info(path, req); return Some(idx) } else { @@ -91,13 +97,17 @@ impl Router { } /// Check if application contains matching route. + /// + /// This method does not take `prefix` into account. + /// For example if prefix is `/test` and router contains route `/name`, + /// following path would be recognizable `/test/name` but `has_route()` call + /// would return `false`. pub fn has_route(&self, path: &str) -> bool { - let p = &path[self.0.prefix.len()..]; - if p.is_empty() { + if path.is_empty() { if self.0.regset.matches("/").into_iter().next().is_some() { return true } - } else if self.0.regset.matches(p).into_iter().next().is_some() { + } else if self.0.regset.matches(path).into_iter().next().is_some() { return true } false @@ -205,12 +215,11 @@ impl Pattern { { let mut iter = elements.into_iter(); let mut path = if let Some(prefix) = prefix { - let mut path = String::from(prefix); - path.push('/'); - path + format!("{}/", prefix) } else { String::new() }; + println!("TEST: {:?} {:?}", path, prefix); for el in &self.elements { match *el { PatternElement::Str(ref s) => path.push_str(s), @@ -297,11 +306,9 @@ impl Hash for Pattern { #[cfg(test)] mod tests { - use regex::Regex; use super::*; - use http::{Uri, Version, Method}; - use http::header::HeaderMap; - use std::str::FromStr; + use regex::Regex; + use test::TestRequest; #[test] fn test_recognizer() { @@ -314,45 +321,63 @@ mod tests { routes.insert(Pattern::new("", "{test}/index.html"), Some(Resource::default())); let (rec, _) = Router::new::<()>("", ServerSettings::default(), routes); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/name").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/name").finish(); assert!(rec.recognize(&mut req).is_some()); assert!(req.match_info().is_empty()); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/name/value").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/name/value").finish(); assert!(rec.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("val").unwrap(), "value"); assert_eq!(&req.match_info()["val"], "value"); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/name/value2/index.html").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/name/value2/index.html").finish(); assert!(rec.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("val").unwrap(), "value2"); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/vtest/ttt/index.html").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/vtest/ttt/index.html").finish(); assert!(rec.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("val").unwrap(), "test"); assert_eq!(req.match_info().get("val2").unwrap(), "ttt"); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/v/blah-blah/index.html").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/v/blah-blah/index.html").finish(); assert!(rec.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("tail").unwrap(), "blah-blah/index.html"); - let mut req = HttpRequest::new( - Method::GET, Uri::from_str("/bbb/index.html").unwrap(), - Version::HTTP_11, HeaderMap::new(), None); + let mut req = TestRequest::with_uri("/bbb/index.html").finish(); assert!(rec.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("test").unwrap(), "bbb"); } + #[test] + fn test_recognizer_with_prefix() { + let mut routes = HashMap::new(); + routes.insert(Pattern::new("", "/name"), Some(Resource::default())); + routes.insert(Pattern::new("", "/name/{val}"), Some(Resource::default())); + let (rec, _) = Router::new::<()>("/test", ServerSettings::default(), routes); + + let mut req = TestRequest::with_uri("/name").finish(); + assert!(rec.recognize(&mut req).is_none()); + + let mut req = TestRequest::with_uri("/test/name").finish(); + assert!(rec.recognize(&mut req).is_some()); + + let mut req = TestRequest::with_uri("/test/name/value").finish(); + assert!(rec.recognize(&mut req).is_some()); + assert_eq!(req.match_info().get("val").unwrap(), "value"); + assert_eq!(&req.match_info()["val"], "value"); + + // same patterns + let mut routes = HashMap::new(); + routes.insert(Pattern::new("", "/name"), Some(Resource::default())); + routes.insert(Pattern::new("", "/name/{val}"), Some(Resource::default())); + let (rec, _) = Router::new::<()>("/test2", ServerSettings::default(), routes); + + let mut req = TestRequest::with_uri("/name").finish(); + assert!(rec.recognize(&mut req).is_none()); + let mut req = TestRequest::with_uri("/test2/name").finish(); + assert!(rec.recognize(&mut req).is_some()); + } + fn assert_parse(pattern: &str, expected_re: &str) -> Regex { let (re_str, _) = Pattern::parse(pattern); assert_eq!(&*re_str, expected_re);