From ffa95b7f6079f8253345762748c243d9b3f25765 Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Wed, 14 May 2025 20:26:28 +0200 Subject: [PATCH 1/7] feat: Add improvements in resource path resolve --- actix-router/src/de.rs | 12 +-- actix-router/src/resource.rs | 176 ++++++++++++++++++++--------------- actix-router/src/router.rs | 48 ++++++++-- actix-router/src/url.rs | 2 +- actix-web/src/app_service.rs | 51 +++++----- actix-web/src/scope.rs | 144 ++++++++++++++-------------- actix-web/src/types/path.rs | 10 +- 7 files changed, 254 insertions(+), 189 deletions(-) diff --git a/actix-router/src/de.rs b/actix-router/src/de.rs index 2f50619f8..3237637bd 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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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/resource.rs b/actix-router/src/resource.rs index b5ee01958..3882140fb 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -8,9 +8,9 @@ use std::{ use tracing::error; use crate::{ + IntoPatterns, path::PathItem, - regex_set::{escape, Regex, RegexSet}, - IntoPatterns, Patterns, Resource, ResourcePath, + Patterns, regex_set::{escape, Regex, RegexSet}, Resource, ResourcePath, }; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -96,7 +96,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_resource_if_matches(&mut path); /// assert_eq!(path.get("id").unwrap(), "123"); /// ``` /// @@ -171,7 +171,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_resource_if_matches(&mut path); /// assert_eq!(path.get("tail").unwrap(), "main/LICENSE"); /// ``` /// @@ -249,6 +249,17 @@ enum PatternType { DynamicSet(RegexSet, Vec<(Regex, Vec<&'static str>)>), } +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. /// @@ -440,7 +451,7 @@ impl ResourceDef { /// assert_eq!(iter.next().unwrap(), "/root"); /// assert_eq!(iter.next().unwrap(), "/backup"); /// assert!(iter.next().is_none()); - pub fn pattern_iter(&self) -> impl Iterator { + pub fn pattern_iter(&self) -> impl Iterator { struct PatternIter<'a> { patterns: &'a Patterns, list_idx: usize, @@ -623,18 +634,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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&self, resource: &mut R) -> bool { + match self.capture_match_info(resource) { + None => { false } + Some(match_info) => { + self.resolve_resource(resource, match_info); + true + } + } } /// Collects dynamic segment values into `resource` after matching paths and executing @@ -674,80 +691,90 @@ impl ResourceDef { /// 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)) + Some(ResourceMatchInfo::Dynamic { + matched_len: captures[1].len() as u16, + matched_vars: names, + segments, + }) } - }; - - if !check_fn(resource) { - return false; } + } - // Modify `path` to skip matched part and store matched segments + + /// + pub fn resolve_resource(&self, resource: &mut R, match_info: ResourceMatchInfo<'_>) + where + R: Resource, + { let path = resource.resource_path(); + match match_info { + ResourceMatchInfo::Static { matched_len } => { + path.skip(matched_len); + } + ResourceMatchInfo::Dynamic { matched_len, matched_vars, mut segments } => { + for i in 0..matched_vars.len() { + path.add(matched_vars[i], mem::take(&mut segments[i])); + } - 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); } } - - path.skip(matched_len as u16); - - true } /// Assembles resource path using a closure that maps variable segment names to values. @@ -1118,9 +1145,10 @@ pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { #[cfg(test)] mod tests { - use super::*; use crate::Path; + use super::*; + #[test] fn equivalence() { assert_eq!( @@ -1171,7 +1199,7 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_resource_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); assert_eq!(re.find_match("/name"), Some(5)); @@ -1189,7 +1217,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_resource_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); } @@ -1202,12 +1230,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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1217,7 +1245,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_resource_if_matches(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); assert_eq!(path.unprocessed(), ""); @@ -1229,7 +1257,7 @@ mod tests { assert!(!re.is_match("/XXXXXX")); let mut path = Path::new("/012345"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); assert_eq!(path.unprocessed(), ""); } @@ -1249,12 +1277,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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1263,7 +1291,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_resource_if_matches(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); @@ -1277,7 +1305,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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); let re = ResourceDef::new([ @@ -1314,7 +1342,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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "151"); assert_eq!(path.unprocessed(), "/res"); } @@ -1324,19 +1352,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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/sdg"); } @@ -1364,7 +1392,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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); assert_eq!(path.get("tail").unwrap(), "sdg"); assert_eq!(path.unprocessed(), ""); @@ -1379,7 +1407,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_resource_if_matches(&mut path)); assert_eq!(path.get("x").unwrap(), "\n"); assert_eq!(path.get("y").unwrap(), "\n"); @@ -1388,12 +1416,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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); } @@ -1403,16 +1431,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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); } @@ -1429,11 +1457,11 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_resource_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/name/test"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_resource_if_matches(&mut path)); assert_eq!(path.unprocessed(), "/test"); assert_eq!(re.find_match("/name"), Some(5)); @@ -1449,10 +1477,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_resource_if_matches(&mut path)); let mut path = Path::new("/name//gs"); - assert!(re.capture_match_info(&mut path)); + assert!(re.resolve_resource_if_matches(&mut path)); assert_eq!(path.unprocessed(), "/gs"); let re = ResourceDef::root_prefix("name/"); @@ -1462,7 +1490,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_resource_if_matches(&mut path)); } #[test] @@ -1481,13 +1509,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_resource_if_matches(&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_resource_if_matches(&mut path)); assert_eq!(&path["name"], "test2"); assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "/subpath1/subpath2/index.html"); @@ -1543,7 +1571,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 +1589,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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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_resource_if_matches(&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..1557146cf 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -13,12 +13,13 @@ 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 +47,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 matches = 0; 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) => { + matches += 1; + if check_fn(resource, ctx) { + rdef.resolve_resource(resource, match_info); + return Some((val, ResourceId(rdef.id()))); + } else if matches == self.max_path_conflicts { + return None; + } + } } } @@ -65,15 +76,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) { + rdef.resolve_resource(resource, match_info); + return Some((val, ResourceId(rdef.id()))); + } else if matches == self.max_path_conflicts { + return None; + } + } } } @@ -84,6 +105,7 @@ impl Router { /// Builder for an ordered [routing](Router) list. pub struct RouterBuilder { routes: Vec<(ResourceDef, T, U)>, + path_conflicts: Vec<(ResourceDef, u16)>, } impl RouterBuilder { @@ -96,6 +118,13 @@ impl RouterBuilder { val: T, ctx: U, ) -> (&mut ResourceDef, &mut T, &mut U) { + if let Some((_, path_conflicts)) = self.path_conflicts.iter_mut() + .find(|(current_rdef, _)| rdef.eq(current_rdef)) { + *path_conflicts += 1; + } else { + self.path_conflicts.push((rdef.clone(), 1)); + } + self.routes.push((rdef, val, ctx)); #[allow(clippy::map_identity)] // map is used to distribute &mut-ness to tuple elements self.routes @@ -106,8 +135,13 @@ 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, } } } diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index b3d9e1121..cfdb34b60 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_resource_if_matches(&mut path)); path } diff --git a/actix-web/src/app_service.rs b/actix-web/src/app_service.rs index 7aa16b790..5eff76c95 100644 --- a/actix-web/src/app_service.rs +++ b/actix-web/src/app_service.rs @@ -1,24 +1,25 @@ use std::{cell::RefCell, mem, rc::Rc}; -use actix_http::Request; -use actix_router::{Path, ResourceDef, Router, Url}; use actix_service::{boxed, fn_service, Service, ServiceFactory}; use futures_core::future::LocalBoxFuture; use futures_util::future::join_all; +use actix_http::Request; +use actix_router::{Path, ResourceDef, Router, Url}; + use crate::{ body::BoxBody, config::{AppConfig, AppService}, data::FnDataFactory, dev::Extensions, + Error, guard::Guard, + HttpResponse, request::{HttpRequest, HttpRequestPool}, - rmap::ResourceMap, - service::{ + rmap::ResourceMap, service::{ AppServiceFactory, BoxedHttpService, BoxedHttpServiceFactory, ServiceRequest, ServiceResponse, }, - Error, HttpResponse, }; /// Service factory to convert [`Request`] to a [`ServiceRequest`]. @@ -28,10 +29,10 @@ pub struct AppInit where T: ServiceFactory< ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), >, { pub(crate) endpoint: T, @@ -47,10 +48,10 @@ impl ServiceFactory for AppInit where T: ServiceFactory< ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), >, T::Future: 'static, { @@ -144,7 +145,7 @@ where /// Wraps a service receiving a [`ServiceRequest`] into one receiving a [`Request`]. pub struct AppInitService where - T: Service, Error = Error>, + T: Service, Error=Error>, { service: T, app_data: Rc, @@ -189,7 +190,7 @@ impl AppInitServiceState { impl Service for AppInitService where - T: Service, Error = Error>, + T: Service, Error=Error>, { type Response = ServiceResponse; type Error = T::Error; @@ -229,7 +230,7 @@ where impl Drop for AppInitService where - T: Service, Error = Error>, + T: Service, Error=Error>, { fn drop(&mut self) { self.app_state.pool().clear(); @@ -306,16 +307,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) } } @@ -346,15 +347,15 @@ impl ServiceFactory for AppEntry { #[cfg(test)] mod tests { use std::sync::{ - atomic::{AtomicBool, Ordering}, Arc, + atomic::{AtomicBool, Ordering}, }; use actix_service::Service; use crate::{ - test::{init_service, TestRequest}, - web, App, HttpResponse, + App, + HttpResponse, test::{init_service, TestRequest}, web, }; struct DropData(Arc); @@ -377,7 +378,7 @@ mod tests { .data(DropData(data.clone())) .service(web::resource("/test").to(HttpResponse::Ok)), ) - .await; + .await; let req = TestRequest::with_uri("/test").to_request(); let _ = app.call(req).await.unwrap(); } diff --git a/actix-web/src/scope.rs b/actix-web/src/scope.rs index e317349da..743385cdc 100644 --- a/actix-web/src/scope.rs +++ b/actix-web/src/scope.rs @@ -1,7 +1,5 @@ use std::{cell::RefCell, fmt, future::Future, mem, rc::Rc}; -use actix_http::{body::MessageBody, Extensions}; -use actix_router::{ResourceDef, Router}; use actix_service::{ apply, apply_fn_factory, boxed, IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt, Transform, @@ -9,17 +7,20 @@ use actix_service::{ use futures_core::future::LocalBoxFuture; use futures_util::future::join_all; +use actix_http::{body::MessageBody, Extensions}; +use actix_router::{ResourceDef, Router}; + use crate::{ config::ServiceConfig, data::Data, dev::AppService, + Error, guard::Guard, - rmap::ResourceMap, - service::{ + Resource, + rmap::ResourceMap, Route, service::{ AppServiceFactory, BoxedHttpService, BoxedHttpServiceFactory, HttpServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse, }, - Error, Resource, Route, }; type Guards = Vec>; @@ -85,7 +86,7 @@ impl Scope { impl Scope where - T: ServiceFactory, + T: ServiceFactory, { /// Add match guard to a scope. /// @@ -272,8 +273,8 @@ where pub fn default_service(mut self, f: F) -> Self where F: IntoServiceFactory, - U: ServiceFactory - + 'static, + U: ServiceFactory + + 'static, U::InitError: fmt::Debug, { // create and configure default resource @@ -300,20 +301,20 @@ where ) -> Scope< impl ServiceFactory< ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), >, > where M: Transform< - T::Service, - ServiceRequest, - Response = ServiceResponse, - Error = Error, - InitError = (), - > + 'static, + T::Service, + ServiceRequest, + Response=ServiceResponse, + Error=Error, + InitError=(), + > + 'static, B: MessageBody, { Scope { @@ -343,15 +344,15 @@ where ) -> Scope< impl ServiceFactory< ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), >, > where F: Fn(ServiceRequest, &T::Service) -> R + Clone + 'static, - R: Future, Error>>, + R: Future, Error>>, B: MessageBody, { Scope { @@ -370,12 +371,12 @@ where impl HttpServiceFactory for Scope where T: ServiceFactory< - ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), - > + 'static, + ServiceRequest, + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), + > + 'static, B: MessageBody + 'static, { fn register(mut self, config: &mut AppService) { @@ -510,16 +511,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) } } @@ -552,18 +553,19 @@ mod tests { use actix_utils::future::ok; use bytes::Bytes; - use super::*; use crate::{ + App, guard, http::{ header::{self, HeaderValue}, Method, StatusCode, }, - middleware::DefaultHeaders, - test::{assert_body_eq, call_service, init_service, read_body, TestRequest}, - web, App, HttpMessage, HttpRequest, HttpResponse, + HttpMessage, + HttpRequest, HttpResponse, middleware::DefaultHeaders, test::{assert_body_eq, call_service, init_service, read_body, TestRequest}, web, }; + use super::*; + #[test] fn can_be_returned_from_fn() { fn my_scope_1() -> Scope { @@ -574,10 +576,10 @@ mod tests { fn my_scope_2() -> Scope< impl ServiceFactory< ServiceRequest, - Config = (), - Response = ServiceResponse, - Error = Error, - InitError = (), + Config=(), + Response=ServiceResponse, + Error=Error, + InitError=(), >, > { web::scope("/test-compat") @@ -604,7 +606,7 @@ mod tests { App::new() .service(web::scope("/app").service(web::resource("/path1").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -620,7 +622,7 @@ mod tests { .service(web::resource("/").to(HttpResponse::Created)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -636,7 +638,7 @@ mod tests { let srv = init_service( App::new().service(web::scope("/app/").service(web::resource("").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -653,7 +655,7 @@ mod tests { App::new() .service(web::scope("/app/").service(web::resource("/").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -673,7 +675,7 @@ mod tests { .route("/path1", web::delete().to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -703,7 +705,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -731,7 +733,7 @@ mod tests { .service(web::resource("/path1").to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1") .method(Method::POST) @@ -753,7 +755,7 @@ mod tests { HttpResponse::Ok().body(format!("project: {}", &r.match_info()["project"])) }), ))) - .await; + .await; let req = TestRequest::with_uri("/ab-project1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -770,7 +772,7 @@ mod tests { let srv = init_service(App::new().service(web::scope("/app").service( web::scope("/t1").service(web::resource("/path1").to(HttpResponse::Created)), ))) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -783,7 +785,7 @@ mod tests { init_service(App::new().service(web::scope("/app").service( web::scope("t1").service(web::resource("/path1").to(HttpResponse::Created)), ))) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -801,7 +803,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/t1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -823,7 +825,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1") .method(Method::POST) @@ -845,7 +847,7 @@ mod tests { HttpResponse::Created().body(format!("project: {}", &r.match_info()["project_id"])) })), ))) - .await; + .await; let req = TestRequest::with_uri("/app/project_1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -866,7 +868,7 @@ mod tests { }), )), ))) - .await; + .await; let req = TestRequest::with_uri("/app/test/1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -889,7 +891,7 @@ mod tests { }), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path2").to_request(); let resp = srv.call(req).await.unwrap(); @@ -910,7 +912,7 @@ mod tests { ok(r.into_response(HttpResponse::MethodNotAllowed())) }), ) - .await; + .await; let req = TestRequest::with_uri("/non-exist").to_request(); let resp = srv.call(req).await.unwrap(); @@ -937,7 +939,7 @@ mod tests { .service(web::resource("/test").route(web::get().to(HttpResponse::Ok))), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -961,7 +963,7 @@ mod tests { .service(web::resource("/test").route(web::get().to(|| async { "hello" }))), ), ) - .await; + .await; // test if `MessageBody::try_into_bytes()` is preserved across scope layer use actix_http::body::MessageBody as _; @@ -988,7 +990,7 @@ mod tests { .route("/test", web::get().to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -1023,7 +1025,7 @@ mod tests { }), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -1047,7 +1049,7 @@ mod tests { }), ), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1067,7 +1069,7 @@ mod tests { }, )), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1085,7 +1087,7 @@ mod tests { }), ), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1097,7 +1099,7 @@ mod tests { let srv = init_service(App::new().service(web::scope("/app").configure(|s| { s.route("/path1", web::get().to(HttpResponse::Ok)); }))) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1111,7 +1113,7 @@ mod tests { s.route("/", web::get().to(HttpResponse::Ok)); })); }))) - .await; + .await; let req = TestRequest::with_uri("/app/v1/").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1132,7 +1134,7 @@ mod tests { ); })); }))) - .await; + .await; let req = TestRequest::with_uri("/app/v1/").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1150,7 +1152,7 @@ mod tests { }, ))), ))) - .await; + .await; let req = TestRequest::with_uri("/a/b/c/test").to_request(); let resp = call_service(&srv, req).await; @@ -1177,7 +1179,7 @@ mod tests { ), ), ) - .await; + .await; // note the unintuitive behavior with trailing slashes on scopes with dynamic segments let req = TestRequest::with_uri("/a//b//c").to_request(); @@ -1209,7 +1211,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/a/b/c").to_request(); let resp = call_service(&srv, req).await; diff --git a/actix-web/src/types/path.rs b/actix-web/src/types/path.rs index 5f22568cc..15b0c878e 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_resource_if_matches(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_resource_if_matches(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_resource_if_matches(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_resource_if_matches(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_resource_if_matches(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let path_items = Path::::from_request(&req, &mut pl).await.unwrap(); From 934f68ebf635ac6f020838ccaced9ceb182a2506 Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Wed, 14 May 2025 20:41:45 +0200 Subject: [PATCH 2/7] feat: Add router bench for guard check failures --- actix-router/benches/router.rs | 13 +++++++++++-- actix-router/src/router.rs | 6 +++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/actix-router/benches/router.rs b/actix-router/benches/router.rs index 6f6b67b48..5b4c53cfa 100644 --- a/actix-router/benches/router.rs +++ b/actix-router/benches/router.rs @@ -1,6 +1,6 @@ //! Based on https://github.com/ibraheemdev/matchit/blob/master/benches/bench.rs -use criterion::{black_box, criterion_group, criterion_main, Criterion}; +use criterion::{black_box, Criterion, criterion_group, criterion_main}; macro_rules! register { (colon) => {{ @@ -150,7 +150,7 @@ macro_rules! register { }}; } -fn call() -> impl Iterator { +fn call() -> impl Iterator { let arr = [ "/authorizations", "/user/repos", @@ -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/router.rs b/actix-router/src/router.rs index 1557146cf..0c9e726ba 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -52,18 +52,18 @@ impl Router { R: Resource, F: FnMut(&R, &U) -> bool, { - let mut matches = 0; + let mut next_match_count = 1; for (rdef, val, ctx) in self.routes.iter() { match rdef.capture_match_info(resource) { None => {} Some(match_info) => { - matches += 1; if check_fn(resource, ctx) { rdef.resolve_resource(resource, match_info); return Some((val, ResourceId(rdef.id()))); - } else if matches == self.max_path_conflicts { + } else if next_match_count == self.max_path_conflicts { return None; } + next_match_count += 1; } } } From 55c38a625f6253eeca2a6e977f5ed66a8b4f30d5 Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Wed, 14 May 2025 20:53:22 +0200 Subject: [PATCH 3/7] feat: Move resolve path to Resource trait as default implementation --- actix-router/src/de.rs | 12 ++-- actix-router/src/resource.rs | 104 ++++++++++++------------------ actix-router/src/resource_path.rs | 19 ++++++ actix-router/src/router.rs | 4 +- actix-router/src/url.rs | 2 +- actix-web/src/types/path.rs | 10 +-- 6 files changed, 74 insertions(+), 77 deletions(-) diff --git a/actix-router/src/de.rs b/actix-router/src/de.rs index 3237637bd..5eddfa33b 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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path); + rdef.resolve_path_if_matches(&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/resource.rs b/actix-router/src/resource.rs index 3882140fb..9775d23f1 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1,8 +1,7 @@ use std::{ borrow::{Borrow, Cow}, collections::HashMap, - hash::{BuildHasher, Hash, Hasher}, - mem, + hash::{BuildHasher, Hash, Hasher} }; use tracing::error; @@ -10,7 +9,7 @@ use tracing::error; use crate::{ IntoPatterns, path::PathItem, - Patterns, regex_set::{escape, Regex, RegexSet}, Resource, ResourcePath, + Patterns, regex_set::{escape, Regex, RegexSet}, Resource, }; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -96,7 +95,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// assert!(!resource.is_match("/user/")); /// /// let mut path = Path::new("/user/123"); -/// resource.resolve_resource_if_matches(&mut path); +/// resource.resolve_path_if_matches(&mut path); /// assert_eq!(path.get("id").unwrap(), "123"); /// ``` /// @@ -171,7 +170,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// assert!(resource.is_match("/blob/HEAD/README.md")); /// /// let mut path = Path::new("/blob/main/LICENSE"); -/// resource.resolve_resource_if_matches(&mut path); +/// resource.resolve_path_if_matches(&mut path); /// assert_eq!(path.get("tail").unwrap(), "main/LICENSE"); /// ``` /// @@ -634,21 +633,21 @@ impl ResourceDef { /// /// let resource = ResourceDef::prefix("/user/{id}"); /// let mut path = Path::new("/user/123/stars"); - /// assert!(resource.resolve_resource_if_matches(&mut path)); + /// assert!(resource.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path)); + /// assert!(resource.resolve_path_if_matches(&mut path)); /// assert_eq!(path.get("path").unwrap(), "HEAD/Cargo.toml"); /// assert_eq!(path.unprocessed(), ""); /// ``` - pub fn resolve_resource_if_matches(&self, resource: &mut R) -> bool { + pub fn resolve_path_if_matches(&self, resource: &mut R) -> bool { match self.capture_match_info(resource) { None => { false } Some(match_info) => { - self.resolve_resource(resource, match_info); + resource.resolve_path(match_info); true } } @@ -756,27 +755,6 @@ impl ResourceDef { } } - - /// - pub fn resolve_resource(&self, resource: &mut R, match_info: ResourceMatchInfo<'_>) - where - R: Resource, - { - let path = resource.resource_path(); - match match_info { - ResourceMatchInfo::Static { matched_len } => { - path.skip(matched_len); - } - ResourceMatchInfo::Dynamic { matched_len, matched_vars, mut segments } => { - for i in 0..matched_vars.len() { - path.add(matched_vars[i], mem::take(&mut segments[i])); - } - - path.skip(matched_len); - } - } - } - /// 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 @@ -1199,7 +1177,7 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); assert_eq!(re.find_match("/name"), Some(5)); @@ -1217,7 +1195,7 @@ mod tests { assert!(!re.is_match("/user/profile/profile")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); } @@ -1230,12 +1208,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1245,7 +1223,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); assert_eq!(path.unprocessed(), ""); @@ -1257,7 +1235,7 @@ mod tests { assert!(!re.is_match("/XXXXXX")); let mut path = Path::new("/012345"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); assert_eq!(path.unprocessed(), ""); } @@ -1277,12 +1255,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/user/1245125"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1291,7 +1269,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); @@ -1305,7 +1283,7 @@ mod tests { assert!(!re.is_match("/static/a")); let mut path = Path::new("/012345"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); let re = ResourceDef::new([ @@ -1342,7 +1320,7 @@ mod tests { assert_eq!(re.find_match("/12345"), None); let mut path = Path::new("/151/res"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "151"); assert_eq!(path.unprocessed(), "/res"); } @@ -1352,19 +1330,19 @@ mod tests { let re = ResourceDef::new("/user/-{id}*"); let mut path = Path::new("/user/-profile"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "profile"); let mut path = Path::new("/user/-2345"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/-2345/"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/"); let mut path = Path::new("/user/-2345/sdg"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/sdg"); } @@ -1392,7 +1370,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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); assert_eq!(path.get("tail").unwrap(), "sdg"); assert_eq!(path.unprocessed(), ""); @@ -1407,7 +1385,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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("x").unwrap(), "\n"); assert_eq!(path.get("y").unwrap(), "\n"); @@ -1416,12 +1394,12 @@ mod tests { let re = ResourceDef::new("/user/{id}*"); let mut path = Path::new("/user/a\nb/a\nb"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); } @@ -1431,16 +1409,16 @@ mod tests { let re = ResourceDef::new("/user/{id}/test"); let mut path = Path::new("/user/2345/test"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "2345"); let mut path = Path::new("/user/qwe%25/test"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); } @@ -1457,11 +1435,11 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/name/test"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.unprocessed(), "/test"); assert_eq!(re.find_match("/name"), Some(5)); @@ -1477,10 +1455,10 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.resolve_resource_if_matches(&mut path)); + assert!(!re.resolve_path_if_matches(&mut path)); let mut path = Path::new("/name//gs"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(path.unprocessed(), "/gs"); let re = ResourceDef::root_prefix("name/"); @@ -1490,7 +1468,7 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.resolve_resource_if_matches(&mut path)); + assert!(!re.resolve_path_if_matches(&mut path)); } #[test] @@ -1509,13 +1487,13 @@ mod tests { assert_eq!(re.find_match(""), None); let mut path = Path::new("/test2/"); - assert!(re.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); assert_eq!(&path["name"], "test2"); assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "/subpath1/subpath2/index.html"); @@ -1589,22 +1567,22 @@ mod tests { let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); let mut path = Path::new("/user/123"); - assert!(resource.resolve_resource_if_matches(&mut path)); + assert!(resource.resolve_path_if_matches(&mut path)); assert!(path.get("id").is_some()); let mut path = Path::new("/profile/123"); - assert!(resource.resolve_resource_if_matches(&mut path)); + assert!(resource.resolve_path_if_matches(&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.resolve_resource_if_matches(&mut path)); + assert!(resource.resolve_path_if_matches(&mut path)); assert!(path.get("id").is_some()); assert!(path.get("uid").is_none()); let mut path = Path::new("/profile/123"); - assert!(resource.resolve_resource_if_matches(&mut path)); + assert!(resource.resolve_path_if_matches(&mut path)); assert!(path.get("id").is_none()); assert!(path.get("uid").is_some()); } diff --git a/actix-router/src/resource_path.rs b/actix-router/src/resource_path.rs index 610dc344d..3fe354105 100644 --- a/actix-router/src/resource_path.rs +++ b/actix-router/src/resource_path.rs @@ -1,4 +1,7 @@ +use std::mem; + use crate::Path; +use crate::resource::ResourceMatchInfo; // TODO: this trait is necessary, document it // see impl Resource for ServiceRequest @@ -7,6 +10,22 @@ pub trait Resource { type Path: ResourcePath; fn resource_path(&mut self) -> &mut Path; + + fn resolve_path(&mut self, match_info: ResourceMatchInfo<'_>) { + let path = self.resource_path(); + match match_info { + ResourceMatchInfo::Static { matched_len } => { + path.skip(matched_len); + } + ResourceMatchInfo::Dynamic { matched_len, matched_vars, mut segments } => { + for i in 0..matched_vars.len() { + path.add(matched_vars[i], mem::take(&mut segments[i])); + } + + path.skip(matched_len); + } + } + } } pub trait ResourcePath { diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 0c9e726ba..f84b5e277 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -58,7 +58,7 @@ impl Router { None => {} Some(match_info) => { if check_fn(resource, ctx) { - rdef.resolve_resource(resource, match_info); + resource.resolve_path(match_info); return Some((val, ResourceId(rdef.id()))); } else if next_match_count == self.max_path_conflicts { return None; @@ -89,7 +89,7 @@ impl Router { Some(match_info) => { matches += 1; if check_fn(resource, ctx) { - rdef.resolve_resource(resource, match_info); + resource.resolve_path(match_info); return Some((val, ResourceId(rdef.id()))); } else if matches == self.max_path_conflicts { return None; diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index cfdb34b60..8baed700e 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.resolve_resource_if_matches(&mut path)); + assert!(re.resolve_path_if_matches(&mut path)); path } diff --git a/actix-web/src/types/path.rs b/actix-web/src/types/path.rs index 15b0c878e..bb34d1f28 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.resolve_resource_if_matches(req.match_info_mut()); + resource.resolve_path_if_matches(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.resolve_resource_if_matches(req.match_info_mut()); + resource.resolve_path_if_matches(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.resolve_resource_if_matches(req.match_info_mut()); + resource.resolve_path_if_matches(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.resolve_resource_if_matches(req.match_info_mut()); + resource.resolve_path_if_matches(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.resolve_resource_if_matches(req.match_info_mut()); + resource.resolve_path_if_matches(req.match_info_mut()); let (req, mut pl) = req.into_parts(); let path_items = Path::::from_request(&req, &mut pl).await.unwrap(); From b47530d66a7b40219f37b8f2ea7effefd48b36c7 Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Wed, 14 May 2025 21:22:09 +0200 Subject: [PATCH 4/7] feat: Add tests for max path conflicts modification --- actix-router/src/router.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index f84b5e277..1e1b2154d 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -314,4 +314,38 @@ 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()); + } } From 9f2b2071378ee1aee928f9d4018e7f406bb53bba Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Wed, 14 May 2025 21:34:50 +0200 Subject: [PATCH 5/7] feat: Add fix to visibility warning and apply code format --- actix-router/benches/router.rs | 4 +- actix-router/src/path.rs | 2 +- actix-router/src/resource.rs | 14 ++-- actix-router/src/resource_path.rs | 8 +- actix-router/src/router.rs | 24 ++++-- actix-web/src/app_service.rs | 36 ++++----- actix-web/src/scope.rs | 126 +++++++++++++++--------------- 7 files changed, 116 insertions(+), 98 deletions(-) diff --git a/actix-router/benches/router.rs b/actix-router/benches/router.rs index 5b4c53cfa..71f40efee 100644 --- a/actix-router/benches/router.rs +++ b/actix-router/benches/router.rs @@ -1,6 +1,6 @@ //! Based on https://github.com/ibraheemdev/matchit/blob/master/benches/bench.rs -use criterion::{black_box, Criterion, criterion_group, criterion_main}; +use criterion::{black_box, criterion_group, criterion_main, Criterion}; macro_rules! register { (colon) => {{ @@ -150,7 +150,7 @@ macro_rules! register { }}; } -fn call() -> impl Iterator { +fn call() -> impl Iterator { let arr = [ "/authorizations", "/user/repos", diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index ab4a943fe..f18a102fe 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -8,7 +8,7 @@ use serde::{de, Deserialize}; use crate::{de::PathDeserializer, Resource, ResourcePath}; #[derive(Debug, Clone)] -pub(crate) enum PathItem { +pub enum PathItem { Static(Cow<'static, str>), Segment(u16, u16), } diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 9775d23f1..9c49cc802 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1,15 +1,15 @@ use std::{ borrow::{Borrow, Cow}, collections::HashMap, - hash::{BuildHasher, Hash, Hasher} + hash::{BuildHasher, Hash, Hasher}, }; use tracing::error; use crate::{ - IntoPatterns, path::PathItem, - Patterns, regex_set::{escape, Regex, RegexSet}, Resource, + regex_set::{escape, Regex, RegexSet}, + IntoPatterns, Patterns, Resource, }; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -450,7 +450,7 @@ impl ResourceDef { /// assert_eq!(iter.next().unwrap(), "/root"); /// assert_eq!(iter.next().unwrap(), "/backup"); /// assert!(iter.next().is_none()); - pub fn pattern_iter(&self) -> impl Iterator { + pub fn pattern_iter(&self) -> impl Iterator { struct PatternIter<'a> { patterns: &'a Patterns, list_idx: usize, @@ -645,7 +645,7 @@ impl ResourceDef { /// ``` pub fn resolve_path_if_matches(&self, resource: &mut R) -> bool { match self.capture_match_info(resource) { - None => { false } + None => false, Some(match_info) => { resource.resolve_path(match_info); true @@ -698,7 +698,9 @@ impl ResourceDef { let path_str = path.unprocessed(); match &self.pat_type { PatternType::Static(pattern) => match self.static_match(pattern, path_str) { - Some(len) => Some(ResourceMatchInfo::Static { matched_len: len as u16 }), + Some(len) => Some(ResourceMatchInfo::Static { + matched_len: len as u16, + }), None => return None, }, diff --git a/actix-router/src/resource_path.rs b/actix-router/src/resource_path.rs index 3fe354105..60ca60aa1 100644 --- a/actix-router/src/resource_path.rs +++ b/actix-router/src/resource_path.rs @@ -1,7 +1,7 @@ use std::mem; -use crate::Path; use crate::resource::ResourceMatchInfo; +use crate::Path; // TODO: this trait is necessary, document it // see impl Resource for ServiceRequest @@ -17,7 +17,11 @@ pub trait Resource { ResourceMatchInfo::Static { matched_len } => { path.skip(matched_len); } - ResourceMatchInfo::Dynamic { matched_len, matched_vars, mut segments } => { + ResourceMatchInfo::Dynamic { + matched_len, + matched_vars, + mut segments, + } => { for i in 0..matched_vars.len() { path.add(matched_vars[i], mem::take(&mut segments[i])); } diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 1e1b2154d..cfcab8546 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -19,7 +19,10 @@ pub struct Router { impl Router { /// Constructs new `RouterBuilder` with empty route list. pub fn build() -> RouterBuilder { - RouterBuilder { routes: Vec::new(), path_conflicts: vec![] } + RouterBuilder { + routes: Vec::new(), + path_conflicts: vec![], + } } /// Finds the value in the router that matches a given [routing resource](Resource). @@ -118,8 +121,11 @@ impl RouterBuilder { val: T, ctx: U, ) -> (&mut ResourceDef, &mut T, &mut U) { - if let Some((_, path_conflicts)) = self.path_conflicts.iter_mut() - .find(|(current_rdef, _)| rdef.eq(current_rdef)) { + if let Some((_, path_conflicts)) = self + .path_conflicts + .iter_mut() + .find(|(current_rdef, _)| rdef.eq(current_rdef)) + { *path_conflicts += 1; } else { self.path_conflicts.push((rdef.clone(), 1)); @@ -135,7 +141,9 @@ impl RouterBuilder { /// Finish configuration and create router instance. pub fn finish(self) -> Router { - let max_path_conflicts = self.path_conflicts.iter() + let max_path_conflicts = self + .path_conflicts + .iter() .map(|(_, path_conflicts)| *path_conflicts) .max() .unwrap_or(1); @@ -345,7 +353,11 @@ mod tests { } }; - 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()); + 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-web/src/app_service.rs b/actix-web/src/app_service.rs index 5eff76c95..a7f0ab274 100644 --- a/actix-web/src/app_service.rs +++ b/actix-web/src/app_service.rs @@ -12,14 +12,14 @@ use crate::{ config::{AppConfig, AppService}, data::FnDataFactory, dev::Extensions, - Error, guard::Guard, - HttpResponse, request::{HttpRequest, HttpRequestPool}, - rmap::ResourceMap, service::{ + rmap::ResourceMap, + service::{ AppServiceFactory, BoxedHttpService, BoxedHttpServiceFactory, ServiceRequest, ServiceResponse, }, + Error, HttpResponse, }; /// Service factory to convert [`Request`] to a [`ServiceRequest`]. @@ -29,10 +29,10 @@ pub struct AppInit where T: ServiceFactory< ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), >, { pub(crate) endpoint: T, @@ -48,10 +48,10 @@ impl ServiceFactory for AppInit where T: ServiceFactory< ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), >, T::Future: 'static, { @@ -145,7 +145,7 @@ where /// Wraps a service receiving a [`ServiceRequest`] into one receiving a [`Request`]. pub struct AppInitService where - T: Service, Error=Error>, + T: Service, Error = Error>, { service: T, app_data: Rc, @@ -190,7 +190,7 @@ impl AppInitServiceState { impl Service for AppInitService where - T: Service, Error=Error>, + T: Service, Error = Error>, { type Response = ServiceResponse; type Error = T::Error; @@ -230,7 +230,7 @@ where impl Drop for AppInitService where - T: Service, Error=Error>, + T: Service, Error = Error>, { fn drop(&mut self) { self.app_state.pool().clear(); @@ -347,15 +347,15 @@ impl ServiceFactory for AppEntry { #[cfg(test)] mod tests { use std::sync::{ - Arc, atomic::{AtomicBool, Ordering}, + Arc, }; use actix_service::Service; use crate::{ - App, - HttpResponse, test::{init_service, TestRequest}, web, + test::{init_service, TestRequest}, + web, App, HttpResponse, }; struct DropData(Arc); @@ -378,7 +378,7 @@ mod tests { .data(DropData(data.clone())) .service(web::resource("/test").to(HttpResponse::Ok)), ) - .await; + .await; let req = TestRequest::with_uri("/test").to_request(); let _ = app.call(req).await.unwrap(); } diff --git a/actix-web/src/scope.rs b/actix-web/src/scope.rs index 743385cdc..5edf1ef1a 100644 --- a/actix-web/src/scope.rs +++ b/actix-web/src/scope.rs @@ -14,13 +14,13 @@ use crate::{ config::ServiceConfig, data::Data, dev::AppService, - Error, guard::Guard, - Resource, - rmap::ResourceMap, Route, service::{ + rmap::ResourceMap, + service::{ AppServiceFactory, BoxedHttpService, BoxedHttpServiceFactory, HttpServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse, }, + Error, Resource, Route, }; type Guards = Vec>; @@ -86,7 +86,7 @@ impl Scope { impl Scope where - T: ServiceFactory, + T: ServiceFactory, { /// Add match guard to a scope. /// @@ -273,8 +273,8 @@ where pub fn default_service(mut self, f: F) -> Self where F: IntoServiceFactory, - U: ServiceFactory - + 'static, + U: ServiceFactory + + 'static, U::InitError: fmt::Debug, { // create and configure default resource @@ -301,20 +301,20 @@ where ) -> Scope< impl ServiceFactory< ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), >, > where M: Transform< - T::Service, - ServiceRequest, - Response=ServiceResponse, - Error=Error, - InitError=(), - > + 'static, + T::Service, + ServiceRequest, + Response = ServiceResponse, + Error = Error, + InitError = (), + > + 'static, B: MessageBody, { Scope { @@ -344,15 +344,15 @@ where ) -> Scope< impl ServiceFactory< ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), >, > where F: Fn(ServiceRequest, &T::Service) -> R + Clone + 'static, - R: Future, Error>>, + R: Future, Error>>, B: MessageBody, { Scope { @@ -371,12 +371,12 @@ where impl HttpServiceFactory for Scope where T: ServiceFactory< - ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), - > + 'static, + ServiceRequest, + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), + > + 'static, B: MessageBody + 'static, { fn register(mut self, config: &mut AppService) { @@ -554,14 +554,14 @@ mod tests { use bytes::Bytes; use crate::{ - App, guard, http::{ header::{self, HeaderValue}, Method, StatusCode, }, - HttpMessage, - HttpRequest, HttpResponse, middleware::DefaultHeaders, test::{assert_body_eq, call_service, init_service, read_body, TestRequest}, web, + middleware::DefaultHeaders, + test::{assert_body_eq, call_service, init_service, read_body, TestRequest}, + web, App, HttpMessage, HttpRequest, HttpResponse, }; use super::*; @@ -576,10 +576,10 @@ mod tests { fn my_scope_2() -> Scope< impl ServiceFactory< ServiceRequest, - Config=(), - Response=ServiceResponse, - Error=Error, - InitError=(), + Config = (), + Response = ServiceResponse, + Error = Error, + InitError = (), >, > { web::scope("/test-compat") @@ -606,7 +606,7 @@ mod tests { App::new() .service(web::scope("/app").service(web::resource("/path1").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -622,7 +622,7 @@ mod tests { .service(web::resource("/").to(HttpResponse::Created)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -638,7 +638,7 @@ mod tests { let srv = init_service( App::new().service(web::scope("/app/").service(web::resource("").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -655,7 +655,7 @@ mod tests { App::new() .service(web::scope("/app/").service(web::resource("/").to(HttpResponse::Ok))), ) - .await; + .await; let req = TestRequest::with_uri("/app").to_request(); let resp = srv.call(req).await.unwrap(); @@ -675,7 +675,7 @@ mod tests { .route("/path1", web::delete().to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -705,7 +705,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -733,7 +733,7 @@ mod tests { .service(web::resource("/path1").to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path1") .method(Method::POST) @@ -755,7 +755,7 @@ mod tests { HttpResponse::Ok().body(format!("project: {}", &r.match_info()["project"])) }), ))) - .await; + .await; let req = TestRequest::with_uri("/ab-project1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -772,7 +772,7 @@ mod tests { let srv = init_service(App::new().service(web::scope("/app").service( web::scope("/t1").service(web::resource("/path1").to(HttpResponse::Created)), ))) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -785,7 +785,7 @@ mod tests { init_service(App::new().service(web::scope("/app").service( web::scope("t1").service(web::resource("/path1").to(HttpResponse::Created)), ))) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -803,7 +803,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/t1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -825,7 +825,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/t1/path1") .method(Method::POST) @@ -847,7 +847,7 @@ mod tests { HttpResponse::Created().body(format!("project: {}", &r.match_info()["project_id"])) })), ))) - .await; + .await; let req = TestRequest::with_uri("/app/project_1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -868,7 +868,7 @@ mod tests { }), )), ))) - .await; + .await; let req = TestRequest::with_uri("/app/test/1/path1").to_request(); let res = srv.call(req).await.unwrap(); @@ -891,7 +891,7 @@ mod tests { }), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/path2").to_request(); let resp = srv.call(req).await.unwrap(); @@ -912,7 +912,7 @@ mod tests { ok(r.into_response(HttpResponse::MethodNotAllowed())) }), ) - .await; + .await; let req = TestRequest::with_uri("/non-exist").to_request(); let resp = srv.call(req).await.unwrap(); @@ -939,7 +939,7 @@ mod tests { .service(web::resource("/test").route(web::get().to(HttpResponse::Ok))), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -963,7 +963,7 @@ mod tests { .service(web::resource("/test").route(web::get().to(|| async { "hello" }))), ), ) - .await; + .await; // test if `MessageBody::try_into_bytes()` is preserved across scope layer use actix_http::body::MessageBody as _; @@ -990,7 +990,7 @@ mod tests { .route("/test", web::get().to(HttpResponse::Ok)), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -1025,7 +1025,7 @@ mod tests { }), ), ) - .await; + .await; let req = TestRequest::with_uri("/app/test").to_request(); let resp = call_service(&srv, req).await; @@ -1049,7 +1049,7 @@ mod tests { }), ), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1069,7 +1069,7 @@ mod tests { }, )), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1087,7 +1087,7 @@ mod tests { }), ), )) - .await; + .await; let req = TestRequest::with_uri("/app/t").to_request(); let resp = call_service(&srv, req).await; @@ -1099,7 +1099,7 @@ mod tests { let srv = init_service(App::new().service(web::scope("/app").configure(|s| { s.route("/path1", web::get().to(HttpResponse::Ok)); }))) - .await; + .await; let req = TestRequest::with_uri("/app/path1").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1113,7 +1113,7 @@ mod tests { s.route("/", web::get().to(HttpResponse::Ok)); })); }))) - .await; + .await; let req = TestRequest::with_uri("/app/v1/").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1134,7 +1134,7 @@ mod tests { ); })); }))) - .await; + .await; let req = TestRequest::with_uri("/app/v1/").to_request(); let resp = srv.call(req).await.unwrap(); @@ -1152,7 +1152,7 @@ mod tests { }, ))), ))) - .await; + .await; let req = TestRequest::with_uri("/a/b/c/test").to_request(); let resp = call_service(&srv, req).await; @@ -1179,7 +1179,7 @@ mod tests { ), ), ) - .await; + .await; // note the unintuitive behavior with trailing slashes on scopes with dynamic segments let req = TestRequest::with_uri("/a//b//c").to_request(); @@ -1211,7 +1211,7 @@ mod tests { ), ), ) - .await; + .await; let req = TestRequest::with_uri("/a/b/c").to_request(); let resp = call_service(&srv, req).await; From 4e56ae0594a3aef9e50c8ff45fdc477808ca945b Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Fri, 16 May 2025 21:46:38 +0200 Subject: [PATCH 6/7] feat: Add minor refactos and changelog --- actix-router/CHANGES.md | 8 +++ actix-router/src/de.rs | 12 ++-- actix-router/src/path.rs | 68 +++++++++++++++++++ actix-router/src/resource.rs | 107 +++++++++++++++--------------- actix-router/src/resource_path.rs | 23 ------- actix-router/src/router.rs | 21 +++--- actix-router/src/url.rs | 2 +- actix-web/src/types/path.rs | 10 +-- 8 files changed, 150 insertions(+), 101 deletions(-) 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/src/de.rs b/actix-router/src/de.rs index 5eddfa33b..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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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 f18a102fe..2a7a76fc5 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -1,10 +1,12 @@ use std::{ borrow::Cow, + mem, ops::{DerefMut, Index}, }; use serde::{de, Deserialize}; +use crate::resource::ResourceMatchInfo; use crate::{de::PathDeserializer, Resource, ResourcePath}; #[derive(Debug, Clone)] @@ -106,6 +108,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 +283,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 9c49cc802..3bd01ed2a 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -79,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. /// @@ -95,7 +94,7 @@ const REGEX_FLAGS: &str = "(?s-m)"; /// assert!(!resource.is_match("/user/")); /// /// let mut path = Path::new("/user/123"); -/// resource.resolve_path_if_matches(&mut path); +/// resource.resolve_path_if_match(&mut path); /// assert_eq!(path.get("id").unwrap(), "123"); /// ``` /// @@ -170,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.resolve_path_if_matches(&mut path); +/// resource.resolve_path_if_match(&mut path); /// assert_eq!(path.get("tail").unwrap(), "main/LICENSE"); /// ``` /// @@ -248,6 +247,7 @@ 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, @@ -633,21 +633,21 @@ impl ResourceDef { /// /// let resource = ResourceDef::prefix("/user/{id}"); /// let mut path = Path::new("/user/123/stars"); - /// assert!(resource.resolve_path_if_matches(&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.resolve_path_if_matches(&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 resolve_path_if_matches(&self, resource: &mut R) -> bool { + pub fn resolve_path_if_match(&self, resource: &mut R) -> bool { match self.capture_match_info(resource) { None => false, Some(match_info) => { - resource.resolve_path(match_info); + resource.resource_path().resolve(match_info); true } } @@ -660,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}"); @@ -685,10 +686,6 @@ 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(&self, resource: &mut R) -> Option> where @@ -1179,7 +1176,7 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); assert_eq!(re.find_match("/name"), Some(5)); @@ -1197,7 +1194,7 @@ mod tests { assert!(!re.is_match("/user/profile/profile")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); } @@ -1210,12 +1207,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1225,7 +1222,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.resolve_path_if_matches(&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(), ""); @@ -1237,7 +1234,7 @@ mod tests { assert!(!re.is_match("/XXXXXX")); let mut path = Path::new("/012345"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); assert_eq!(path.unprocessed(), ""); } @@ -1257,12 +1254,12 @@ mod tests { assert!(!re.is_match("/user/2345/sdg")); let mut path = Path::new("/user/profile"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "1245125"); assert_eq!(path.unprocessed(), ""); @@ -1271,7 +1268,7 @@ mod tests { assert!(!re.is_match("/resource")); let mut path = Path::new("/v151/resource/adage32"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("version").unwrap(), "151"); assert_eq!(path.get("id").unwrap(), "adage32"); @@ -1285,7 +1282,7 @@ mod tests { assert!(!re.is_match("/static/a")); let mut path = Path::new("/012345"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "012345"); let re = ResourceDef::new([ @@ -1322,7 +1319,7 @@ mod tests { assert_eq!(re.find_match("/12345"), None); let mut path = Path::new("/151/res"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "151"); assert_eq!(path.unprocessed(), "/res"); } @@ -1332,19 +1329,19 @@ mod tests { let re = ResourceDef::new("/user/-{id}*"); let mut path = Path::new("/user/-profile"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "2345/sdg"); } @@ -1372,7 +1369,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.resolve_path_if_matches(&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(), ""); @@ -1387,7 +1384,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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("x").unwrap(), "\n"); assert_eq!(path.get("y").unwrap(), "\n"); @@ -1396,12 +1393,12 @@ mod tests { let re = ResourceDef::new("/user/{id}*"); let mut path = Path::new("/user/a\nb/a\nb"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "a\nb/a\nb"); } @@ -1411,16 +1408,16 @@ mod tests { let re = ResourceDef::new("/user/{id}/test"); let mut path = Path::new("/user/2345/test"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.get("id").unwrap(), "qwe%25"); } @@ -1437,11 +1434,11 @@ mod tests { assert!(!re.is_match("/name~")); let mut path = Path::new("/name"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), ""); let mut path = Path::new("/name/test"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), "/test"); assert_eq!(re.find_match("/name"), Some(5)); @@ -1457,10 +1454,10 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.resolve_path_if_matches(&mut path)); + assert!(!re.resolve_path_if_match(&mut path)); let mut path = Path::new("/name//gs"); - assert!(re.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); assert_eq!(path.unprocessed(), "/gs"); let re = ResourceDef::root_prefix("name/"); @@ -1470,7 +1467,7 @@ mod tests { assert!(!re.is_match("/name")); let mut path = Path::new("/name/gs"); - assert!(!re.resolve_path_if_matches(&mut path)); + assert!(!re.resolve_path_if_match(&mut path)); } #[test] @@ -1489,13 +1486,13 @@ mod tests { assert_eq!(re.find_match(""), None); let mut path = Path::new("/test2/"); - assert!(re.resolve_path_if_matches(&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.resolve_path_if_matches(&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"); @@ -1569,22 +1566,22 @@ mod tests { let resource = ResourceDef::new(["/user/{id}", "/profile/{id}"]); let mut path = Path::new("/user/123"); - assert!(resource.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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.resolve_path_if_matches(&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/resource_path.rs b/actix-router/src/resource_path.rs index 60ca60aa1..610dc344d 100644 --- a/actix-router/src/resource_path.rs +++ b/actix-router/src/resource_path.rs @@ -1,6 +1,3 @@ -use std::mem; - -use crate::resource::ResourceMatchInfo; use crate::Path; // TODO: this trait is necessary, document it @@ -10,26 +7,6 @@ pub trait Resource { type Path: ResourcePath; fn resource_path(&mut self) -> &mut Path; - - fn resolve_path(&mut self, match_info: ResourceMatchInfo<'_>) { - let path = self.resource_path(); - match match_info { - ResourceMatchInfo::Static { matched_len } => { - path.skip(matched_len); - } - ResourceMatchInfo::Dynamic { - matched_len, - matched_vars, - mut segments, - } => { - for i in 0..matched_vars.len() { - path.add(matched_vars[i], mem::take(&mut segments[i])); - } - - path.skip(matched_len); - } - } - } } pub trait ResourcePath { diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index cfcab8546..cc31f2029 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -55,18 +55,18 @@ impl Router { R: Resource, F: FnMut(&R, &U) -> bool, { - let mut next_match_count = 1; + let mut next_resource_match_count = 1; for (rdef, val, ctx) in self.routes.iter() { match rdef.capture_match_info(resource) { None => {} Some(match_info) => { if check_fn(resource, ctx) { - resource.resolve_path(match_info); + resource.resource_path().resolve(match_info); return Some((val, ResourceId(rdef.id()))); - } else if next_match_count == self.max_path_conflicts { + } else if next_resource_match_count == self.max_path_conflicts { return None; } - next_match_count += 1; + next_resource_match_count += 1; } } } @@ -92,7 +92,7 @@ impl Router { Some(match_info) => { matches += 1; if check_fn(resource, ctx) { - resource.resolve_path(match_info); + resource.resource_path().resolve(match_info); return Some((val, ResourceId(rdef.id()))); } else if matches == self.max_path_conflicts { return None; @@ -108,7 +108,7 @@ impl Router { /// Builder for an ordered [routing](Router) list. pub struct RouterBuilder { routes: Vec<(ResourceDef, T, U)>, - path_conflicts: Vec<(ResourceDef, u16)>, + path_conflicts: Vec<(usize, u16)>, } impl RouterBuilder { @@ -124,14 +124,15 @@ impl RouterBuilder { if let Some((_, path_conflicts)) = self .path_conflicts .iter_mut() - .find(|(current_rdef, _)| rdef.eq(current_rdef)) + .find(|(route_idx, _)| rdef.eq(&self.routes.get(*route_idx).unwrap().0)) { *path_conflicts += 1; } else { - self.path_conflicts.push((rdef.clone(), 1)); + 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() @@ -141,9 +142,7 @@ impl RouterBuilder { /// Finish configuration and create router instance. pub fn finish(self) -> Router { - let max_path_conflicts = self - .path_conflicts - .iter() + let max_path_conflicts = self.path_conflicts.iter() .map(|(_, path_conflicts)| *path_conflicts) .max() .unwrap_or(1); diff --git a/actix-router/src/url.rs b/actix-router/src/url.rs index 8baed700e..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.resolve_path_if_matches(&mut path)); + assert!(re.resolve_path_if_match(&mut path)); path } diff --git a/actix-web/src/types/path.rs b/actix-web/src/types/path.rs index bb34d1f28..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.resolve_path_if_matches(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.resolve_path_if_matches(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.resolve_path_if_matches(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.resolve_path_if_matches(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.resolve_path_if_matches(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(); From 6f4bdb31f9152f49e3a28671069d65718ef06d9e Mon Sep 17 00:00:00 2001 From: manuelgdlvh Date: Fri, 16 May 2025 22:27:19 +0200 Subject: [PATCH 7/7] feat: Add code formatting --- actix-router/src/path.rs | 3 +-- actix-router/src/resource.rs | 3 +-- actix-router/src/router.rs | 4 +++- actix-web/src/app_service.rs | 7 +++---- actix-web/src/scope.rs | 8 +++----- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/actix-router/src/path.rs b/actix-router/src/path.rs index 2a7a76fc5..c417b722b 100644 --- a/actix-router/src/path.rs +++ b/actix-router/src/path.rs @@ -6,8 +6,7 @@ use std::{ use serde::{de, Deserialize}; -use crate::resource::ResourceMatchInfo; -use crate::{de::PathDeserializer, Resource, ResourcePath}; +use crate::{de::PathDeserializer, resource::ResourceMatchInfo, Resource, ResourcePath}; #[derive(Debug, Clone)] pub enum PathItem { diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 3bd01ed2a..4ae75eced 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -1122,9 +1122,8 @@ pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { #[cfg(test)] mod tests { - use crate::Path; - use super::*; + use crate::Path; #[test] fn equivalence() { diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index cc31f2029..364195f4c 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -142,7 +142,9 @@ impl RouterBuilder { /// Finish configuration and create router instance. pub fn finish(self) -> Router { - let max_path_conflicts = self.path_conflicts.iter() + let max_path_conflicts = self + .path_conflicts + .iter() .map(|(_, path_conflicts)| *path_conflicts) .max() .unwrap_or(1); diff --git a/actix-web/src/app_service.rs b/actix-web/src/app_service.rs index a7f0ab274..37b0e3ccf 100644 --- a/actix-web/src/app_service.rs +++ b/actix-web/src/app_service.rs @@ -1,11 +1,10 @@ use std::{cell::RefCell, mem, rc::Rc}; -use actix_service::{boxed, fn_service, Service, ServiceFactory}; -use futures_core::future::LocalBoxFuture; -use futures_util::future::join_all; - use actix_http::Request; use actix_router::{Path, ResourceDef, Router, Url}; +use actix_service::{boxed, fn_service, Service, ServiceFactory}; +use futures_core::future::LocalBoxFuture; +use futures_util::future::join_all; use crate::{ body::BoxBody, diff --git a/actix-web/src/scope.rs b/actix-web/src/scope.rs index 5edf1ef1a..b11e7b89f 100644 --- a/actix-web/src/scope.rs +++ b/actix-web/src/scope.rs @@ -1,5 +1,7 @@ use std::{cell::RefCell, fmt, future::Future, mem, rc::Rc}; +use actix_http::{body::MessageBody, Extensions}; +use actix_router::{ResourceDef, Router}; use actix_service::{ apply, apply_fn_factory, boxed, IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt, Transform, @@ -7,9 +9,6 @@ use actix_service::{ use futures_core::future::LocalBoxFuture; use futures_util::future::join_all; -use actix_http::{body::MessageBody, Extensions}; -use actix_router::{ResourceDef, Router}; - use crate::{ config::ServiceConfig, data::Data, @@ -553,6 +552,7 @@ mod tests { use actix_utils::future::ok; use bytes::Bytes; + use super::*; use crate::{ guard, http::{ @@ -564,8 +564,6 @@ mod tests { web, App, HttpMessage, HttpRequest, HttpResponse, }; - use super::*; - #[test] fn can_be_returned_from_fn() { fn my_scope_1() -> Scope {