diff --git a/actix-router/src/resource.rs b/actix-router/src/resource.rs index 58016bb3..7136f3e7 100644 --- a/actix-router/src/resource.rs +++ b/actix-router/src/resource.rs @@ -572,6 +572,20 @@ impl ResourceDef { PatternType::Prefix(ref prefix) if prefix == path => true, PatternType::Prefix(ref prefix) => is_strict_prefix(prefix, path), + // dynamic prefix + PatternType::Dynamic(ref re, _) if !re.as_str().ends_with('$') => { + match re.find(path) { + // prefix matches exactly + Some(m) if m.end() == path.len() => true, + + // prefix matches part + Some(m) => is_strict_prefix(m.as_str(), path), + + // prefix does not match + None => false, + } + } + PatternType::Dynamic(ref re, _) => re.is_match(path), PatternType::DynamicSet(ref re, _) => re.is_match(path), } @@ -616,18 +630,27 @@ impl ResourceDef { profile_method!(find_match); match &self.pat_type { - PatternType::Static(segment) => { - if segment == path { - Some(segment.len()) - } else { - None - } - } + PatternType::Static(segment) if path == segment => Some(segment.len()), + PatternType::Static(_) => None, PatternType::Prefix(prefix) if path == prefix => Some(prefix.len()), PatternType::Prefix(prefix) if is_strict_prefix(prefix, path) => Some(prefix.len()), PatternType::Prefix(_) => None, + // dynamic prefix + PatternType::Dynamic(ref re, _) if !re.as_str().ends_with('$') => { + match re.find(path) { + // prefix matches exactly + Some(m) if m.end() == path.len() => Some(m.end()), + + // prefix matches part + Some(m) if is_strict_prefix(m.as_str(), path) => Some(m.end()), + + // prefix does not match + _ => None, + } + } + PatternType::Dynamic(re, _) => re.find(path).map(|m| m.end()), PatternType::DynamicSet(re, params) => { @@ -659,8 +682,8 @@ impl ResourceDef { /// assert_eq!(path.unprocessed(), ""); /// ``` pub fn capture_match_info(&self, path: &mut Path) -> bool { - profile_method!(is_path_match); - self.capture_match_info_fn(path, &|_, _| true, &None::<()>) + profile_method!(capture_match_info); + self.capture_match_info_fn(path, |_, _| true, ()) } /// Collects dynamic segment values into `resource` after matching paths and executing @@ -683,7 +706,7 @@ impl ResourceDef { /// resource.capture_match_info_fn( /// path, /// // when env var is not set, reject when path contains "admin" - /// &|res, admin_allowed| !res.path().contains("admin"), + /// |res, admin_allowed| !res.path().contains("admin"), /// &admin_allowed /// ) /// } @@ -704,15 +727,15 @@ impl ResourceDef { pub fn capture_match_info_fn( &self, resource: &mut R, - check_fn: &F, - user_data: &Option, + check_fn: F, + user_data: U, ) -> bool where R: Resource, T: ResourcePath, - F: Fn(&R, &Option) -> bool, + F: FnOnce(&R, U) -> bool, { - profile_method!(is_path_match_fn); + profile_method!(capture_match_info_fn); let mut segments = <[PathItem; MAX_DYNAMIC_SEGMENTS]>::default(); let path = resource.resource_path(); @@ -932,7 +955,9 @@ impl ResourceDef { } _ => false, }) - .expect("malformed dynamic segment"); + .unwrap_or_else(|| { + panic!(r#"path "{}" contains malformed dynamic segment"#, pattern) + }); let (mut param, mut unprocessed) = pattern.split_at(close_idx + 1); @@ -1022,13 +1047,32 @@ impl ResourceDef { dyn_segment_count += 1; } + if is_prefix && has_tail_segment { + // tail segments in prefixes have no defined semantics + + #[cfg(not(test))] + log::warn!( + "Prefix resources should not have tail segments. \ + Use `ResourceDef::new` constructor. \ + This may become a panic in the future." + ); + + // panic in tests to make this case detectable + #[cfg(test)] + panic!("prefix resource definitions should not have tail segments"); + } + if unprocessed.ends_with('*') { // unnamed tail segment #[cfg(not(test))] - log::warn!("tail segments must have names; consider `{{tail}}*`"); + log::warn!( + "Tail segments must have names. \ + Consider `.../{{tail}}*`. \ + This may become a panic in the future." + ); - // to this case detectable in tests + // panic in tests to make this case detectable #[cfg(test)] panic!("tail segments must have names"); } else if !has_tail_segment && !unprocessed.is_empty() { @@ -1149,7 +1193,6 @@ mod tests { assert_eq!(ResourceDef::new("/"), ResourceDef::new(["/"])); assert_eq!(ResourceDef::new("/"), ResourceDef::new(vec!["/"])); - assert_eq!(ResourceDef::new("/{id}*"), ResourceDef::prefix("/{id}*")); assert_ne!(ResourceDef::new(""), ResourceDef::prefix("")); assert_ne!(ResourceDef::new("/"), ResourceDef::prefix("/")); @@ -1478,10 +1521,6 @@ mod tests { assert_eq!(&path[0], "test2"); assert_eq!(path.unprocessed(), "subpath1/subpath2/index.html"); - let resource = ResourceDef::prefix(r"/id/{id:\d{3}}"); - assert!(resource.is_match("/id/1234")); - assert_eq!(resource.find_match("/id/1234"), Some(7)); - let resource = ResourceDef::prefix("/user"); // input string shorter than prefix assert!(resource.find_match("/foo").is_none()); @@ -1554,6 +1593,21 @@ mod tests { assert!(path.get("uid").is_some()); } + #[test] + fn dynamic_prefix_proper_segmentation() { + let resource = ResourceDef::prefix(r"/id/{id:\d{3}}"); + + assert!(resource.is_match("/id/123")); + assert!(resource.is_match("/id/123/foo")); + assert!(!resource.is_match("/id/1234")); + assert!(!resource.is_match("/id/123a")); + + assert_eq!(resource.find_match("/id/123"), Some(7)); + assert_eq!(resource.find_match("/id/123/foo"), Some(7)); + assert_eq!(resource.find_match("/id/1234"), None); + assert_eq!(resource.find_match("/id/123a"), None); + } + #[test] fn build_path_map() { let resource = ResourceDef::new("/user/{item1}/{item2}/"); @@ -1670,11 +1724,11 @@ mod tests { #[test] #[should_panic] fn invalid_unnamed_tail_segment() { - ResourceDef::new(r"/*"); + ResourceDef::new("/*"); } #[test] - // #[should_panic] // TODO: consider if this should be allowed + #[should_panic] fn prefix_plus_tail_match_is_allowed() { ResourceDef::prefix("/user/{id}*"); }