From 7d01ece3556e77c0555f4e7da6c8699d8fc34fb1 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Tue, 31 Aug 2021 16:15:22 +0300 Subject: [PATCH 1/2] ResourceDef: support multiple-patterns as prefix (#2356) Co-authored-by: Rob Ede --- actix-router/CHANGES.md | 4 + actix-router/src/resource.rs | 253 +++++++++++++++++------------------ 2 files changed, 128 insertions(+), 129 deletions(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 804f7778d..990382512 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -7,6 +7,9 @@ * Improve malformed path error message. [#384] * Prefix segments now always end with with a segment delimiter or end-of-input. [#2355] * Prefix segments with trailing slashes define a trailing empty segment. [#2355] +* Support multi-pattern prefixes and joins. [#2356] +* `ResourceDef::pattern` now returns the first pattern in multi-pattern resources. [#2356] +* Support `build_resource_path` on multi-pattern resources. [#2356] * Minimum supported Rust version (MSRV) is now 1.51. [#378]: https://github.com/actix/actix-net/pull/378 @@ -14,6 +17,7 @@ [#380]: https://github.com/actix/actix-net/pull/380 [#384]: https://github.com/actix/actix-net/pull/384 [#2355]: https://github.com/actix/actix-web/pull/2355 +[#2356]: https://github.com/actix/actix-web/pull/2356 ## 0.5.0-beta.1 - 2021-07-20 diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 57ce36804..be54336e9 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -31,13 +31,13 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// # Pattern Format and Matching Behavior /// /// Resource pattern is defined as a string of zero or more _segments_ where each segment is -/// preceeded by a slash `/`. +/// preceded by a slash `/`. /// /// This means that pattern string __must__ either be empty or begin with a slash (`/`). /// This also implies that a trailing slash in pattern defines an empty segment. /// For example, the pattern `"/user/"` has two segments: `["user", ""]` /// -/// A key point to undertand is that `ResourceDef` matches segments, not strings. +/// A key point to underhand is that `ResourceDef` matches segments, not strings. /// It matches segments individually. /// For example, the pattern `/user/` is not considered a prefix for the path `/user/123/456`, /// because the second segment doesn't match: `["user", ""]` vs `["user", "123", "456"]`. @@ -220,17 +220,15 @@ pub struct ResourceDef { name: Option, /// Pattern that generated the resource definition. - /// - /// `None` when pattern type is `DynamicSet`. patterns: Patterns, + is_prefix: bool, + /// Pattern type. pat_type: PatternType, /// List of segments that compose the pattern, in order. - /// - /// `None` when pattern type is `DynamicSet`. - segments: Option>, + segments: Vec, } #[derive(Debug, Clone, PartialEq)] @@ -248,9 +246,6 @@ enum PatternType { /// Single constant/literal segment. Static(String), - /// Single constant/literal prefix segment. - Prefix(String), - /// Single regular expression and list of dynamic segment names. Dynamic(Regex, Vec<&'static str>), @@ -284,45 +279,7 @@ impl ResourceDef { /// ``` pub fn new(paths: T) -> Self { profile_method!(new); - - match paths.patterns() { - Patterns::Single(pattern) => ResourceDef::from_single_pattern(&pattern, false), - - // since zero length pattern sets are possible - // just return a useless `ResourceDef` - Patterns::List(patterns) if patterns.is_empty() => ResourceDef { - id: 0, - name: None, - patterns: Patterns::List(patterns), - pat_type: PatternType::DynamicSet(RegexSet::empty(), Vec::new()), - segments: None, - }, - - Patterns::List(patterns) => { - let mut re_set = Vec::with_capacity(patterns.len()); - let mut pattern_data = Vec::new(); - - for pattern in &patterns { - match ResourceDef::parse(pattern, false, true) { - (PatternType::Dynamic(re, names), _) => { - re_set.push(re.as_str().to_owned()); - pattern_data.push((re, names)); - } - _ => unreachable!(), - } - } - - let pattern_re_set = RegexSet::new(re_set).unwrap(); - - ResourceDef { - id: 0, - name: None, - patterns: Patterns::List(patterns), - pat_type: PatternType::DynamicSet(pattern_re_set, pattern_data), - segments: None, - } - } - } + Self::new2(paths, false) } /// Constructs a new resource definition using a pattern that performs prefix matching. @@ -348,9 +305,9 @@ impl ResourceDef { /// assert!(!resource.is_match("user/123/stars")); /// assert!(!resource.is_match("/foo")); /// ``` - pub fn prefix(path: &str) -> Self { + pub fn prefix(paths: T) -> Self { profile_method!(prefix); - ResourceDef::from_single_pattern(path, true) + ResourceDef::new2(paths, true) } /// Constructs a new resource definition using a string pattern that performs prefix matching, @@ -375,7 +332,7 @@ impl ResourceDef { /// ``` pub fn root_prefix(path: &str) -> Self { profile_method!(root_prefix); - ResourceDef::prefix(&insert_slash(path)) + ResourceDef::prefix(insert_slash(path).into_owned()) } /// Returns a numeric resource ID. @@ -453,17 +410,14 @@ impl ResourceDef { /// assert!(!ResourceDef::new("/user").is_prefix()); /// ``` pub fn is_prefix(&self) -> bool { - match &self.pat_type { - PatternType::Prefix(_) => true, - PatternType::Dynamic(re, _) if !re.as_str().ends_with('$') => true, - _ => false, - } + self.is_prefix } /// Returns the pattern string that generated the resource definition. /// - /// Returns `None` if definition was constructed with multiple patterns. - /// See [`patterns_iter`][Self::pattern_iter]. + /// If definition is constructed with multiple patterns, the first pattern is returned. To get + /// all patterns, use [`patterns_iter`][Self::pattern_iter]. If resource has 0 patterns, + /// returns `None`. /// /// # Examples /// ``` @@ -472,11 +426,11 @@ impl ResourceDef { /// assert_eq!(resource.pattern().unwrap(), "/user/{id}"); /// /// let mut resource = ResourceDef::new(["/profile", "/user/{id}"]); - /// assert!(resource.pattern().is_none()); + /// assert_eq!(resource.pattern(), Some("/profile")); pub fn pattern(&self) -> Option<&str> { match &self.patterns { Patterns::Single(pattern) => Some(pattern.as_str()), - Patterns::List(_) => None, + Patterns::List(patterns) => patterns.first().map(AsRef::as_ref), } } @@ -563,8 +517,8 @@ impl ResourceDef { .collect::>(); match patterns.len() { - 1 => ResourceDef::from_single_pattern(&patterns[0], other.is_prefix()), - _ => ResourceDef::new(patterns), + 1 => ResourceDef::new2(&patterns[0], other.is_prefix()), + _ => ResourceDef::new2(patterns, other.is_prefix()), } } @@ -609,11 +563,10 @@ impl ResourceDef { // `self.find_match(path).is_some()` // but this skips some checks and uses potentially faster regex methods - match self.pat_type { - PatternType::Static(ref s) => s == path, - PatternType::Prefix(ref prefix) => is_prefix(prefix, path), - PatternType::Dynamic(ref re, _) => re.is_match(path), - PatternType::DynamicSet(ref re, _) => re.is_match(path), + match &self.pat_type { + PatternType::Static(pattern) => self.static_match(pattern, path).is_some(), + PatternType::Dynamic(re, _) => re.is_match(path), + PatternType::DynamicSet(re, _) => re.is_match(path), } } @@ -656,11 +609,7 @@ impl ResourceDef { profile_method!(find_match); match &self.pat_type { - PatternType::Static(segment) if path == segment => Some(segment.len()), - PatternType::Static(_) => None, - - PatternType::Prefix(prefix) if is_prefix(prefix, path) => Some(prefix.len()), - PatternType::Prefix(_) => None, + PatternType::Static(pattern) => self.static_match(pattern, path), PatternType::Dynamic(re, _) => Some(re.captures(path)?[1].len()), @@ -753,10 +702,10 @@ impl ResourceDef { let path_str = path.path(); let (matched_len, matched_vars) = match &self.pat_type { - PatternType::Static(_) | PatternType::Prefix(_) => { + PatternType::Static(pattern) => { profile_section!(pattern_static_or_prefix); - match self.find_match(path_str) { + match self.static_match(pattern, path_str) { Some(len) => (len, None), None => return false, } @@ -844,13 +793,10 @@ impl ResourceDef { F: FnMut(&str) -> Option, I: AsRef, { - for el in match self.segments { - Some(ref segments) => segments, - None => return false, - } { - match *el { - PatternSegment::Const(ref val) => path.push_str(val), - PatternSegment::Var(ref name) => match vars(name) { + for segment in &self.segments { + match segment { + PatternSegment::Const(val) => path.push_str(val), + PatternSegment::Var(name) => match vars(name) { Some(val) => path.push_str(val.as_ref()), _ => return false, }, @@ -864,8 +810,8 @@ impl ResourceDef { /// /// Returns `true` on success. /// - /// Resource paths can not be built from multi-pattern resources; this call will always return - /// false and will not add anything to the string buffer. + /// For multi-pattern resources, the first pattern is used under the assumption that it would be + /// equivalent to any other choice. /// /// # Examples /// ``` @@ -890,8 +836,8 @@ impl ResourceDef { /// /// Returns `true` on success. /// - /// Resource paths can not be built from multi-pattern resources; this call will always return - /// false and will not add anything to the string buffer. + /// For multi-pattern resources, the first pattern is used under the assumption that it would be + /// equivalent to any other choice. /// /// # Examples /// ``` @@ -921,19 +867,69 @@ impl ResourceDef { self.build_resource_path(path, |name| values.get(name).map(AsRef::::as_ref)) } - /// Parse path pattern and create a new instance. - fn from_single_pattern(pattern: &str, is_prefix: bool) -> Self { - profile_method!(from_single_pattern); + /// Returns true if `prefix` acts as a proper prefix (i.e., separated by a slash) in `path`. + fn static_match(&self, pattern: &str, path: &str) -> Option { + let rem = path.strip_prefix(pattern)?; - let pattern = pattern.to_owned(); - let (pat_type, segments) = ResourceDef::parse(&pattern, is_prefix, false); + match self.is_prefix { + // resource is not a prefix so an exact match is needed + false if rem.is_empty() => Some(pattern.len()), + + // resource is a prefix so rem should start with a path delimiter + true if rem.is_empty() || rem.starts_with('/') => Some(pattern.len()), + + // otherwise, no match + _ => None, + } + } + + fn new2(paths: T, is_prefix: bool) -> Self { + profile_method!(new2); + + let patterns = paths.patterns(); + let (pat_type, segments) = match &patterns { + Patterns::Single(pattern) => ResourceDef::parse(pattern, is_prefix, false), + + // since zero length pattern sets are possible + // just return a useless `ResourceDef` + Patterns::List(patterns) if patterns.is_empty() => ( + PatternType::DynamicSet(RegexSet::empty(), Vec::new()), + Vec::new(), + ), + + Patterns::List(patterns) => { + let mut re_set = Vec::with_capacity(patterns.len()); + let mut pattern_data = Vec::new(); + let mut segments = None; + + for pattern in patterns { + match ResourceDef::parse(pattern, is_prefix, true) { + (PatternType::Dynamic(re, names), segs) => { + re_set.push(re.as_str().to_owned()); + pattern_data.push((re, names)); + segments.get_or_insert(segs); + } + _ => unreachable!(), + } + } + + let pattern_re_set = RegexSet::new(re_set).unwrap(); + let segments = segments.unwrap_or_else(Vec::new); + + ( + PatternType::DynamicSet(pattern_re_set, pattern_data), + segments, + ) + } + }; ResourceDef { id: 0, name: None, - patterns: Patterns::Single(pattern), + patterns, + is_prefix, pat_type, - segments: Some(segments), + segments, } } @@ -1023,20 +1019,15 @@ impl ResourceDef { ) -> (PatternType, Vec) { profile_method!(parse); - let mut unprocessed = pattern; - - if !force_dynamic && unprocessed.find('{').is_none() && !unprocessed.ends_with('*') { + if !force_dynamic && pattern.find('{').is_none() && !pattern.ends_with('*') { // pattern is static - - let tp = if is_prefix { - PatternType::Prefix(unprocessed.to_owned()) - } else { - PatternType::Static(unprocessed.to_owned()) - }; - - return (tp, vec![PatternSegment::Const(unprocessed.to_owned())]); + return ( + PatternType::Static(pattern.to_owned()), + vec![PatternSegment::Const(pattern.to_owned())], + ); } + let mut unprocessed = pattern; let mut segments = Vec::new(); let mut re = format!("{}^", REGEX_FLAGS); let mut dyn_segment_count = 0; @@ -1137,18 +1128,7 @@ impl Eq for ResourceDef {} impl PartialEq for ResourceDef { fn eq(&self, other: &ResourceDef) -> bool { - self.patterns == other.patterns - && match &self.pat_type { - PatternType::Static(_) => matches!(&other.pat_type, PatternType::Static(_)), - PatternType::Prefix(_) => matches!(&other.pat_type, PatternType::Prefix(_)), - PatternType::Dynamic(re, _) => match &other.pat_type { - PatternType::Dynamic(other_re, _) => re.as_str() == other_re.as_str(), - _ => false, - }, - PatternType::DynamicSet(_, _) => { - matches!(&other.pat_type, PatternType::DynamicSet(..)) - } - } + self.patterns == other.patterns && self.is_prefix == other.is_prefix } } @@ -1183,15 +1163,6 @@ pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { } } -/// Returns true if `prefix` acts as a proper prefix (i.e., separated by a slash) in `path`. -fn is_prefix(prefix: &str, path: &str) -> bool { - match path.strip_prefix(prefix) { - // Ensure the match ends at segment boundary - Some(rem) if rem.is_empty() || rem.starts_with('/') => true, - _ => false, - } -} - #[cfg(test)] mod tests { use super::*; @@ -1376,6 +1347,24 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); } + #[test] + fn dynamic_set_prefix() { + let re = ResourceDef::prefix(vec!["/u/{id}", "/{id:[[:digit:]]{3}}"]); + + assert_eq!(re.find_match("/u/abc"), Some(6)); + assert_eq!(re.find_match("/u/abc/123"), Some(6)); + assert_eq!(re.find_match("/s/user/profile"), None); + + assert_eq!(re.find_match("/123"), Some(4)); + assert_eq!(re.find_match("/123/456"), Some(4)); + assert_eq!(re.find_match("/12345"), None); + + let mut path = Path::new("/151/res"); + assert!(re.capture_match_info(&mut path)); + assert_eq!(path.get("id").unwrap(), "151"); + assert_eq!(path.unprocessed(), "/res"); + } + #[test] fn parse_tail() { let re = ResourceDef::new("/user/-{id}*"); @@ -1602,10 +1591,11 @@ mod tests { } #[test] - fn multi_pattern_cannot_build_path() { + fn multi_pattern_build_path() { let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); let mut s = String::new(); - assert!(!resource.resource_path_from_iter(&mut s, &mut ["123"].iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut ["123"].iter())); + assert_eq!(s, "/user/123"); } #[test] @@ -1738,8 +1728,12 @@ mod tests { join_test!("", "" => "", "/hello", "/"); join_test!("/user", "" => "", "/user", "/user/123", "/user11", "user", "user/123"); - join_test!("", "/user"=> "", "/user", "foo", "/user11", "user", "user/123"); - join_test!("/user", "/xx"=> "", "", "/", "/user", "/xx", "/userxx", "/user/xx"); + join_test!("", "/user" => "", "/user", "foo", "/user11", "user", "user/123"); + join_test!("/user", "/xx" => "", "", "/", "/user", "/xx", "/userxx", "/user/xx"); + + join_test!(["/ver/{v}", "/v{v}"], ["/req/{req}", "/{req}"] => "/v1/abc", + "/ver/1/abc", "/v1/req/abc", "/ver/1/req/abc", "/v1/abc/def", + "/ver1/req/abc/def", "", "/", "/v1/"); } #[test] @@ -1777,6 +1771,7 @@ mod tests { match_methods_agree!(prefix "" => "", "/", "/foo"); match_methods_agree!(prefix "/user" => "user", "/user", "/users", "/user/123", "/foo"); match_methods_agree!(prefix r"/id/{id:\d{3}}" => "/id/123", "/id/1234"); + match_methods_agree!(["/v{v}", "/ver/{v}"] => "", "s/v", "/v1", "/v1/xx", "/ver/i3/5", "/ver/1"); } #[test] From 373b3f91dff58ac6c5b1158206e7380376906541 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Wed, 1 Sep 2021 06:48:43 +0300 Subject: [PATCH 2/2] rework `ResourceMap` internals (#2337) --- src/app_service.rs | 4 +- src/request.rs | 9 +- src/rmap.rs | 422 ++++++++++++++++++++++++--------------------- 3 files changed, 233 insertions(+), 202 deletions(-) diff --git a/src/app_service.rs b/src/app_service.rs index ce52543b8..cf34b302e 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -79,7 +79,7 @@ where .into_iter() .for_each(|mut srv| srv.register(&mut config)); - let mut rmap = ResourceMap::new(ResourceDef::new("")); + let mut rmap = ResourceMap::new(ResourceDef::prefix("")); let (config, services) = config.into_services(); @@ -104,7 +104,7 @@ where // complete ResourceMap tree creation let rmap = Rc::new(rmap); - rmap.finish(rmap.clone()); + ResourceMap::finish(&rmap); // construct all async data factory futures let factory_futs = join_all(self.async_data_factories.iter().map(|f| f())); diff --git a/src/request.rs b/src/request.rs index 59850b4ca..c25a5397a 100644 --- a/src/request.rs +++ b/src/request.rs @@ -511,7 +511,7 @@ mod tests { let mut res = ResourceDef::new("/user/{name}.{ext}"); res.set_name("index"); - let mut rmap = ResourceMap::new(ResourceDef::new("")); + let mut rmap = ResourceMap::new(ResourceDef::prefix("")); rmap.add(&mut res, None); assert!(rmap.has_resource("/user/test.html")); assert!(!rmap.has_resource("/test/unknown")); @@ -541,7 +541,7 @@ mod tests { let mut rdef = ResourceDef::new("/index.html"); rdef.set_name("index"); - let mut rmap = ResourceMap::new(ResourceDef::new("")); + let mut rmap = ResourceMap::new(ResourceDef::prefix("")); rmap.add(&mut rdef, None); assert!(rmap.has_resource("/index.html")); @@ -562,7 +562,7 @@ mod tests { let mut rdef = ResourceDef::new("/index.html"); rdef.set_name("index"); - let mut rmap = ResourceMap::new(ResourceDef::new("")); + let mut rmap = ResourceMap::new(ResourceDef::prefix("")); rmap.add(&mut rdef, None); assert!(rmap.has_resource("/index.html")); @@ -581,9 +581,8 @@ mod tests { rdef.set_name("youtube"); - let mut rmap = ResourceMap::new(ResourceDef::new("")); + let mut rmap = ResourceMap::new(ResourceDef::prefix("")); rmap.add(&mut rdef, None); - assert!(rmap.has_resource("https://youtube.com/watch/unknown")); let req = TestRequest::default().rmap(rmap).to_http_request(); let url = req.url_for("youtube", &["oHg5SJYRHA0"]); diff --git a/src/rmap.rs b/src/rmap.rs index 0ee4de47e..8466eda28 100644 --- a/src/rmap.rs +++ b/src/rmap.rs @@ -10,43 +10,75 @@ use crate::request::HttpRequest; #[derive(Clone, Debug)] pub struct ResourceMap { - root: ResourceDef, + pattern: ResourceDef, + + /// Named resources within the tree or, for external resources, + /// it points to isolated nodes outside the tree. + named: AHashMap>, + parent: RefCell>, - named: AHashMap, - patterns: Vec<(ResourceDef, Option>)>, + + /// Must be `None` for "edge" nodes. + nodes: Option>>, } impl ResourceMap { + /// Creates a _container_ node in the `ResourceMap` tree. pub fn new(root: ResourceDef) -> Self { ResourceMap { - root, - parent: RefCell::new(Weak::new()), + pattern: root, named: AHashMap::default(), - patterns: Vec::new(), + parent: RefCell::new(Weak::new()), + nodes: Some(Vec::new()), } } + /// Adds a (possibly nested) resource. + /// + /// To add a non-prefix pattern, `nested` must be `None`. + /// To add external resource, supply a pattern without a leading `/`. + /// The root pattern of `nested`, if present, should match `pattern`. pub fn add(&mut self, pattern: &mut ResourceDef, nested: Option>) { - pattern.set_id(self.patterns.len() as u16); - self.patterns.push((pattern.clone(), nested)); - if let Some(name) = pattern.name() { - self.named.insert(name.to_owned(), pattern.clone()); + pattern.set_id(self.nodes.as_ref().unwrap().len() as u16); + + if let Some(new_node) = nested { + assert_eq!(&new_node.pattern, pattern, "`patern` and `nested` mismatch"); + self.named.extend(new_node.named.clone().into_iter()); + self.nodes.as_mut().unwrap().push(new_node); + } else { + let new_node = Rc::new(ResourceMap { + pattern: pattern.clone(), + named: AHashMap::default(), + parent: RefCell::new(Weak::new()), + nodes: None, + }); + + if let Some(name) = pattern.name() { + self.named.insert(name.to_owned(), Rc::clone(&new_node)); + } + + let is_external = match pattern.pattern() { + Some(p) => !p.is_empty() && !p.starts_with('/'), + None => false, + }; + + // Don't add external resources to the tree + if !is_external { + self.nodes.as_mut().unwrap().push(new_node); + } } } - pub(crate) fn finish(&self, current: Rc) { - for (_, nested) in &self.patterns { - if let Some(ref nested) = nested { - *nested.parent.borrow_mut() = Rc::downgrade(¤t); - nested.finish(nested.clone()); - } + pub(crate) fn finish(self: &Rc) { + for node in self.nodes.iter().flatten() { + node.parent.replace(Rc::downgrade(self)); + ResourceMap::finish(node); } } /// Generate url for named resource /// - /// Check [`HttpRequest::url_for()`](../struct.HttpRequest.html#method. - /// url_for) for detailed information. + /// Check [`HttpRequest::url_for`] for detailed information. pub fn url_for( &self, req: &HttpRequest, @@ -57,197 +89,97 @@ impl ResourceMap { U: IntoIterator, I: AsRef, { - let mut path = String::new(); let mut elements = elements.into_iter(); - if self.patterns_for(name, &mut path, &mut elements)?.is_some() { - if path.starts_with('/') { - let conn = req.connection_info(); - Ok(Url::parse(&format!( - "{}://{}{}", - conn.scheme(), - conn.host(), - path - ))?) - } else { - Ok(Url::parse(&path)?) - } + let path = self + .named + .get(name) + .ok_or(UrlGenerationError::ResourceNotFound)? + .root_rmap_fn(String::with_capacity(24), |mut acc, node| { + node.pattern + .resource_path_from_iter(&mut acc, &mut elements) + .then(|| acc) + }) + .ok_or(UrlGenerationError::NotEnoughElements)?; + + if path.starts_with('/') { + let conn = req.connection_info(); + Ok(Url::parse(&format!( + "{}://{}{}", + conn.scheme(), + conn.host(), + path + ))?) } else { - Err(UrlGenerationError::ResourceNotFound) + Ok(Url::parse(&path)?) } } pub fn has_resource(&self, path: &str) -> bool { - let path = if path.is_empty() { "/" } else { path }; - - for (pattern, rmap) in &self.patterns { - if let Some(ref rmap) = rmap { - if let Some(pat_len) = pattern.find_match(path) { - return rmap.has_resource(&path[pat_len..]); - } - } else if pattern.is_match(path) || pattern.pattern() == Some("") && path == "/" { - return true; - } - } - false + self.find_matching_node(path).is_some() } /// Returns the name of the route that matches the given path or None if no full match - /// is possible. + /// is possible or the matching resource is not named. pub fn match_name(&self, path: &str) -> Option<&str> { - let path = if path.is_empty() { "/" } else { path }; - - for (pattern, rmap) in &self.patterns { - if let Some(ref rmap) = rmap { - if let Some(plen) = pattern.find_match(path) { - return rmap.match_name(&path[plen..]); - } - } else if pattern.is_match(path) { - return pattern.name(); - } - } - - None + self.find_matching_node(path)?.pattern.name() } /// Returns the full resource pattern matched against a path or None if no full match /// is possible. pub fn match_pattern(&self, path: &str) -> Option { - let path = if path.is_empty() { "/" } else { path }; - - // ensure a full match exists - if !self.has_resource(path) { - return None; - } - - Some(self.traverse_resource_pattern(path)) + self.find_matching_node(path)?.root_rmap_fn( + String::with_capacity(24), + |mut acc, node| { + acc.push_str(node.pattern.pattern()?); + Some(acc) + }, + ) } - /// Takes remaining path and tries to match it up against a resource definition within the - /// current resource map recursively, returning a concatenation of all resource prefixes and - /// patterns matched in the tree. - /// - /// Should only be used after checking the resource exists in the map so that partial match - /// patterns are not returned. - fn traverse_resource_pattern(&self, remaining: &str) -> String { - for (pattern, rmap) in &self.patterns { - if let Some(ref rmap) = rmap { - if let Some(prefix_len) = pattern.find_match(remaining) { - // TODO: think about unwrap_or - let prefix = pattern.pattern().unwrap_or("").to_owned(); - - return [ - prefix, - rmap.traverse_resource_pattern(&remaining[prefix_len..]), - ] - .concat(); - } - } else if pattern.is_match(remaining) { - // TODO: think about unwrap_or - return pattern.pattern().unwrap_or("").to_owned(); - } - } - - String::new() + fn find_matching_node(&self, path: &str) -> Option<&ResourceMap> { + self._find_matching_node(path).flatten() } - fn patterns_for( - &self, - name: &str, - path: &mut String, - elements: &mut U, - ) -> Result, UrlGenerationError> + /// Returns `None` if root pattern doesn't match; + /// `Some(None)` if root pattern matches but there is no matching child pattern. + /// Don't search sideways when `Some(none)` is returned. + fn _find_matching_node(&self, path: &str) -> Option> { + let matched_len = self.pattern.find_match(path)?; + let path = &path[matched_len..]; + + Some(match &self.nodes { + // find first sub-node to match remaining path + Some(nodes) => nodes + .iter() + .filter_map(|node| node._find_matching_node(path)) + .next() + .flatten(), + + // only terminate at edge nodes + None => Some(self), + }) + } + + /// Find `self`'s highest ancestor and then run `F`, providing `B`, in that rmap context. + fn root_rmap_fn(&self, init: B, mut f: F) -> Option where - U: Iterator, - I: AsRef, + F: FnMut(B, &ResourceMap) -> Option, { - if self.pattern_for(name, path, elements)?.is_some() { - Ok(Some(())) - } else { - self.parent_pattern_for(name, path, elements) - } + self._root_rmap_fn(init, &mut f) } - fn pattern_for( - &self, - name: &str, - path: &mut String, - elements: &mut U, - ) -> Result, UrlGenerationError> + /// Run `F`, providing `B`, if `self` is top-level resource map, else recurse to parent map. + fn _root_rmap_fn(&self, init: B, f: &mut F) -> Option where - U: Iterator, - I: AsRef, + F: FnMut(B, &ResourceMap) -> Option, { - if let Some(pattern) = self.named.get(name) { - if pattern - .pattern() - .map(|pat| pat.starts_with('/')) - .unwrap_or(false) - { - self.fill_root(path, elements)?; - } + let data = match self.parent.borrow().upgrade() { + Some(ref parent) => parent._root_rmap_fn(init, f)?, + None => init, + }; - if pattern.resource_path_from_iter(path, elements) { - Ok(Some(())) - } else { - Err(UrlGenerationError::NotEnoughElements) - } - } else { - for (_, rmap) in &self.patterns { - if let Some(ref rmap) = rmap { - 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().upgrade() { - parent.fill_root(path, elements)?; - } - - if self.root.resource_path_from_iter(path, elements) { - Ok(()) - } else { - Err(UrlGenerationError::NotEnoughElements) - } - } - - 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().upgrade() { - if let Some(pattern) = parent.named.get(name) { - self.fill_root(path, elements)?; - if pattern.resource_path_from_iter(path, elements) { - Ok(Some(())) - } else { - Err(UrlGenerationError::NotEnoughElements) - } - } else { - parent.parent_pattern_for(name, path, elements) - } - } else { - Ok(None) - } + f(data, self) } } @@ -259,7 +191,7 @@ mod tests { fn extract_matched_pattern() { let mut root = ResourceMap::new(ResourceDef::root_prefix("")); - let mut user_map = ResourceMap::new(ResourceDef::root_prefix("")); + let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}")); user_map.add(&mut ResourceDef::new("/"), None); user_map.add(&mut ResourceDef::new("/profile"), None); user_map.add(&mut ResourceDef::new("/article/{id}"), None); @@ -275,9 +207,10 @@ mod tests { &mut ResourceDef::root_prefix("/user/{id}"), Some(Rc::new(user_map)), ); + root.add(&mut ResourceDef::new("/info"), None); let root = Rc::new(root); - root.finish(Rc::clone(&root)); + ResourceMap::finish(&root); // sanity check resource map setup @@ -288,7 +221,7 @@ mod tests { assert!(root.has_resource("/v2")); assert!(!root.has_resource("/v33")); - assert!(root.has_resource("/user/22")); + assert!(!root.has_resource("/user/22")); assert!(root.has_resource("/user/22/")); assert!(root.has_resource("/user/22/profile")); @@ -336,7 +269,7 @@ mod tests { rdef.set_name("root_info"); root.add(&mut rdef, None); - let mut user_map = ResourceMap::new(ResourceDef::root_prefix("")); + let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}")); let mut rdef = ResourceDef::new("/"); user_map.add(&mut rdef, None); @@ -350,14 +283,14 @@ mod tests { ); let root = Rc::new(root); - root.finish(Rc::clone(&root)); + ResourceMap::finish(&root); // sanity check resource map setup assert!(root.has_resource("/info")); assert!(!root.has_resource("/bar")); - assert!(root.has_resource("/user/22")); + assert!(!root.has_resource("/user/22")); assert!(root.has_resource("/user/22/")); assert!(root.has_resource("/user/22/post/55")); @@ -377,7 +310,7 @@ mod tests { // ref: https://github.com/actix/actix-web/issues/1582 let mut root = ResourceMap::new(ResourceDef::root_prefix("")); - let mut user_map = ResourceMap::new(ResourceDef::root_prefix("")); + let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}")); user_map.add(&mut ResourceDef::new("/"), None); user_map.add(&mut ResourceDef::new("/profile"), None); user_map.add(&mut ResourceDef::new("/article/{id}"), None); @@ -393,20 +326,119 @@ mod tests { ); let root = Rc::new(root); - root.finish(Rc::clone(&root)); + ResourceMap::finish(&root); // check root has no parent assert!(root.parent.borrow().upgrade().is_none()); // check child has parent reference - assert!(root.patterns[0].1.is_some()); + assert!(root.nodes.as_ref().unwrap()[0] + .parent + .borrow() + .upgrade() + .is_some()); // check child's parent root id matches root's root id - assert_eq!( - root.patterns[0].1.as_ref().unwrap().root.id(), - root.root.id() - ); + assert!(Rc::ptr_eq( + &root.nodes.as_ref().unwrap()[0] + .parent + .borrow() + .upgrade() + .unwrap(), + &root + )); let output = format!("{:?}", root); assert!(output.starts_with("ResourceMap {")); assert!(output.ends_with(" }")); } + + #[test] + fn short_circuit() { + let mut root = ResourceMap::new(ResourceDef::prefix("")); + + let mut user_root = ResourceDef::prefix("/user"); + let mut user_map = ResourceMap::new(user_root.clone()); + user_map.add(&mut ResourceDef::new("/u1"), None); + user_map.add(&mut ResourceDef::new("/u2"), None); + + root.add(&mut ResourceDef::new("/user/u3"), None); + root.add(&mut user_root, Some(Rc::new(user_map))); + root.add(&mut ResourceDef::new("/user/u4"), None); + + let rmap = Rc::new(root); + ResourceMap::finish(&rmap); + + assert!(rmap.has_resource("/user/u1")); + assert!(rmap.has_resource("/user/u2")); + assert!(rmap.has_resource("/user/u3")); + assert!(!rmap.has_resource("/user/u4")); + } + + #[test] + fn url_for() { + let mut root = ResourceMap::new(ResourceDef::prefix("")); + + let mut user_scope_rdef = ResourceDef::prefix("/user"); + let mut user_scope_map = ResourceMap::new(user_scope_rdef.clone()); + + let mut user_rdef = ResourceDef::new("/{user_id}"); + let mut user_map = ResourceMap::new(user_rdef.clone()); + + let mut post_rdef = ResourceDef::new("/post/{sub_id}"); + post_rdef.set_name("post"); + + user_map.add(&mut post_rdef, None); + user_scope_map.add(&mut user_rdef, Some(Rc::new(user_map))); + root.add(&mut user_scope_rdef, Some(Rc::new(user_scope_map))); + + let rmap = Rc::new(root); + ResourceMap::finish(&rmap); + + let mut req = crate::test::TestRequest::default(); + req.set_server_hostname("localhost:8888"); + let req = req.to_http_request(); + + let url = rmap + .url_for(&req, "post", &["u123", "foobar"]) + .unwrap() + .to_string(); + assert_eq!(url, "http://localhost:8888/user/u123/post/foobar"); + + assert!(rmap.url_for(&req, "missing", &["u123"]).is_err()); + } + + #[test] + fn external_resource_with_no_name() { + let mut root = ResourceMap::new(ResourceDef::prefix("")); + + let mut rdef = ResourceDef::new("https://duck.com/{query}"); + root.add(&mut rdef, None); + + let rmap = Rc::new(root); + ResourceMap::finish(&rmap); + + assert!(!rmap.has_resource("https://duck.com/abc")); + } + + #[test] + fn external_resource_with_name() { + let mut root = ResourceMap::new(ResourceDef::prefix("")); + + let mut rdef = ResourceDef::new("https://duck.com/{query}"); + rdef.set_name("duck"); + root.add(&mut rdef, None); + + let rmap = Rc::new(root); + ResourceMap::finish(&rmap); + + assert!(!rmap.has_resource("https://duck.com/abc")); + + let mut req = crate::test::TestRequest::default(); + req.set_server_hostname("localhost:8888"); + let req = req.to_http_request(); + + assert_eq!( + rmap.url_for(&req, "duck", &["abcd"]).unwrap().to_string(), + "https://duck.com/abcd" + ); + } }