From c2d5b2398a367f83e146b6d98b71a631a082446a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 16 Jul 2021 19:43:48 +0100 Subject: [PATCH 01/15] Rename `Path::{len => segment_count}` (#370) --- actix-router/CHANGES.md | 3 + actix-router/Cargo.toml | 20 ++-- actix-router/src/de.rs | 28 +++--- actix-router/src/path.rs | 32 ++----- actix-router/src/resource.rs | 181 ++++++++++++++++++++++++----------- 5 files changed, 162 insertions(+), 102 deletions(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index bc22ee7e..40dcf7e2 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -4,9 +4,12 @@ * 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] [#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 ## 0.4.0 - 2021-06-06 diff --git a/actix-router/Cargo.toml b/actix-router/Cargo.toml index 5f0c22e2..216d4bff 100644 --- a/actix-router/Cargo.toml +++ b/actix-router/Cargo.toml @@ -1,10 +1,14 @@ [package] name = "actix-router" version = "0.4.0" -authors = ["Nikolay Kim "] +authors = [ + "Nikolay Kim ", + "Ali MJ Al-Nasrawy ", + "Rob Ede ", +] description = "Resource path matching library" keywords = ["actix", "router", "routing"] -repository = "https://github.com/actix/actix-net" +repository = "https://github.com/actix/actix-net.git" license = "MIT OR Apache-2.0" edition = "2018" @@ -16,12 +20,12 @@ path = "src/lib.rs" default = ["http"] [dependencies] -regex = "1.3.1" -serde = "1.0.104" +regex = "1.5" +serde = "1" bytestring = ">=0.1.5, <2" -log = "0.4.8" -http = { version = "0.2.2", optional = true } +log = "0.4" +http = { version = "0.2.3", optional = true } [dev-dependencies] -http = "0.2.2" -serde_derive = "1.0" +http = "0.2.3" +serde = { version = "1", features = ["derive"] } diff --git a/actix-router/src/de.rs b/actix-router/src/de.rs index 81796348..775c48b8 100644 --- a/actix-router/src/de.rs +++ b/actix-router/src/de.rs @@ -24,10 +24,13 @@ macro_rules! parse_single_value { where V: Visitor<'de>, { - if self.path.len() != 1 { + if self.path.segment_count() != 1 { Err(de::value::Error::custom( - format!("wrong number of parameters: {} expected 1", self.path.len()) - .as_str(), + format!( + "wrong number of parameters: {} expected 1", + self.path.segment_count() + ) + .as_str(), )) } else { let v = self.path[0].parse().map_err(|_| { @@ -110,11 +113,11 @@ impl<'de, T: ResourcePath + 'de> Deserializer<'de> for PathDeserializer<'de, T> where V: Visitor<'de>, { - if self.path.len() < len { + if self.path.segment_count() < len { Err(de::value::Error::custom( format!( "wrong number of parameters: {} expected {}", - self.path.len(), + self.path.segment_count(), len ) .as_str(), @@ -135,11 +138,11 @@ impl<'de, T: ResourcePath + 'de> Deserializer<'de> for PathDeserializer<'de, T> where V: Visitor<'de>, { - if self.path.len() < len { + if self.path.segment_count() < len { Err(de::value::Error::custom( format!( "wrong number of parameters: {} expected {}", - self.path.len(), + self.path.segment_count(), len ) .as_str(), @@ -173,9 +176,13 @@ impl<'de, T: ResourcePath + 'de> Deserializer<'de> for PathDeserializer<'de, T> where V: Visitor<'de>, { - if self.path.len() != 1 { + if self.path.segment_count() != 1 { Err(de::value::Error::custom( - format!("wrong number of parameters: {} expected 1", self.path.len()).as_str(), + format!( + "wrong number of parameters: {} expected 1", + self.path.segment_count() + ) + .as_str(), )) } else { visitor.visit_str(&self.path[0]) @@ -485,8 +492,7 @@ impl<'de> de::VariantAccess<'de> for UnitVariant { #[cfg(test)] mod tests { - use serde::de; - use serde_derive::Deserialize; + use serde::{de, Deserialize}; use super::*; use crate::path::Path; diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index 6e4e2fdf..f6ae6f2d 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -18,36 +18,16 @@ impl Default for PathItem { } } -/// Resource path match information +/// Resource path match information. /// /// If resource path contains variable patterns, `Path` stores them. -#[derive(Debug)] +#[derive(Debug, Clone, Default)] pub struct Path { path: T, pub(crate) skip: u16, pub(crate) segments: Vec<(Cow<'static, str>, PathItem)>, } -impl Default for Path { - fn default() -> Self { - Path { - path: T::default(), - skip: 0, - segments: Vec::new(), - } - } -} - -impl Clone for Path { - fn clone(&self) -> Self { - Path { - path: self.path.clone(), - skip: self.skip, - segments: self.segments.clone(), - } - } -} - impl Path { pub fn new(path: T) -> Path { Path { @@ -122,15 +102,15 @@ impl Path { .push((name.into(), PathItem::Static(value.into()))); } - /// Check if there are any matched patterns + /// Check if there are any matched patterns. #[inline] pub fn is_empty(&self) -> bool { self.segments.is_empty() } - /// Check number of extracted parameters + /// Returns number of interpolated segments. #[inline] - pub fn len(&self) -> usize { + pub fn segment_count(&self) -> usize { self.segments.len() } @@ -195,7 +175,7 @@ impl<'a, T: ResourcePath> Iterator for PathIter<'a, T> { #[inline] fn next(&mut self) -> Option<(&'a str, &'a str)> { - if self.idx < self.params.len() { + if self.idx < self.params.segment_count() { let idx = self.idx; let res = match self.params.segments[idx].1 { PathItem::Static(ref s) => &s, diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 65cb5cf1..e8556809 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1,7 +1,10 @@ -use std::cmp::min; -use std::collections::HashMap; -use std::hash::{Hash, Hasher}; -use std::mem; +use std::{ + borrow::Cow, + cmp::min, + collections::HashMap, + hash::{Hash, Hasher}, + mem, +}; use regex::{escape, Regex, RegexSet}; @@ -15,30 +18,51 @@ const MAX_DYNAMIC_SEGMENTS: usize = 16; /// See the docs under: https://docs.rs/regex/1.5.4/regex/#grouping-and-flags const REGEX_FLAGS: &str = "(?s-m)"; -/// ResourceDef describes an entry in resources table +/// Describes an entry in a resource table. /// -/// Resource definition can contain only 16 dynamic segments +/// Resource definition can contain at most 16 dynamic segments. #[derive(Clone, Debug)] pub struct ResourceDef { id: u16, - tp: PatternType, + + /// Pattern type. + pat_type: PatternType, + + /// Optional name of resource definition. Defaults to "". name: String, + + /// Pattern that generated the resource definition. + // TODO: Sort of, in dynamic set pattern type it is blank, consider change to option. pattern: String, + + /// List of elements that compose the pattern, in order. + /// + /// `None` with pattern type is DynamicSet. elements: Option>, } #[derive(Debug, Clone, PartialEq)] enum PatternElement { + /// Literal slice of pattern. Const(String), + + /// Name of dynamic segment. Var(String), } #[derive(Clone, Debug)] #[allow(clippy::large_enum_variant)] 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>), + + /// Regular expression set and list of component expressions plus dynamic segment names. DynamicSet(RegexSet, Vec<(Regex, Vec<&'static str>)>), } @@ -65,7 +89,7 @@ impl ResourceDef { ResourceDef { id: 0, - tp: PatternType::DynamicSet(RegexSet::new(re_set).unwrap(), data), + pat_type: PatternType::DynamicSet(RegexSet::new(re_set).unwrap(), data), elements: None, name: String::new(), pattern: "".to_owned(), @@ -82,9 +106,8 @@ impl ResourceDef { ResourceDef::from_single_pattern(path, true) } - /// Parse path pattern and create new `Pattern` instance. - /// Inserts `/` to begging of the pattern. - /// + /// Parse path pattern and create new `Pattern` instance, inserting a `/` to beginning of + /// the pattern if absent. /// /// Use `prefix` type instead of `static`. /// @@ -93,12 +116,12 @@ impl ResourceDef { ResourceDef::from_single_pattern(&insert_slash(path), true) } - /// Resource id + /// Resource ID. pub fn id(&self) -> u16 { self.id } - /// Set resource id + /// Set resource ID. pub fn set_id(&mut self, id: u16) { self.id = id; } @@ -106,10 +129,10 @@ impl ResourceDef { /// Parse path pattern and create a new instance fn from_single_pattern(pattern: &str, for_prefix: bool) -> Self { let pattern = pattern.to_owned(); - let (tp, elements) = ResourceDef::parse(&pattern, for_prefix, false); + let (pat_type, elements) = ResourceDef::parse(&pattern, for_prefix, false); ResourceDef { - tp, + pat_type, pattern, elements: Some(elements), id: 0, @@ -117,7 +140,7 @@ impl ResourceDef { } } - /// Resource pattern name + /// Resource pattern name. pub fn name(&self) -> &str { &self.name } @@ -135,7 +158,7 @@ impl ResourceDef { /// Check if path matches this pattern. #[inline] pub fn is_match(&self, path: &str) -> bool { - match self.tp { + match self.pat_type { PatternType::Static(ref s) => s == path, PatternType::Prefix(ref s) => path.starts_with(s), PatternType::Dynamic(ref re, _) => re.is_match(path), @@ -145,34 +168,52 @@ impl ResourceDef { /// Is prefix path a match against this resource. pub fn is_prefix_match(&self, path: &str) -> Option { - let p_len = path.len(); + let path_len = path.len(); let path = if path.is_empty() { "/" } else { path }; - match self.tp { - PatternType::Static(ref s) => { - if s == path { - Some(p_len) + match self.pat_type { + PatternType::Static(ref segment) => { + if segment == path { + Some(path_len) } else { None } } - PatternType::Dynamic(ref re, _) => re.find(path).map(|m| m.end()), - PatternType::Prefix(ref s) => { - let len = if path == s { - s.len() - } else if path.starts_with(s) - && (s.ends_with('/') || path.split_at(s.len()).1.starts_with('/')) - { - if s.ends_with('/') { - s.len() - 1 - } else { - s.len() - } + + PatternType::Prefix(ref prefix) => { + let prefix_len = if path == prefix { + // path length === prefix segment length + path_len } else { - return None; + let is_slash_next = + prefix.ends_with('/') || path.split_at(prefix.len()).1.starts_with('/'); + + if path.starts_with(prefix) && is_slash_next { + // enters this branch if segment delimiter ("/") is present after prefix + // + // i.e., path starts with prefix segment + // and prefix segment ends with / + // or first character in path after prefix segment length is / + // + // eg: Prefix("/test/") or Prefix("/test") would match: + // - /test/foo + // - /test/foo + + if prefix.ends_with('/') { + prefix.len() - 1 + } else { + prefix.len() + } + } else { + return None; + } }; - Some(min(p_len, len)) + + Some(min(path_len, prefix_len)) } + + PatternType::Dynamic(ref re, _) => re.find(path).map(|m| m.end()), + PatternType::DynamicSet(ref re, ref params) => { let idx = re.matches(path).into_iter().next()?; let (ref pattern, _) = params[idx]; @@ -201,37 +242,48 @@ impl ResourceDef { let mut segments: [PathItem; MAX_DYNAMIC_SEGMENTS] = Default::default(); let path = res.resource_path(); - let (matched_len, matched_vars) = match self.tp { - PatternType::Static(ref s) => { - if s != path.path() { + let (matched_len, matched_vars) = match self.pat_type { + PatternType::Static(ref segment) => { + if segment != path.path() { return false; } + (path.path().len(), None) } - PatternType::Prefix(ref s) => { + + PatternType::Prefix(ref prefix) => { + let path_str = path.path(); + let path_len = path_str.len(); + let len = { - let r_path = path.path(); - if s == r_path { - s.len() - } else if r_path.starts_with(s) - && (s.ends_with('/') || r_path.split_at(s.len()).1.starts_with('/')) - { - if s.ends_with('/') { - s.len() - 1 - } else { - s.len() - } + if prefix == path_str { + // prefix length === path length + path_len } else { - return false; + let is_slash_next = prefix.ends_with('/') + || path_str.split_at(prefix.len()).1.starts_with('/'); + + if path_str.starts_with(prefix) && is_slash_next { + if prefix.ends_with('/') { + prefix.len() - 1 + } else { + prefix.len() + } + } else { + return false; + } } }; + (min(path.path().len(), len), None) } + PatternType::Dynamic(ref re, ref names) => { let captures = match re.captures(path.path()) { Some(captures) => captures, _ => return false, }; + for (no, name) in names.iter().enumerate() { if let Some(m) = captures.name(&name) { segments[no] = PathItem::Segment(m.start() as u16, m.end() as u16); @@ -240,18 +292,22 @@ impl ResourceDef { return false; } } + (captures[0].len(), Some(names)) } + PatternType::DynamicSet(ref re, ref params) => { let path = path.path(); let (pattern, names) = match re.matches(path).into_iter().next() { Some(idx) => ¶ms[idx], _ => return false, }; + let captures = match pattern.captures(path.path()) { Some(captures) => captures, _ => return false, }; + for (no, name) in names.iter().enumerate() { if let Some(m) = captures.name(&name) { segments[no] = PathItem::Segment(m.start() as u16, m.end() as u16); @@ -260,6 +316,7 @@ impl ResourceDef { return false; } } + (captures[0].len(), Some(names)) } }; @@ -298,6 +355,7 @@ impl ResourceDef { }, } } + true } @@ -327,6 +385,7 @@ impl ResourceDef { fn parse_param(pattern: &str) -> (PatternElement, String, &str, bool) { const DEFAULT_PATTERN: &str = "[^/]+"; const DEFAULT_PATTERN_TAIL: &str = ".*"; + let mut params_nesting = 0usize; let close_idx = pattern .find(|c| match c { @@ -341,6 +400,7 @@ impl ResourceDef { _ => false, }) .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 == "*"; @@ -363,6 +423,7 @@ impl ResourceDef { }, ), }; + ( PatternElement::Var(name.to_string()), format!(r"(?P<{}>{})", &name, &pattern), @@ -392,15 +453,19 @@ impl ResourceDef { while let Some(idx) = pattern.find('{') { let (prefix, rem) = pattern.split_at(idx); + elements.push(PatternElement::Const(String::from(prefix))); re.push_str(&escape(prefix)); + let (param_pattern, re_part, rem, tail) = Self::parse_param(rem); + if tail { for_prefix = true; } elements.push(param_pattern); re.push_str(&re_part); + pattern = rem; dyn_elements += 1; } @@ -466,12 +531,14 @@ impl From for ResourceDef { } } -pub(crate) fn insert_slash(path: &str) -> String { - let mut path = path.to_owned(); +pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { if !path.is_empty() && !path.starts_with('/') { - path.insert(0, '/'); - }; - path + let mut new_path = "/".to_owned(); + new_path.push_str(path); + Cow::Owned(new_path) + } else { + Cow::Borrowed(path) + } } #[cfg(test)] From ad22a93466ffff2252f63d2ef0ebfc2ad501c11a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 16 Jul 2021 21:41:57 +0100 Subject: [PATCH 02/15] 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/{}"); + } } From a0fe2a9b2e48a30aeca50f5756acbf0219548603 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 16 Jul 2021 21:46:32 +0100 Subject: [PATCH 03/15] clippy --- actix-router/src/url.rs | 2 +- actix-rt/tests/tests.rs | 2 +- actix-server/src/worker.rs | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index 130ac76f..f84b1778 100644 --- a/actix-router/src/url.rs +++ b/actix-router/src/url.rs @@ -211,7 +211,7 @@ mod tests { } fn percent_encode(data: &[u8]) -> String { - data.into_iter().map(|c| format!("%{:02X}", c)).collect() + data.iter().map(|c| format!("%{:02X}", c)).collect() } #[test] diff --git a/actix-rt/tests/tests.rs b/actix-rt/tests/tests.rs index 86fba96d..839b1fbc 100644 --- a/actix-rt/tests/tests.rs +++ b/actix-rt/tests/tests.rs @@ -286,7 +286,7 @@ fn new_arbiter_with_tokio() { arb.join().unwrap(); - assert_eq!(false, counter.load(Ordering::SeqCst)); + assert!(!counter.load(Ordering::SeqCst)); } #[test] diff --git a/actix-server/src/worker.rs b/actix-server/src/worker.rs index 79f15b16..a974522a 100644 --- a/actix-server/src/worker.rs +++ b/actix-server/src/worker.rs @@ -73,7 +73,7 @@ fn handle_pair( /// On reaching counter limit worker would use `mio::Waker` and `WakerQueue` to wake up `Accept` /// and notify it to update cached `Availability` again to mark worker as able to accept work again. /// -/// Hense a wake up would only happen after `Accept` increment it to limit. +/// Hence, a wake up would only happen after `Accept` increment it to limit. /// And a decrement to limit always wake up `Accept`. #[derive(Clone)] pub(crate) struct Counter { @@ -179,8 +179,9 @@ impl WorkerHandleAccept { /// Handle to worker than can send stop message to worker. /// /// Held by [ServerBuilder](crate::builder::ServerBuilder). +#[derive(Debug)] pub(crate) struct WorkerHandleServer { - pub idx: usize, + idx: usize, tx: UnboundedSender, } From 5687e81d9fc220f17c5420adb6f94c5ef7ad50ff Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 17 Jul 2021 01:06:23 +0100 Subject: [PATCH 04/15] rework IntoPatterns trait and codegen (#372) --- Cargo.toml | 5 + actix-router/CHANGES.md | 2 + actix-router/Cargo.toml | 11 +- actix-router/benches/router.rs | 194 +++++++++++++++++++++++++++++++++ actix-router/src/lib.rs | 122 ++++++++------------- actix-router/src/resource.rs | 66 ++++++----- actix-router/src/router.rs | 4 +- 7 files changed, 300 insertions(+), 104 deletions(-) create mode 100644 actix-router/benches/router.rs diff --git a/Cargo.toml b/Cargo.toml index 5bf72300..6ed9b50a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,3 +27,8 @@ actix-utils = { path = "actix-utils" } bytestring = { path = "bytestring" } local-channel = { path = "local-channel" } local-waker = { path = "local-waker" } + +[profile.release] +lto = true +opt-level = 3 +codegen-units = 1 diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 77251c21..9ec87716 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -6,6 +6,7 @@ * 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] +* Re-work `IntoPatterns` trait. [#372] * 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] @@ -15,6 +16,7 @@ [#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 +[#372]: https://github.com/actix/actix-net/pull/372 ## 0.4.0 - 2021-06-06 diff --git a/actix-router/Cargo.toml b/actix-router/Cargo.toml index 216d4bff..02bd99ad 100644 --- a/actix-router/Cargo.toml +++ b/actix-router/Cargo.toml @@ -20,12 +20,17 @@ path = "src/lib.rs" default = ["http"] [dependencies] +bytestring = ">=0.1.5, <2" +http = { version = "0.2.3", optional = true } +log = "0.4" regex = "1.5" serde = "1" -bytestring = ">=0.1.5, <2" -log = "0.4" -http = { version = "0.2.3", optional = true } [dev-dependencies] http = "0.2.3" serde = { version = "1", features = ["derive"] } +criterion = { version = "0.3", features = ["html_reports"] } + +[[bench]] +name = "router" +harness = false diff --git a/actix-router/benches/router.rs b/actix-router/benches/router.rs new file mode 100644 index 00000000..a428b9f1 --- /dev/null +++ b/actix-router/benches/router.rs @@ -0,0 +1,194 @@ +//! Based on https://github.com/ibraheemdev/matchit/blob/master/benches/bench.rs + +use criterion::{black_box, criterion_group, criterion_main, Criterion}; + +macro_rules! register { + (colon) => {{ + register!(finish => ":p1", ":p2", ":p3", ":p4") + }}; + (brackets) => {{ + register!(finish => "{p1}", "{p2}", "{p3}", "{p4}") + }}; + (regex) => {{ + register!(finish => "(.*)", "(.*)", "(.*)", "(.*)") + }}; + (finish => $p1:literal, $p2:literal, $p3:literal, $p4:literal) => {{ + let arr = [ + concat!("/authorizations"), + concat!("/authorizations/", $p1), + concat!("/applications/", $p1, "/tokens/", $p2), + concat!("/events"), + concat!("/repos/", $p1, "/", $p2, "/events"), + concat!("/networks/", $p1, "/", $p2, "/events"), + concat!("/orgs/", $p1, "/events"), + concat!("/users/", $p1, "/received_events"), + concat!("/users/", $p1, "/received_events/public"), + concat!("/users/", $p1, "/events"), + concat!("/users/", $p1, "/events/public"), + concat!("/users/", $p1, "/events/orgs/", $p2), + concat!("/feeds"), + concat!("/notifications"), + concat!("/repos/", $p1, "/", $p2, "/notifications"), + concat!("/notifications/threads/", $p1), + concat!("/notifications/threads/", $p1, "/subscription"), + concat!("/repos/", $p1, "/", $p2, "/stargazers"), + concat!("/users/", $p1, "/starred"), + concat!("/user/starred"), + concat!("/user/starred/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/subscribers"), + concat!("/users/", $p1, "/subscriptions"), + concat!("/user/subscriptions"), + concat!("/repos/", $p1, "/", $p2, "/subscription"), + concat!("/user/subscriptions/", $p1, "/", $p2), + concat!("/users/", $p1, "/gists"), + concat!("/gists"), + concat!("/gists/", $p1), + concat!("/gists/", $p1, "/star"), + concat!("/repos/", $p1, "/", $p2, "/git/blobs/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/refs"), + concat!("/repos/", $p1, "/", $p2, "/git/tags/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/trees/", $p3), + concat!("/issues"), + concat!("/user/issues"), + concat!("/orgs/", $p1, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3), + concat!("/repos/", $p1, "/", $p2, "/assignees"), + concat!("/repos/", $p1, "/", $p2, "/assignees/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/events"), + concat!("/repos/", $p1, "/", $p2, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/labels/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3), + concat!("/emojis"), + concat!("/gitignore/templates"), + concat!("/gitignore/templates/", $p1), + concat!("/meta"), + concat!("/rate_limit"), + concat!("/users/", $p1, "/orgs"), + concat!("/user/orgs"), + concat!("/orgs/", $p1), + concat!("/orgs/", $p1, "/members"), + concat!("/orgs/", $p1, "/members", $p2), + concat!("/orgs/", $p1, "/public_members"), + concat!("/orgs/", $p1, "/public_members/", $p2), + concat!("/orgs/", $p1, "/teams"), + concat!("/teams/", $p1), + concat!("/teams/", $p1, "/members"), + concat!("/teams/", $p1, "/members", $p2), + concat!("/teams/", $p1, "/repos"), + concat!("/teams/", $p1, "/repos/", $p2, "/", $p3), + concat!("/user/teams"), + concat!("/repos/", $p1, "/", $p2, "/pulls"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/files"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/merge"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/comments"), + concat!("/user/repos"), + concat!("/users/", $p1, "/repos"), + concat!("/orgs/", $p1, "/repos"), + concat!("/repositories"), + concat!("/repos/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/contributors"), + concat!("/repos/", $p1, "/", $p2, "/languages"), + concat!("/repos/", $p1, "/", $p2, "/teams"), + concat!("/repos/", $p1, "/", $p2, "/tags"), + concat!("/repos/", $p1, "/", $p2, "/branches"), + concat!("/repos/", $p1, "/", $p2, "/branches/", $p3), + concat!("/repos/", $p1, "/", $p2, "/collaborators"), + concat!("/repos/", $p1, "/", $p2, "/collaborators/", $p3), + concat!("/repos/", $p1, "/", $p2, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/readme"), + concat!("/repos/", $p1, "/", $p2, "/keys"), + concat!("/repos/", $p1, "/", $p2, "/keys", $p3), + concat!("/repos/", $p1, "/", $p2, "/downloads"), + concat!("/repos/", $p1, "/", $p2, "/downloads", $p3), + concat!("/repos/", $p1, "/", $p2, "/forks"), + concat!("/repos/", $p1, "/", $p2, "/hooks"), + concat!("/repos/", $p1, "/", $p2, "/hooks", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases"), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3, "/assets"), + concat!("/repos/", $p1, "/", $p2, "/stats/contributors"), + concat!("/repos/", $p1, "/", $p2, "/stats/commit_activity"), + concat!("/repos/", $p1, "/", $p2, "/stats/code_frequency"), + concat!("/repos/", $p1, "/", $p2, "/stats/participation"), + concat!("/repos/", $p1, "/", $p2, "/stats/punch_card"), + concat!("/repos/", $p1, "/", $p2, "/statuses/", $p3), + concat!("/search/repositories"), + concat!("/search/code"), + concat!("/search/issues"), + concat!("/search/users"), + concat!("/legacy/issues/search/", $p1, "/", $p2, "/", $p3, "/", $p4), + concat!("/legacy/repos/search/", $p1), + concat!("/legacy/user/search/", $p1), + concat!("/legacy/user/email/", $p1), + concat!("/users/", $p1), + concat!("/user"), + concat!("/users"), + concat!("/user/emails"), + concat!("/users/", $p1, "/followers"), + concat!("/user/followers"), + concat!("/users/", $p1, "/following"), + concat!("/user/following"), + concat!("/user/following/", $p1), + concat!("/users/", $p1, "/following", $p2), + concat!("/users/", $p1, "/keys"), + concat!("/user/keys"), + concat!("/user/keys/", $p1), + ]; + std::array::IntoIter::new(arr) + }}; +} + +fn call() -> impl Iterator { + let arr = [ + "/authorizations", + "/user/repos", + "/repos/rust-lang/rust/stargazers", + "/orgs/rust-lang/public_members/nikomatsakis", + "/repos/rust-lang/rust/releases/1.51.0", + ]; + + std::array::IntoIter::new(arr) +} + +fn compare_routers(c: &mut Criterion) { + let mut group = c.benchmark_group("Compare Routers"); + + let mut actix = actix_router::Router::::build(); + for route in register!(brackets) { + actix.path(route, true); + } + let actix = actix.finish(); + group.bench_function("actix", |b| { + b.iter(|| { + for route in call() { + let mut path = actix_router::Path::new(route); + black_box(actix.recognize(&mut path).unwrap()); + } + }); + }); + + let regex_set = regex::RegexSet::new(register!(regex)).unwrap(); + group.bench_function("regex", |b| { + b.iter(|| { + for route in call() { + black_box(regex_set.matches(route)); + } + }); + }); + + group.finish(); +} + +criterion_group!(benches, compare_routers); +criterion_main!(benches); diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 5850b103..53ae9db6 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -40,98 +40,71 @@ impl ResourcePath for bytestring::ByteString { } } -/// Helper trait for type that could be converted to path pattern -pub trait IntoPattern { - fn is_single(&self) -> bool; - - fn patterns(&self) -> Vec; +/// One or many patterns. +#[derive(Debug, Clone)] +pub enum Patterns { + Single(String), + List(Vec), } -impl IntoPattern for String { - fn is_single(&self) -> bool { - true - } +/// Helper trait for type that could be converted to one or more path pattern. +pub trait IntoPatterns { + fn patterns(&self) -> Patterns; +} - fn patterns(&self) -> Vec { - vec![self.clone()] +impl IntoPatterns for String { + fn patterns(&self) -> Patterns { + Patterns::Single(self.clone()) } } -impl<'a> IntoPattern for &'a String { - fn is_single(&self) -> bool { - true - } - - fn patterns(&self) -> Vec { - vec![self.as_str().to_string()] +impl<'a> IntoPatterns for &'a String { + fn patterns(&self) -> Patterns { + Patterns::Single((*self).clone()) } } -impl<'a> IntoPattern for &'a str { - fn is_single(&self) -> bool { - true - } - - fn patterns(&self) -> Vec { - vec![(*self).to_string()] +impl<'a> IntoPatterns for &'a str { + fn patterns(&self) -> Patterns { + Patterns::Single((*self).to_owned()) } } -impl> IntoPattern for Vec { - fn is_single(&self) -> bool { - self.len() == 1 - } +impl> IntoPatterns for Vec { + fn patterns(&self) -> Patterns { + let mut patterns = self.iter().map(|v| v.as_ref().to_owned()); - fn patterns(&self) -> Vec { - self.iter().map(|v| v.as_ref().to_string()).collect() - } -} - -macro_rules! array_patterns (($tp:ty, $num:tt) => { - impl IntoPattern for [$tp; $num] { - fn is_single(&self) -> bool { - $num == 1 + match patterns.size_hint() { + (1, _) => Patterns::Single(patterns.next().unwrap()), + _ => Patterns::List(patterns.collect()), } + } +} - fn patterns(&self) -> Vec { - self.iter().map(|v| v.to_string()).collect() +macro_rules! array_patterns_single (($tp:ty) => { + impl IntoPatterns for [$tp; 1] { + fn patterns(&self) -> Patterns { + Patterns::Single(self[0].to_owned()) } } }); -array_patterns!(&str, 1); -array_patterns!(&str, 2); -array_patterns!(&str, 3); -array_patterns!(&str, 4); -array_patterns!(&str, 5); -array_patterns!(&str, 6); -array_patterns!(&str, 7); -array_patterns!(&str, 8); -array_patterns!(&str, 9); -array_patterns!(&str, 10); -array_patterns!(&str, 11); -array_patterns!(&str, 12); -array_patterns!(&str, 13); -array_patterns!(&str, 14); -array_patterns!(&str, 15); -array_patterns!(&str, 16); +macro_rules! array_patterns_multiple (($tp:ty, $str_fn:expr, $($num:tt) +) => { + // for each array length specified in $num + $( + impl IntoPatterns for [$tp; $num] { + fn patterns(&self) -> Patterns { + Patterns::List(self.iter().map($str_fn).collect()) + } + } + )+ +}); -array_patterns!(String, 1); -array_patterns!(String, 2); -array_patterns!(String, 3); -array_patterns!(String, 4); -array_patterns!(String, 5); -array_patterns!(String, 6); -array_patterns!(String, 7); -array_patterns!(String, 8); -array_patterns!(String, 9); -array_patterns!(String, 10); -array_patterns!(String, 11); -array_patterns!(String, 12); -array_patterns!(String, 13); -array_patterns!(String, 14); -array_patterns!(String, 15); -array_patterns!(String, 16); +array_patterns_single!(&str); +array_patterns_multiple!(&str, |&v| v.to_owned(), 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16); + +array_patterns_single!(String); +array_patterns_multiple!(String, |v| v.clone(), 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16); #[cfg(feature = "http")] mod url; @@ -140,10 +113,11 @@ mod url; pub use self::url::{Quoter, Url}; #[cfg(feature = "http")] -mod http_support { - use super::ResourcePath; +mod http_impls { use http::Uri; + use super::ResourcePath; + impl ResourcePath for Uri { fn path(&self) -> &str { self.path() diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index ad3d47e2..cb0264ae 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -8,8 +8,10 @@ use std::{ use regex::{escape, Regex, RegexSet}; -use crate::path::{Path, PathItem}; -use crate::{IntoPattern, Resource, ResourcePath}; +use crate::{ + path::{Path, PathItem}, + IntoPatterns, Patterns, Resource, ResourcePath, +}; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -25,9 +27,6 @@ const REGEX_FLAGS: &str = "(?s-m)"; pub struct ResourceDef { id: u16, - /// Pattern type. - pat_type: PatternType, - /// Optional name of resource definition. Defaults to "". name: String, @@ -35,6 +34,9 @@ pub struct ResourceDef { // TODO: Sort of, in dynamic set pattern type it is blank, consider change to option. pattern: String, + /// Pattern type. + pat_type: PatternType, + /// List of elements that compose the pattern, in order. /// /// `None` with pattern type is DynamicSet. @@ -75,29 +77,43 @@ impl ResourceDef { /// Parse path pattern and create new `Pattern` instance. /// /// Panics if path pattern is malformed. - pub fn new(path: T) -> Self { - if path.is_single() { - ResourceDef::from_single_pattern(&path.patterns()[0], false) - } else { - let mut data = Vec::new(); - let mut re_set = Vec::new(); + pub fn new(path: T) -> Self { + match path.patterns() { + Patterns::Single(pattern) => ResourceDef::from_single_pattern(&pattern, false), - for pattern in path.patterns() { - match ResourceDef::parse(&pattern, false, true) { - (PatternType::Dynamic(re, names), _) => { - re_set.push(re.as_str().to_owned()); - data.push((re, names)); - } - _ => unreachable!(), - } - } - - ResourceDef { + // since zero length pattern sets are possible + // just return a useless `ResourceDef` + Patterns::List(patterns) if patterns.is_empty() => ResourceDef { id: 0, - pat_type: PatternType::DynamicSet(RegexSet::new(re_set).unwrap(), data), - elements: None, name: String::new(), - pattern: "".to_owned(), + pattern: String::new(), + pat_type: PatternType::DynamicSet(RegexSet::empty(), Vec::new()), + elements: 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: String::new(), + pattern: String::new(), + pat_type: PatternType::DynamicSet(pattern_re_set, pattern_data), + elements: None, + } } } } diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index aeb1aa36..007ef495 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -1,4 +1,4 @@ -use crate::{IntoPattern, Resource, ResourceDef, ResourcePath}; +use crate::{IntoPatterns, Resource, ResourceDef, ResourcePath}; #[derive(Debug, Copy, Clone, PartialEq)] pub struct ResourceId(pub u16); @@ -88,7 +88,7 @@ pub struct RouterBuilder { impl RouterBuilder { /// Register resource for specified path. - pub fn path( + pub fn path( &mut self, path: P, resource: T, From 95cba659fff80ea48dfa067aed2d24cb15e8190d Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 17 Jul 2021 01:07:07 +0100 Subject: [PATCH 05/15] add zero cost profiling to router --- actix-router/Cargo.toml | 4 +- actix-router/examples/flamegraph.rs | 172 ++++++++++++++++++++++++++++ actix-router/src/path.rs | 27 +++-- actix-router/src/resource.rs | 63 ++++++++-- actix-router/src/router.rs | 16 +++ 5 files changed, 262 insertions(+), 20 deletions(-) create mode 100644 actix-router/examples/flamegraph.rs diff --git a/actix-router/Cargo.toml b/actix-router/Cargo.toml index 02bd99ad..b0bcd9da 100644 --- a/actix-router/Cargo.toml +++ b/actix-router/Cargo.toml @@ -21,15 +21,17 @@ default = ["http"] [dependencies] bytestring = ">=0.1.5, <2" +firestorm = "0.4" http = { version = "0.2.3", optional = true } log = "0.4" regex = "1.5" serde = "1" [dev-dependencies] +criterion = { version = "0.3", features = ["html_reports"] } +firestorm = { version = "0.4", features = ["enable_system_time"] } http = "0.2.3" serde = { version = "1", features = ["derive"] } -criterion = { version = "0.3", features = ["html_reports"] } [[bench]] name = "router" diff --git a/actix-router/examples/flamegraph.rs b/actix-router/examples/flamegraph.rs new file mode 100644 index 00000000..0c69566b --- /dev/null +++ b/actix-router/examples/flamegraph.rs @@ -0,0 +1,172 @@ +macro_rules! register { + (brackets) => {{ + register!(finish => "{p1}", "{p2}", "{p3}", "{p4}") + }}; + (finish => $p1:literal, $p2:literal, $p3:literal, $p4:literal) => {{ + let arr = [ + concat!("/authorizations"), + concat!("/authorizations/", $p1), + concat!("/applications/", $p1, "/tokens/", $p2), + concat!("/events"), + concat!("/repos/", $p1, "/", $p2, "/events"), + concat!("/networks/", $p1, "/", $p2, "/events"), + concat!("/orgs/", $p1, "/events"), + concat!("/users/", $p1, "/received_events"), + concat!("/users/", $p1, "/received_events/public"), + concat!("/users/", $p1, "/events"), + concat!("/users/", $p1, "/events/public"), + concat!("/users/", $p1, "/events/orgs/", $p2), + concat!("/feeds"), + concat!("/notifications"), + concat!("/repos/", $p1, "/", $p2, "/notifications"), + concat!("/notifications/threads/", $p1), + concat!("/notifications/threads/", $p1, "/subscription"), + concat!("/repos/", $p1, "/", $p2, "/stargazers"), + concat!("/users/", $p1, "/starred"), + concat!("/user/starred"), + concat!("/user/starred/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/subscribers"), + concat!("/users/", $p1, "/subscriptions"), + concat!("/user/subscriptions"), + concat!("/repos/", $p1, "/", $p2, "/subscription"), + concat!("/user/subscriptions/", $p1, "/", $p2), + concat!("/users/", $p1, "/gists"), + concat!("/gists"), + concat!("/gists/", $p1), + concat!("/gists/", $p1, "/star"), + concat!("/repos/", $p1, "/", $p2, "/git/blobs/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/refs"), + concat!("/repos/", $p1, "/", $p2, "/git/tags/", $p3), + concat!("/repos/", $p1, "/", $p2, "/git/trees/", $p3), + concat!("/issues"), + concat!("/user/issues"), + concat!("/orgs/", $p1, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3), + concat!("/repos/", $p1, "/", $p2, "/assignees"), + concat!("/repos/", $p1, "/", $p2, "/assignees/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/events"), + concat!("/repos/", $p1, "/", $p2, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/labels/", $p3), + concat!("/repos/", $p1, "/", $p2, "/issues/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3, "/labels"), + concat!("/repos/", $p1, "/", $p2, "/milestones/"), + concat!("/repos/", $p1, "/", $p2, "/milestones/", $p3), + concat!("/emojis"), + concat!("/gitignore/templates"), + concat!("/gitignore/templates/", $p1), + concat!("/meta"), + concat!("/rate_limit"), + concat!("/users/", $p1, "/orgs"), + concat!("/user/orgs"), + concat!("/orgs/", $p1), + concat!("/orgs/", $p1, "/members"), + concat!("/orgs/", $p1, "/members", $p2), + concat!("/orgs/", $p1, "/public_members"), + concat!("/orgs/", $p1, "/public_members/", $p2), + concat!("/orgs/", $p1, "/teams"), + concat!("/teams/", $p1), + concat!("/teams/", $p1, "/members"), + concat!("/teams/", $p1, "/members", $p2), + concat!("/teams/", $p1, "/repos"), + concat!("/teams/", $p1, "/repos/", $p2, "/", $p3), + concat!("/user/teams"), + concat!("/repos/", $p1, "/", $p2, "/pulls"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/files"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/merge"), + concat!("/repos/", $p1, "/", $p2, "/pulls/", $p3, "/comments"), + concat!("/user/repos"), + concat!("/users/", $p1, "/repos"), + concat!("/orgs/", $p1, "/repos"), + concat!("/repositories"), + concat!("/repos/", $p1, "/", $p2), + concat!("/repos/", $p1, "/", $p2, "/contributors"), + concat!("/repos/", $p1, "/", $p2, "/languages"), + concat!("/repos/", $p1, "/", $p2, "/teams"), + concat!("/repos/", $p1, "/", $p2, "/tags"), + concat!("/repos/", $p1, "/", $p2, "/branches"), + concat!("/repos/", $p1, "/", $p2, "/branches/", $p3), + concat!("/repos/", $p1, "/", $p2, "/collaborators"), + concat!("/repos/", $p1, "/", $p2, "/collaborators/", $p3), + concat!("/repos/", $p1, "/", $p2, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3, "/comments"), + concat!("/repos/", $p1, "/", $p2, "/commits"), + concat!("/repos/", $p1, "/", $p2, "/commits/", $p3), + concat!("/repos/", $p1, "/", $p2, "/readme"), + concat!("/repos/", $p1, "/", $p2, "/keys"), + concat!("/repos/", $p1, "/", $p2, "/keys", $p3), + concat!("/repos/", $p1, "/", $p2, "/downloads"), + concat!("/repos/", $p1, "/", $p2, "/downloads", $p3), + concat!("/repos/", $p1, "/", $p2, "/forks"), + concat!("/repos/", $p1, "/", $p2, "/hooks"), + concat!("/repos/", $p1, "/", $p2, "/hooks", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases"), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3), + concat!("/repos/", $p1, "/", $p2, "/releases/", $p3, "/assets"), + concat!("/repos/", $p1, "/", $p2, "/stats/contributors"), + concat!("/repos/", $p1, "/", $p2, "/stats/commit_activity"), + concat!("/repos/", $p1, "/", $p2, "/stats/code_frequency"), + concat!("/repos/", $p1, "/", $p2, "/stats/participation"), + concat!("/repos/", $p1, "/", $p2, "/stats/punch_card"), + concat!("/repos/", $p1, "/", $p2, "/statuses/", $p3), + concat!("/search/repositories"), + concat!("/search/code"), + concat!("/search/issues"), + concat!("/search/users"), + concat!("/legacy/issues/search/", $p1, "/", $p2, "/", $p3, "/", $p4), + concat!("/legacy/repos/search/", $p1), + concat!("/legacy/user/search/", $p1), + concat!("/legacy/user/email/", $p1), + concat!("/users/", $p1), + concat!("/user"), + concat!("/users"), + concat!("/user/emails"), + concat!("/users/", $p1, "/followers"), + concat!("/user/followers"), + concat!("/users/", $p1, "/following"), + concat!("/user/following"), + concat!("/user/following/", $p1), + concat!("/users/", $p1, "/following", $p2), + concat!("/users/", $p1, "/keys"), + concat!("/user/keys"), + concat!("/user/keys/", $p1), + ]; + std::array::IntoIter::new(arr) + }}; +} + +fn call() -> impl Iterator { + let arr = [ + "/authorizations", + "/user/repos", + "/repos/rust-lang/rust/stargazers", + "/orgs/rust-lang/public_members/nikomatsakis", + "/repos/rust-lang/rust/releases/1.51.0", + ]; + + std::array::IntoIter::new(arr) +} + +fn main() { + let mut router = actix_router::Router::::build(); + + for route in register!(brackets) { + router.path(route, true); + } + + let actix = router.finish(); + + if firestorm::enabled() { + firestorm::bench("target", || { + for route in call() { + let mut path = actix_router::Path::new(route); + actix.recognize(&mut path).unwrap(); + } + }) + .unwrap(); + } +} diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index f6ae6f2d..b937665c 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::ops::Index; +use firestorm::profile_method; use serde::de; use crate::de::PathDeserializer; @@ -37,21 +38,23 @@ impl Path { } } - /// Get reference to inner path instance + /// Get reference to inner path instance. #[inline] pub fn get_ref(&self) -> &T { &self.path } - /// Get mutable reference to inner path instance + /// Get mutable reference to inner path instance. #[inline] pub fn get_mut(&mut self) -> &mut T { &mut self.path } - /// Path + /// Path. #[inline] pub fn path(&self) -> &str { + profile_method!(path); + let skip = self.skip as usize; let path = self.path.path(); if skip <= path.len() { @@ -61,7 +64,7 @@ impl Path { } } - /// Set new path + /// Set new path. #[inline] pub fn set(&mut self, path: T) { self.skip = 0; @@ -69,20 +72,22 @@ impl Path { self.segments.clear(); } - /// Reset state + /// Reset state. #[inline] pub fn reset(&mut self) { self.skip = 0; self.segments.clear(); } - /// Skip first `n` chars in path + /// Skip first `n` chars in path. #[inline] pub fn skip(&mut self, n: u16) { self.skip += n; } pub(crate) fn add(&mut self, name: impl Into>, value: PathItem) { + profile_method!(add); + match value { PathItem::Static(s) => self.segments.push((name.into(), PathItem::Static(s))), PathItem::Segment(begin, end) => self.segments.push(( @@ -116,6 +121,8 @@ impl Path { /// Get matched parameter by name without type conversion pub fn get(&self, key: &str) -> Option<&str> { + profile_method!(get); + for item in self.segments.iter() { if key == item.0 { return match item.1 { @@ -140,9 +147,10 @@ impl Path { /// Get matched parameter by name. /// - /// If keyed parameter is not available empty string is used as default - /// value. + /// If keyed parameter is not available empty string is used as default value. pub fn query(&self, key: &str) -> &str { + profile_method!(query); + if let Some(s) = self.get(key) { s } else { @@ -150,7 +158,7 @@ impl Path { } } - /// Return iterator to items in parameter container + /// Return iterator to items in parameter container. pub fn iter(&self) -> PathIter<'_, T> { PathIter { idx: 0, @@ -160,6 +168,7 @@ impl Path { /// Try to deserialize matching parameters to a specified type `U` pub fn load<'de, U: serde::Deserialize<'de>>(&'de self) -> Result { + profile_method!(load); de::Deserialize::deserialize(PathDeserializer::new(self)) } } diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index cb0264ae..8d838c97 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -6,6 +6,7 @@ use std::{ mem, }; +use firestorm::{profile_fn, profile_method, profile_section}; use regex::{escape, Regex, RegexSet}; use crate::{ @@ -78,6 +79,8 @@ impl ResourceDef { /// /// Panics if path pattern is malformed. pub fn new(path: T) -> Self { + profile_method!(new); + match path.patterns() { Patterns::Single(pattern) => ResourceDef::from_single_pattern(&pattern, false), @@ -124,6 +127,7 @@ impl ResourceDef { /// /// Panics if path regex pattern is malformed. pub fn prefix(path: &str) -> Self { + profile_method!(prefix); ResourceDef::from_single_pattern(path, true) } @@ -134,6 +138,7 @@ impl ResourceDef { /// /// Panics if path regex pattern is malformed. pub fn root_prefix(path: &str) -> Self { + profile_method!(root_prefix); ResourceDef::from_single_pattern(&insert_slash(path), true) } @@ -149,6 +154,8 @@ impl ResourceDef { /// Parse path pattern and create a new instance fn from_single_pattern(pattern: &str, for_prefix: bool) -> Self { + profile_method!(from_single_pattern); + let pattern = pattern.to_owned(); let (pat_type, elements) = ResourceDef::parse(&pattern, for_prefix, false); @@ -179,6 +186,8 @@ impl ResourceDef { /// Check if path matches this pattern. #[inline] pub fn is_match(&self, path: &str) -> bool { + profile_method!(is_match); + match self.pat_type { PatternType::Static(ref s) => s == path, PatternType::Prefix(ref s) => path.starts_with(s), @@ -189,6 +198,8 @@ impl ResourceDef { /// Is prefix path a match against this resource. pub fn is_prefix_match(&self, path: &str) -> Option { + profile_method!(is_prefix_match); + let path_len = path.len(); let path = if path.is_empty() { "/" } else { path }; @@ -245,6 +256,7 @@ impl ResourceDef { /// Is the given path and parameters a match against this pattern. pub fn match_path(&self, path: &mut Path) -> bool { + profile_method!(match_path); self.match_path_checked(path, &|_, _| true, &Some(())) } @@ -260,11 +272,15 @@ impl ResourceDef { R: Resource, F: Fn(&R, &Option) -> bool, { + profile_method!(match_path_checked); + let mut segments: [PathItem; MAX_DYNAMIC_SEGMENTS] = Default::default(); let path = res.resource_path(); let (matched_len, matched_vars) = match self.pat_type { PatternType::Static(ref segment) => { + profile_section!(pattern_static); + if segment != path.path() { return false; } @@ -273,6 +289,8 @@ impl ResourceDef { } PatternType::Prefix(ref prefix) => { + profile_section!(pattern_dynamic); + let path_str = path.path(); let path_len = path_str.len(); @@ -300,24 +318,39 @@ impl ResourceDef { } PatternType::Dynamic(ref re, ref names) => { - let captures = match re.captures(path.path()) { - Some(captures) => captures, - _ => return false, + profile_section!(pattern_dynamic); + + let captures = { + profile_section!(pattern_dynamic_regex_exec); + + match re.captures(path.path()) { + Some(captures) => captures, + _ => return false, + } }; - for (no, name) in names.iter().enumerate() { - if let Some(m) = captures.name(&name) { - segments[no] = PathItem::Segment(m.start() as u16, m.end() as u16); - } else { - log::error!("Dynamic path match but not all segments found: {}", name); - return false; + { + profile_section!(pattern_dynamic_extract_captures); + + for (no, name) in names.iter().enumerate() { + if let Some(m) = captures.name(&name) { + segments[no] = PathItem::Segment(m.start() as u16, m.end() as u16); + } else { + log::error!( + "Dynamic path match but not all segments found: {}", + name + ); + return false; + } } - } + }; (captures[0].len(), Some(names)) } PatternType::DynamicSet(ref re, ref params) => { + profile_section!(pattern_dynamic_set); + let path = path.path(); let (pattern, names) = match re.matches(path).into_iter().next() { Some(idx) => ¶ms[idx], @@ -394,6 +427,7 @@ impl ResourceDef { U: Iterator, I: AsRef, { + profile_method!(resource_path_from_iter); self.build_resource_path(path, |_| elements.next()) } @@ -404,6 +438,7 @@ impl ResourceDef { U: Iterator, I: AsRef, { + profile_method!(build_resource_path); self.resource_path_from_iter(path, elements) } @@ -420,6 +455,7 @@ impl ResourceDef { V: AsRef, S: BuildHasher, { + profile_method!(resource_path_from_map); self.build_resource_path(path, |name| { name.and_then(|name| elements.get(name).map(AsRef::::as_ref)) }) @@ -458,6 +494,7 @@ impl ResourceDef { S: BuildHasher, T: AsRef, { + profile_method!(resource_path_from_map_with_tail); self.build_resource_path(path, |name| match name { Some(name) => elements.get(name).map(AsRef::::as_ref), None => Some(tail.as_ref()), @@ -465,6 +502,8 @@ impl ResourceDef { } fn parse_param(pattern: &str) -> (PatternElement, String, &str) { + profile_method!(parse_param); + const DEFAULT_PATTERN: &str = "[^/]+"; const DEFAULT_PATTERN_TAIL: &str = ".*"; @@ -526,6 +565,8 @@ impl ResourceDef { for_prefix: bool, force_dynamic: bool, ) -> (PatternType, Vec) { + profile_method!(parse); + let mut unprocessed = pattern; if !force_dynamic && unprocessed.find('{').is_none() && !unprocessed.ends_with('*') { @@ -637,6 +678,8 @@ impl From for ResourceDef { } pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { + profile_fn!(insert_slash); + if !path.is_empty() && !path.starts_with('/') { let mut new_path = String::with_capacity(path.len() + 1); new_path.push('/'); diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 007ef495..2f345ecf 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -1,3 +1,5 @@ +use firestorm::profile_method; + use crate::{IntoPatterns, Resource, ResourceDef, ResourcePath}; #[derive(Debug, Copy, Clone, PartialEq)] @@ -24,6 +26,8 @@ impl Router { R: Resource

, P: ResourcePath, { + profile_method!(recognize); + for item in self.0.iter() { if item.0.match_path(resource.resource_path()) { return Some((&item.1, ResourceId(item.0.id()))); @@ -37,6 +41,8 @@ impl Router { R: Resource

, P: ResourcePath, { + profile_method!(recognize_mut); + for item in self.0.iter_mut() { if item.0.match_path(resource.resource_path()) { return Some((&mut item.1, ResourceId(item.0.id()))); @@ -55,6 +61,8 @@ impl Router { R: Resource

, P: ResourcePath, { + profile_method!(recognize_checked); + for item in self.0.iter() { if item.0.match_path_checked(resource, &check, &item.2) { return Some((&item.1, ResourceId(item.0.id()))); @@ -73,6 +81,8 @@ impl Router { R: Resource

, P: ResourcePath, { + profile_method!(recognize_mut_checked); + for item in self.0.iter_mut() { if item.0.match_path_checked(resource, &check, &item.2) { return Some((&mut item.1, ResourceId(item.0.id()))); @@ -93,6 +103,8 @@ impl RouterBuilder { path: P, resource: T, ) -> &mut (ResourceDef, T, Option) { + profile_method!(path); + self.resources .push((ResourceDef::new(path), resource, None)); self.resources.last_mut().unwrap() @@ -100,6 +112,8 @@ impl RouterBuilder { /// Register resource for specified path prefix. pub fn prefix(&mut self, prefix: &str, resource: T) -> &mut (ResourceDef, T, Option) { + profile_method!(prefix); + self.resources .push((ResourceDef::prefix(prefix), resource, None)); self.resources.last_mut().unwrap() @@ -107,6 +121,8 @@ impl RouterBuilder { /// Register resource for ResourceDef pub fn rdef(&mut self, rdef: ResourceDef, resource: T) -> &mut (ResourceDef, T, Option) { + profile_method!(rdef); + self.resources.push((rdef, resource, None)); self.resources.last_mut().unwrap() } From e4ec956001ef9bf14be25da3e1bfd622ef3f4bc8 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 17 Jul 2021 03:11:25 +0100 Subject: [PATCH 06/15] fix examples on msrv --- actix-router/examples/flamegraph.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/actix-router/examples/flamegraph.rs b/actix-router/examples/flamegraph.rs index 0c69566b..89527e2f 100644 --- a/actix-router/examples/flamegraph.rs +++ b/actix-router/examples/flamegraph.rs @@ -135,21 +135,18 @@ macro_rules! register { concat!("/user/keys"), concat!("/user/keys/", $p1), ]; - std::array::IntoIter::new(arr) + + arr.to_vec() }}; } -fn call() -> impl Iterator { - let arr = [ - "/authorizations", - "/user/repos", - "/repos/rust-lang/rust/stargazers", - "/orgs/rust-lang/public_members/nikomatsakis", - "/repos/rust-lang/rust/releases/1.51.0", - ]; - - std::array::IntoIter::new(arr) -} +static PATHS: [&'static str; 5] = [ + "/authorizations", + "/user/repos", + "/repos/rust-lang/rust/stargazers", + "/orgs/rust-lang/public_members/nikomatsakis", + "/repos/rust-lang/rust/releases/1.51.0", +]; fn main() { let mut router = actix_router::Router::::build(); @@ -162,7 +159,7 @@ fn main() { if firestorm::enabled() { firestorm::bench("target", || { - for route in call() { + for &route in &PATHS { let mut path = actix_router::Path::new(route); actix.recognize(&mut path).unwrap(); } From a83dfaa162e2b969fbac8926de2431dfb697c83a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 17 Jul 2021 20:54:53 +0100 Subject: [PATCH 07/15] Update macros.rs closes #234 --- actix-service/src/macros.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/actix-service/src/macros.rs b/actix-service/src/macros.rs index e23b2960..6cf3ef08 100644 --- a/actix-service/src/macros.rs +++ b/actix-service/src/macros.rs @@ -1,5 +1,9 @@ /// An implementation of [`poll_ready`]() that always signals readiness. /// +/// This should only be used for basic leaf services that have no concept of un-readiness. +/// For wrapper or other serivice types, use [`forward_ready!`] for simple cases or write a bespoke +/// `poll_ready` implementation. +/// /// [`poll_ready`]: crate::Service::poll_ready /// /// # Examples From c65e8524b2dd412577db33ee5218cb92addc6ef4 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 19 Jul 2021 22:37:54 +0100 Subject: [PATCH 08/15] rework resourcedef (#373) --- .cargo/config.toml | 4 +- .github/workflows/clippy-fmt.yml | 2 +- actix-router/CHANGES.md | 23 +- actix-router/examples/flamegraph.rs | 2 +- actix-router/src/lib.rs | 10 +- actix-router/src/path.rs | 24 +- actix-router/src/resource.rs | 1239 +++++++++++++++++++-------- actix-router/src/router.rs | 20 +- actix-router/src/url.rs | 4 +- 9 files changed, 956 insertions(+), 372 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 40fe3e57..75362685 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,3 +1,3 @@ [alias] -chk = "hack check --workspace --all-features --tests --examples" -lint = "hack --clean-per-run clippy --workspace --tests --examples" +chk = "check --workspace --all-features --tests --examples --bins" +lint = "clippy --workspace --all-features --tests --examples --bins -- -Dclippy::todo" diff --git a/.github/workflows/clippy-fmt.yml b/.github/workflows/clippy-fmt.yml index 3bef81db..ca637beb 100644 --- a/.github/workflows/clippy-fmt.yml +++ b/.github/workflows/clippy-fmt.yml @@ -39,4 +39,4 @@ jobs: uses: actions-rs/clippy-check@v1 with: token: ${{ secrets.GITHUB_TOKEN }} - args: --workspace --tests --all-features + args: --workspace --all-features --tests --examples --bins -- -Dclippy::todo diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 9ec87716..8ce805bf 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -1,15 +1,23 @@ # 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 a bug in multi-patterns where static patterns are interpreted as regex. [#366] +* Introduce `ResourceDef::pattern_iter` to get an iterator over all patterns in a multi-pattern resource. [#373] * 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] -* Re-work `IntoPatterns` trait. [#372] +* Fix `ResourceDef` `PartialEq` implementation. +* Re-work `IntoPatterns` trait, adding a `Patterns` enum. [#372] +* Implement `IntoPatterns` for `bytestring::ByteString`. [#372] * 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] +* Rename `ResourceDef::{resource_path => resource_path_from_iter}`. [#371] +* `ResourceDef::resource_path_from_iter` now takes an `IntoIterator`. [#373] +* Rename `ResourceDef::{resource_path_named => resource_path_from_map}`. [#371] +* Rename `ResourceDef::{is_prefix_match => find_match}`. [#373] +* Rename `ResourceDef::{match_path => capture_match_info}`. [#373] +* Rename `ResourceDef::{match_path_checked => capture_match_info_fn}`. [#373] +* Remove `ResourceDef::name_mut` and introduce `ResourceDef::set_name`. [#373] +* Rename `Router::{*_checked => *_fn}`. [#373] +* Return type of `ResourceDef::name` is now `Option<&str>`. [#373] +* Return type of `ResourceDef::pattern` is now `Option<&str>`. [#373] [#368]: https://github.com/actix/actix-net/pull/368 [#366]: https://github.com/actix/actix-net/pull/366 @@ -17,6 +25,7 @@ [#370]: https://github.com/actix/actix-net/pull/370 [#371]: https://github.com/actix/actix-net/pull/371 [#372]: https://github.com/actix/actix-net/pull/372 +[#373]: https://github.com/actix/actix-net/pull/373 ## 0.4.0 - 2021-06-06 diff --git a/actix-router/examples/flamegraph.rs b/actix-router/examples/flamegraph.rs index 89527e2f..798cc22d 100644 --- a/actix-router/examples/flamegraph.rs +++ b/actix-router/examples/flamegraph.rs @@ -140,7 +140,7 @@ macro_rules! register { }}; } -static PATHS: [&'static str; 5] = [ +static PATHS: [&str; 5] = [ "/authorizations", "/user/repos", "/repos/rust-lang/rust/stargazers", diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 53ae9db6..6082c1a3 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -14,6 +14,8 @@ pub use self::path::Path; pub use self::resource::ResourceDef; pub use self::router::{ResourceInfo, Router, RouterBuilder}; +// TODO: this trait is necessary, document it +// see impl Resource for ServiceRequest pub trait Resource { fn resource_path(&mut self) -> &mut Path; } @@ -41,7 +43,7 @@ impl ResourcePath for bytestring::ByteString { } /// One or many patterns. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Patterns { Single(String), List(Vec), @@ -70,6 +72,12 @@ impl<'a> IntoPatterns for &'a str { } } +impl IntoPatterns for bytestring::ByteString { + fn patterns(&self) -> Patterns { + Patterns::Single(self.to_string()) + } +} + impl> IntoPatterns for Vec { fn patterns(&self) -> Patterns { let mut patterns = self.iter().map(|v| v.as_ref().to_owned()); diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index b937665c..e29591f9 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -4,8 +4,7 @@ use std::ops::Index; use firestorm::profile_method; use serde::de; -use crate::de::PathDeserializer; -use crate::{Resource, ResourcePath}; +use crate::{de::PathDeserializer, Resource, ResourcePath}; #[derive(Debug, Clone)] pub(crate) enum PathItem { @@ -120,24 +119,21 @@ impl Path { } /// Get matched parameter by name without type conversion - pub fn get(&self, key: &str) -> Option<&str> { + pub fn get(&self, name: &str) -> Option<&str> { profile_method!(get); - - for item in self.segments.iter() { - if key == item.0 { - return match item.1 { + + for (seg_name, val) in self.segments.iter() { + if name == seg_name { + return match val { PathItem::Static(ref s) => Some(&s), PathItem::Segment(s, e) => { - Some(&self.path.path()[(s as usize)..(e as usize)]) + Some(&self.path.path()[(*s as usize)..(*e as usize)]) } }; } } - if key == "tail" { - Some(&self.path.path()[(self.skip as usize)..]) - } else { - None - } + + None } /// Get unprocessed part of the path @@ -150,7 +146,7 @@ impl Path { /// If keyed parameter is not available empty string is used as default value. pub fn query(&self, key: &str) -> &str { profile_method!(query); - + if let Some(s) = self.get(key) { s } else { diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 8d838c97..58016bb3 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1,6 +1,5 @@ use std::{ borrow::{Borrow, Cow}, - cmp::min, collections::HashMap, hash::{BuildHasher, Hash, Hasher}, mem, @@ -18,44 +17,202 @@ const MAX_DYNAMIC_SEGMENTS: usize = 16; /// Regex flags to allow '.' in regex to match '\n' /// -/// See the docs under: https://docs.rs/regex/1.5.4/regex/#grouping-and-flags +/// See the docs under: https://docs.rs/regex/1/regex/#grouping-and-flags const REGEX_FLAGS: &str = "(?s-m)"; -/// Describes an entry in a resource table. +/// Describes the set of paths that match to a resource. /// -/// Resource definition can contain at most 16 dynamic segments. +/// `ResourceDef`s are effectively a way to transform the a custom resource pattern syntax into +/// suitable regular expressions from which to check matches with paths and capture portions of a +/// matched path into variables. Common cases are on a fast path that avoids going through the +/// regex engine. +/// +/// +/// # Static Resources +/// A static resource is the most basic type of definition. Pass a regular string to +/// [new][Self::new]. Conforming paths must match the string exactly. +/// +/// ## Examples +/// ``` +/// # use actix_router::ResourceDef; +/// let resource = ResourceDef::new("/home"); +/// +/// assert!(resource.is_match("/home")); +/// +/// assert!(!resource.is_match("/home/new")); +/// assert!(!resource.is_match("/homes")); +/// assert!(!resource.is_match("/search")); +/// ``` +/// +/// +/// # Dynamic Segments +/// Also known as "path parameters". Resources can define sections of a pattern that be extracted +/// from a conforming path, if it conforms to (one of) the resource pattern(s). +/// +/// The marker for a dynamic segment is curly braces wrapping an identifier. For example, +/// `/user/{id}` would match paths like `/user/123` or `/user/james` and be able to extract the user +/// IDs "123" and "james", respectively. +/// +/// However, this resource pattern (`/user/{id}`) would, not cover `/user/123/stars` (unless +/// constructed as a prefix; see next section) since the default pattern for segments matches all +/// characters until it finds a `/` character (or the end of the path). Custom segment patterns are +/// covered further down. +/// +/// Dynamic segments do not need to be delimited by `/` characters, they can be defined within a +/// path segment. For example, `/rust-is-{opinion}` can match the paths `/rust-is-cool` and +/// `/rust-is-hard`. +/// +/// For information on capturing segment values from paths or other custom resource types, +/// see [`capture_match_info`][Self::capture_match_info] +/// and [`capture_match_info_fn`][Self::capture_match_info_fn]. +/// +/// A resource definition can contain at most 16 dynamic segments. +/// +/// ## Examples +/// ``` +/// use actix_router::{Path, ResourceDef}; +/// +/// let resource = ResourceDef::prefix("/user/{id}"); +/// +/// assert!(resource.is_match("/user/123")); +/// assert!(!resource.is_match("/user")); +/// assert!(!resource.is_match("/user/")); +/// +/// let mut path = Path::new("/user/123"); +/// resource.capture_match_info(&mut path); +/// assert_eq!(path.get("id").unwrap(), "123"); +/// ``` +/// +/// +/// # Prefix Resources +/// A prefix resource is defined as pattern that can match just the start of a path. +/// +/// This library chooses to restrict that definition slightly. In particular, when matching, the +/// prefix must be separated from the remaining part of the path by a `/` character, either at the +/// end of the prefix pattern or at the start of the the remaining slice. In practice, this is not +/// much of a limitation. +/// +/// Prefix resources can contain dynamic segments. +/// +/// ## Examples +/// ``` +/// # use actix_router::ResourceDef; +/// let resource = ResourceDef::prefix("/home"); +/// assert!(resource.is_match("/home")); +/// assert!(resource.is_match("/home/new")); +/// assert!(!resource.is_match("/homes")); +/// +/// let resource = ResourceDef::prefix("/user/{id}/"); +/// assert!(resource.is_match("/user/123/")); +/// assert!(resource.is_match("/user/123/stars")); +/// ``` +/// +/// +/// # Custom Regex Segments +/// Dynamic segments can be customised to only match a specific regular expression. It can be +/// helpful to do this if resource definitions would otherwise conflict and cause one to +/// be inaccessible. +/// +/// The regex used when capturing segment values can be specified explicitly using this syntax: +/// `{name:regex}`. For example, `/user/{id:\d+}` will only match paths where the user ID +/// is numeric. +/// +/// By default, dynamic segments use this regex: `[^/]+`. This shows why it is the case, as shown in +/// the earlier section, that segments capture a slice of the path up to the next `/` character. +/// +/// Custom regex segments can be used in static and prefix resource definition variants. +/// +/// ## Examples +/// ``` +/// # use actix_router::ResourceDef; +/// let resource = ResourceDef::new(r"/user/{id:\d+}"); +/// assert!(resource.is_match("/user/123")); +/// assert!(resource.is_match("/user/314159")); +/// assert!(!resource.is_match("/user/abc")); +/// ``` +/// +/// +/// # Tail Segments +/// As a shortcut to defining a custom regex for matching _all_ remaining characters (not just those +/// up until a `/` character), there is a special pattern to match (and capture) the remaining +/// path portion. +/// +/// To do this, use the segment pattern: `{name}*`. Since a tail segment also has a name, values are +/// extracted in the same way as non-tail dynamic segments. +/// +/// ## Examples +/// ```rust +/// # use actix_router::{Path, ResourceDef}; +/// let resource = ResourceDef::new("/blob/{tail}*"); +/// assert!(resource.is_match("/blob/HEAD/Cargo.toml")); +/// assert!(resource.is_match("/blob/HEAD/README.md")); +/// +/// let mut path = Path::new("/blob/main/LICENSE"); +/// resource.capture_match_info(&mut path); +/// assert_eq!(path.get("tail").unwrap(), "main/LICENSE"); +/// ``` +/// +/// +/// # Multi-Pattern Resources +/// For resources that can map to multiple distinct paths, it may be suitable to use +/// multi-pattern resources by passing an array/vec to [`new`][Self::new]. They will be combined +/// into a regex set which is usually quicker to check matches on than checking each +/// pattern individually. +/// +/// Multi-pattern resources can contain dynamic segments just like single pattern ones. +/// However, take care to use consistent and semantically-equivalent segment names; it could affect +/// expectations in the router using these definitions and cause runtime panics. +/// +/// ## Examples +/// ```rust +/// # use actix_router::ResourceDef; +/// let resource = ResourceDef::new(["/home", "/index"]); +/// assert!(resource.is_match("/home")); +/// assert!(resource.is_match("/index")); +/// ``` +/// +/// +/// # Trailing Slashes +/// It should be noted that this library takes no steps to normalize intra-path or trailing slashes. +/// As such, all resource definitions implicitly expect a pre-processing step to normalize paths if +/// they you wish to accommodate "recoverable" path errors. Below are several examples of +/// resource-path pairs that would not be compatible. +/// +/// ## Examples +/// ```rust +/// # use actix_router::ResourceDef; +/// assert!(!ResourceDef::new("/root").is_match("/root/")); +/// assert!(!ResourceDef::new("/root/").is_match("/root")); +/// assert!(!ResourceDef::prefix("/root/").is_match("/root")); +/// ``` #[derive(Clone, Debug)] pub struct ResourceDef { id: u16, - /// Optional name of resource definition. Defaults to "". - name: String, + /// Optional name of resource. + name: Option, /// Pattern that generated the resource definition. - // TODO: Sort of, in dynamic set pattern type it is blank, consider change to option. - pattern: String, + /// + /// `None` when pattern type is `DynamicSet`. + patterns: Patterns, /// Pattern type. pat_type: PatternType, - /// List of elements that compose the pattern, in order. + /// List of segments that compose the pattern, in order. /// - /// `None` with pattern type is DynamicSet. - elements: Option>, + /// `None` when pattern type is `DynamicSet`. + segments: Option>, } #[derive(Debug, Clone, PartialEq)] -enum PatternElement { +enum PatternSegment { /// Literal slice of pattern. Const(String), /// 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)] @@ -75,30 +232,50 @@ enum PatternType { } impl ResourceDef { - /// Parse path pattern and create new `Pattern` instance. + /// Constructs a new resource definition from patterns. /// + /// Multi-pattern resources can be constructed by providing a slice (or vec) of patterns. + /// + /// # Panics /// Panics if path pattern is malformed. - pub fn new(path: T) -> Self { + /// + /// # Examples + /// ``` + /// use actix_router::ResourceDef; + /// + /// let resource = ResourceDef::new("/user/{id}"); + /// assert!(resource.is_match("/user/123")); + /// assert!(!resource.is_match("/user/123/stars")); + /// assert!(!resource.is_match("user/1234")); + /// assert!(!resource.is_match("/foo")); + /// + /// let resource = ResourceDef::new(["/profile", "/user/{id}"]); + /// assert!(resource.is_match("/profile")); + /// assert!(resource.is_match("/user/123")); + /// assert!(!resource.is_match("user/123")); + /// assert!(!resource.is_match("/foo")); + /// ``` + pub fn new(paths: T) -> Self { profile_method!(new); - match path.patterns() { + 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: String::new(), - pattern: String::new(), + name: None, + patterns: Patterns::List(patterns), pat_type: PatternType::DynamicSet(RegexSet::empty(), Vec::new()), - elements: None, + segments: None, }, Patterns::List(patterns) => { let mut re_set = Vec::with_capacity(patterns.len()); let mut pattern_data = Vec::new(); - for pattern in patterns { + for pattern in &patterns { match ResourceDef::parse(&pattern, false, true) { (PatternType::Dynamic(re, names), _) => { re_set.push(re.as_str().to_owned()); @@ -112,141 +289,348 @@ impl ResourceDef { ResourceDef { id: 0, - name: String::new(), - pattern: String::new(), + name: None, + patterns: Patterns::List(patterns), pat_type: PatternType::DynamicSet(pattern_re_set, pattern_data), - elements: None, + segments: None, } } } } - /// Parse path pattern and create new `Pattern` instance. + /// Constructs a new resource definition using a string pattern that performs prefix matching. /// - /// Use `prefix` type instead of `static`. + /// More specifically, the regular expressions generated for matching are different when using + /// this method vs using `new`; they will not be appended with the `$` meta-character that + /// matches the end of an input. /// + /// Although it will compile and run correctly, it is meaningless to construct a prefix + /// resource definition with a tail segment; use [`new`][Self::new] in this case. + /// + /// # Panics /// Panics if path regex pattern is malformed. + /// + /// # Examples + /// ``` + /// use actix_router::ResourceDef; + /// + /// let resource = ResourceDef::prefix("/user/{id}"); + /// assert!(resource.is_match("/user/123")); + /// assert!(resource.is_match("/user/123/stars")); + /// assert!(!resource.is_match("user/123")); + /// assert!(!resource.is_match("user/123/stars")); + /// assert!(!resource.is_match("/foo")); + /// + /// let resource = ResourceDef::prefix("user/{id}"); + /// assert!(resource.is_match("user/123")); + /// assert!(resource.is_match("user/123/stars")); + /// assert!(!resource.is_match("/user/123")); + /// assert!(!resource.is_match("/user/123/stars")); + /// assert!(!resource.is_match("foo")); + /// ``` pub fn prefix(path: &str) -> Self { profile_method!(prefix); ResourceDef::from_single_pattern(path, true) } - /// Parse path pattern and create new `Pattern` instance, inserting a `/` to beginning of - /// the pattern if absent. - /// - /// Use `prefix` type instead of `static`. + /// Constructs a new resource definition using a string pattern that performs prefix matching, + /// inserting a `/` to beginning of the pattern if absent and pattern is not empty. /// + /// # Panics /// Panics if path regex pattern is malformed. + /// + /// # Examples + /// ``` + /// use actix_router::ResourceDef; + /// + /// let resource = ResourceDef::root_prefix("user/{id}"); + /// + /// assert_eq!(&resource, &ResourceDef::prefix("/user/{id}")); + /// assert_eq!(&resource, &ResourceDef::root_prefix("/user/{id}")); + /// assert_ne!(&resource, &ResourceDef::new("user/{id}")); + /// assert_ne!(&resource, &ResourceDef::new("/user/{id}")); + /// + /// assert!(resource.is_match("/user/123")); + /// assert!(!resource.is_match("user/123")); + /// ``` pub fn root_prefix(path: &str) -> Self { profile_method!(root_prefix); - ResourceDef::from_single_pattern(&insert_slash(path), true) + ResourceDef::prefix(&insert_slash(path)) } - /// Resource ID. + /// Returns a numeric resource ID. + /// + /// If not explicitly set using [`set_id`][Self::set_id], this will return `0`. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/root"); + /// assert_eq!(resource.id(), 0); + /// + /// resource.set_id(42); + /// assert_eq!(resource.id(), 42); + /// ``` pub fn id(&self) -> u16 { self.id } - /// Set resource ID. + /// Set numeric resource ID. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/root"); + /// resource.set_id(42); + /// assert_eq!(resource.id(), 42); + /// ``` pub fn set_id(&mut self, id: u16) { self.id = id; } - /// Parse path pattern and create a new instance - fn from_single_pattern(pattern: &str, for_prefix: bool) -> Self { - profile_method!(from_single_pattern); + /// Returns resource definition name, if set. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/root"); + /// assert!(resource.name().is_none()); + /// + /// resource.set_name("root"); + /// assert_eq!(resource.name().unwrap(), "root"); + pub fn name(&self) -> Option<&str> { + self.name.as_deref() + } - let pattern = pattern.to_owned(); - let (pat_type, elements) = ResourceDef::parse(&pattern, for_prefix, false); + /// Assigns a new name to the resource. + /// + /// # Panics + /// Panics if `name` is an empty string. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/root"); + /// resource.set_name("root"); + /// assert_eq!(resource.name().unwrap(), "root"); + /// ``` + pub fn set_name(&mut self, name: impl Into) { + let name = name.into(); - ResourceDef { - pat_type, - pattern, - elements: Some(elements), - id: 0, - name: String::new(), + if name.is_empty() { + panic!("resource name should not be empty"); + } + + self.name = Some(name) + } + + /// Returns `true` if pattern type is prefix. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// assert!(ResourceDef::prefix("/user").is_prefix()); + /// 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, } } - /// Resource pattern name. - pub fn name(&self) -> &str { - &self.name + /// Returns the pattern string that generated the resource definition. + /// + /// Returns `None` if definition was constructed with multiple patterns. + /// See [`patterns_iter`][Self::pattern_iter]. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/user/{id}"); + /// assert_eq!(resource.pattern().unwrap(), "/user/{id}"); + /// + /// let mut resource = ResourceDef::new(["/profile", "/user/{id}"]); + /// assert!(resource.pattern().is_none()); + pub fn pattern(&self) -> Option<&str> { + match &self.patterns { + Patterns::Single(pattern) => Some(pattern.as_str()), + Patterns::List(_) => None, + } } - /// Mutable reference to a name of a resource definition. - pub fn name_mut(&mut self) -> &mut String { - &mut self.name + /// Returns iterator of pattern strings that generated the resource definition. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut resource = ResourceDef::new("/root"); + /// let mut iter = resource.pattern_iter(); + /// assert_eq!(iter.next().unwrap(), "/root"); + /// assert!(iter.next().is_none()); + /// + /// let mut resource = ResourceDef::new(["/root", "/backup"]); + /// let mut iter = resource.pattern_iter(); + /// assert_eq!(iter.next().unwrap(), "/root"); + /// assert_eq!(iter.next().unwrap(), "/backup"); + /// assert!(iter.next().is_none()); + pub fn pattern_iter(&self) -> impl Iterator { + struct PatternIter<'a> { + patterns: &'a Patterns, + list_idx: usize, + done: bool, + } + + impl<'a> Iterator for PatternIter<'a> { + type Item = &'a str; + + fn next(&mut self) -> Option { + match &self.patterns { + Patterns::Single(pattern) => { + if self.done { + return None; + } + + self.done = true; + Some(pattern.as_str()) + } + Patterns::List(patterns) if patterns.is_empty() => None, + Patterns::List(patterns) => match patterns.get(self.list_idx) { + Some(pattern) => { + self.list_idx += 1; + Some(pattern.as_str()) + } + None => { + // fast path future call + self.done = true; + None + } + }, + } + } + + fn size_hint(&self) -> (usize, Option) { + match &self.patterns { + Patterns::Single(_) => (1, Some(1)), + Patterns::List(patterns) => (patterns.len(), Some(patterns.len())), + } + } + } + + PatternIter { + patterns: &self.patterns, + list_idx: 0, + done: false, + } } - /// Path pattern of the resource - pub fn pattern(&self) -> &str { - &self.pattern - } - - /// Check if path matches this pattern. + /// Returns `true` if `path` matches this resource. + /// + /// The behavior of this method depends on how the `ResourceDef` was constructed. For example, + /// static resources will not be able to match as many paths as dynamic and prefix resources. + /// See [`ResourceDef`] struct docs for details on resource definition types. + /// + /// This method will always agree with [`find_match`][Self::find_match] on whether the path + /// matches or not. + /// + /// # Examples + /// ``` + /// use actix_router::ResourceDef; + /// + /// // static resource + /// let resource = ResourceDef::new("/user"); + /// assert!(resource.is_match("/user")); + /// assert!(!resource.is_match("/users")); + /// assert!(!resource.is_match("/user/123")); + /// assert!(!resource.is_match("/foo")); + /// + /// // dynamic resource + /// let resource = ResourceDef::new("/user/{user_id}"); + /// assert!(resource.is_match("/user/123")); + /// assert!(!resource.is_match("/user/123/stars")); + /// + /// // prefix resource + /// let resource = ResourceDef::prefix("/root"); + /// assert!(resource.is_match("/root")); + /// assert!(resource.is_match("/root/leaf")); + /// assert!(!resource.is_match("/roots")); + /// + /// // more examples are shown in the `ResourceDef` struct docs + /// ``` #[inline] pub fn is_match(&self, path: &str) -> bool { profile_method!(is_match); + // this function could be expressed as: + // `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 s) => path.starts_with(s), + + PatternType::Prefix(ref prefix) if prefix == path => true, + PatternType::Prefix(ref prefix) => is_strict_prefix(prefix, path), + PatternType::Dynamic(ref re, _) => re.is_match(path), PatternType::DynamicSet(ref re, _) => re.is_match(path), } } - /// Is prefix path a match against this resource. - pub fn is_prefix_match(&self, path: &str) -> Option { - profile_method!(is_prefix_match); + /// Tries to match `path` to this resource, returning the position in the path where the + /// match ends. + /// + /// This method will always agree with [`is_match`][Self::is_match] on whether the path matches + /// or not. + /// + /// # Examples + /// ``` + /// use actix_router::ResourceDef; + /// + /// // static resource + /// let resource = ResourceDef::new("/user"); + /// assert_eq!(resource.find_match("/user"), Some(5)); + /// assert!(resource.find_match("/user/").is_none()); + /// assert!(resource.find_match("/user/123").is_none()); + /// assert!(resource.find_match("/foo").is_none()); + /// + /// // constant prefix resource + /// let resource = ResourceDef::prefix("/user"); + /// assert_eq!(resource.find_match("/user"), Some(5)); + /// assert_eq!(resource.find_match("/user/"), Some(5)); + /// assert_eq!(resource.find_match("/user/123"), Some(5)); + /// + /// // dynamic prefix resource + /// let resource = ResourceDef::prefix("/user/{id}"); + /// assert_eq!(resource.find_match("/user/123"), Some(9)); + /// assert_eq!(resource.find_match("/user/1234/"), Some(10)); + /// assert_eq!(resource.find_match("/user/12345/stars"), Some(11)); + /// assert!(resource.find_match("/user/").is_none()); + /// + /// // multi-pattern resource + /// let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); + /// assert_eq!(resource.find_match("/user/123"), Some(9)); + /// assert_eq!(resource.find_match("/profile/1234"), Some(13)); + /// ``` + pub fn find_match(&self, path: &str) -> Option { + profile_method!(find_match); - let path_len = path.len(); - let path = if path.is_empty() { "/" } else { path }; - - match self.pat_type { - PatternType::Static(ref segment) => { + match &self.pat_type { + PatternType::Static(segment) => { if segment == path { - Some(path_len) + Some(segment.len()) } else { None } } - PatternType::Prefix(ref prefix) => { - let prefix_len = if path == prefix { - // path length === prefix segment length - path_len - } else { - let is_slash_next = - prefix.ends_with('/') || path.split_at(prefix.len()).1.starts_with('/'); + PatternType::Prefix(prefix) if path == prefix => Some(prefix.len()), + PatternType::Prefix(prefix) if is_strict_prefix(prefix, path) => Some(prefix.len()), + PatternType::Prefix(_) => None, - if path.starts_with(prefix) && is_slash_next { - // enters this branch if segment delimiter ("/") is present after prefix - // - // i.e., path starts with prefix segment - // and prefix segment ends with / - // or first character in path after prefix segment length is / - // - // eg: Prefix("/test/") or Prefix("/test") would match: - // - /test/foo - // - /test/foo + PatternType::Dynamic(re, _) => re.find(path).map(|m| m.end()), - if prefix.ends_with('/') { - prefix.len() - 1 - } else { - prefix.len() - } - } else { - return None; - } - }; - - Some(min(path_len, prefix_len)) - } - - PatternType::Dynamic(ref re, _) => re.find(path).map(|m| m.end()), - - PatternType::DynamicSet(ref re, ref params) => { + PatternType::DynamicSet(re, params) => { let idx = re.matches(path).into_iter().next()?; let (ref pattern, _) = params[idx]; pattern.find(path).map(|m| m.end()) @@ -254,70 +638,97 @@ impl ResourceDef { } } - /// Is the given path and parameters a match against this pattern. - pub fn match_path(&self, path: &mut Path) -> bool { - profile_method!(match_path); - self.match_path_checked(path, &|_, _| true, &Some(())) + /// Collects dynamic segment values into `path`. + /// + /// Returns `true` if `path` matches this resource. + /// + /// # Examples + /// ``` + /// use actix_router::{Path, ResourceDef}; + /// + /// let resource = ResourceDef::prefix("/user/{id}"); + /// let mut path = Path::new("/user/123/stars"); + /// assert!(resource.capture_match_info(&mut path)); + /// assert_eq!(path.get("id").unwrap(), "123"); + /// assert_eq!(path.unprocessed(), "/stars"); + /// + /// let resource = ResourceDef::new("/blob/{path}*"); + /// let mut path = Path::new("/blob/HEAD/Cargo.toml"); + /// assert!(resource.capture_match_info(&mut path)); + /// assert_eq!(path.get("path").unwrap(), "HEAD/Cargo.toml"); + /// assert_eq!(path.unprocessed(), ""); + /// ``` + pub fn capture_match_info(&self, path: &mut Path) -> bool { + profile_method!(is_path_match); + self.capture_match_info_fn(path, &|_, _| true, &None::<()>) } - /// Is the given path and parameters a match against this pattern? - pub fn match_path_checked( + /// Collects dynamic segment values into `resource` after matching paths and executing + /// check function. + /// + /// The check function is given a reference to the passed resource and optional arbitrary data. + /// This is useful if you want to conditionally match on some non-path related aspect of the + /// resource type. + /// + /// Returns `true` if resource path matches this resource definition _and_ satisfies the + /// given check function. + /// + /// # Examples + /// ``` + /// use actix_router::{Path, ResourceDef}; + /// + /// fn try_match(resource: &ResourceDef, path: &mut Path<&str>) -> bool { + /// let admin_allowed = std::env::var("ADMIN_ALLOWED").ok(); + /// + /// resource.capture_match_info_fn( + /// path, + /// // when env var is not set, reject when path contains "admin" + /// &|res, admin_allowed| !res.path().contains("admin"), + /// &admin_allowed + /// ) + /// } + /// + /// let resource = ResourceDef::prefix("/user/{id}"); + /// + /// // path matches; segment values are collected into path + /// let mut path = Path::new("/user/james/stars"); + /// assert!(try_match(&resource, &mut path)); + /// assert_eq!(path.get("id").unwrap(), "james"); + /// assert_eq!(path.unprocessed(), "/stars"); + /// + /// // path matches but fails check function; no segments are collected + /// let mut path = Path::new("/user/admin/stars"); + /// assert!(!try_match(&resource, &mut path)); + /// assert_eq!(path.unprocessed(), "/user/admin/stars"); + /// ``` + pub fn capture_match_info_fn( &self, - res: &mut R, - check: &F, + resource: &mut R, + check_fn: &F, user_data: &Option, ) -> bool where - T: ResourcePath, R: Resource, + T: ResourcePath, F: Fn(&R, &Option) -> bool, { - profile_method!(match_path_checked); + profile_method!(is_path_match_fn); - let mut segments: [PathItem; MAX_DYNAMIC_SEGMENTS] = Default::default(); - let path = res.resource_path(); + let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); + let path = resource.resource_path(); + let path_str = path.path(); - let (matched_len, matched_vars) = match self.pat_type { - PatternType::Static(ref segment) => { - profile_section!(pattern_static); + let (matched_len, matched_vars) = match &self.pat_type { + PatternType::Static(_) | PatternType::Prefix(_) => { + profile_section!(pattern_static_or_prefix); - if segment != path.path() { - return false; + match self.find_match(path_str) { + Some(len) => (len, None), + None => return false, } - - (path.path().len(), None) } - PatternType::Prefix(ref prefix) => { - profile_section!(pattern_dynamic); - - let path_str = path.path(); - let path_len = path_str.len(); - - let len = { - if prefix == path_str { - // prefix length === path length - path_len - } else { - let is_slash_next = prefix.ends_with('/') - || path_str.split_at(prefix.len()).1.starts_with('/'); - - if path_str.starts_with(prefix) && is_slash_next { - if prefix.ends_with('/') { - prefix.len() - 1 - } else { - prefix.len() - } - } else { - return false; - } - } - }; - - (min(path.path().len(), len), None) - } - - PatternType::Dynamic(ref re, ref names) => { + PatternType::Dynamic(re, names) => { profile_section!(pattern_dynamic); let captures = { @@ -348,7 +759,7 @@ impl ResourceDef { (captures[0].len(), Some(names)) } - PatternType::DynamicSet(ref re, ref params) => { + PatternType::DynamicSet(re, params) => { profile_section!(pattern_dynamic_set); let path = path.path(); @@ -375,80 +786,97 @@ impl ResourceDef { } }; - if !check(res, user_data) { + if !check_fn(resource, user_data) { return false; } // Modify `path` to skip matched part and store matched segments - let path = res.resource_path(); + let path = resource.resource_path(); + if let Some(vars) = matched_vars { for i in 0..vars.len() { path.add(vars[i], mem::take(&mut segments[i])); } } + path.skip(matched_len as u16); true } - /// Builds resource path with a closure that maps variable elements' names to values. - /// - /// Unnamed tail pattern elements will receive `None`. + /// Assembles resource path using a closure that maps variable segment names to values. fn build_resource_path(&self, path: &mut String, mut vars: F) -> bool where - F: FnMut(Option<&str>) -> Option, + F: FnMut(&str) -> Option, I: AsRef, { - for el in match self.elements { - Some(ref elements) => elements, + for el in match self.segments { + Some(ref segments) => segments, None => return false, } { match *el { - PatternElement::Const(ref val) => path.push_str(val), - PatternElement::Var(ref name) => match vars(Some(name)) { + PatternSegment::Const(ref val) => path.push_str(val), + PatternSegment::Var(ref name) => match vars(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 } - /// Builds resource path from elements. Returns `true` on success. + /// Assembles full resource path from iterator of dynamic segment values. /// - /// 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 + /// 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. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let mut s = String::new(); + /// let resource = ResourceDef::new("/user/{id}/post/{title}"); + /// + /// assert!(resource.resource_path_from_iter(&mut s, &["123", "my-post"])); + /// assert_eq!(s, "/user/123/post/my-post"); + /// ``` + pub fn resource_path_from_iter(&self, path: &mut String, values: I) -> bool where - U: Iterator, - I: AsRef, + I: IntoIterator, + I::Item: AsRef, { profile_method!(resource_path_from_iter); - self.build_resource_path(path, |_| elements.next()) + let mut iter = values.into_iter(); + self.build_resource_path(path, |_| iter.next()) } - // intentionally not deprecated yet - #[doc(hidden)] - pub fn resource_path(&self, path: &mut String, elements: &mut U) -> bool - where - U: Iterator, - I: AsRef, - { - profile_method!(build_resource_path); - self.resource_path_from_iter(path, elements) - } - - /// Builds resource path from map of elements. Returns `true` on success. + /// Assembles resource path from map of dynamic segment values. /// - /// If resource pattern has an unnamed tail segment, path building will fail. + /// 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. + /// + /// # Examples + /// ``` + /// # use std::collections::HashMap; + /// # use actix_router::ResourceDef; + /// let mut s = String::new(); + /// let resource = ResourceDef::new("/user/{id}/post/{title}"); + /// + /// let mut map = HashMap::new(); + /// map.insert("id", "123"); + /// map.insert("title", "my-post"); + /// + /// assert!(resource.resource_path_from_map(&mut s, &map)); + /// assert_eq!(s, "/user/123/post/my-post"); + /// ``` pub fn resource_path_from_map( &self, path: &mut String, - elements: &HashMap, + values: &HashMap, ) -> bool where K: Borrow + Eq + Hash, @@ -456,52 +884,36 @@ impl ResourceDef { S: BuildHasher, { profile_method!(resource_path_from_map); - self.build_resource_path(path, |name| { - name.and_then(|name| elements.get(name).map(AsRef::::as_ref)) - }) + self.build_resource_path(path, |name| values.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: Borrow + Eq + Hash, - V: AsRef, - S: BuildHasher, - { - self.resource_path_from_map(path, elements) + /// Parse path pattern and create a new instance. + fn from_single_pattern(pattern: &str, is_prefix: bool) -> Self { + profile_method!(from_single_pattern); + + let pattern = pattern.to_owned(); + let (pat_type, segments) = ResourceDef::parse(&pattern, is_prefix, false); + + ResourceDef { + id: 0, + name: None, + patterns: Patterns::Single(pattern), + pat_type, + segments: Some(segments), + } } - /// Build resource path from map of elements, allowing tail segments to be appended. + /// Parses a dynamic segment definition from a pattern. /// - /// 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. + /// The returned tuple includes: + /// - the segment descriptor, either `Var` or `Tail` + /// - the segment's regex to check values against + /// - the remaining, unprocessed string slice + /// - whether the parsed parameter represents a tail pattern /// - /// 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, - { - profile_method!(resource_path_from_map_with_tail); - 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) { + /// # Panics + /// Panics if given patterns does not contain a dynamic segment. + fn parse_param(pattern: &str) -> (PatternSegment, String, &str, bool) { profile_method!(parse_param); const DEFAULT_PATTERN: &str = "[^/]+"; @@ -532,7 +944,7 @@ impl ResourceDef { let (name, pattern) = match param.find(':') { Some(idx) => { if tail { - panic!("Custom regex is not supported for remainder match"); + panic!("custom regex is not supported for tail match"); } let (name, pattern) = param.split_at(idx); @@ -549,22 +961,27 @@ impl ResourceDef { ), }; - let element = if tail { - PatternElement::Tail(Some(name.to_string())) - } else { - PatternElement::Var(name.to_string()) - }; - + let segment = PatternSegment::Var(name.to_string()); let regex = format!(r"(?P<{}>{})", &name, &pattern); - (element, regex, unprocessed) + (segment, regex, unprocessed, tail) } + /// Parse `pattern` using `is_prefix` and `force_dynamic` flags. + /// + /// Parameters: + /// - `is_prefix`: Use `true` if `pattern` should be treated as a prefix; i.e., a conforming + /// path will be a match even if it has parts remaining to process + /// - `force_dynamic`: Use `true` to disallow the return of static and prefix segments. + /// + /// The returned tuple includes: + /// - the pattern type detected, either `Static`, `Prefix`, or `Dynamic` + /// - a list of segment descriptors from the pattern fn parse( pattern: &str, - for_prefix: bool, + is_prefix: bool, force_dynamic: bool, - ) -> (PatternType, Vec) { + ) -> (PatternType, Vec) { profile_method!(parse); let mut unprocessed = pattern; @@ -572,64 +989,63 @@ impl ResourceDef { if !force_dynamic && unprocessed.find('{').is_none() && !unprocessed.ends_with('*') { // pattern is static - let tp = if for_prefix { + let tp = if is_prefix { PatternType::Prefix(unprocessed.to_owned()) } else { PatternType::Static(unprocessed.to_owned()) }; - return (tp, vec![PatternElement::Const(unprocessed.to_owned())]); + return (tp, vec![PatternSegment::Const(unprocessed.to_owned())]); } - let mut elements = Vec::new(); + let mut segments = Vec::new(); let mut re = format!("{}^", REGEX_FLAGS); - let mut dyn_elements = 0; + let mut dyn_segment_count = 0; let mut has_tail_segment = false; while let Some(idx) = unprocessed.find('{') { let (prefix, rem) = unprocessed.split_at(idx); - elements.push(PatternElement::Const(prefix.to_owned())); + segments.push(PatternSegment::Const(prefix.to_owned())); re.push_str(&escape(prefix)); - let (param_pattern, re_part, rem) = Self::parse_param(rem); + let (param_pattern, re_part, rem, tail) = Self::parse_param(rem); - if matches!(param_pattern, PatternElement::Tail(_)) { + if tail { has_tail_segment = true; } - elements.push(param_pattern); + segments.push(param_pattern); re.push_str(&re_part); unprocessed = rem; - dyn_elements += 1; + dyn_segment_count += 1; } - if let Some(path) = unprocessed.strip_suffix('*') { + if unprocessed.ends_with('*') { // unnamed tail segment - elements.push(PatternElement::Const(path.to_owned())); - elements.push(PatternElement::Tail(None)); + #[cfg(not(test))] + log::warn!("tail segments must have names; consider `{{tail}}*`"); - re.push_str(&escape(path)); - re.push_str("(.*)"); - - dyn_elements += 1; + // to this case detectable in tests + #[cfg(test)] + panic!("tail segments must have names"); } 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())); + segments.push(PatternSegment::Const(unprocessed.to_owned())); re.push_str(&escape(unprocessed)); } - if dyn_elements > MAX_DYNAMIC_SEGMENTS { + if dyn_segment_count > MAX_DYNAMIC_SEGMENTS { panic!( "Only {} dynamic segments are allowed, provided: {}", - MAX_DYNAMIC_SEGMENTS, dyn_elements + MAX_DYNAMIC_SEGMENTS, dyn_segment_count ); } - if !for_prefix && !has_tail_segment { + if !is_prefix && !has_tail_segment { re.push('$'); } @@ -647,7 +1063,7 @@ impl ResourceDef { .filter_map(|name| name.map(|name| Box::leak(Box::new(name.to_owned())).as_str())) .collect(); - (PatternType::Dynamic(re, names), elements) + (PatternType::Dynamic(re, names), segments) } } @@ -655,13 +1071,24 @@ impl Eq for ResourceDef {} impl PartialEq for ResourceDef { fn eq(&self, other: &ResourceDef) -> bool { - self.pattern == other.pattern + 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(..)) + } + } } } impl Hash for ResourceDef { fn hash(&self, state: &mut H) { - self.pattern.hash(state); + self.patterns.hash(state); } } @@ -690,15 +1117,60 @@ 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`. +/// +/// The `strict` refers to the fact that this will return `false` if `prefix == path`. +fn is_strict_prefix(prefix: &str, path: &str) -> bool { + path.starts_with(prefix) && (prefix.ends_with('/') || path[prefix.len()..].starts_with('/')) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn equivalence() { + assert_eq!( + ResourceDef::root_prefix("/root"), + ResourceDef::prefix("/root") + ); + assert_eq!( + ResourceDef::root_prefix("root"), + ResourceDef::prefix("/root") + ); + assert_eq!( + ResourceDef::root_prefix("/{id}"), + ResourceDef::prefix("/{id}") + ); + assert_eq!( + ResourceDef::root_prefix("{id}"), + ResourceDef::prefix("/{id}") + ); + + assert_eq!(ResourceDef::new("/"), ResourceDef::new(["/"])); + assert_eq!(ResourceDef::new("/"), ResourceDef::new(vec!["/"])); + assert_eq!(ResourceDef::new("/{id}*"), ResourceDef::prefix("/{id}*")); + + assert_ne!(ResourceDef::new(""), ResourceDef::prefix("")); + assert_ne!(ResourceDef::new("/"), ResourceDef::prefix("/")); + assert_ne!(ResourceDef::new("/{id}"), ResourceDef::prefix("/{id}")); + } + #[test] fn parse_static() { + let re = ResourceDef::new(""); + + assert!(!re.is_prefix()); + + assert!(re.is_match("")); + assert!(!re.is_match("/")); + assert_eq!(re.find_match(""), Some(0)); + assert_eq!(re.find_match("/"), None); + let re = ResourceDef::new("/"); assert!(re.is_match("/")); - assert!(!re.is_match("/a")); + assert!(!re.is_match("")); + assert!(!re.is_match("/foo")); let re = ResourceDef::new("/name"); assert!(re.is_match("/name")); @@ -707,13 +1179,13 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.unprocessed(), ""); - assert_eq!(re.is_prefix_match("/name"), Some(5)); - assert_eq!(re.is_prefix_match("/name1"), None); - assert_eq!(re.is_prefix_match("/name/"), None); - assert_eq!(re.is_prefix_match("/name~"), None); + assert_eq!(re.find_match("/name"), Some(5)); + assert_eq!(re.find_match("/name1"), None); + assert_eq!(re.find_match("/name/"), None); + assert_eq!(re.find_match("/name~"), None); let re = ResourceDef::new("/name/"); assert!(re.is_match("/name/")); @@ -725,7 +1197,7 @@ mod tests { assert!(!re.is_match("/user/profile/profile")); let mut path = Path::new("/user/profile"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.unprocessed(), ""); } @@ -738,12 +1210,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -753,7 +1225,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); assert_eq!(path.unprocessed(), ""); @@ -765,7 +1237,7 @@ mod tests { assert!(!re.is_match("/XXXXXX")); let mut path = Path::new("/012345"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); assert_eq!(path.unprocessed(), ""); } @@ -785,12 +1257,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -799,7 +1271,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); @@ -813,7 +1285,7 @@ mod tests { assert!(!re.is_match("/static/a")); let mut path = Path::new("/012345"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); let re = ResourceDef::new([ @@ -842,32 +1314,34 @@ mod tests { let re = ResourceDef::new("/user/-{id}*"); let mut path = Path::new("/user/-profile"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); let mut path = Path::new("/user/-2345"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/-2345/"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/"); let mut path = Path::new("/user/-2345/sdg"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/sdg"); } #[test] fn static_tail() { - let re = ResourceDef::new("/user*"); + let re = ResourceDef::new("/user{tail}*"); + assert!(re.is_match("/users")); + assert!(re.is_match("/user-foo")); 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/*"); + let re = ResourceDef::new("/user/{tail}*"); assert!(re.is_match("/user/profile")); assert!(re.is_match("/user/2345")); assert!(re.is_match("/user/2345/")); @@ -877,36 +1351,38 @@ mod tests { #[test] fn dynamic_tail() { - let re = ResourceDef::new("/user/{id}/*"); + let re = ResourceDef::new("/user/{id}/{tail}*"); assert!(!re.is_match("/user/2345")); let mut path = Path::new("/user/2345/sdg"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); + assert_eq!(path.get("tail").unwrap(), "sdg"); + assert_eq!(path.unprocessed(), ""); } #[test] - fn newline() { + fn newline_patterns_and_paths() { let re = ResourceDef::new("/user/a\nb"); assert!(re.is_match("/user/a\nb")); assert!(!re.is_match("/user/a\nb/profile")); let re = ResourceDef::new("/a{x}b/test/a{y}b"); let mut path = Path::new("/a\nb/test/a\nb"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("x").unwrap(), "\n"); assert_eq!(path.get("y").unwrap(), "\n"); - let re = ResourceDef::new("/user/*"); + let re = ResourceDef::new("/user/{tail}*"); assert!(re.is_match("/user/a\nb/")); let re = ResourceDef::new("/user/{id}*"); let mut path = Path::new("/user/a\nb/a\nb"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); let re = ResourceDef::new("/user/{id:.*}"); let mut path = Path::new("/user/a\nb/a\nb"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); } @@ -918,16 +1394,16 @@ mod tests { let re = ResourceDef::new("/user/{id}/test"); let mut path = Path::new("/user/2345/test"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/qwe%25/test"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); let uri = http::Uri::try_from("/user/qwe%25/test").unwrap(); let mut path = Path::new(uri); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); } @@ -935,64 +1411,80 @@ mod tests { fn prefix_static() { let re = ResourceDef::prefix("/name"); + assert!(re.is_prefix()); + assert!(re.is_match("/name")); assert!(re.is_match("/name/")); assert!(re.is_match("/name/test/test")); - assert!(re.is_match("/name1")); - assert!(re.is_match("/name~")); + assert!(!re.is_match("/name1")); + assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/name/test"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(path.unprocessed(), "/test"); - assert_eq!(re.is_prefix_match("/name"), Some(5)); - assert_eq!(re.is_prefix_match("/name/"), Some(5)); - assert_eq!(re.is_prefix_match("/name/test/test"), Some(5)); - assert_eq!(re.is_prefix_match("/name1"), None); - assert_eq!(re.is_prefix_match("/name~"), None); + assert_eq!(re.find_match("/name"), Some(5)); + assert_eq!(re.find_match("/name/"), Some(5)); + assert_eq!(re.find_match("/name/test/test"), Some(5)); + assert_eq!(re.find_match("/name1"), None); + assert_eq!(re.find_match("/name~"), None); let re = ResourceDef::prefix("/name/"); assert!(re.is_match("/name/")); assert!(re.is_match("/name/gs")); assert!(!re.is_match("/name")); + let mut path = Path::new("/name/gs"); + assert!(re.capture_match_info(&mut path)); + assert_eq!(path.unprocessed(), "gs"); + let re = ResourceDef::root_prefix("name/"); assert!(re.is_match("/name/")); assert!(re.is_match("/name/gs")); assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(re.match_path(&mut path)); - assert_eq!(path.unprocessed(), "/gs"); + assert!(re.capture_match_info(&mut path)); + assert_eq!(path.unprocessed(), "gs"); } #[test] fn prefix_dynamic() { let re = ResourceDef::prefix("/{name}/"); + assert!(re.is_prefix()); + assert!(re.is_match("/name/")); assert!(re.is_match("/name/gs")); assert!(!re.is_match("/name")); - assert_eq!(re.is_prefix_match("/name/"), Some(6)); - assert_eq!(re.is_prefix_match("/name/gs"), Some(6)); - assert_eq!(re.is_prefix_match("/name"), None); + assert_eq!(re.find_match("/name/"), Some(6)); + assert_eq!(re.find_match("/name/gs"), Some(6)); + assert_eq!(re.find_match("/name"), None); let mut path = Path::new("/test2/"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(&path["name"], "test2"); assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/test2/subpath1/subpath2/index.html"); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); assert_eq!(&path["name"], "test2"); assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "subpath1/subpath2/index.html"); + + let resource = ResourceDef::prefix(r"/id/{id:\d{3}}"); + assert!(resource.is_match("/id/1234")); + assert_eq!(resource.find_match("/id/1234"), Some(7)); + + let resource = ResourceDef::prefix("/user"); + // input string shorter than prefix + assert!(resource.find_match("/foo").is_none()); } #[test] @@ -1026,12 +1518,42 @@ mod tests { assert!(!resource.resource_path_from_iter(&mut s, &mut (&["item"]).iter())); let mut s = String::new(); - assert!( - resource.resource_path_from_iter(&mut s, &mut vec!["item", "item2"].into_iter()) - ); + assert!(resource.resource_path_from_iter(&mut s, &mut vec!["item", "item2"].iter())); assert_eq!(s, "/user/item/item2/"); } + #[test] + fn multi_pattern_cannot_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())); + } + + #[test] + fn multi_pattern_capture_segment_values() { + let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); + + let mut path = Path::new("/user/123"); + assert!(resource.capture_match_info(&mut path)); + assert!(path.get("id").is_some()); + + let mut path = Path::new("/profile/123"); + assert!(resource.capture_match_info(&mut path)); + assert!(path.get("id").is_some()); + + let resource = ResourceDef::new(["/user/{id}", "/profile/{uid}"]); + + let mut path = Path::new("/user/123"); + assert!(resource.capture_match_info(&mut path)); + assert!(path.get("id").is_some()); + assert!(path.get("uid").is_none()); + + let mut path = Path::new("/profile/123"); + assert!(resource.capture_match_info(&mut path)); + assert!(path.get("id").is_none()); + assert!(path.get("uid").is_some()); + } + #[test] fn build_path_map() { let resource = ResourceDef::new("/user/{item1}/{item2}/"); @@ -1051,24 +1573,6 @@ mod tests { #[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(); @@ -1086,14 +1590,51 @@ mod tests { } #[test] - fn build_path_tail_when_resource_has_no_tail() { - let resource = ResourceDef::new("/user/{item1}"); + fn consistent_match_length() { + let result = Some(5); - 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"); + let re = ResourceDef::prefix("/abc/"); + assert_eq!(re.find_match("/abc/def"), result); + + let re = ResourceDef::prefix("/{id}/"); + assert_eq!(re.find_match("/abc/def"), result); + } + + #[test] + fn match_methods_agree() { + macro_rules! match_methods_agree { + ($pat:expr => $($test:expr),+) => {{ + match_methods_agree!(finish $pat, ResourceDef::new($pat), $($test),+); + }}; + (prefix $pat:expr => $($test:expr),+) => {{ + match_methods_agree!(finish $pat, ResourceDef::prefix($pat), $($test),+); + }}; + (finish $pat:expr, $re:expr, $($test:expr),+) => {{ + let re = $re; + $({ + let _is = re.is_match($test); + let _find = re.find_match($test).is_some(); + assert_eq!( + _is, _find, + "pattern: {:?}; mismatch on \"{}\"; is={}; find={}", + $pat, $test, _is, _find + ); + })+ + }} + } + + match_methods_agree!("" => "", "/", "/foo"); + match_methods_agree!("/" => "", "/", "/foo"); + match_methods_agree!("/user" => "user", "/user", "/users", "/user/123", "/foo"); + match_methods_agree!("/v{v}" => "v", "/v", "/v1", "/v222", "/foo"); + match_methods_agree!(["/v{v}", "/version/{v}"] => "/v", "/v1", "/version", "/version/1", "/foo"); + + match_methods_agree!("/path{tail}*" => "/path", "/path1", "/path/123"); + match_methods_agree!("/path/{tail}*" => "/path", "/path1", "/path/123"); + + 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"); } #[test] @@ -1107,4 +1648,34 @@ mod tests { fn invalid_dynamic_segment_name() { ResourceDef::new("/user/{}"); } + + #[test] + #[should_panic] + fn invalid_too_many_dynamic_segments() { + // valid + ResourceDef::new("/{a}/{b}/{c}/{d}/{e}/{f}/{g}/{h}/{i}/{j}/{k}/{l}/{m}/{n}/{o}/{p}"); + + // panics + ResourceDef::new( + "/{a}/{b}/{c}/{d}/{e}/{f}/{g}/{h}/{i}/{j}/{k}/{l}/{m}/{n}/{o}/{p}/{q}", + ); + } + + #[test] + #[should_panic] + fn invalid_custom_regex_for_tail() { + ResourceDef::new(r"/{tail:\d+}*"); + } + + #[test] + #[should_panic] + fn invalid_unnamed_tail_segment() { + ResourceDef::new(r"/*"); + } + + #[test] + // #[should_panic] // TODO: consider if this should be allowed + fn prefix_plus_tail_match_is_allowed() { + ResourceDef::prefix("/user/{id}*"); + } } diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 2f345ecf..057f7e17 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -29,10 +29,11 @@ impl Router { profile_method!(recognize); for item in self.0.iter() { - if item.0.match_path(resource.resource_path()) { + if item.0.capture_match_info(resource.resource_path()) { return Some((&item.1, ResourceId(item.0.id()))); } } + None } @@ -44,18 +45,15 @@ impl Router { profile_method!(recognize_mut); for item in self.0.iter_mut() { - if item.0.match_path(resource.resource_path()) { + if item.0.capture_match_info(resource.resource_path()) { return Some((&mut item.1, ResourceId(item.0.id()))); } } + None } - pub fn recognize_checked( - &self, - resource: &mut R, - check: F, - ) -> Option<(&T, ResourceId)> + pub fn recognize_fn(&self, resource: &mut R, check: F) -> Option<(&T, ResourceId)> where F: Fn(&R, &Option) -> bool, R: Resource

, @@ -64,14 +62,15 @@ impl Router { profile_method!(recognize_checked); for item in self.0.iter() { - if item.0.match_path_checked(resource, &check, &item.2) { + if item.0.capture_match_info_fn(resource, &check, &item.2) { return Some((&item.1, ResourceId(item.0.id()))); } } + None } - pub fn recognize_mut_checked( + pub fn recognize_mut_fn( &mut self, resource: &mut R, check: F, @@ -84,10 +83,11 @@ impl Router { profile_method!(recognize_mut_checked); for item in self.0.iter_mut() { - if item.0.match_path_checked(resource, &check, &item.2) { + if item.0.capture_match_info_fn(resource, &check, &item.2) { return Some((&mut item.1, ResourceId(item.0.id()))); } } + None } } diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index f84b1778..e08a7171 100644 --- a/actix-router/src/url.rs +++ b/actix-router/src/url.rs @@ -206,7 +206,7 @@ mod tests { let re = ResourceDef::new(pattern); let uri = Uri::try_from(url.as_ref()).unwrap(); let mut path = Path::new(Url::new(uri)); - assert!(re.match_path(&mut path)); + assert!(re.capture_match_info(&mut path)); path } @@ -221,7 +221,7 @@ mod tests { let path = match_url(re, "/user/2345/test"); assert_eq!(path.get("id").unwrap(), "2345"); - // "%25" should never be decoded into '%' to gurantee the output is a valid + // "%25" should never be decoded into '%' to guarantee the output is a valid // percent-encoded format let path = match_url(re, "/user/qwe%25/test"); assert_eq!(path.get("id").unwrap(), "qwe%25"); From 82cd5b8290d70c2a4fdb1e953fea8a13555fdde7 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Tue, 20 Jul 2021 07:43:50 +0100 Subject: [PATCH 09/15] prepare router release 0.5.0-beta.1 --- actix-router/CHANGES.md | 3 +++ actix-router/Cargo.toml | 4 ++-- actix-router/src/lib.rs | 2 +- actix-router/src/router.rs | 18 ++++++++++++------ 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 8ce805bf..49248d05 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -1,6 +1,9 @@ # Changes ## Unreleased - 2021-xx-xx + + +## 0.5.0-beta.1 - 2021-07-20 * Fix a bug in multi-patterns where static patterns are interpreted as regex. [#366] * Introduce `ResourceDef::pattern_iter` to get an iterator over all patterns in a multi-pattern resource. [#373] * Fix segment interpolation leaving `Path` in unintended state after matching. [#368] diff --git a/actix-router/Cargo.toml b/actix-router/Cargo.toml index b0bcd9da..2a2ce1cc 100644 --- a/actix-router/Cargo.toml +++ b/actix-router/Cargo.toml @@ -1,12 +1,12 @@ [package] name = "actix-router" -version = "0.4.0" +version = "0.5.0-beta.1" authors = [ "Nikolay Kim ", "Ali MJ Al-Nasrawy ", "Rob Ede ", ] -description = "Resource path matching library" +description = "Resource path matching and router" keywords = ["actix", "router", "routing"] repository = "https://github.com/actix/actix-net.git" license = "MIT OR Apache-2.0" diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 6082c1a3..f08b25a8 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -1,4 +1,4 @@ -//! Resource path matching library. +//! Resource path matching and router. #![deny(rust_2018_idioms, nonstandard_style)] #![doc(html_logo_url = "https://actix.rs/img/logo.png")] diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 057f7e17..f5deb858 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -12,7 +12,11 @@ pub struct ResourceInfo { } /// Resource router. -pub struct Router(Vec<(ResourceDef, T, Option)>); +// T is the resource itself +// U is any other data needed for routing like method guards +pub struct Router { + routes: Vec<(ResourceDef, T, Option)>, +} impl Router { pub fn build() -> RouterBuilder { @@ -28,7 +32,7 @@ impl Router { { profile_method!(recognize); - for item in self.0.iter() { + for item in self.routes.iter() { if item.0.capture_match_info(resource.resource_path()) { return Some((&item.1, ResourceId(item.0.id()))); } @@ -44,7 +48,7 @@ impl Router { { profile_method!(recognize_mut); - for item in self.0.iter_mut() { + for item in self.routes.iter_mut() { if item.0.capture_match_info(resource.resource_path()) { return Some((&mut item.1, ResourceId(item.0.id()))); } @@ -61,7 +65,7 @@ impl Router { { profile_method!(recognize_checked); - for item in self.0.iter() { + for item in self.routes.iter() { if item.0.capture_match_info_fn(resource, &check, &item.2) { return Some((&item.1, ResourceId(item.0.id()))); } @@ -82,7 +86,7 @@ impl Router { { profile_method!(recognize_mut_checked); - for item in self.0.iter_mut() { + for item in self.routes.iter_mut() { if item.0.capture_match_info_fn(resource, &check, &item.2) { return Some((&mut item.1, ResourceId(item.0.id()))); } @@ -129,7 +133,9 @@ impl RouterBuilder { /// Finish configuration and create router instance. pub fn finish(self) -> Router { - Router(self.resources) + Router { + routes: self.resources, + } } } From f8f1ac94bce0309ee72a6d04ac49cd5c00e3dca2 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Tue, 20 Jul 2021 08:18:50 +0100 Subject: [PATCH 10/15] add Patterns::is_empty and impl IntoPatterns for Patterns --- actix-router/CHANGES.md | 2 +- actix-router/src/lib.rs | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 49248d05..087d67ac 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -7,7 +7,7 @@ * Fix a bug in multi-patterns where static patterns are interpreted as regex. [#366] * Introduce `ResourceDef::pattern_iter` to get an iterator over all patterns in a multi-pattern resource. [#373] * Fix segment interpolation leaving `Path` in unintended state after matching. [#368] -* Fix `ResourceDef` `PartialEq` implementation. +* Fix `ResourceDef` `PartialEq` implementation. [#373] * Re-work `IntoPatterns` trait, adding a `Patterns` enum. [#372] * Implement `IntoPatterns` for `bytestring::ByteString`. [#372] * Rename `Path::{len => segment_count}` to be more descriptive of it's purpose. [#370] diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index f08b25a8..463e59e4 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -49,6 +49,15 @@ pub enum Patterns { List(Vec), } +impl Patterns { + pub fn is_empty(&self) -> bool { + match self { + Patterns::Single(_) => false, + Patterns::List(pats) => pats.is_empty(), + } + } +} + /// Helper trait for type that could be converted to one or more path pattern. pub trait IntoPatterns { fn patterns(&self) -> Patterns; @@ -78,6 +87,12 @@ impl IntoPatterns for bytestring::ByteString { } } +impl IntoPatterns for Patterns { + fn patterns(&self) -> Patterns { + self.clone() + } +} + impl> IntoPatterns for Vec { fn patterns(&self) -> Patterns { let mut patterns = self.iter().map(|v| v.as_ref().to_owned()); From 5379a46a99e1415d68c73c0048e5f9dbb628dea3 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Fri, 6 Aug 2021 19:45:10 +0300 Subject: [PATCH 11/15] ResourceDef: relax unnecessary bounds (#381) --- actix-router/src/resource.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 58016bb3..eb0483a5 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -616,13 +616,8 @@ impl ResourceDef { profile_method!(find_match); match &self.pat_type { - PatternType::Static(segment) => { - if segment == path { - Some(segment.len()) - } else { - None - } - } + PatternType::Static(segment) if path == segment => Some(segment.len()), + PatternType::Static(_) => None, PatternType::Prefix(prefix) if path == prefix => Some(prefix.len()), PatternType::Prefix(prefix) if is_strict_prefix(prefix, path) => Some(prefix.len()), @@ -660,7 +655,7 @@ impl ResourceDef { /// ``` pub fn capture_match_info(&self, path: &mut Path) -> bool { profile_method!(is_path_match); - self.capture_match_info_fn(path, &|_, _| true, &None::<()>) + self.capture_match_info_fn(path, |_, _| true, ()) } /// Collects dynamic segment values into `resource` after matching paths and executing @@ -683,7 +678,7 @@ impl ResourceDef { /// resource.capture_match_info_fn( /// path, /// // when env var is not set, reject when path contains "admin" - /// &|res, admin_allowed| !res.path().contains("admin"), + /// |res, admin_allowed| !res.path().contains("admin"), /// &admin_allowed /// ) /// } @@ -704,13 +699,13 @@ impl ResourceDef { pub fn capture_match_info_fn( &self, resource: &mut R, - check_fn: &F, - user_data: &Option, + check_fn: F, + user_data: U, ) -> bool where R: Resource, T: ResourcePath, - F: Fn(&R, &Option) -> bool, + F: FnOnce(&R, U) -> bool, { profile_method!(is_path_match_fn); From 48b2e115099b5bfbe378f1427d8bf2897f1dc75f Mon Sep 17 00:00:00 2001 From: Aravinth Manivannan Date: Fri, 6 Aug 2021 22:36:29 +0530 Subject: [PATCH 12/15] improve malformed path error message (#384) --- actix-router/src/resource.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index eb0483a5..a3ea8a1c 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -927,7 +927,9 @@ impl ResourceDef { } _ => false, }) - .expect("malformed dynamic segment"); + .unwrap_or_else(|| { + panic!(r#"path "{}" contains malformed dynamic segment"#, pattern) + }); let (mut param, mut unprocessed) = pattern.split_at(close_idx + 1); From 430305824371047405441eeaf43db0bf840e0cc2 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 6 Aug 2021 18:25:21 +0100 Subject: [PATCH 13/15] enforce path / separators on dynamic prefixes (#378) --- actix-router/src/resource.rs | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 6 deletions(-) diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index a3ea8a1c..5c98493d 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -572,6 +572,20 @@ impl ResourceDef { PatternType::Prefix(ref prefix) if prefix == path => true, PatternType::Prefix(ref prefix) => is_strict_prefix(prefix, path), + // dynamic prefix + PatternType::Dynamic(ref re, _) if !re.as_str().ends_with('$') => { + match re.find(path) { + // prefix matches exactly + Some(m) if m.end() == path.len() => true, + + // prefix matches part + Some(m) => is_strict_prefix(m.as_str(), path), + + // prefix does not match + None => false, + } + } + PatternType::Dynamic(ref re, _) => re.is_match(path), PatternType::DynamicSet(ref re, _) => re.is_match(path), } @@ -623,6 +637,20 @@ impl ResourceDef { PatternType::Prefix(prefix) if is_strict_prefix(prefix, path) => Some(prefix.len()), PatternType::Prefix(_) => None, + // dynamic prefix + PatternType::Dynamic(ref re, _) if !re.as_str().ends_with('$') => { + match re.find(path) { + // prefix matches exactly + Some(m) if m.end() == path.len() => Some(m.end()), + + // prefix matches part + Some(m) if is_strict_prefix(m.as_str(), path) => Some(m.end()), + + // prefix does not match + _ => None, + } + } + PatternType::Dynamic(re, _) => re.find(path).map(|m| m.end()), PatternType::DynamicSet(re, params) => { @@ -654,7 +682,7 @@ impl ResourceDef { /// assert_eq!(path.unprocessed(), ""); /// ``` pub fn capture_match_info(&self, path: &mut Path) -> bool { - profile_method!(is_path_match); + profile_method!(capture_match_info); self.capture_match_info_fn(path, |_, _| true, ()) } @@ -707,7 +735,7 @@ impl ResourceDef { T: ResourcePath, F: FnOnce(&R, U) -> bool, { - profile_method!(is_path_match_fn); + profile_method!(capture_match_info_fn); let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); let path = resource.resource_path(); @@ -1475,10 +1503,6 @@ mod tests { assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "subpath1/subpath2/index.html"); - let resource = ResourceDef::prefix(r"/id/{id:\d{3}}"); - assert!(resource.is_match("/id/1234")); - assert_eq!(resource.find_match("/id/1234"), Some(7)); - let resource = ResourceDef::prefix("/user"); // input string shorter than prefix assert!(resource.find_match("/foo").is_none()); @@ -1551,6 +1575,21 @@ mod tests { assert!(path.get("uid").is_some()); } + #[test] + fn dynamic_prefix_proper_segmentation() { + let resource = ResourceDef::prefix(r"/id/{id:\d{3}}"); + + assert!(resource.is_match("/id/123")); + assert!(resource.is_match("/id/123/foo")); + assert!(!resource.is_match("/id/1234")); + assert!(!resource.is_match("/id/123a")); + + assert_eq!(resource.find_match("/id/123"), Some(7)); + assert_eq!(resource.find_match("/id/123/foo"), Some(7)); + assert_eq!(resource.find_match("/id/1234"), None); + assert_eq!(resource.find_match("/id/123a"), None); + } + #[test] fn build_path_map() { let resource = ResourceDef::new("/user/{item1}/{item2}/"); From b122a1ae1a7a6ebfde87d0e3249975a37d68dcd7 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Fri, 6 Aug 2021 20:48:30 +0300 Subject: [PATCH 14/15] ResourceDef::join (#380) --- actix-router/CHANGES.md | 3 ++ actix-router/src/resource.rs | 68 ++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 087d67ac..0ace6c8f 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -1,6 +1,9 @@ # Changes ## Unreleased - 2021-xx-xx +* Introduce `ResourceDef::join`. [#380] + +[#380]: https://github.com/actix/actix-net/pull/380 ## 0.5.0-beta.1 - 2021-07-20 diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 5c98493d..2ed1229d 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -525,6 +525,29 @@ impl ResourceDef { } } + /// Joins two resources. + /// + /// Resulting resource is prefix if `other` is prefix. + /// + /// # Examples + /// ``` + /// # use actix_router::ResourceDef; + /// let joined = ResourceDef::prefix("/root").join(&ResourceDef::prefix("/seg")); + /// assert_eq!(joined, ResourceDef::prefix("/root/seg")); + /// ``` + pub fn join(&self, other: &ResourceDef) -> ResourceDef { + let patterns = self + .pattern_iter() + .flat_map(move |this| other.pattern_iter().map(move |other| (this, other))) + .map(|(this, other)| [this, other].join("")) + .collect::>(); + + match patterns.len() { + 1 => ResourceDef::from_single_pattern(&patterns[0], other.is_prefix()), + _ => ResourceDef::new(patterns), + } + } + /// Returns `true` if `path` matches this resource. /// /// The behavior of this method depends on how the `ResourceDef` was constructed. For example, @@ -1636,6 +1659,51 @@ mod tests { assert_eq!(re.find_match("/abc/def"), result); } + #[test] + fn join() { + // test joined defs match the same paths as each component separately + + fn seq_find_match(re1: &ResourceDef, re2: &ResourceDef, path: &str) -> Option { + let len1 = re1.find_match(path)?; + let len2 = re2.find_match(&path[len1..])?; + Some(len1 + len2) + } + + macro_rules! join_test { + ($pat1:expr, $pat2:expr => $($test:expr),+) => {{ + let pat1 = $pat1; + let pat2 = $pat2; + $({ + let _path = $test; + let (re1, re2) = (ResourceDef::prefix(pat1), ResourceDef::new(pat2)); + let _seq = seq_find_match(&re1, &re2, _path); + let _join = re1.join(&re2).find_match(_path); + assert_eq!( + _seq, _join, + "patterns: prefix {:?}, {:?}; mismatch on \"{}\"; seq={:?}; join={:?}", + pat1, pat2, _path, _seq, _join + ); + assert!(!re1.join(&re2).is_prefix()); + + let (re1, re2) = (ResourceDef::prefix(pat1), ResourceDef::prefix(pat2)); + let _seq = seq_find_match(&re1, &re2, _path); + let _join = re1.join(&re2).find_match(_path); + assert_eq!( + _seq, _join, + "patterns: prefix {:?}, prefix {:?}; mismatch on \"{}\"; seq={:?}; join={:?}", + pat1, pat2, _path, _seq, _join + ); + assert!(re1.join(&re2).is_prefix()); + })+ + }} + } + + 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"); + } + #[test] fn match_methods_agree() { macro_rules! match_methods_agree { From 0183b0f8ccc63a19e263be536400c73912df4669 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 6 Aug 2021 18:48:49 +0100 Subject: [PATCH 15/15] soft-disallow prefix resources with tail segments (#379) --- actix-router/src/resource.rs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 2ed1229d..61ff587a 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1070,13 +1070,32 @@ impl ResourceDef { dyn_segment_count += 1; } + if is_prefix && has_tail_segment { + // tail segments in prefixes have no defined semantics + + #[cfg(not(test))] + log::warn!( + "Prefix resources should not have tail segments. \ + Use `ResourceDef::new` constructor. \ + This may become a panic in the future." + ); + + // panic in tests to make this case detectable + #[cfg(test)] + panic!("prefix resource definitions should not have tail segments"); + } + if unprocessed.ends_with('*') { // unnamed tail segment #[cfg(not(test))] - log::warn!("tail segments must have names; consider `{{tail}}*`"); + log::warn!( + "Tail segments must have names. \ + Consider `.../{{tail}}*`. \ + This may become a panic in the future." + ); - // to this case detectable in tests + // panic in tests to make this case detectable #[cfg(test)] panic!("tail segments must have names"); } else if !has_tail_segment && !unprocessed.is_empty() { @@ -1197,7 +1216,6 @@ mod tests { assert_eq!(ResourceDef::new("/"), ResourceDef::new(["/"])); assert_eq!(ResourceDef::new("/"), ResourceDef::new(vec!["/"])); - assert_eq!(ResourceDef::new("/{id}*"), ResourceDef::prefix("/{id}*")); assert_ne!(ResourceDef::new(""), ResourceDef::prefix("")); assert_ne!(ResourceDef::new("/"), ResourceDef::prefix("/")); @@ -1774,11 +1792,11 @@ mod tests { #[test] #[should_panic] fn invalid_unnamed_tail_segment() { - ResourceDef::new(r"/*"); + ResourceDef::new("/*"); } #[test] - // #[should_panic] // TODO: consider if this should be allowed + #[should_panic] fn prefix_plus_tail_match_is_allowed() { ResourceDef::prefix("/user/{id}*"); }