From 9fde5b30dbf7b861cc328ba1353bc6d6cbfc2f30 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Tue, 1 Feb 2022 01:12:48 +0300 Subject: [PATCH] tweak and document router (#2612) Co-authored-by: Rob Ede --- actix-router/CHANGES.md | 6 ++ actix-router/src/lib.rs | 2 +- actix-router/src/quoter.rs | 6 +- actix-router/src/resource.rs | 30 +++---- actix-router/src/router.rs | 152 +++++++++++++++++++---------------- src/app_service.rs | 23 ++---- src/scope.rs | 17 +--- 7 files changed, 115 insertions(+), 121 deletions(-) diff --git a/actix-router/CHANGES.md b/actix-router/CHANGES.md index 1f22e5764..368138603 100644 --- a/actix-router/CHANGES.md +++ b/actix-router/CHANGES.md @@ -1,8 +1,14 @@ # Changes ## Unreleased - 2021-xx-xx +- Remove unused `ResourceInfo`. [#2612] +- Add `RouterBuilder::push`. [#2612] +- Change signature of `ResourceDef::capture_match_info_fn` to remove `user_data` parameter. [#2612] +- Replace `Option` with `U` in `Router` API. [#2612] +- Relax bounds on `Router::recognize*` and `ResourceDef::capture_match_info`. [#2612] - `Quoter::requote` now returns `Option>`. [#2613] +[#2612]: https://github.com/actix/actix-web/pull/2612 [#2613]: https://github.com/actix/actix-web/pull/2613 diff --git a/actix-router/src/lib.rs b/actix-router/src/lib.rs index 22f294b9d..0febcf1ac 100644 --- a/actix-router/src/lib.rs +++ b/actix-router/src/lib.rs @@ -22,7 +22,7 @@ pub use self::pattern::{IntoPatterns, Patterns}; pub use self::quoter::Quoter; pub use self::resource::ResourceDef; pub use self::resource_path::{Resource, ResourcePath}; -pub use self::router::{ResourceInfo, Router, RouterBuilder}; +pub use self::router::{ResourceId, Router, RouterBuilder}; #[cfg(feature = "http")] pub use self::url::Url; diff --git a/actix-router/src/quoter.rs b/actix-router/src/quoter.rs index 73b1e72dd..8a1e99e1d 100644 --- a/actix-router/src/quoter.rs +++ b/actix-router/src/quoter.rs @@ -64,14 +64,14 @@ impl Quoter { quoter } - /// Re-quotes... ? + /// Decodes safe percent-encoded sequences from `val`. /// /// Returns `None` when no modification to the original byte string was required. /// /// Non-ASCII bytes are accepted as valid input. /// - /// Behavior for invalid/incomplete percent-encoding sequences is unspecified and may include removing - /// the invalid sequence from the output or passing it as it is. + /// Behavior for invalid/incomplete percent-encoding sequences is unspecified and may include + /// removing the invalid sequence from the output or passing it as-is. pub fn requote(&self, val: &[u8]) -> Option> { let mut has_pct = 0; let mut pct = [b'%', 0, 0]; diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index c0b5522af..f3eaa9f42 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -8,10 +8,7 @@ use std::{ use firestorm::{profile_fn, profile_method, profile_section}; use regex::{escape, Regex, RegexSet}; -use crate::{ - path::{Path, PathItem}, - IntoPatterns, Patterns, Resource, ResourcePath, -}; +use crate::{path::PathItem, IntoPatterns, Patterns, Resource, ResourcePath}; const MAX_DYNAMIC_SEGMENTS: usize = 16; @@ -615,7 +612,7 @@ impl ResourceDef { } } - /// Collects dynamic segment values into `path`. + /// Collects dynamic segment values into `resource`. /// /// Returns `true` if `path` matches this resource. /// @@ -635,9 +632,9 @@ impl ResourceDef { /// assert_eq!(path.get("path").unwrap(), "HEAD/Cargo.toml"); /// assert_eq!(path.unprocessed(), ""); /// ``` - pub fn capture_match_info(&self, path: &mut Path) -> bool { + pub fn capture_match_info(&self, resource: &mut R) -> bool { profile_method!(capture_match_info); - self.capture_match_info_fn(path, |_, _| true, ()) + self.capture_match_info_fn(resource, |_| true) } /// Collects dynamic segment values into `resource` after matching paths and executing @@ -655,13 +652,12 @@ impl ResourceDef { /// use actix_router::{Path, ResourceDef}; /// /// fn try_match(resource: &ResourceDef, path: &mut Path<&str>) -> bool { - /// let admin_allowed = std::env::var("ADMIN_ALLOWED").ok(); + /// 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" - /// |res, admin_allowed| !res.path().contains("admin"), - /// &admin_allowed + /// |res| !(!admin_allowed && res.path().contains("admin")), /// ) /// } /// @@ -678,15 +674,10 @@ 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, - user_data: U, - ) -> bool + pub fn capture_match_info_fn(&self, resource: &mut R, check_fn: F) -> bool where R: Resource, - F: FnOnce(&R, U) -> bool, + F: FnOnce(&R) -> bool, { profile_method!(capture_match_info_fn); @@ -762,7 +753,7 @@ impl ResourceDef { } }; - if !check_fn(resource, user_data) { + if !check_fn(resource) { return false; } @@ -857,7 +848,7 @@ impl ResourceDef { S: BuildHasher, { profile_method!(resource_path_from_map); - self.build_resource_path(path, |name| values.get(name).map(AsRef::::as_ref)) + self.build_resource_path(path, |name| values.get(name)) } /// Returns true if `prefix` acts as a proper prefix (i.e., separated by a slash) in `path`. @@ -1157,6 +1148,7 @@ pub(crate) fn insert_slash(path: &str) -> Cow<'_, str> { #[cfg(test)] mod tests { use super::*; + use crate::Path; #[test] fn equivalence() { diff --git a/actix-router/src/router.rs b/actix-router/src/router.rs index 4652ef678..f0e598683 100644 --- a/actix-router/src/router.rs +++ b/actix-router/src/router.rs @@ -5,87 +5,83 @@ use crate::{IntoPatterns, Resource, ResourceDef}; #[derive(Debug, Copy, Clone, PartialEq)] pub struct ResourceId(pub u16); -/// Information about current resource -#[derive(Debug, Clone)] -pub struct ResourceInfo { - #[allow(dead_code)] - resource: ResourceId, -} - /// Resource router. -// T is the resource itself -// U is any other data needed for routing like method guards +/// +/// It matches a [routing resource](Resource) to an ordered list of _routes_. Each is defined by a +/// single [`ResourceDef`] and contains two types of custom data: +/// 1. The route _value_, of the generic type `T`. +/// 1. Some _context_ data, of the generic type `U`, which is only provided to the check function in +/// [`recognize_fn`](Self::recognize_fn). This parameter defaults to `()` and can be omitted if +/// not required. pub struct Router { - routes: Vec<(ResourceDef, T, Option)>, + routes: Vec<(ResourceDef, T, U)>, } impl Router { + /// Constructs new `RouterBuilder` with empty route list. pub fn build() -> RouterBuilder { - RouterBuilder { - resources: Vec::new(), - } + RouterBuilder { routes: Vec::new() } } + /// Finds the value in the router that matches a given [routing resource](Resource). + /// + /// The match result, including the captured dynamic segments, in the `resource`. pub fn recognize(&self, resource: &mut R) -> Option<(&T, ResourceId)> where R: Resource, { profile_method!(recognize); - - for item in self.routes.iter() { - if item.0.capture_match_info(resource.resource_path()) { - return Some((&item.1, ResourceId(item.0.id()))); - } - } - - None + self.recognize_fn(resource, |_, _| true) } + /// Same as [`recognize`](Self::recognize) but returns a mutable reference to the matched value. pub fn recognize_mut(&mut self, resource: &mut R) -> Option<(&mut T, ResourceId)> where R: Resource, { profile_method!(recognize_mut); - - for item in self.routes.iter_mut() { - if item.0.capture_match_info(resource.resource_path()) { - return Some((&mut item.1, ResourceId(item.0.id()))); - } - } - - None + self.recognize_mut_fn(resource, |_, _| true) } - pub fn recognize_fn(&self, resource: &mut R, check: F) -> Option<(&T, ResourceId)> + /// Finds the value in the router that matches a given [routing resource](Resource) and passes + /// an additional predicate check using context data. + /// + /// Similar to [`recognize`](Self::recognize). However, before accepting the route as matched, + /// 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)> where - F: Fn(&R, &Option) -> bool, R: Resource, + F: FnMut(&R, &U) -> bool, { profile_method!(recognize_checked); - for item in self.routes.iter() { - if item.0.capture_match_info_fn(resource, &check, &item.2) { - return Some((&item.1, ResourceId(item.0.id()))); + 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()))); } } None } + /// Same as [`recognize_fn`](Self::recognize_fn) but returns a mutable reference to the matched + /// value. pub fn recognize_mut_fn( &mut self, resource: &mut R, - check: F, + mut check: F, ) -> Option<(&mut T, ResourceId)> where - F: Fn(&R, &Option) -> bool, R: Resource, + F: FnMut(&R, &U) -> bool, { profile_method!(recognize_mut_checked); - for item in self.routes.iter_mut() { - if item.0.capture_match_info_fn(resource, &check, &item.2) { - return Some((&mut item.1, ResourceId(item.0.id()))); + 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()))); } } @@ -93,49 +89,69 @@ impl Router { } } +/// Builder for an ordered [routing](Router) list. pub struct RouterBuilder { - resources: Vec<(ResourceDef, T, Option)>, + routes: Vec<(ResourceDef, T, U)>, } impl RouterBuilder { - /// Register resource for specified path. - pub fn path( + /// Adds a new route to the end of the routing list. + /// + /// Returns mutable references to elements of the new route. + pub fn push( &mut self, - path: P, - resource: T, - ) -> &mut (ResourceDef, T, Option) { - profile_method!(path); - - self.resources - .push((ResourceDef::new(path), resource, None)); - self.resources.last_mut().unwrap() - } - - /// Register resource for specified path prefix. - pub fn prefix(&mut self, prefix: &str, resource: T) -> &mut (ResourceDef, T, Option) { - profile_method!(prefix); - - self.resources - .push((ResourceDef::prefix(prefix), resource, None)); - self.resources.last_mut().unwrap() - } - - /// Register resource for ResourceDef - pub fn rdef(&mut self, rdef: ResourceDef, resource: T) -> &mut (ResourceDef, T, Option) { - profile_method!(rdef); - - self.resources.push((rdef, resource, None)); - self.resources.last_mut().unwrap() + rdef: ResourceDef, + val: T, + ctx: U, + ) -> (&mut ResourceDef, &mut T, &mut U) { + profile_method!(push); + self.routes.push((rdef, val, ctx)); + self.routes + .last_mut() + .map(|(rdef, val, ctx)| (rdef, val, ctx)) + .unwrap() } /// Finish configuration and create router instance. pub fn finish(self) -> Router { Router { - routes: self.resources, + routes: self.routes, } } } +/// Convenience methods provided when context data impls [`Default`] +impl RouterBuilder +where + U: Default, +{ + /// Registers resource for specified path. + pub fn path( + &mut self, + path: impl IntoPatterns, + val: T, + ) -> (&mut ResourceDef, &mut T, &mut U) { + profile_method!(path); + self.push(ResourceDef::new(path), val, U::default()) + } + + /// Registers resource for specified path prefix. + pub fn prefix( + &mut self, + prefix: impl IntoPatterns, + val: T, + ) -> (&mut ResourceDef, &mut T, &mut U) { + profile_method!(prefix); + self.push(ResourceDef::prefix(prefix), val, U::default()) + } + + /// Registers resource for [`ResourceDef`]. + pub fn rdef(&mut self, rdef: ResourceDef, val: T) -> (&mut ResourceDef, &mut T, &mut U) { + profile_method!(rdef); + self.push(rdef, val, U::default()) + } +} + #[cfg(test)] mod tests { use crate::path::Path; diff --git a/src/app_service.rs b/src/app_service.rs index dbd718330..3ef31ac75 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -21,8 +21,6 @@ use crate::{ Error, HttpResponse, }; -type Guards = Vec>; - /// Service factory to convert `Request` to a `ServiceRequest`. /// /// It also executes data factories. @@ -244,7 +242,7 @@ pub struct AppRoutingFactory { [( ResourceDef, BoxedHttpServiceFactory, - RefCell>, + RefCell>>>, )], >, default: Rc, @@ -262,7 +260,7 @@ impl ServiceFactory for AppRoutingFactory { // construct all services factory future with it's resource def and guards. let factory_fut = join_all(self.services.iter().map(|(path, factory, guards)| { let path = path.clone(); - let guards = guards.borrow_mut().take(); + let guards = guards.borrow_mut().take().unwrap_or_default(); let factory_fut = factory.new_service(()); async move { let service = factory_fut.await?; @@ -283,7 +281,7 @@ impl ServiceFactory for AppRoutingFactory { .collect::, _>>()? .drain(..) .fold(Router::build(), |mut router, (path, guards, service)| { - router.rdef(path, service).2 = guards; + router.push(path, service, guards); router }) .finish(); @@ -295,7 +293,7 @@ impl ServiceFactory for AppRoutingFactory { /// The Actix Web router default entry point. pub struct AppRouting { - router: Router, + router: Router>>, default: BoxedHttpService, } @@ -308,17 +306,8 @@ impl Service for AppRouting { fn call(&self, mut req: ServiceRequest) -> Self::Future { let res = self.router.recognize_fn(&mut req, |req, guards| { - if let Some(ref guards) = guards { - let guard_ctx = req.guard_ctx(); - - for guard in guards { - if !guard.check(&guard_ctx) { - return false; - } - } - } - - true + let guard_ctx = req.guard_ctx(); + guards.iter().all(|guard| guard.check(&guard_ctx)) }); if let Some((srv, _info)) = res { diff --git a/src/scope.rs b/src/scope.rs index dad727430..0fcc83d70 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -467,7 +467,7 @@ impl ServiceFactory for ScopeFactory { // construct all services factory future with it's resource def and guards. let factory_fut = join_all(self.services.iter().map(|(path, factory, guards)| { let path = path.clone(); - let guards = guards.borrow_mut().take(); + let guards = guards.borrow_mut().take().unwrap_or_default(); let factory_fut = factory.new_service(()); async move { let service = factory_fut.await?; @@ -485,7 +485,7 @@ impl ServiceFactory for ScopeFactory { .collect::, _>>()? .drain(..) .fold(Router::build(), |mut router, (path, guards, service)| { - router.rdef(path, service).2 = guards; + router.push(path, service, guards); router }) .finish(); @@ -509,17 +509,8 @@ impl Service for ScopeService { fn call(&self, mut req: ServiceRequest) -> Self::Future { let res = self.router.recognize_fn(&mut req, |req, guards| { - if let Some(ref guards) = guards { - let guard_ctx = req.guard_ctx(); - - for guard in guards { - if !guard.check(&guard_ctx) { - return false; - } - } - } - - true + let guard_ctx = req.guard_ctx(); + guards.iter().all(|guard| guard.check(&guard_ctx)) }); if let Some((srv, _info)) = res {