diff --git a/CHANGES.md b/CHANGES.md index 95144ce19..237b4bfbc 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,8 @@ * Gz streaming, use `flate2::write::GzDecoder` #228 +* HttpRequest::url_for is not working with scopes #429 + ## [0.7.2] - 2018-07-26 diff --git a/src/application.rs b/src/application.rs index a5cd3386f..6885185f2 100644 --- a/src/application.rs +++ b/src/application.rs @@ -140,7 +140,7 @@ where parts: Some(ApplicationParts { state, prefix: "".to_owned(), - router: Router::new(), + router: Router::new(ResourceDef::prefix("")), middlewares: Vec::new(), filters: Vec::new(), encoding: ContentEncoding::Auto, @@ -198,6 +198,7 @@ where if !prefix.starts_with('/') { prefix.insert(0, '/') } + parts.router.set_prefix(&prefix); parts.prefix = prefix; } self diff --git a/src/extractor.rs b/src/extractor.rs index aa4fdea7a..5c2c7f600 100644 --- a/src/extractor.rs +++ b/src/extractor.rs @@ -934,7 +934,7 @@ mod tests { fn test_request_extract() { let req = TestRequest::with_uri("/name/user1/?id=test").finish(); - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/{key}/{value}/"))); let info = router.recognize(&req, &(), 0); let req = req.with_route_info(info); @@ -950,7 +950,7 @@ mod tests { let s = Query::::from_request(&req, &()).unwrap(); assert_eq!(s.id, "test"); - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/{key}/{value}/"))); let req = TestRequest::with_uri("/name/32/").finish(); let info = router.recognize(&req, &(), 0); @@ -971,7 +971,7 @@ mod tests { #[test] fn test_extract_path_single() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/{value}/"))); let req = TestRequest::with_uri("/32/").finish(); @@ -982,7 +982,7 @@ mod tests { #[test] fn test_tuple_extract() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/{key}/{value}/"))); let req = TestRequest::with_uri("/name/user1/?id=test").finish(); diff --git a/src/httprequest.rs b/src/httprequest.rs index 83017dfa0..6f3bfe13e 100644 --- a/src/httprequest.rs +++ b/src/httprequest.rs @@ -420,7 +420,7 @@ mod tests { #[test] fn test_request_match_info() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/{key}/"))); let req = TestRequest::with_uri("/value/?id=test").finish(); @@ -430,7 +430,7 @@ mod tests { #[test] fn test_url_for() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); let mut resource = Resource::new(ResourceDef::new("/user/{name}.{ext}")); resource.name("index"); router.register_resource(resource); @@ -464,7 +464,8 @@ mod tests { fn test_url_for_with_prefix() { let mut resource = Resource::new(ResourceDef::new("/user/{name}.html")); resource.name("index"); - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); + router.set_prefix("/prefix"); router.register_resource(resource); let mut info = router.default_route_info(); @@ -490,7 +491,8 @@ mod tests { fn test_url_for_static() { let mut resource = Resource::new(ResourceDef::new("/index.html")); resource.name("index"); - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); + router.set_prefix("/prefix"); router.register_resource(resource); let mut info = router.default_route_info(); @@ -513,7 +515,7 @@ mod tests { #[test] fn test_url_for_external() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_external( "youtube", ResourceDef::external("https://youtube.com/watch/{video_id}"), diff --git a/src/router.rs b/src/router.rs index f3f657b58..3d112bf60 100644 --- a/src/router.rs +++ b/src/router.rs @@ -1,3 +1,4 @@ +use std::cell::RefCell; use std::cmp::min; use std::collections::HashMap; use std::hash::{Hash, Hasher}; @@ -111,9 +112,14 @@ impl ResourceInfo { U: IntoIterator, I: AsRef, { - if let Some(pattern) = self.rmap.named.get(name) { - let path = - pattern.resource_path(elements, &req.path()[..(self.prefix as usize)])?; + let mut path = String::new(); + let mut elements = elements.into_iter(); + + if self + .rmap + .patterns_for(name, &mut path, &mut elements)? + .is_some() + { if path.starts_with('/') { let conn = req.connection_info(); Ok(Url::parse(&format!( @@ -160,12 +166,15 @@ impl ResourceInfo { } pub(crate) struct ResourceMap { + root: ResourceDef, + parent: RefCell>>, named: HashMap, patterns: Vec<(ResourceDef, Option>)>, + nested: Vec>, } impl ResourceMap { - pub fn has_resource(&self, path: &str) -> bool { + fn has_resource(&self, path: &str) -> bool { let path = if path.is_empty() { "/" } else { path }; for (pattern, rmap) in &self.patterns { @@ -179,20 +188,91 @@ impl ResourceMap { } false } + + fn patterns_for( + &self, name: &str, path: &mut String, elements: &mut U, + ) -> Result, UrlGenerationError> + where + U: Iterator, + I: AsRef, + { + if self.pattern_for(name, path, elements)?.is_some() { + Ok(Some(())) + } else { + self.parent_pattern_for(name, path, elements) + } + } + + fn pattern_for( + &self, name: &str, path: &mut String, elements: &mut U, + ) -> Result, UrlGenerationError> + where + U: Iterator, + I: AsRef, + { + if let Some(pattern) = self.named.get(name) { + self.fill_root(path, elements)?; + pattern.resource_path(path, elements)?; + Ok(Some(())) + } else { + for rmap in &self.nested { + if rmap.pattern_for(name, path, elements)?.is_some() { + return Ok(Some(())); + } + } + Ok(None) + } + } + + fn fill_root( + &self, path: &mut String, elements: &mut U, + ) -> Result<(), UrlGenerationError> + where + U: Iterator, + I: AsRef, + { + if let Some(ref parent) = *self.parent.borrow() { + parent.fill_root(path, elements)?; + } + self.root.resource_path(path, elements) + } + + fn parent_pattern_for( + &self, name: &str, path: &mut String, elements: &mut U, + ) -> Result, UrlGenerationError> + where + U: Iterator, + I: AsRef, + { + if let Some(ref parent) = *self.parent.borrow() { + if let Some(pattern) = parent.named.get(name) { + self.fill_root(path, elements)?; + pattern.resource_path(path, elements)?; + Ok(Some(())) + } else { + parent.parent_pattern_for(name, path, elements) + } + } else { + Ok(None) + } + } } impl Default for Router { fn default() -> Self { - Router::new() + Router::new(ResourceDef::new("")) } } impl Router { - pub(crate) fn new() -> Self { + pub(crate) fn new(root: ResourceDef) -> Self { Router { rmap: Rc::new(ResourceMap { + root, + parent: RefCell::new(None), named: HashMap::new(), patterns: Vec::new(), + nested: Vec::new(), }), resources: Vec::new(), patterns: Vec::new(), @@ -233,6 +313,10 @@ impl Router { } } + pub(crate) fn set_prefix(&mut self, path: &str) { + Rc::get_mut(&mut self.rmap).unwrap().root = ResourceDef::new(path); + } + pub(crate) fn register_resource(&mut self, resource: Resource) { { let rmap = Rc::get_mut(&mut self.rmap).unwrap(); @@ -258,6 +342,11 @@ impl Router { .unwrap() .patterns .push((scope.rdef().clone(), Some(scope.router().rmap.clone()))); + Rc::get_mut(&mut self.rmap) + .unwrap() + .nested + .push(scope.router().rmap.clone()); + let filters = scope.take_filters(); self.patterns .push(ResourcePattern::Scope(scope.rdef().clone(), filters)); @@ -286,22 +375,25 @@ impl Router { } pub(crate) fn finish(&mut self) { - if let Some(ref default) = self.default { - for resource in &mut self.resources { - match resource { - ResourceItem::Resource(_) => (), - ResourceItem::Scope(scope) => { - if !scope.has_default_resource() { + for resource in &mut self.resources { + match resource { + ResourceItem::Resource(_) => (), + ResourceItem::Scope(scope) => { + if !scope.has_default_resource() { + if let Some(ref default) = self.default { scope.default_resource(default.clone()); } - scope.finish() } - ResourceItem::Handler(hnd) => { - if !hnd.has_default_resource() { + *scope.router().rmap.parent.borrow_mut() = Some(self.rmap.clone()); + scope.finish(); + } + ResourceItem::Handler(hnd) => { + if !hnd.has_default_resource() { + if let Some(ref default) = self.default { hnd.default_resource(default.clone()); } - hnd.finish() } + hnd.finish() } } } @@ -459,35 +551,38 @@ pub struct ResourceDef { } impl ResourceDef { - /// Parse path pattern and create new `Resource` instance. + /// Parse path pattern and create new `ResourceDef` instance. /// /// Panics if path pattern is wrong. pub fn new(path: &str) -> Self { - ResourceDef::with_prefix(path, if path.is_empty() { "" } else { "/" }, false) + ResourceDef::with_prefix(path, false, !path.is_empty()) } - /// Parse path pattern and create new `Resource` instance. + /// Parse path pattern and create new `ResourceDef` instance. /// /// Use `prefix` type instead of `static`. /// /// Panics if path regex pattern is wrong. pub fn prefix(path: &str) -> Self { - ResourceDef::with_prefix(path, "/", true) + ResourceDef::with_prefix(path, true, !path.is_empty()) } - /// Construct external resource + /// Construct external resource def /// /// Panics if path pattern is wrong. pub fn external(path: &str) -> Self { - let mut resource = ResourceDef::with_prefix(path, "/", false); + let mut resource = ResourceDef::with_prefix(path, false, false); resource.rtp = ResourceType::External; resource } - /// Parse path pattern and create new `Resource` instance with custom prefix - pub fn with_prefix(path: &str, prefix: &str, for_prefix: bool) -> Self { - let (pattern, elements, is_dynamic, len) = - ResourceDef::parse(path, prefix, for_prefix); + /// Parse path pattern and create new `ResourceDef` instance with custom prefix + pub fn with_prefix(path: &str, for_prefix: bool, slash: bool) -> Self { + let mut path = path.to_owned(); + if slash && !path.starts_with('/') { + path.insert(0, '/'); + } + let (pattern, elements, is_dynamic, len) = ResourceDef::parse(&path, for_prefix); let tp = if is_dynamic { let re = match Regex::new(&pattern) { @@ -705,23 +800,21 @@ impl ResourceDef { /// Build resource path. pub fn resource_path( - &self, elements: U, prefix: &str, - ) -> Result + &self, path: &mut String, elements: &mut U, + ) -> Result<(), UrlGenerationError> where - U: IntoIterator, + U: Iterator, I: AsRef, { - let mut path = match self.tp { - PatternType::Prefix(ref p) => p.to_owned(), - PatternType::Static(ref p) => p.to_owned(), + match self.tp { + PatternType::Prefix(ref p) => path.push_str(p), + PatternType::Static(ref p) => path.push_str(p), PatternType::Dynamic(..) => { - let mut path = String::new(); - let mut iter = elements.into_iter(); for el in &self.elements { match *el { PatternElement::Str(ref s) => path.push_str(s), PatternElement::Var(_) => { - if let Some(val) = iter.next() { + if let Some(val) = elements.next() { path.push_str(val.as_ref()) } else { return Err(UrlGenerationError::NotEnoughElements); @@ -729,34 +822,18 @@ impl ResourceDef { } } } - path } }; - - if self.rtp != ResourceType::External { - if prefix.ends_with('/') { - if path.starts_with('/') { - path.insert_str(0, &prefix[..prefix.len() - 1]); - } else { - path.insert_str(0, prefix); - } - } else { - if !path.starts_with('/') { - path.insert(0, '/'); - } - path.insert_str(0, prefix); - } - } - Ok(path) + Ok(()) } fn parse( - pattern: &str, prefix: &str, for_prefix: bool, + pattern: &str, for_prefix: bool, ) -> (String, Vec, bool, usize) { const DEFAULT_PATTERN: &str = "[^/]+"; - let mut re1 = String::from("^") + prefix; - let mut re2 = String::from(prefix); + let mut re1 = String::from("^"); + let mut re2 = String::new(); let mut el = String::new(); let mut in_param = false; let mut in_param_pattern = false; @@ -766,12 +843,7 @@ impl ResourceDef { let mut elems = Vec::new(); let mut len = 0; - for (index, ch) in pattern.chars().enumerate() { - // All routes must have a leading slash so its optional to have one - if index == 0 && ch == '/' { - continue; - } - + for ch in pattern.chars() { if in_param { // In parameter segment: `{....}` if ch == '}' { @@ -846,7 +918,7 @@ mod tests { #[test] fn test_recognizer10() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/name"))); router.register_resource(Resource::new(ResourceDef::new("/name/{val}"))); router.register_resource(Resource::new(ResourceDef::new( @@ -858,7 +930,7 @@ mod tests { ))); router.register_resource(Resource::new(ResourceDef::new("/v/{tail:.*}"))); router.register_resource(Resource::new(ResourceDef::new("/test2/{test}.html"))); - router.register_resource(Resource::new(ResourceDef::new("{test}/index.html"))); + router.register_resource(Resource::new(ResourceDef::new("/{test}/index.html"))); let req = TestRequest::with_uri("/name").finish(); let info = router.recognize(&req, &(), 0); @@ -909,7 +981,7 @@ mod tests { #[test] fn test_recognizer_2() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/index.json"))); router.register_resource(Resource::new(ResourceDef::new("/{source}.json"))); @@ -924,7 +996,7 @@ mod tests { #[test] fn test_recognizer_with_prefix() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/name"))); router.register_resource(Resource::new(ResourceDef::new("/name/{val}"))); @@ -943,7 +1015,7 @@ mod tests { assert_eq!(&info.match_info()["val"], "value"); // same patterns - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); router.register_resource(Resource::new(ResourceDef::new("/name"))); router.register_resource(Resource::new(ResourceDef::new("/name/{val}"))); @@ -1049,7 +1121,7 @@ mod tests { #[test] fn test_request_resource() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); let mut resource = Resource::new(ResourceDef::new("/index.json")); resource.name("r1"); router.register_resource(resource); @@ -1071,7 +1143,7 @@ mod tests { #[test] fn test_has_resource() { - let mut router = Router::<()>::new(); + let mut router = Router::<()>::default(); let scope = Scope::new("/test").resource("/name", |_| "done"); router.register_scope(scope); @@ -1088,4 +1160,93 @@ mod tests { let info = router.default_route_info(); assert!(info.has_resource("/test2/test10/name")); } + + #[test] + fn test_url_for() { + let mut router = Router::<()>::new(ResourceDef::prefix("")); + + let mut resource = Resource::new(ResourceDef::new("/tttt")); + resource.name("r0"); + router.register_resource(resource); + + let scope = Scope::new("/test").resource("/name", |r| { + r.name("r1"); + }); + router.register_scope(scope); + + let scope = Scope::new("/test2") + .nested("/test10", |s| s.resource("/name", |r| r.name("r2"))); + router.register_scope(scope); + router.finish(); + + let req = TestRequest::with_uri("/test").request(); + { + let info = router.default_route_info(); + + let res = info + .url_for(&req, "r0", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/tttt"); + + let res = info + .url_for(&req, "r1", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/test/name"); + + let res = info + .url_for(&req, "r2", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/test2/test10/name"); + } + + let req = TestRequest::with_uri("/test/name").request(); + let info = router.recognize(&req, &(), 0); + assert_eq!(info.resource, ResourceId::Normal(1)); + + let res = info + .url_for(&req, "r0", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/tttt"); + + let res = info + .url_for(&req, "r1", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/test/name"); + + let res = info + .url_for(&req, "r2", Vec::<&'static str>::new()) + .unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/test2/test10/name"); + } + + #[test] + fn test_url_for_dynamic() { + let mut router = Router::<()>::new(ResourceDef::prefix("")); + + let mut resource = Resource::new(ResourceDef::new("/{name}/test/index.{ext}")); + resource.name("r0"); + router.register_resource(resource); + + let scope = Scope::new("/{name1}").nested("/{name2}", |s| { + s.resource("/{name3}/test/index.{ext}", |r| r.name("r2")) + }); + router.register_scope(scope); + router.finish(); + + let req = TestRequest::with_uri("/test").request(); + { + let info = router.default_route_info(); + + let res = info.url_for(&req, "r0", vec!["sec1", "html"]).unwrap(); + assert_eq!(res.as_str(), "http://localhost:8080/sec1/test/index.html"); + + let res = info + .url_for(&req, "r2", vec!["sec1", "sec2", "sec3", "html"]) + .unwrap(); + assert_eq!( + res.as_str(), + "http://localhost:8080/sec1/sec2/sec3/test/index.html" + ); + } + } } diff --git a/src/scope.rs b/src/scope.rs index 43d078529..d8a0a81ad 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -58,11 +58,11 @@ pub struct Scope { #[cfg_attr(feature = "cargo-clippy", allow(new_without_default_derive))] impl Scope { /// Create a new scope - // TODO: Why is this not exactly the default impl? pub fn new(path: &str) -> Scope { + let rdef = ResourceDef::prefix(path); Scope { - rdef: ResourceDef::prefix(path), - router: Rc::new(Router::new()), + rdef: rdef.clone(), + router: Rc::new(Router::new(rdef)), filters: Vec::new(), middlewares: Rc::new(Vec::new()), } @@ -132,10 +132,11 @@ impl Scope { where F: FnOnce(Scope) -> Scope, { + let rdef = ResourceDef::prefix(path); let scope = Scope { - rdef: ResourceDef::prefix(path), + rdef: rdef.clone(), filters: Vec::new(), - router: Rc::new(Router::new()), + router: Rc::new(Router::new(rdef)), middlewares: Rc::new(Vec::new()), }; let mut scope = f(scope); @@ -178,10 +179,11 @@ impl Scope { where F: FnOnce(Scope) -> Scope, { + let rdef = ResourceDef::prefix(&path); let scope = Scope { - rdef: ResourceDef::prefix(&path), + rdef: rdef.clone(), filters: Vec::new(), - router: Rc::new(Router::new()), + router: Rc::new(Router::new(rdef)), middlewares: Rc::new(Vec::new()), }; Rc::get_mut(&mut self.router) @@ -258,12 +260,7 @@ impl Scope { F: FnOnce(&mut Resource) -> R + 'static, { // add resource - let pattern = ResourceDef::with_prefix( - path, - if path.is_empty() { "" } else { "/" }, - false, - ); - let mut resource = Resource::new(pattern); + let mut resource = Resource::new(ResourceDef::new(path)); f(&mut resource); Rc::get_mut(&mut self.router) diff --git a/src/test.rs b/src/test.rs index f466db2d5..f94732dd7 100644 --- a/src/test.rs +++ b/src/test.rs @@ -147,13 +147,11 @@ impl TestServer { #[cfg(feature = "rust-tls")] { use rustls::ClientConfig; - use std::io::BufReader; use std::fs::File; + use std::io::BufReader; let mut config = ClientConfig::new(); let pem_file = &mut BufReader::new(File::open("tests/cert.pem").unwrap()); - config - .root_store - .add_pem_file(pem_file).unwrap(); + config.root_store.add_pem_file(pem_file).unwrap(); ClientConnector::with_connector(Arc::new(config)).start() } #[cfg(not(any(feature = "alpn", feature = "rust-tls")))] @@ -574,7 +572,7 @@ impl TestRequest { payload, prefix, } = self; - let router = Router::<()>::new(); + let router = Router::<()>::default(); let pool = RequestPool::pool(ServerSettings::default()); let mut req = RequestPool::get(pool);