From a817ddb57b358748b47797b2d11b22236edf471d Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Mon, 7 May 2018 13:50:43 -0700 Subject: [PATCH] add variable segments support for scope prefix --- src/application.rs | 98 +++++++++++++++++-------- src/error.rs | 4 +- src/router.rs | 114 +++++++++++++++++++++++++++--- src/scope.rs | 149 ++++++++++++++++++++++++++++++++++----- src/ws/context.rs | 2 +- tests/test_middleware.rs | 2 +- 6 files changed, 306 insertions(+), 63 deletions(-) diff --git a/src/application.rs b/src/application.rs index 76ae1ba73..20dfa7901 100644 --- a/src/application.rs +++ b/src/application.rs @@ -29,7 +29,12 @@ pub(crate) struct Inner { default: ResourceHandler, encoding: ContentEncoding, resources: Vec>, - handlers: Vec<(String, Box>)>, + handlers: Vec>, +} + +enum PrefixHandlerType { + Handler(String, Box>), + Scope(Resource, Box>), } impl PipelineHandler for Inner { @@ -44,7 +49,10 @@ impl PipelineHandler for Inner { HandlerType::Normal(idx) => { self.resources[idx].handle(req, Some(&mut self.default)) } - HandlerType::Handler(idx) => self.handlers[idx].1.handle(req), + HandlerType::Handler(idx) => match self.handlers[idx] { + PrefixHandlerType::Handler(_, ref mut hnd) => hnd.handle(req), + PrefixHandlerType::Scope(_, ref mut hnd) => hnd.handle(req), + }, HandlerType::Default => self.default.handle(req, None), } } @@ -62,27 +70,49 @@ impl HttpApplication { HandlerType::Normal(idx) } else { let inner = self.as_ref(); + let path: &'static str = + unsafe { &*(&req.path()[inner.prefix..] as *const _) }; + let path_len = path.len(); for idx in 0..inner.handlers.len() { - let &(ref prefix, _) = &inner.handlers[idx]; - let m = { - let path = &req.path()[inner.prefix..]; - path.starts_with(prefix) - && (path.len() == prefix.len() - || path.split_at(prefix.len()).1.starts_with('/')) - }; + match &inner.handlers[idx] { + PrefixHandlerType::Handler(ref prefix, _) => { + let m = { + path.starts_with(prefix) + && (path_len == prefix.len() + || path.split_at(prefix.len()).1.starts_with('/')) + }; - if m { - let prefix_len = inner.prefix + prefix.len(); - let path: &'static str = - unsafe { &*(&req.path()[prefix_len..] as *const _) }; + if m { + let prefix_len = inner.prefix + prefix.len(); + let path: &'static str = + unsafe { &*(&req.path()[prefix_len..] as *const _) }; - req.set_prefix_len(prefix_len as u16); - if path.is_empty() { - req.match_info_mut().add("tail", "/"); - } else { - req.match_info_mut().add("tail", path); + req.set_prefix_len(prefix_len as u16); + if path.is_empty() { + req.match_info_mut().add("tail", "/"); + } else { + req.match_info_mut().add("tail", path); + } + return HandlerType::Handler(idx); + } + } + PrefixHandlerType::Scope(ref pattern, _) => { + if let Some(prefix_len) = + pattern.match_prefix_with_params(path, req.match_info_mut()) + { + let prefix_len = inner.prefix + prefix_len - 1; + let path: &'static str = + unsafe { &*(&req.path()[prefix_len..] as *const _) }; + + req.set_prefix_len(prefix_len as u16); + if path.is_empty() { + req.match_info_mut().set("tail", "/"); + } else { + req.match_info_mut().set("tail", path); + } + return HandlerType::Handler(idx); + } } - return HandlerType::Handler(idx); } } HandlerType::Default @@ -131,7 +161,7 @@ struct ApplicationParts { settings: ServerSettings, default: ResourceHandler, resources: Vec<(Resource, Option>)>, - handlers: Vec<(String, Box>)>, + handlers: Vec>, external: HashMap, encoding: ContentEncoding, middlewares: Vec>>, @@ -305,7 +335,7 @@ where /// Configure scope for common root path. /// /// Scopes collect multiple paths under a common path prefix. - /// Scope path can not contain variable path segments as resources. + /// Scope path can contain variable path segments as resources. /// /// ```rust /// # extern crate actix_web; @@ -313,7 +343,7 @@ where /// /// fn main() { /// let app = App::new() - /// .scope("/app", |scope| { + /// .scope("/{project_id}", |scope| { /// scope.resource("/path1", |r| r.f(|_| HttpResponse::Ok())) /// .resource("/path2", |r| r.f(|_| HttpResponse::Ok())) /// .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed())) @@ -322,9 +352,9 @@ where /// ``` /// /// In the above example, three routes get added: - /// * /app/path1 - /// * /app/path2 - /// * /app/path3 + /// * /{project_id}/path1 + /// * /{project_id}/path2 + /// * /{project_id}/path3 /// pub fn scope(mut self, path: &str, f: F) -> App where @@ -337,9 +367,15 @@ where if !path.is_empty() && !path.starts_with('/') { path.insert(0, '/') } + if !path.ends_with('/') { + path.push('/'); + } let parts = self.parts.as_mut().expect("Use after finish"); - parts.handlers.push((path, scope)); + parts.handlers.push(PrefixHandlerType::Scope( + Resource::prefix("", &path), + scope, + )); } self } @@ -496,11 +532,15 @@ where if !path.is_empty() && !path.starts_with('/') { path.insert(0, '/') } + if path.len() > 1 && path.ends_with('/') { + path.pop(); + } let parts = self.parts.as_mut().expect("Use after finish"); - parts - .handlers - .push((path, Box::new(WrapHandler::new(handler)))); + parts.handlers.push(PrefixHandlerType::Handler( + path, + Box::new(WrapHandler::new(handler)), + )); } self } diff --git a/src/error.rs b/src/error.rs index 9482e067b..8c46774e5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -850,7 +850,7 @@ mod tests { } macro_rules! from { - ($from: expr => $error: pat) => { + ($from:expr => $error:pat) => { match ParseError::from($from) { e @ $error => { assert!(format!("{}", e).len() >= 5); @@ -861,7 +861,7 @@ mod tests { } macro_rules! from_and_cause { - ($from: expr => $error: pat) => { + ($from:expr => $error:pat) => { match ParseError::from($from) { e @ $error => { let desc = format!("{}", e.cause().unwrap()); diff --git a/src/router.rs b/src/router.rs index 2c7d5c32e..1e7126b63 100644 --- a/src/router.rs +++ b/src/router.rs @@ -142,7 +142,8 @@ enum PatternElement { #[derive(Clone, Debug)] enum PatternType { Static(String), - Dynamic(Regex, Vec), + Prefix(String), + Dynamic(Regex, Vec, usize), } #[derive(Debug, Copy, Clone, PartialEq)] @@ -173,14 +174,23 @@ impl Resource { /// /// Panics if path pattern is wrong. pub fn new(name: &str, path: &str) -> Self { - Resource::with_prefix(name, path, "/") + Resource::with_prefix(name, path, "/", false) + } + + /// Parse path pattern and create new `Resource` instance. + /// + /// Use `prefix` type instead of `static`. + /// + /// Panics if path regex pattern is wrong. + pub fn prefix(name: &str, path: &str) -> Self { + Resource::with_prefix(name, path, "/", true) } /// Construct external resource /// /// Panics if path pattern is wrong. pub fn external(name: &str, path: &str) -> Self { - let mut resource = Resource::with_prefix(name, path, "/"); + let mut resource = Resource::with_prefix(name, path, "/", false); resource.rtp = ResourceType::External; resource } @@ -197,8 +207,9 @@ impl Resource { } /// Parse path pattern and create new `Resource` instance with custom prefix - pub fn with_prefix(name: &str, path: &str, prefix: &str) -> Self { - let (pattern, elements, is_dynamic) = Resource::parse(path, prefix); + pub fn with_prefix(name: &str, path: &str, prefix: &str, for_prefix: bool) -> Self { + let (pattern, elements, is_dynamic, len) = + Resource::parse(path, prefix, for_prefix); let tp = if is_dynamic { let re = match Regex::new(&pattern) { @@ -208,7 +219,9 @@ impl Resource { let names = re.capture_names() .filter_map(|name| name.map(|name| name.to_owned())) .collect(); - PatternType::Dynamic(re, names) + PatternType::Dynamic(re, names, len) + } else if for_prefix { + PatternType::Prefix(pattern.clone()) } else { PatternType::Static(pattern.clone()) }; @@ -240,7 +253,8 @@ impl Resource { pub fn is_match(&self, path: &str) -> bool { match self.tp { PatternType::Static(ref s) => s == path, - PatternType::Dynamic(ref re, _) => re.is_match(path), + PatternType::Dynamic(ref re, _, _) => re.is_match(path), + PatternType::Prefix(ref s) => path.starts_with(s), } } @@ -249,7 +263,7 @@ impl Resource { ) -> bool { match self.tp { PatternType::Static(ref s) => s == path, - PatternType::Dynamic(ref re, ref names) => { + PatternType::Dynamic(ref re, ref names, _) => { if let Some(captures) = re.captures(path) { let mut idx = 0; for capture in captures.iter() { @@ -265,6 +279,42 @@ impl Resource { false } } + PatternType::Prefix(ref s) => path.starts_with(s), + } + } + + pub fn match_prefix_with_params<'a>( + &'a self, path: &'a str, params: &'a mut Params<'a>, + ) -> Option { + match self.tp { + PatternType::Static(ref s) => if s == path { + Some(s.len()) + } else { + None + }, + PatternType::Dynamic(ref re, ref names, len) => { + if let Some(captures) = re.captures(path) { + let mut idx = 0; + let mut pos = 0; + for capture in captures.iter() { + if let Some(ref m) = capture { + if idx != 0 { + params.add(names[idx - 1].as_str(), m.as_str()); + } + idx += 1; + pos = m.end(); + } + } + Some(pos + len) + } else { + None + } + } + PatternType::Prefix(ref s) => if path.starts_with(s) { + Some(s.len()) + } else { + None + }, } } @@ -297,7 +347,9 @@ impl Resource { Ok(path) } - fn parse(pattern: &str, prefix: &str) -> (String, Vec, bool) { + fn parse( + pattern: &str, prefix: &str, for_prefix: bool, + ) -> (String, Vec, bool, usize) { const DEFAULT_PATTERN: &str = "[^/]+"; let mut re1 = String::from("^") + prefix; @@ -309,6 +361,7 @@ impl Resource { let mut param_pattern = String::from(DEFAULT_PATTERN); let mut is_dynamic = false; let mut elems = Vec::new(); + let mut len = 0; for (index, ch) in pattern.chars().enumerate() { // All routes must have a leading slash so its optional to have one @@ -325,6 +378,7 @@ impl Resource { param_name.clear(); param_pattern = String::from(DEFAULT_PATTERN); + len = 0; in_param_pattern = false; in_param = false; } else if ch == ':' { @@ -348,16 +402,19 @@ impl Resource { re1.push_str(escape(&ch.to_string()).as_str()); re2.push(ch); el.push(ch); + len += 1; } } let re = if is_dynamic { - re1.push('$'); + if !for_prefix { + re1.push('$'); + } re1 } else { re2 }; - (re, elems, is_dynamic) + (re, elems, is_dynamic, len) } } @@ -570,6 +627,41 @@ mod tests { assert_eq!(req.match_info().get("id").unwrap(), "adahg32"); } + #[test] + fn test_resource_prefix() { + let re = Resource::prefix("test", "/name"); + assert!(re.is_match("/name")); + assert!(re.is_match("/name/")); + assert!(re.is_match("/name/test/test")); + assert!(re.is_match("/name1")); + assert!(re.is_match("/name~")); + + let re = Resource::prefix("test", "/name/"); + assert!(re.is_match("/name/")); + assert!(re.is_match("/name/gs")); + assert!(!re.is_match("/name")); + } + + #[test] + fn test_reousrce_prefix_dynamic() { + let re = Resource::prefix("test", "/{name}/"); + assert!(re.is_match("/name/")); + assert!(re.is_match("/name/gs")); + assert!(!re.is_match("/name")); + + let mut req = TestRequest::with_uri("/test2/").finish(); + assert!(re.match_with_params("/test2/", req.match_info_mut())); + assert_eq!(&req.match_info()["name"], "test2"); + + let mut req = + TestRequest::with_uri("/test2/subpath1/subpath2/index.html").finish(); + assert!(re.match_with_params( + "/test2/subpath1/subpath2/index.html", + req.match_info_mut() + )); + assert_eq!(&req.match_info()["name"], "test2"); + } + #[test] fn test_request_resource() { let routes = vec![ diff --git a/src/scope.rs b/src/scope.rs index fedc7ceda..b9ee0e8e9 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -21,7 +21,12 @@ type ScopeResources = Rc>>)>> /// /// Scope is a set of resources with common root path. /// Scopes collect multiple paths under a common path prefix. -/// Scope path can not contain variable path segments as resources. +/// Scope path can contain variable path segments as resources. +/// Scope prefix is always complete path segment, i.e `/app` would +/// be converted to a `/app/` and it would not match `/app` path. +/// +/// You can use variable path segments with `Path` extractor, also variable +/// segments are available in `HttpRequest::match_info()`. /// /// ```rust /// # extern crate actix_web; @@ -29,7 +34,7 @@ type ScopeResources = Rc>>)>> /// /// fn main() { /// let app = App::new() -/// .scope("/app", |scope| { +/// .scope("/{project_id}/", |scope| { /// scope.resource("/path1", |r| r.f(|_| HttpResponse::Ok())) /// .resource("/path2", |r| r.f(|_| HttpResponse::Ok())) /// .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed())) @@ -38,12 +43,12 @@ type ScopeResources = Rc>>)>> /// ``` /// /// In the above example three routes get registered: -/// * /app/path1 - reponds to all http method -/// * /app/path2 - `GET` requests -/// * /app/path3 - `HEAD` requests +/// * /{project_id}/path1 - reponds to all http method +/// * /{project_id}/path2 - `GET` requests +/// * /{project_id}/path3 - `HEAD` requests /// pub struct Scope { - nested: Vec<(String, Route)>, + nested: Vec<(Resource, Route)>, middlewares: Rc>>>, default: Rc>>, resources: ScopeResources, @@ -102,12 +107,16 @@ impl Scope { if !path.is_empty() && !path.starts_with('/') { path.insert(0, '/') } + if !path.ends_with('/') { + path.push('/'); + } let handler = UnsafeCell::new(Box::new(Wrapper { scope, state: Rc::new(state), })); - self.nested.push((path, handler)); + self.nested + .push((Resource::prefix("", &path), handler)); self } @@ -149,9 +158,14 @@ impl Scope { if !path.is_empty() && !path.starts_with('/') { path.insert(0, '/') } + if !path.ends_with('/') { + path.push('/'); + } - self.nested - .push((path, UnsafeCell::new(Box::new(scope)))); + self.nested.push(( + Resource::prefix("", &path), + UnsafeCell::new(Box::new(scope)), + )); self } @@ -298,17 +312,14 @@ impl RouteHandler for Scope { } // nested scopes - for &(ref prefix, ref handler) in &self.nested { - let len = req.prefix_len() as usize; - let m = { - let path = &req.path()[len..]; - path.starts_with(prefix) - && (path.len() == prefix.len() - || path.split_at(prefix.len()).1.starts_with('/')) - }; + let len = req.prefix_len() as usize; + let path: &'static str = unsafe { &*(&req.path()[len..] as *const _) }; - if m { - let prefix_len = len + prefix.len(); + for &(ref prefix, ref handler) in &self.nested { + if let Some(prefix_len) = + prefix.match_prefix_with_params(path, req.match_info_mut()) + { + let prefix_len = len + prefix_len - 1; let path: &'static str = unsafe { &*(&req.path()[prefix_len..] as *const _) }; @@ -698,7 +709,10 @@ impl Response { #[cfg(test)] mod tests { + use bytes::Bytes; + use application::App; + use body::Body; use http::StatusCode; use httpresponse::HttpResponse; use test::TestRequest; @@ -716,6 +730,36 @@ mod tests { assert_eq!(resp.as_msg().status(), StatusCode::OK); } + #[test] + fn test_scope_variable_segment() { + let mut app = App::new() + .scope("/ab-{project}", |scope| { + scope.resource("/path1", |r| { + r.f(|r| { + HttpResponse::Ok() + .body(format!("project: {}", &r.match_info()["project"])) + }) + }) + }) + .finish(); + + let req = TestRequest::with_uri("/ab-project1/path1").finish(); + let resp = app.run(req); + assert_eq!(resp.as_msg().status(), StatusCode::OK); + + match resp.as_msg().body() { + Body::Binary(ref b) => { + let bytes: Bytes = b.clone().into(); + assert_eq!(bytes, Bytes::from_static(b"project: project1")); + } + _ => panic!(), + } + + let req = TestRequest::with_uri("/aa-project1/path1").finish(); + let resp = app.run(req); + assert_eq!(resp.as_msg().status(), StatusCode::NOT_FOUND); + } + #[test] fn test_scope_with_state() { struct State; @@ -748,6 +792,73 @@ mod tests { assert_eq!(resp.as_msg().status(), StatusCode::CREATED); } + #[test] + fn test_nested_scope_with_variable_segment() { + let mut app = App::new() + .scope("/app", |scope| { + scope.nested("/{project_id}", |scope| { + scope.resource("/path1", |r| { + r.f(|r| { + HttpResponse::Created().body(format!( + "project: {}", + &r.match_info()["project_id"] + )) + }) + }) + }) + }) + .finish(); + + let req = TestRequest::with_uri("/app/project_1/path1").finish(); + let resp = app.run(req); + assert_eq!(resp.as_msg().status(), StatusCode::CREATED); + + match resp.as_msg().body() { + Body::Binary(ref b) => { + let bytes: Bytes = b.clone().into(); + assert_eq!(bytes, Bytes::from_static(b"project: project_1")); + } + _ => panic!(), + } + } + + #[test] + fn test_nested2_scope_with_variable_segment() { + let mut app = App::new() + .scope("/app", |scope| { + scope.nested("/{project}", |scope| { + scope.nested("/{id}", |scope| { + scope.resource("/path1", |r| { + r.f(|r| { + HttpResponse::Created().body(format!( + "project: {} - {}", + &r.match_info()["project"], + &r.match_info()["id"], + )) + }) + }) + }) + }) + }) + .finish(); + + let req = TestRequest::with_uri("/app/test/1/path1").finish(); + let resp = app.run(req); + assert_eq!(resp.as_msg().status(), StatusCode::CREATED); + + match resp.as_msg().body() { + Body::Binary(ref b) => { + let bytes: Bytes = b.clone().into(); + assert_eq!(bytes, Bytes::from_static(b"project: test - 1")); + } + _ => panic!(), + } + + let req = TestRequest::with_uri("/app/test/1/path2").finish(); + let resp = app.run(req); + assert_eq!(resp.as_msg().status(), StatusCode::NOT_FOUND); + } + #[test] fn test_default_resource() { let mut app = App::new() diff --git a/src/ws/context.rs b/src/ws/context.rs index 723b215ad..6114a0301 100644 --- a/src/ws/context.rs +++ b/src/ws/context.rs @@ -13,9 +13,9 @@ use context::{ActorHttpContext, Drain, Frame as ContextFrame}; use error::{Error, ErrorInternalServerError}; use httprequest::HttpRequest; -use ws::WsWriter; use ws::frame::Frame; use ws::proto::{CloseReason, OpCode}; +use ws::WsWriter; /// Execution context for `WebSockets` actors pub struct WebsocketContext diff --git a/tests/test_middleware.rs b/tests/test_middleware.rs index 9a6bf0040..99151afd7 100644 --- a/tests/test_middleware.rs +++ b/tests/test_middleware.rs @@ -551,7 +551,7 @@ fn test_async_middleware_multiple() { assert_eq!(num1.load(Ordering::Relaxed), 2); assert_eq!(num2.load(Ordering::Relaxed), 2); - thread::sleep(Duration::from_millis(30)); + thread::sleep(Duration::from_millis(50)); assert_eq!(num3.load(Ordering::Relaxed), 2); }