Rename Path::{len => segment_count} (#370)

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

actix-router/Cargo.toml

@@ -1,10 +1,14 @@
 [package]
 name = "actix-router"
 version = "0.4.0"
-authors = ["Nikolay Kim <fafhrd91@gmail.com>"]
+authors = [
+    "Nikolay Kim <fafhrd91@gmail.com>",
+    "Ali MJ Al-Nasrawy <alimjalnasrawy@gmail.com>",
+    "Rob Ede <robjtede@icloud.com>",
+]
 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"
+regex = "1.5"
-serde = "1.0.104"
+serde = "1"
 bytestring = ">=0.1.5, <2"
-log = "0.4.8"
+log = "0.4"
-http = { version = "0.2.2", optional = true }
+http = { version = "0.2.3", optional = true }
 
 [dev-dependencies]
-http = "0.2.2"
+http = "0.2.3"
-serde_derive = "1.0"
+serde = { version = "1", features = ["derive"] }

actix-router/src/de.rs

@@ -24,9 +24,12 @@ 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())
+                    format!(
+                        "wrong number of parameters: {} expected 1",
+                        self.path.segment_count()
+                    )
                     .as_str(),
                 ))
             } else {
@@ -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::{de, Deserialize};
-    use serde_derive::Deserialize;
 
     use super::*;
     use crate::path::Path;

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<T> {
     path: T,
     pub(crate) skip: u16,
     pub(crate) segments: Vec<(Cow<'static, str>, PathItem)>,
 }
 
-impl<T: Default> Default for Path<T> {
-    fn default() -> Self {
-        Path {
-            path: T::default(),
-            skip: 0,
-            segments: Vec::new(),
-        }
-    }
-}
-
-impl<T: Clone> Clone for Path<T> {
-    fn clone(&self) -> Self {
-        Path {
-            path: self.path.clone(),
-            skip: self.skip,
-            segments: self.segments.clone(),
-        }
-    }
-}
-
 impl<T: ResourcePath> Path<T> {
     pub fn new(path: T) -> Path<T> {
         Path {
@@ -122,15 +102,15 @@ impl<T: ResourcePath> Path<T> {
             .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,

actix-router/src/resource.rs

@@ -1,7 +1,10 @@
-use std::cmp::min;
+use std::{
-use std::collections::HashMap;
+    borrow::Cow,
-use std::hash::{Hash, Hasher};
+    cmp::min,
-use std::mem;
+    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<Vec<PatternElement>>,
 }
 
 #[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.
+    /// Parse path pattern and create new `Pattern` instance, inserting a `/` to beginning of
-    /// Inserts `/` to begging of the pattern.
+    /// 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<usize> {
-        let p_len = path.len();
+        let path_len = path.len();
         let path = if path.is_empty() { "/" } else { path };
 
-        match self.tp {
+        match self.pat_type {
-            PatternType::Static(ref s) => {
+            PatternType::Static(ref segment) => {
-                if s == path {
+                if segment == path {
-                    Some(p_len)
+                    Some(path_len)
                 } else {
                     None
                 }
             }
-            PatternType::Dynamic(ref re, _) => re.find(path).map(|m| m.end()),
+
-            PatternType::Prefix(ref s) => {
+            PatternType::Prefix(ref prefix) => {
-                let len = if path == s {
+                let prefix_len = if path == prefix {
-                    s.len()
+                    // path length === prefix segment length
-                } else if path.starts_with(s)
+                    path_len
-                    && (s.ends_with('/') || path.split_at(s.len()).1.starts_with('/'))
-                {
-                    if s.ends_with('/') {
-                        s.len() - 1
                 } else {
-                        s.len()
+                    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 {
+        let (matched_len, matched_vars) = match self.pat_type {
-            PatternType::Static(ref s) => {
+            PatternType::Static(ref segment) => {
-                if s != path.path() {
+                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 prefix == path_str {
-                    if s == r_path {
+                        // prefix length === path length
-                        s.len()
+                        path_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()
+                        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) => &params[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 = &param[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<String> for ResourceDef {
     }
 }
 
-pub(crate) fn insert_slash(path: &str) -> String {
+pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> {
-    let mut path = path.to_owned();
     if !path.is_empty() && !path.starts_with('/') {
-        path.insert(0, '/');
+        let mut new_path = "/".to_owned();
-    };
+        new_path.push_str(path);
-    path
+        Cow::Owned(new_path)
+    } else {
+        Cow::Borrowed(path)
+    }
 }
 
 #[cfg(test)]