From ad22a93466ffff2252f63d2ef0ebfc2ad501c11a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 16 Jul 2021 21:41:57 +0100 Subject: [PATCH] allow path building when resource has tail (#371) --- actix-router/CHANGES.md | 5 + actix-router/src/resource.rs | 298 +++++++++++++++++++++++++++-------- 2 files changed, 236 insertions(+), 67 deletions(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 40dcf7e2..77251c21 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -1,15 +1,20 @@ # Changes ## Unreleased - 2021-xx-xx +* Resource definitions with unnamed tail segments now correctly interpolate the tail when constructed from an iterator. [#371] +* Introduce `ResourceDef::resource_path_from_map_with_tail` method to allow building paths in the presence of unnamed tail segments. [#371] * Fix segment interpolation leaving `Path` in unintended state after matching. [#368] * Path tail pattern now works as expected after a dynamic segment (e.g. `/user/{uid}/*`). [#366] * Fixed a bug where, in multi-patterns, static patterns are interpreted as regex. [#366] * Rename `Path::{len => segment_count}` to be more descriptive of it's purpose. [#370] +* Alias `ResourceDef::{resource_path => resource_path_from_iter}` pending eventual deprecation. [#371] +* Alias `ResourceDef::{resource_path_named => resource_path_from_map}` pending eventual deprecation. [#371] [#368]: https://github.com/actix/actix-net/pull/368 [#366]: https://github.com/actix/actix-net/pull/366 [#368]: https://github.com/actix/actix-net/pull/368 [#370]: https://github.com/actix/actix-net/pull/370 +[#371]: https://github.com/actix/actix-net/pull/371 ## 0.4.0 - 2021-06-06 diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index e8556809..ad3d47e2 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1,8 +1,8 @@ use std::{ - borrow::Cow, + borrow::{Borrow, Cow}, cmp::min, collections::HashMap, - hash::{Hash, Hasher}, + hash::{BuildHasher, Hash, Hasher}, mem, }; @@ -48,6 +48,11 @@ enum PatternElement { /// Name of dynamic segment. Var(String), + + /// Tail segment. If present in elements list, it will always be last. + /// + /// Tail has optional name for patterns like `/foo/{tail}*` vs "/foo/*". + Tail(Option), } #[derive(Clone, Debug)] @@ -337,10 +342,12 @@ impl ResourceDef { true } - /// Build resource path with a closure that maps variable elements' names to values. + /// Builds resource path with a closure that maps variable elements' names to values. + /// + /// Unnamed tail pattern elements will receive `None`. fn build_resource_path(&self, path: &mut String, mut vars: F) -> bool where - F: FnMut(&str) -> Option, + F: FnMut(Option<&str>) -> Option, I: AsRef, { for el in match self.elements { @@ -349,18 +356,24 @@ impl ResourceDef { } { match *el { PatternElement::Const(ref val) => path.push_str(val), - PatternElement::Var(ref name) => match vars(name) { + PatternElement::Var(ref name) => match vars(Some(name)) { Some(val) => path.push_str(val.as_ref()), _ => return false, }, + PatternElement::Tail(ref name) => match vars(name.as_deref()) { + Some(val) => path.push_str(val.as_ref()), + None => return false, + }, } } true } - /// Build resource path from elements. Returns `true` on success. - pub fn resource_path(&self, path: &mut String, elements: &mut U) -> bool + /// Builds resource path from elements. Returns `true` on success. + /// + /// If resource pattern has a tail segment, an element must be provided for it. + pub fn resource_path_from_iter(&self, path: &mut String, elements: &mut U) -> bool where U: Iterator, I: AsRef, @@ -368,21 +381,74 @@ impl ResourceDef { self.build_resource_path(path, |_| elements.next()) } - /// Build resource path from elements. Returns `true` on success. + // intentionally not deprecated yet + #[doc(hidden)] + pub fn resource_path(&self, path: &mut String, elements: &mut U) -> bool + where + U: Iterator, + I: AsRef, + { + self.resource_path_from_iter(path, elements) + } + + /// Builds resource path from map of elements. Returns `true` on success. + /// + /// If resource pattern has an unnamed tail segment, path building will fail. + pub fn resource_path_from_map( + &self, + path: &mut String, + elements: &HashMap, + ) -> bool + where + K: Borrow + Eq + Hash, + V: AsRef, + S: BuildHasher, + { + self.build_resource_path(path, |name| { + name.and_then(|name| elements.get(name).map(AsRef::::as_ref)) + }) + } + + // intentionally not deprecated yet + #[doc(hidden)] pub fn resource_path_named( &self, path: &mut String, elements: &HashMap, ) -> bool where - K: std::borrow::Borrow + Eq + Hash, + K: Borrow + Eq + Hash, V: AsRef, - S: std::hash::BuildHasher, + S: BuildHasher, { - self.build_resource_path(path, |name| elements.get(name)) + self.resource_path_from_map(path, elements) } - fn parse_param(pattern: &str) -> (PatternElement, String, &str, bool) { + /// Build resource path from map of elements, allowing tail segments to be appended. + /// + /// If resource pattern does not define a tail segment, the `tail` parameter will be unused. + /// In this case, use [`resource_path_from_map`][Self::resource_path_from_map] instead. + /// + /// Returns `true` on success. + pub fn resource_path_from_map_with_tail( + &self, + path: &mut String, + elements: &HashMap, + tail: T, + ) -> bool + where + K: Borrow + Eq + Hash, + V: AsRef, + S: BuildHasher, + T: AsRef, + { + self.build_resource_path(path, |name| match name { + Some(name) => elements.get(name).map(AsRef::::as_ref), + None => Some(tail.as_ref()), + }) + } + + fn parse_param(pattern: &str) -> (PatternElement, String, &str) { const DEFAULT_PATTERN: &str = "[^/]+"; const DEFAULT_PATTERN_TAIL: &str = ".*"; @@ -401,22 +467,26 @@ impl ResourceDef { }) .expect("malformed dynamic segment"); - let (mut param, mut rem) = pattern.split_at(close_idx + 1); - param = ¶m[1..param.len() - 1]; // Remove outer brackets - let tail = rem == "*"; + let (mut param, mut unprocessed) = pattern.split_at(close_idx + 1); + + // remove outer curly brackets + param = ¶m[1..param.len() - 1]; + + let tail = unprocessed == "*"; let (name, pattern) = match param.find(':') { Some(idx) => { if tail { panic!("Custom regex is not supported for remainder match"); } + let (name, pattern) = param.split_at(idx); (name, &pattern[1..]) } None => ( param, if tail { - rem = &rem[1..]; + unprocessed = &unprocessed[1..]; DEFAULT_PATTERN_TAIL } else { DEFAULT_PATTERN @@ -424,61 +494,76 @@ impl ResourceDef { ), }; - ( - PatternElement::Var(name.to_string()), - format!(r"(?P<{}>{})", &name, &pattern), - rem, - tail, - ) + let element = if tail { + PatternElement::Tail(Some(name.to_string())) + } else { + PatternElement::Var(name.to_string()) + }; + + let regex = format!(r"(?P<{}>{})", &name, &pattern); + + (element, regex, unprocessed) } fn parse( - mut pattern: &str, - mut for_prefix: bool, + pattern: &str, + for_prefix: bool, force_dynamic: bool, ) -> (PatternType, Vec) { - if !force_dynamic && pattern.find('{').is_none() && !pattern.ends_with('*') { + let mut unprocessed = pattern; + + if !force_dynamic && unprocessed.find('{').is_none() && !unprocessed.ends_with('*') { + // pattern is static + let tp = if for_prefix { - PatternType::Prefix(String::from(pattern)) + PatternType::Prefix(unprocessed.to_owned()) } else { - PatternType::Static(String::from(pattern)) + PatternType::Static(unprocessed.to_owned()) }; - return (tp, vec![PatternElement::Const(String::from(pattern))]); + + return (tp, vec![PatternElement::Const(unprocessed.to_owned())]); } - let pattern_orig = pattern; let mut elements = Vec::new(); let mut re = format!("{}^", REGEX_FLAGS); let mut dyn_elements = 0; + let mut has_tail_segment = false; - while let Some(idx) = pattern.find('{') { - let (prefix, rem) = pattern.split_at(idx); + while let Some(idx) = unprocessed.find('{') { + let (prefix, rem) = unprocessed.split_at(idx); - elements.push(PatternElement::Const(String::from(prefix))); + elements.push(PatternElement::Const(prefix.to_owned())); re.push_str(&escape(prefix)); - let (param_pattern, re_part, rem, tail) = Self::parse_param(rem); + let (param_pattern, re_part, rem) = Self::parse_param(rem); - if tail { - for_prefix = true; + if matches!(param_pattern, PatternElement::Tail(_)) { + has_tail_segment = true; } elements.push(param_pattern); re.push_str(&re_part); - pattern = rem; + unprocessed = rem; dyn_elements += 1; } - if let Some(path) = pattern.strip_suffix('*') { - elements.push(PatternElement::Const(String::from(path))); + if let Some(path) = unprocessed.strip_suffix('*') { + // unnamed tail segment + + elements.push(PatternElement::Const(path.to_owned())); + elements.push(PatternElement::Tail(None)); + re.push_str(&escape(path)); re.push_str("(.*)"); - pattern = ""; - } - elements.push(PatternElement::Const(String::from(pattern))); - re.push_str(&escape(pattern)); + dyn_elements += 1; + } else if !has_tail_segment && !unprocessed.is_empty() { + // prevent `Const("")` element from being added after last dynamic segment + + elements.push(PatternElement::Const(unprocessed.to_owned())); + re.push_str(&escape(unprocessed)); + } if dyn_elements > MAX_DYNAMIC_SEGMENTS { panic!( @@ -487,15 +572,19 @@ impl ResourceDef { ); } - if !for_prefix { + if !for_prefix && !has_tail_segment { re.push('$'); } let re = match Regex::new(&re) { Ok(re) => re, - Err(err) => panic!("Wrong path pattern: \"{}\" {}", pattern_orig, err), + Err(err) => panic!("Wrong path pattern: \"{}\" {}", pattern, err), }; - // actix creates one router per thread + + // `Bok::leak(Box::new(name))` is an intentional memory leak. In typical applications the + // routing table is only constructed once (per worker) so leak is bounded. If you are + // constructing `ResourceDef`s more than once in your application's lifecycle you would + // expect a linear increase in leaked memory over time. let names = re .capture_names() .filter_map(|name| name.map(|name| Box::leak(Box::new(name.to_owned())).as_str())) @@ -533,7 +622,8 @@ impl From for ResourceDef { pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { if !path.is_empty() && !path.starts_with('/') { - let mut new_path = "/".to_owned(); + let mut new_path = String::with_capacity(path.len() + 1); + new_path.push('/'); new_path.push_str(path); Cow::Owned(new_path) } else { @@ -546,7 +636,7 @@ mod tests { use super::*; #[test] - fn test_parse_static() { + fn parse_static() { let re = ResourceDef::new("/"); assert!(re.is_match("/")); assert!(!re.is_match("/a")); @@ -581,7 +671,7 @@ mod tests { } #[test] - fn test_parse_param() { + fn parse_param() { let re = ResourceDef::new("/user/{id}"); assert!(re.is_match("/user/profile")); assert!(re.is_match("/user/2345")); @@ -623,7 +713,7 @@ mod tests { #[allow(clippy::cognitive_complexity)] #[test] - fn test_dynamic_set() { + fn dynamic_set() { let re = ResourceDef::new(vec![ "/user/{id}", "/v{version}/resource/{id}", @@ -689,7 +779,7 @@ mod tests { } #[test] - fn test_parse_tail() { + fn parse_tail() { let re = ResourceDef::new("/user/-{id}*"); let mut path = Path::new("/user/-profile"); @@ -710,19 +800,24 @@ mod tests { } #[test] - fn test_static_tail() { + fn static_tail() { let re = ResourceDef::new("/user*"); assert!(re.is_match("/user/profile")); assert!(re.is_match("/user/2345")); assert!(re.is_match("/user/2345/")); assert!(re.is_match("/user/2345/sdg")); + assert!(!re.is_match("/foo/profile")); let re = ResourceDef::new("/user/*"); assert!(re.is_match("/user/profile")); assert!(re.is_match("/user/2345")); assert!(re.is_match("/user/2345/")); assert!(re.is_match("/user/2345/sdg")); + assert!(!re.is_match("/foo/profile")); + } + #[test] + fn dynamic_tail() { let re = ResourceDef::new("/user/{id}/*"); assert!(!re.is_match("/user/2345")); let mut path = Path::new("/user/2345/sdg"); @@ -731,7 +826,7 @@ mod tests { } #[test] - fn test_newline() { + fn newline() { let re = ResourceDef::new("/user/a\nb"); assert!(re.is_match("/user/a\nb")); assert!(!re.is_match("/user/a\nb/profile")); @@ -758,7 +853,7 @@ mod tests { #[cfg(feature = "http")] #[test] - fn test_parse_urlencoded_param() { + fn parse_urlencoded_param() { use std::convert::TryFrom; let re = ResourceDef::new("/user/{id}/test"); @@ -778,8 +873,9 @@ mod tests { } #[test] - fn test_resource_prefix() { + fn prefix_static() { let re = ResourceDef::prefix("/name"); + assert!(re.is_match("/name")); assert!(re.is_match("/name/")); assert!(re.is_match("/name/test/test")); @@ -816,8 +912,9 @@ mod tests { } #[test] - fn test_resource_prefix_dynamic() { + fn prefix_dynamic() { let re = ResourceDef::prefix("/{name}/"); + assert!(re.is_match("/name/")); assert!(re.is_match("/name/gs")); assert!(!re.is_match("/name")); @@ -840,48 +937,115 @@ mod tests { } #[test] - fn test_resource_path() { + fn build_path_list() { let mut s = String::new(); let resource = ResourceDef::new("/user/{item1}/test"); - assert!(resource.resource_path(&mut s, &mut (&["user1"]).iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["user1"]).iter())); assert_eq!(s, "/user/user1/test"); let mut s = String::new(); let resource = ResourceDef::new("/user/{item1}/{item2}/test"); - assert!(resource.resource_path(&mut s, &mut (&["item", "item2"]).iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["item", "item2"]).iter())); assert_eq!(s, "/user/item/item2/test"); let mut s = String::new(); let resource = ResourceDef::new("/user/{item1}/{item2}"); - assert!(resource.resource_path(&mut s, &mut (&["item", "item2"]).iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["item", "item2"]).iter())); assert_eq!(s, "/user/item/item2"); let mut s = String::new(); let resource = ResourceDef::new("/user/{item1}/{item2}/"); - assert!(resource.resource_path(&mut s, &mut (&["item", "item2"]).iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["item", "item2"]).iter())); assert_eq!(s, "/user/item/item2/"); let mut s = String::new(); - assert!(!resource.resource_path(&mut s, &mut (&["item"]).iter())); + assert!(!resource.resource_path_from_iter(&mut s, &mut (&["item"]).iter())); let mut s = String::new(); - assert!(resource.resource_path(&mut s, &mut (&["item", "item2"]).iter())); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["item", "item2"]).iter())); assert_eq!(s, "/user/item/item2/"); - assert!(!resource.resource_path(&mut s, &mut (&["item"]).iter())); + assert!(!resource.resource_path_from_iter(&mut s, &mut (&["item"]).iter())); let mut s = String::new(); - assert!(resource.resource_path(&mut s, &mut vec!["item", "item2"].into_iter())); + assert!( + resource.resource_path_from_iter(&mut s, &mut vec!["item", "item2"].into_iter()) + ); assert_eq!(s, "/user/item/item2/"); + } + + #[test] + fn build_path_map() { + let resource = ResourceDef::new("/user/{item1}/{item2}/"); let mut map = HashMap::new(); map.insert("item1", "item"); let mut s = String::new(); - assert!(!resource.resource_path_named(&mut s, &map)); + assert!(!resource.resource_path_from_map(&mut s, &map)); + + map.insert("item2", "item2"); let mut s = String::new(); - map.insert("item2", "item2"); - assert!(resource.resource_path_named(&mut s, &map)); + assert!(resource.resource_path_from_map(&mut s, &map)); assert_eq!(s, "/user/item/item2/"); } + + #[test] + fn build_path_tail() { + let resource = ResourceDef::new("/user/{item1}/*"); + + let mut s = String::new(); + assert!(!resource.resource_path_from_iter(&mut s, &mut (&["user1"]).iter())); + + let mut s = String::new(); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["user1", "2345"]).iter())); + assert_eq!(s, "/user/user1/2345"); + + let mut s = String::new(); + let mut map = HashMap::new(); + map.insert("item1", "item"); + assert!(!resource.resource_path_from_map(&mut s, &map)); + + let mut s = String::new(); + assert!(resource.resource_path_from_map_with_tail(&mut s, &map, "2345")); + assert_eq!(s, "/user/item/2345"); + + let resource = ResourceDef::new("/user/{item1}*"); + + let mut s = String::new(); + assert!(!resource.resource_path_from_iter(&mut s, &mut (&[""; 0]).iter())); + + let mut s = String::new(); + assert!(resource.resource_path_from_iter(&mut s, &mut (&["user1"]).iter())); + assert_eq!(s, "/user/user1"); + + let mut s = String::new(); + let mut map = HashMap::new(); + map.insert("item1", "item"); + assert!(resource.resource_path_from_map(&mut s, &map)); + assert_eq!(s, "/user/item"); + } + + #[test] + fn build_path_tail_when_resource_has_no_tail() { + let resource = ResourceDef::new("/user/{item1}"); + + let mut map = HashMap::new(); + map.insert("item1", "item"); + let mut s = String::new(); + assert!(resource.resource_path_from_map_with_tail(&mut s, &map, "2345")); + assert_eq!(s, "/user/item"); + } + + #[test] + #[should_panic] + fn invalid_dynamic_segment_delimiter() { + ResourceDef::new("/user/{username"); + } + + #[test] + #[should_panic] + fn invalid_dynamic_segment_name() { + ResourceDef::new("/user/{}"); + } }