diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 6305b45c3..b51cccc6e 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -2,6 +2,14 @@ ## Unreleased +### Added + +- Add conflict path detection and handling to enhance routing performance. + +### Changed + +- Refactor capture_match_info_fn by splitting it into three distinct functions: capture_match_info(), resolve_path_if_match(), and resolve(). + ## 0.5.3 - Add `unicode` crate feature (on-by-default) to switch between `regex` and `regex-lite` as a trade-off between full unicode support and binary size. diff --git a/actix-router/benches/router.rs b/actix-router/benches/router.rs index 6f6b67b48..71f40efee 100644 --- a/actix-router/benches/router.rs +++ b/actix-router/benches/router.rs @@ -179,6 +179,15 @@ fn compare_routers(c: &mut Criterion) { }); }); + group.bench_function("actix_guard_failures", |b| { + b.iter(|| { + for route in call() { + let mut path = actix_router::Path::new(route); + black_box(actix.recognize_fn(&mut path, |_, _| false)); + } + }); + }); + let regex_set = regex::RegexSet::new(register!(regex)).unwrap(); group.bench_function("regex", |b| { b.iter(|| { diff --git a/actix-router/src/de.rs b/actix-router/src/de.rs index 2f50619f8..5ce3623ef 100644 --- a/actix-router/src/de.rs +++ b/actix-router/src/de.rs @@ -662,13 +662,13 @@ mod tests { let rdef = ResourceDef::new("/{key}"); let mut path = Path::new("/%25"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); let segment: String = serde::Deserialize::deserialize(de).unwrap(); assert_eq!(segment, "%"); let mut path = Path::new("/%2F"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); let segment: String = serde::Deserialize::deserialize(de).unwrap(); assert_eq!(segment, "/") @@ -679,7 +679,7 @@ mod tests { let rdef = ResourceDef::new("/{key}/{value}"); let mut path = Path::new("/%30%25/%30%2F"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); let segment: (String, String) = serde::Deserialize::deserialize(de).unwrap(); assert_eq!(segment.0, "0%"); @@ -697,7 +697,7 @@ mod tests { let rdef = ResourceDef::new("/{key}/{value}"); let mut path = Path::new("/%25/%2F"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); let vals: Vals = serde::Deserialize::deserialize(de).unwrap(); assert_eq!(vals.key, "%"); @@ -714,7 +714,7 @@ mod tests { let rdef = ResourceDef::new("/{val}"); let mut path = Path::new("/X"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); let params: Params<'_> = serde::Deserialize::deserialize(de).unwrap(); assert_eq!(params.val, "X"); @@ -723,7 +723,7 @@ mod tests { assert_eq!(params, "X"); let mut path = Path::new("/%2F"); - rdef.capture_match_info(&mut path); + rdef.resolve_path_if_match(&mut path); let de = PathDeserializer::new(&path); assert!( as serde::Deserialize>::deserialize(de).is_err()); let de = PathDeserializer::new(&path); diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index ab4a943fe..c417b722b 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -1,14 +1,15 @@ use std::{ borrow::Cow, + mem, ops::{DerefMut, Index}, }; use serde::{de, Deserialize}; -use crate::{de::PathDeserializer, Resource, ResourcePath}; +use crate::{de::PathDeserializer, resource::ResourceMatchInfo, Resource, ResourcePath}; #[derive(Debug, Clone)] -pub(crate) enum PathItem { +pub enum PathItem { Static(Cow<'static, str>), Segment(u16, u16), } @@ -106,6 +107,27 @@ impl Path { self.skip += n; } + /// Post-processes the path to resolve dynamic segments, if any, and determines the character offset to skip. + pub fn resolve(&mut self, match_info: ResourceMatchInfo<'_>) { + match match_info { + ResourceMatchInfo::Static { matched_len } => { + self.resource_path().skip(matched_len); + } + ResourceMatchInfo::Dynamic { + matched_len, + matched_vars, + mut segments, + } => { + for i in 0..matched_vars.len() { + self.resource_path() + .add(matched_vars[i], mem::take(&mut segments[i])); + } + + self.resource_path().skip(matched_len); + } + } + } + pub(crate) fn add(&mut self, name: impl Into>, value: PathItem) { match value { PathItem::Static(seg) => self.segments.push((name.into(), PathItem::Static(seg))), @@ -260,4 +282,49 @@ mod tests { let foo = RefCell::new(foo); let _ = foo.borrow_mut().resource_path(); } + + #[test] + fn test_dynamic_path_resolve() { + let mut path = Path::new("/foo/{var1}/{var2}"); + + assert_eq!(0, path.segments.len()); + assert_eq!(0, path.skip); + + let mut segments = <[PathItem; 16]>::default(); + segments[0] = PathItem::Static(Cow::Borrowed("foo")); + segments[1] = PathItem::Segment(2, 5); + let match_info = ResourceMatchInfo::Dynamic { + matched_len: 3, + matched_vars: &["var1", "var2"], + segments, + }; + + path.resolve(match_info); + + assert_eq!(2, path.segments.len()); + assert_eq!(3, path.skip); + + let (name, value) = path.segments.get(0).unwrap(); + assert_eq!(name.as_ref(), "var1"); + assert!(matches!(value, PathItem::Static(Cow::Borrowed("foo")))); + + let (name, value) = path.segments.get(1).unwrap(); + assert_eq!(name.as_ref(), "var2"); + assert!(matches!(value, PathItem::Segment(2, 5))); + } + + #[test] + fn test_static_path_resolve() { + let mut path = Path::new("/foo"); + + assert_eq!(0, path.segments.len()); + assert_eq!(0, path.skip); + + let match_info = ResourceMatchInfo::Static { matched_len: 2 }; + + path.resolve(match_info); + + assert_eq!(0, path.segments.len()); + assert_eq!(2, path.skip); + } } diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index b5ee01958..4ae75eced 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -2,7 +2,6 @@ use std::{ borrow::{Borrow, Cow}, collections::HashMap, hash::{BuildHasher, Hash, Hasher}, - mem, }; use tracing::error; @@ -10,7 +9,7 @@ use tracing::error; use crate::{ path::PathItem, regex_set::{escape, Regex, RegexSet}, - IntoPatterns, Patterns, Resource, ResourcePath, + IntoPatterns, Patterns, Resource, }; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -80,8 +79,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// `/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]. +/// see [`capture_match_info`][Self::capture_match_info]. /// /// A resource definition can contain at most 16 dynamic segments. /// @@ -96,7 +94,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// assert!(!resource.is_match("/user/")); /// /// let mut path = Path::new("/user/123"); -/// resource.capture_match_info(&mut path); +/// resource.resolve_path_if_match(&mut path); /// assert_eq!(path.get("id").unwrap(), "123"); /// ``` /// @@ -171,7 +169,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// assert!(resource.is_match("/blob/HEAD/README.md")); /// /// let mut path = Path::new("/blob/main/LICENSE"); -/// resource.capture_match_info(&mut path); +/// resource.resolve_path_if_match(&mut path); /// assert_eq!(path.get("tail").unwrap(), "main/LICENSE"); /// ``` /// @@ -249,6 +247,18 @@ enum PatternType { DynamicSet(RegexSet, Vec<(Regex, Vec<&'static str>)>), } +/// Holds metadata and parameters used during path resolution. +pub enum ResourceMatchInfo<'a> { + Static { + matched_len: u16, + }, + Dynamic { + matched_len: u16, + matched_vars: &'a [&'static str], + segments: [PathItem; MAX_DYNAMIC_SEGMENTS], + }, +} + impl ResourceDef { /// Constructs a new resource definition from patterns. /// @@ -623,18 +633,24 @@ impl ResourceDef { /// /// let resource = ResourceDef::prefix("/user/{id}"); /// let mut path = Path::new("/user/123/stars"); - /// assert!(resource.capture_match_info(&mut path)); + /// assert!(resource.resolve_path_if_match(&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!(resource.resolve_path_if_match(&mut path)); /// assert_eq!(path.get("path").unwrap(), "HEAD/Cargo.toml"); /// assert_eq!(path.unprocessed(), ""); /// ``` - pub fn capture_match_info(&self, resource: &mut R) -> bool { - self.capture_match_info_fn(resource, |_| true) + pub fn resolve_path_if_match(&self, resource: &mut R) -> bool { + match self.capture_match_info(resource) { + None => false, + Some(match_info) => { + resource.resource_path().resolve(match_info); + true + } + } } /// Collects dynamic segment values into `resource` after matching paths and executing @@ -644,21 +660,22 @@ impl ResourceDef { /// 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. - /// + /// Returns `ResourceMatchInfo` if the given resource path matches this resource definition, + /// containing the information required to perform path resolution. /// # Examples /// ``` - /// use actix_router::{Path, ResourceDef}; + /// use actix_router::{Path, Resource, ResourceDef}; /// /// fn try_match(resource: &ResourceDef, path: &mut Path<&str>) -> bool { - /// let admin_allowed = std::env::var("ADMIN_ALLOWED").is_ok(); /// - /// resource.capture_match_info_fn( - /// path, - /// // when env var is not set, reject when path contains "admin" - /// |path| !(!admin_allowed && path.as_str().contains("admin")), - /// ) + /// let match_info = resource.capture_match_info(path); + /// match match_info{ + /// None => {false} + /// Some(match_info) => { + /// path.resource_path().resolve(match_info); + /// true + /// } + /// } /// } /// /// let resource = ResourceDef::prefix("/user/{id}"); @@ -669,85 +686,72 @@ impl ResourceDef { /// 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, resource: &mut R, check_fn: F) -> bool + pub fn capture_match_info(&self, resource: &mut R) -> Option> where R: Resource, - F: FnOnce(&R) -> bool, { - let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); let path = resource.resource_path(); let path_str = path.unprocessed(); - - let (matched_len, matched_vars) = match &self.pat_type { + match &self.pat_type { PatternType::Static(pattern) => match self.static_match(pattern, path_str) { - Some(len) => (len, None), - None => return false, + Some(len) => Some(ResourceMatchInfo::Static { + matched_len: len as u16, + }), + None => return None, }, PatternType::Dynamic(re, names) => { - let captures = match re.captures(path.unprocessed()) { + let captures = match re.captures(path_str) { Some(captures) => captures, - _ => return false, + _ => return None, }; + let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); 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 { error!("Dynamic path match but not all segments found: {}", name); - return false; + return None; } } - (captures[1].len(), Some(names)) + Some(ResourceMatchInfo::Dynamic { + matched_len: captures[1].len() as u16, + matched_vars: names, + segments, + }) } PatternType::DynamicSet(re, params) => { - let path = path.unprocessed(); - let (pattern, names) = match re.first_match_idx(path) { + let (pattern, names) = match re.first_match_idx(path_str) { Some(idx) => ¶ms[idx], - _ => return false, + _ => return None, }; - let captures = match pattern.captures(path.path()) { + let captures = match pattern.captures(path_str) { Some(captures) => captures, - _ => return false, + _ => return None, }; + let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); 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 { error!("Dynamic path match but not all segments found: {}", name); - return false; + return None; } } - (captures[1].len(), Some(names)) - } - }; - - if !check_fn(resource) { - return false; - } - - // Modify `path` to skip matched part and store matched segments - 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])); + Some(ResourceMatchInfo::Dynamic { + matched_len: captures[1].len() as u16, + matched_vars: names, + segments, + }) } } - - path.skip(matched_len as u16); - - true } /// Assembles resource path using a closure that maps variable segment names to values. @@ -1171,7 +1175,7 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); assert_eq!(re.find_match("/name"), Some(5)); @@ -1189,7 +1193,7 @@ mod tests { assert!(!re.is_match("/user/profile/profile")); let mut path = Path::new("/user/profile"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); } @@ -1202,12 +1206,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1217,7 +1221,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); assert_eq!(path.unprocessed(), ""); @@ -1229,7 +1233,7 @@ mod tests { assert!(!re.is_match("/XXXXXX")); let mut path = Path::new("/012345"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); assert_eq!(path.unprocessed(), ""); } @@ -1249,12 +1253,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1263,7 +1267,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); @@ -1277,7 +1281,7 @@ mod tests { assert!(!re.is_match("/static/a")); let mut path = Path::new("/012345"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); let re = ResourceDef::new([ @@ -1314,7 +1318,7 @@ mod tests { assert_eq!(re.find_match("/12345"), None); let mut path = Path::new("/151/res"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "151"); assert_eq!(path.unprocessed(), "/res"); } @@ -1324,19 +1328,19 @@ mod tests { let re = ResourceDef::new("/user/-{id}*"); let mut path = Path::new("/user/-profile"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); let mut path = Path::new("/user/-2345"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/-2345/"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/"); let mut path = Path::new("/user/-2345/sdg"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/sdg"); } @@ -1364,7 +1368,7 @@ mod tests { let re = ResourceDef::new("/user/{id}/{tail}*"); assert!(!re.is_match("/user/2345")); let mut path = Path::new("/user/2345/sdg"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); assert_eq!(path.get("tail").unwrap(), "sdg"); assert_eq!(path.unprocessed(), ""); @@ -1379,7 +1383,7 @@ mod tests { let re = ResourceDef::new("/a{x}b/test/a{y}b"); let mut path = Path::new("/a\nb/test/a\nb"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("x").unwrap(), "\n"); assert_eq!(path.get("y").unwrap(), "\n"); @@ -1388,12 +1392,12 @@ mod tests { let re = ResourceDef::new("/user/{id}*"); let mut path = Path::new("/user/a\nb/a\nb"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&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.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); } @@ -1403,16 +1407,16 @@ mod tests { let re = ResourceDef::new("/user/{id}/test"); let mut path = Path::new("/user/2345/test"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/qwe%25/test"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&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.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); } @@ -1429,11 +1433,11 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/name/test"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), "/test"); assert_eq!(re.find_match("/name"), Some(5)); @@ -1449,10 +1453,10 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.capture_match_info(&mut path)); + assert!(!re.resolve_path_if_match(&mut path)); let mut path = Path::new("/name//gs"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), "/gs"); let re = ResourceDef::root_prefix("name/"); @@ -1462,7 +1466,7 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.capture_match_info(&mut path)); + assert!(!re.resolve_path_if_match(&mut path)); } #[test] @@ -1481,13 +1485,13 @@ mod tests { assert_eq!(re.find_match(""), None); let mut path = Path::new("/test2/"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&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.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(&path["name"], "test2"); assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "/subpath1/subpath2/index.html"); @@ -1543,7 +1547,7 @@ mod tests { assert!(resource.resource_path_from_iter( &mut s, #[allow(clippy::useless_vec)] - &mut vec!["item", "item2"].iter() + &mut vec!["item", "item2"].iter(), )); assert_eq!(s, "/user/item/item2/"); } @@ -1561,22 +1565,22 @@ mod tests { let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); let mut path = Path::new("/user/123"); - assert!(resource.capture_match_info(&mut path)); + assert!(resource.resolve_path_if_match(&mut path)); assert!(path.get("id").is_some()); let mut path = Path::new("/profile/123"); - assert!(resource.capture_match_info(&mut path)); + assert!(resource.resolve_path_if_match(&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!(resource.resolve_path_if_match(&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!(resource.resolve_path_if_match(&mut path)); assert!(path.get("id").is_none()); assert!(path.get("uid").is_some()); } diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index b20cb7ee3..364195f4c 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -13,12 +13,16 @@ pub struct ResourceId(pub u16); /// not required. pub struct Router { routes: Vec<(ResourceDef, T, U)>, + max_path_conflicts: u16, } impl Router { /// Constructs new `RouterBuilder` with empty route list. pub fn build() -> RouterBuilder { - RouterBuilder { routes: Vec::new() } + RouterBuilder { + routes: Vec::new(), + path_conflicts: vec![], + } } /// Finds the value in the router that matches a given [routing resource](Resource). @@ -46,14 +50,24 @@ impl Router { /// the `check` closure is executed, passing the resource and each route's context data. If the /// closure returns true then the match result is stored into `resource` and a reference to /// the matched _value_ is returned. - pub fn recognize_fn(&self, resource: &mut R, mut check: F) -> Option<(&T, ResourceId)> + pub fn recognize_fn(&self, resource: &mut R, mut check_fn: F) -> Option<(&T, ResourceId)> where R: Resource, F: FnMut(&R, &U) -> bool, { + let mut next_resource_match_count = 1; for (rdef, val, ctx) in self.routes.iter() { - if rdef.capture_match_info_fn(resource, |res| check(res, ctx)) { - return Some((val, ResourceId(rdef.id()))); + match rdef.capture_match_info(resource) { + None => {} + Some(match_info) => { + if check_fn(resource, ctx) { + resource.resource_path().resolve(match_info); + return Some((val, ResourceId(rdef.id()))); + } else if next_resource_match_count == self.max_path_conflicts { + return None; + } + next_resource_match_count += 1; + } } } @@ -65,15 +79,25 @@ impl Router { pub fn recognize_mut_fn( &mut self, resource: &mut R, - mut check: F, + mut check_fn: F, ) -> Option<(&mut T, ResourceId)> where R: Resource, F: FnMut(&R, &U) -> bool, { + let mut matches = 0; for (rdef, val, ctx) in self.routes.iter_mut() { - if rdef.capture_match_info_fn(resource, |res| check(res, ctx)) { - return Some((val, ResourceId(rdef.id()))); + match rdef.capture_match_info(resource) { + None => {} + Some(match_info) => { + matches += 1; + if check_fn(resource, ctx) { + resource.resource_path().resolve(match_info); + return Some((val, ResourceId(rdef.id()))); + } else if matches == self.max_path_conflicts { + return None; + } + } } } @@ -84,6 +108,7 @@ impl Router { /// Builder for an ordered [routing](Router) list. pub struct RouterBuilder { routes: Vec<(ResourceDef, T, U)>, + path_conflicts: Vec<(usize, u16)>, } impl RouterBuilder { @@ -96,7 +121,18 @@ impl RouterBuilder { val: T, ctx: U, ) -> (&mut ResourceDef, &mut T, &mut U) { + if let Some((_, path_conflicts)) = self + .path_conflicts + .iter_mut() + .find(|(route_idx, _)| rdef.eq(&self.routes.get(*route_idx).unwrap().0)) + { + *path_conflicts += 1; + } else { + self.path_conflicts.push((self.routes.len(), 1)); + } + self.routes.push((rdef, val, ctx)); + #[allow(clippy::map_identity)] // map is used to distribute &mut-ness to tuple elements self.routes .last_mut() @@ -106,8 +142,15 @@ impl RouterBuilder { /// Finish configuration and create router instance. pub fn finish(self) -> Router { + let max_path_conflicts = self + .path_conflicts + .iter() + .map(|(_, path_conflicts)| *path_conflicts) + .max() + .unwrap_or(1); Router { routes: self.routes, + max_path_conflicts, } } } @@ -280,4 +323,42 @@ mod tests { assert_eq!(*h, 11); assert_eq!(&path["val"], "ttt"); } + + #[test] + fn test_max_path_conflicts() { + let mut router = Router::::build(); + router.path("/test", 10).0.set_id(0); + router.path("/test/{val}", 11).0.set_id(1); + let router = router.finish(); + + assert_eq!(1, router.max_path_conflicts); + + let mut router = Router::::build(); + router.path("/test", 10).0.set_id(0); + router.path("/test", 11).0.set_id(1); + router.path("/test2", 11).0.set_id(1); + router.path("/test2", 11).0.set_id(1); + router.path("/test2", 11).0.set_id(1); + + let router = router.finish(); + + assert_eq!(3, router.max_path_conflicts); + + let failures_until_fn_builder = |mut num_failures: u16| { + move |_: &Path<&str>, _: &()| { + if num_failures == 0 { + return true; + } + num_failures -= 1; + false + } + }; + + assert!(router + .recognize_fn(&mut Path::new("/test2"), failures_until_fn_builder(3)) + .is_none()); + assert!(router + .recognize_fn(&mut Path::new("/test2"), failures_until_fn_builder(2)) + .is_some()); + } } diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index b3d9e1121..fe85ea2de 100644 --- a/actix-router/src/url.rs +++ b/actix-router/src/url.rs @@ -75,7 +75,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.capture_match_info(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); path } diff --git a/actix-web/src/app_service.rs b/actix-web/src/app_service.rs index 7aa16b790..37b0e3ccf 100644 --- a/actix-web/src/app_service.rs +++ b/actix-web/src/app_service.rs @@ -306,16 +306,16 @@ impl Service for AppRouting { actix_service::always_ready!(); fn call(&self, mut req: ServiceRequest) -> Self::Future { - let res = self.router.recognize_fn(&mut req, |req, guards| { + let guards_check_fn = |req: &ServiceRequest, guards: &Vec>| { let guard_ctx = req.guard_ctx(); guards.iter().all(|guard| guard.check(&guard_ctx)) - }); + }; + let res = self.router.recognize_fn(&mut req, guards_check_fn); if let Some((srv, _info)) = res { - srv.call(req) - } else { - self.default.call(req) + return srv.call(req); } + self.default.call(req) } } diff --git a/actix-web/src/scope.rs b/actix-web/src/scope.rs index e317349da..b11e7b89f 100644 --- a/actix-web/src/scope.rs +++ b/actix-web/src/scope.rs @@ -510,16 +510,16 @@ impl Service for ScopeService { actix_service::always_ready!(); fn call(&self, mut req: ServiceRequest) -> Self::Future { - let res = self.router.recognize_fn(&mut req, |req, guards| { + let guards_check_fn = |req: &ServiceRequest, guards: &Vec>| { let guard_ctx = req.guard_ctx(); guards.iter().all(|guard| guard.check(&guard_ctx)) - }); + }; + let res = self.router.recognize_fn(&mut req, guards_check_fn); if let Some((srv, _info)) = res { - srv.call(req) - } else { - self.default.call(req) + return srv.call(req); } + self.default.call(req) } } diff --git a/actix-web/src/types/path.rs b/actix-web/src/types/path.rs index 5f22568cc..4da7ec3c8 100644 --- a/actix-web/src/types/path.rs +++ b/actix-web/src/types/path.rs @@ -176,7 +176,7 @@ mod tests { let resource = ResourceDef::new("/{value}/"); let mut req = TestRequest::with_uri("/32/").to_srv_request(); - resource.capture_match_info(req.match_info_mut()); + resource.resolve_path_if_match(req.match_info_mut()); let (req, mut pl) = req.into_parts(); assert_eq!(*Path::::from_request(&req, &mut pl).await.unwrap(), 32); @@ -189,7 +189,7 @@ mod tests { let resource = ResourceDef::new("/{key}/{value}/"); let mut req = TestRequest::with_uri("/name/user1/?id=test").to_srv_request(); - resource.capture_match_info(req.match_info_mut()); + resource.resolve_path_if_match(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let (Path(res),) = <(Path<(String, String)>,)>::from_request(&req, &mut pl) @@ -215,7 +215,7 @@ mod tests { let mut req = TestRequest::with_uri("/name/user1/?id=test").to_srv_request(); let resource = ResourceDef::new("/{key}/{value}/"); - resource.capture_match_info(req.match_info_mut()); + resource.resolve_path_if_match(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let mut s = Path::::from_request(&req, &mut pl).await.unwrap(); @@ -238,7 +238,7 @@ mod tests { let mut req = TestRequest::with_uri("/name/32/").to_srv_request(); let resource = ResourceDef::new("/{key}/{value}/"); - resource.capture_match_info(req.match_info_mut()); + resource.resolve_path_if_match(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let s = Path::::from_request(&req, &mut pl).await.unwrap(); @@ -262,7 +262,7 @@ mod tests { async fn paths_decoded() { let resource = ResourceDef::new("/{key}/{value}"); let mut req = TestRequest::with_uri("/na%2Bme/us%2Fer%254%32").to_srv_request(); - resource.capture_match_info(req.match_info_mut()); + resource.resolve_path_if_match(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let path_items = Path::::from_request(&req, &mut pl).await.unwrap();