From c2d5b2398a367f83e146b6d98b71a631a082446a Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 16 Jul 2021 19:43:48 +0100 Subject: [PATCH] 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)]