From 912ed9d7840e0018f93af429abc0dc9375eb292b Mon Sep 17 00:00:00 2001 From: NVT Date: Sat, 6 Jun 2026 16:46:09 +0200 Subject: [PATCH 1/2] Add wildcard matching in the middle of the path --- README.md | 9 ++- benches/bench.rs | 45 ++++++++++++- src/error.rs | 12 ++-- src/lib.rs | 9 ++- src/params.rs | 6 +- src/tree.rs | 163 ++++++++++++++++++++++++++++++++++++++--------- tests/insert.rs | 4 +- tests/match.rs | 117 ++++++++++++++++++++++++++++++++++ 8 files changed, 321 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 82e9b03..35cc48b 100644 --- a/README.md +++ b/README.md @@ -52,11 +52,12 @@ assert_eq!(matched.params.get("id"), Some("1")); assert!(router.at("/images/img-1.jpg").is_err()); ``` -Catch-all parameters start with a `*` and match anything until the end of the path. They must always be at the *end* of the route. +Catch-all parameters start with a `*` and match anything until the next static segment, or the end of the path. ```rust,ignore let mut router = Router::new(); router.insert("/{*rest}", true)?; +router.insert("/v2/{*name}/manifests/{reference}", true)?; let matched = router.at("/foo.html")?; assert_eq!(matched.params.get("rest"), Some("foo.html")); @@ -64,6 +65,10 @@ assert_eq!(matched.params.get("rest"), Some("foo.html")); let matched = router.at("/static/bar.css")?; assert_eq!(matched.params.get("rest"), Some("static/bar.css")); +let matched = router.at("/v2/library/ubuntu/manifests/latest")?; +assert_eq!(matched.params.get("name"), Some("library/ubuntu")); +assert_eq!(matched.params.get("reference"), Some("latest")); + // Note that this would lead to an empty parameter value. assert!(router.at("/").is_err()); ``` @@ -107,7 +112,7 @@ Given set of routes, their overlapping segments may include, in order of priorit - A single route parameter with both a prefix and a suffix (`/a{x}b`). - *One* of the following; - A single standalone parameter (`/{x}`). - - A single standalone catch-all parameter (`/{*rest}`). Note this only applies to the final route segment. + - A single standalone catch-all parameter (`/{*segments}/foo`, `/{*rest}`). Any other combination of route segments is considered ambiguous, and attempting to insert such a route will result in an error. diff --git a/benches/bench.rs b/benches/bench.rs index 26929e3..2463b4f 100644 --- a/benches/bench.rs +++ b/benches/bench.rs @@ -111,7 +111,50 @@ fn compare_routers(c: &mut Criterion) { group.finish(); } -criterion_group!(benches, compare_routers); +fn catchall(c: &mut Criterion) { + let mut group = c.benchmark_group("Catch-all"); + + let mut terminal = matchit::Router::new(); + terminal.insert("/src/{*path}", true).unwrap(); + group.bench_function("terminal", |b| { + b.iter(|| { + let result = black_box(terminal.at(black_box("/src/some/nested/file.png")).unwrap()); + assert!(*result.value); + assert_eq!(result.params.get("path"), Some("some/nested/file.png")); + }); + }); + + let mut non_terminal = matchit::Router::new(); + non_terminal + .insert("/v2/{*name}/blobs/{digest}", true) + .unwrap(); + non_terminal + .insert("/v2/{*name}/manifests/{reference}", true) + .unwrap(); + group.bench_function("non-terminal", |b| { + b.iter(|| { + let result = black_box( + non_terminal + .at(black_box("/v2/team/project/image/blobs/sha256:abc")) + .unwrap(), + ); + assert!(*result.value); + assert_eq!(result.params.get("name"), Some("team/project/image")); + assert_eq!(result.params.get("digest"), Some("sha256:abc")); + }); + }); + + group.bench_function("non-terminal repeated-boundary miss", |b| { + b.iter(|| { + let result = black_box(non_terminal.at(black_box("/v2/a/blobs/b/blobs/c/tags/list"))); + assert!(result.is_err()); + }); + }); + + group.finish(); +} + +criterion_group!(benches, compare_routers, catchall); criterion_main!(benches); macro_rules! routes { diff --git a/src/error.rs b/src/error.rs index 1c18afc..9cdb84b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -25,7 +25,7 @@ pub enum InsertError { /// Note you can use `{{` or `}}` to escape literal brackets. InvalidParam, - /// Catch-all parameters are only allowed at the end of a path. + /// Catch-all parameters cannot have a suffix in the same path segment. InvalidCatchAll, } @@ -42,10 +42,12 @@ impl fmt::Display for InsertError { write!(f, "Only one parameter is allowed per path segment") } Self::InvalidParam => write!(f, "Parameters must be registered with a valid name"), - Self::InvalidCatchAll => write!( - f, - "Catch-all parameters are only allowed at the end of a route" - ), + Self::InvalidCatchAll => { + write!( + f, + "Catch-all parameters cannot have a suffix in the same path segment", + ) + } } } } diff --git a/src/lib.rs b/src/lib.rs index 6ef310b..698a67e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,13 +55,14 @@ assert!(router.at("/images/img-1.jpg").is_err()); # } ``` -Catch-all parameters start with a `*` and match anything until the end of the path. They must always be at the *end* of the route. +Catch-all parameters start with a `*` and match anything until the next static segment, or the end of the path. ```rust # use matchit::Router; # fn main() -> Result<(), Box> { let mut router = Router::new(); router.insert("/{*rest}", true)?; +router.insert("/v2/{*name}/manifests/{reference}", true)?; let matched = router.at("/foo.html")?; assert_eq!(matched.params.get("rest"), Some("foo.html")); @@ -69,6 +70,10 @@ assert_eq!(matched.params.get("rest"), Some("foo.html")); let matched = router.at("/static/bar.css")?; assert_eq!(matched.params.get("rest"), Some("static/bar.css")); +let matched = router.at("/v2/library/ubuntu/manifests/latest")?; +assert_eq!(matched.params.get("name"), Some("library/ubuntu")); +assert_eq!(matched.params.get("reference"), Some("latest")); + // Note that this would lead to an empty parameter value. assert!(router.at("/").is_err()); # Ok(()) @@ -121,7 +126,7 @@ Given set of routes, their overlapping segments may include, in order of priorit - A single route parameter with both a prefix and a suffix (`/a{x}b`). - *One* of the following; - A single standalone parameter (`/{x}`). - - A single standalone catch-all parameter (`/{*rest}`). Note this only applies to the final route segment. + - A single standalone catch-all parameter (`/{*segments}/foo`, `/{*rest}`). Any other combination of route segments is considered ambiguous, and attempting to insert such a route will result in an error. diff --git a/src/params.rs b/src/params.rs index 82859ff..1654430 100644 --- a/src/params.rs +++ b/src/params.rs @@ -168,10 +168,10 @@ impl<'k, 'v> Params<'k, 'v> { // Applies a transformation function to each key. #[inline] - pub(crate) fn for_each_key_mut(&mut self, f: impl Fn((usize, &mut Param<'k, 'v>))) { + pub(crate) fn for_each_key_mut(&mut self, mut f: impl FnMut((usize, &mut Param<'k, 'v>))) { match &mut self.kind { - ParamsKind::Small(arr, len) => arr.iter_mut().take(*len).enumerate().for_each(f), - ParamsKind::Large(vec) => vec.iter_mut().enumerate().for_each(f), + ParamsKind::Small(arr, len) => arr.iter_mut().take(*len).enumerate().for_each(&mut f), + ParamsKind::Large(vec) => vec.iter_mut().enumerate().for_each(&mut f), } } } diff --git a/src/tree.rs b/src/tree.rs index f7c9a2d..ed873ca 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -59,13 +59,25 @@ pub(crate) enum NodeType { /// complexity. Param { suffix: bool }, - /// A catch-all parameter, e.g. '/{*file}'. - CatchAll, + /// A catch-all parameter, e.g. '/{*file}' or '/{*file}/info'. + /// The inner kind stores whether matching stops here, or resumes at a + /// child node. + CatchAll(CatchAllKind), /// A static prefix, e.g. '/foo'. Static, } +/// Whether a [`NodeType::CatchAll`] consumes the rest of the path or resumes matching later. +#[derive(PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Copy)] +pub(crate) enum CatchAllKind { + /// Captures the rest of the path, e.g. '/{*file}'. + Terminal, + + /// Captures until a following route boundary, e.g. '/{*file}/info'. + NonTerminal, +} + /// Safety: We expose `value` per Rust's usual borrowing rules, so we can just /// delegate these traits. unsafe impl Send for Node {} @@ -306,7 +318,7 @@ impl Node { // // If we're not inserting a wildcard we have to create a static child. if (next != b'{' || remaining.is_escaped(0)) - && state.node().node_type != NodeType::CatchAll + && !matches!(state.node().node_type, NodeType::CatchAll(_)) { let node = state.node_mut(); @@ -350,14 +362,10 @@ impl Node { state.node_mut().priority += 1; // Make sure the route parameter matches. - if let Some(wildcard) = remaining.get(..state.node().prefix.len()) { - if *wildcard != *state.node().prefix { - return Err(InsertError::conflict(&route, remaining, state.node())); - } - } - - // Catch-all routes cannot have children. - if state.node().node_type == NodeType::CatchAll { + if remaining + .get(..state.node().prefix.len()) + .map_or(true, |wildcard| *wildcard != *state.node().prefix) + { return Err(InsertError::conflict(&route, remaining, state.node())); } @@ -479,27 +487,60 @@ impl Node { // Inserting a catch-all route. if prefix[wildcard.clone()][1] == b'*' { - // Ensure there is no suffix after the parameter, e.g. `/foo/{*x}/bar`. - if wildcard.end != prefix.len() { - return Err(InsertError::InvalidCatchAll); - } - // Add the prefix before the wildcard into the current node. if wildcard.start > 0 { node.prefix = prefix.slice_until(wildcard.start).to_owned(); prefix = prefix.slice_off(wildcard.start); } - // Add the catch-all as a child node. + let wildcard = prefix.slice_until(wildcard.len()); + prefix = prefix.slice_off(wildcard.len()); + + if prefix.is_empty() { + // Terminal catch-alls keep the value directly on the catch-all node + // and consume the rest of the path during matching. + let child = node.add_child(Node { + prefix: wildcard.to_owned(), + node_type: NodeType::CatchAll(CatchAllKind::Terminal), + value: Some(UnsafeCell::new(val)), + priority: 1, + ..Node::default() + }); + node.wild_child = true; + return Ok(&mut node.children[child]); + } + + // Non-terminal catch-alls are only delimited by complete path + // segments. Same-segment suffixes, e.g. `/{*path}.json`, would + // make the capture boundary ambiguous with the current tree shape. + if prefix[0] != b'/' { + return Err(InsertError::InvalidCatchAll); + } + + // Store the catch-all as an intermediate wildcard node and keep + // inserting the remaining static path below it. At match time, the + // static child prefix is used as the boundary for the catch-all. let child = node.add_child(Node { - prefix: prefix.to_owned(), - node_type: NodeType::CatchAll, - value: Some(UnsafeCell::new(val)), + prefix: wildcard.to_owned(), + node_type: NodeType::CatchAll(CatchAllKind::NonTerminal), priority: 1, ..Node::default() }); node.wild_child = true; - return Ok(&mut node.children[child]); + node = &mut node.children[child]; + + // If there is a static segment after the catch-all, setup the node + // for the rest of the route. + if prefix[0] != b'{' || prefix.is_escaped(0) { + node.indices.push(prefix[0]); + let child = node.add_child(Node { + priority: 1, + ..Node::default() + }); + node = &mut node.children[child]; + } + + continue; } // Otherwise, we're inserting a regular route parameter. @@ -809,7 +850,7 @@ impl Node { // Found the matching value. if let Some(ref value) = node.value { // Remap the keys of any route parameters we accumulated during the search. - params.for_each_key_mut(|(i, param)| param.key = &node.remapping[i]); + remap_params(&mut params, &node.remapping); return Ok((value, params)); } } @@ -884,8 +925,7 @@ impl Node { params.push(b"", path); // Remap the keys of any route parameters we accumulated during the search. - params - .for_each_key_mut(|(i, param)| param.key = &node.remapping[i]); + remap_params(&mut params, &node.remapping); return Ok((value, params)); } @@ -956,14 +996,14 @@ impl Node { params.push(b"", path); // Remap the keys of any route parameters we accumulated during the search. - params.for_each_key_mut(|(i, param)| param.key = &node.remapping[i]); + remap_params(&mut params, &node.remapping); return Ok((value, params)); } - NodeType::CatchAll => { - // Catch-all segments are only allowed at the end of the route, meaning - // this node must contain the value. + NodeType::CatchAll(CatchAllKind::Terminal) => { + // Terminal catch-all segments are at the end of the + // route, meaning this node must contain the value. let value = match node.value { // Found the matching value. Some(ref value) => value, @@ -973,7 +1013,7 @@ impl Node { }; // Remap the keys of any route parameters we accumulated during the search. - params.for_each_key_mut(|(i, param)| param.key = &node.remapping[i]); + remap_params(&mut params, &node.remapping); // Store the final catch-all parameter (`{*...}`). let key = &node.prefix[2..node.prefix.len() - 1]; @@ -982,6 +1022,30 @@ impl Node { return Ok((value, params)); } + NodeType::CatchAll(CatchAllKind::NonTerminal) => { + // A non-terminal catch-all must leave "enough" path + // for one of its children to match. The child prefix + // marks the point where the catch-all capture stops. + if let Some((child, boundary)) = node.children.iter().find_map(|child| { + find_wildcard_boundary(path, child) + // Empty catch-all parameters do not match. + .filter(|&boundary| boundary != 0) + .map(|boundary| (child, boundary)) + }) { + // Capture everything before the chosen boundary, + // then resume matching at the child node with the + // boundary still present in `path`. + let key = &node.prefix[2..node.prefix.len() - 1]; + params.push(key, &path[..boundary]); + path = &path[boundary..]; + node = child; + backtracking = false; + continue 'walk; + } + + break 'walk; + } + _ => unreachable!(), } } @@ -1023,6 +1087,37 @@ impl Node { } } +/// Finds the position where a non-terminal catch-all should stop capturing. +/// +/// `child` is the route node that follows the catch-all. Its prefix is the boundary marker, +/// such as `/blobs/` in `/v2/{*name}/blobs/{digest}`. The search proceeds from the end +/// of `path` so captures are greedy, but a boundary is only accepted if the child can +/// plausibly continue matching after its prefix. That avoids stopping at an earlier repeated +/// marker in paths like `/v2/a/blobs/b/blobs/sha256:abc`. +fn find_wildcard_boundary(path: &[u8], child: &Node) -> Option { + let boundary = child.prefix.as_ref(); + + if boundary.is_empty() || path.len() < boundary.len() { + return None; + } + + (0..=path.len() - boundary.len()).rev().find(|&i| { + if !path[i..].starts_with(&boundary) { + return false; + } + + // Do not accept a repeated boundary unless the child could match what follows it. + // This keeps `/v2/{*name}/blobs/{digest}` greedy for paths containing `blobs` + // inside `{*name}`. + let rest = &path[i + boundary.len()..]; + rest.is_empty() && child.value.is_some() + || rest + .first() + .map_or(false, |next| child.indices.contains(next)) + || !rest.is_empty() && child.wild_child + }) +} + /// An ordered list of route parameters keys for a specific route. /// /// To support conflicting routes like `/{a}/foo` and `/{b}/bar`, route parameters @@ -1031,6 +1126,16 @@ impl Node { /// for the given route. type ParamRemapping = Vec>; +fn remap_params<'node, 'path>(params: &mut Params<'node, 'path>, remapping: &'node ParamRemapping) { + let mut next = 0; + params.for_each_key_mut(|(_, param)| { + if param.key.is_empty() { + param.key = &remapping[next]; + next += 1; + } + }); +} + /// Returns `path` with normalized route parameters, and a parameter remapping /// to store at the node for this route. /// diff --git a/tests/insert.rs b/tests/insert.rs index 0587fdb..2cc8d9f 100644 --- a/tests/insert.rs +++ b/tests/insert.rs @@ -271,9 +271,9 @@ fn invalid_catchall() { ("/non-leading-{*catchall}", Ok(())), ("/foo/bar{*catchall}", Ok(())), ("/src/{*filepath}x", Err(InsertError::InvalidCatchAll)), - ("/src/{*filepath}/x", Err(InsertError::InvalidCatchAll)), + ("/src/{*filepath}/x", Ok(())), ("/src2/", Ok(())), - ("/src2/{*filepath}/x", Err(InsertError::InvalidCatchAll)), + ("/src2/{*filepath}/x", Ok(())), ]) .run() } diff --git a/tests/match.rs b/tests/match.rs index 97b20f7..6995cd2 100644 --- a/tests/match.rs +++ b/tests/match.rs @@ -553,6 +553,123 @@ fn catchall_overlap() { .run(); } +#[test] +fn non_terminal_catchalls() { + MatchTest { + routes: vec![ + "/v2/{*name}/", + "/v2/{*name}/blobs/{digest}", + "/v2/{*name}/manifests/{reference}", + ], + matches: vec![ + ( + "/v2/library/ubuntu/", + "/v2/{*name}/", + p! { "name" => "library/ubuntu" }, + ), + ( + "/v2/library/ubuntu/blobs/sha256:abc", + "/v2/{*name}/blobs/{digest}", + p! { "name" => "library/ubuntu", "digest" => "sha256:abc" }, + ), + ( + "/v2/team/project/image/manifests/latest", + "/v2/{*name}/manifests/{reference}", + p! { "name" => "team/project/image", "reference" => "latest" }, + ), + ( + "/v2/library/blobs/sha256:abc", + "/v2/{*name}/blobs/{digest}", + p! { "name" => "library", "digest" => "sha256:abc" }, + ), + ( + "/v2/blobs/blobs/sha256:abc", + "/v2/{*name}/blobs/{digest}", + p! { "name" => "blobs", "digest" => "sha256:abc" }, + ), + ( + "/v2/ubuntu/blobs/sha256:abc", + "/v2/{*name}/blobs/{digest}", + p! { "name" => "ubuntu", "digest" => "sha256:abc" }, + ), + ("/v2/library/ubuntu/tags/list", "", Err(())), + ], + } + .run(); +} + +#[test] +fn multiple_non_terminal_catchalls() { + MatchTest { + routes: vec!["/api/{*namespace}/repos/{*name}/manifests/{reference}"], + matches: vec![ + ( + "/api/org/team/repos/library/ubuntu/manifests/latest", + "/api/{*namespace}/repos/{*name}/manifests/{reference}", + p! { "namespace" => "org/team", "name" => "library/ubuntu", "reference" => "latest" }, + ), + ( + "/api/repos/repos/library/ubuntu/manifests/latest", + "/api/{*namespace}/repos/{*name}/manifests/{reference}", + p! { "namespace" => "repos", "name" => "library/ubuntu", "reference" => "latest" }, + ), + ( + "/api/org/team/repos/manifests/manifests/latest", + "/api/{*namespace}/repos/{*name}/manifests/{reference}", + p! { "namespace" => "org/team", "name" => "manifests", "reference" => "latest" }, + ), + ( + "/api/repos/repos/manifests/manifests/latest", + "/api/{*namespace}/repos/{*name}/manifests/{reference}", + p! { "namespace" => "repos", "name" => "manifests", "reference" => "latest" }, + ), + ( + "/api/org/repos/repos/library/repos/ubuntu/manifests/latest", + "/api/{*namespace}/repos/{*name}/manifests/{reference}", + p! { "namespace" => "org/repos/repos/library", "name" => "ubuntu", "reference" => "latest" }, + ), + ("/api/org/team/repos/library/ubuntu/tags/list", "", Err(())), + ], + } + .run(); +} + +#[test] +fn multiple_non_terminal_catchalls_with_terminal_catchall() { + MatchTest { + routes: vec!["/api/{*namespace}/repos/{*name}/blobs/{*digest}"], + matches: vec![ + ( + "/api/org/team/repos/library/ubuntu/blobs/sha256/abc", + "/api/{*namespace}/repos/{*name}/blobs/{*digest}", + p! { "namespace" => "org/team", "name" => "library/ubuntu", "digest" => "sha256/abc" }, + ), + ( + "/api/repos/repos/library/ubuntu/blobs/sha256/abc", + "/api/{*namespace}/repos/{*name}/blobs/{*digest}", + p! { "namespace" => "repos", "name" => "library/ubuntu", "digest" => "sha256/abc" }, + ), + ( + "/api/org/team/repos/blobs/blobs/sha256/abc", + "/api/{*namespace}/repos/{*name}/blobs/{*digest}", + p! { "namespace" => "org/team", "name" => "blobs", "digest" => "sha256/abc" }, + ), + ( + "/api/repos/repos/blobs/blobs/sha256/abc", + "/api/{*namespace}/repos/{*name}/blobs/{*digest}", + p! { "namespace" => "repos", "name" => "blobs", "digest" => "sha256/abc" }, + ), + ( + "/api/org/repos/repos/library/repos/ubuntu/blobs/sha256/abc", + "/api/{*namespace}/repos/{*name}/blobs/{*digest}", + p! { "namespace" => "org/repos/repos/library", "name" => "ubuntu", "digest" => "sha256/abc" }, + ), + ("/api/org/team/repos/library/ubuntu/tags/list", "", Err(())), + ], + } + .run(); +} + #[test] fn escaped() { MatchTest { From 74a1ecb57102eee3fb6de951df1ac8fa9614167d Mon Sep 17 00:00:00 2001 From: NVT Date: Sat, 6 Jun 2026 16:46:09 +0200 Subject: [PATCH 2/2] Allow backtracking --- src/tree.rs | 185 +++++++++++++++++++++++++++++++++++++------------ tests/match.rs | 156 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 295 insertions(+), 46 deletions(-) diff --git a/src/tree.rs b/src/tree.rs index ed873ca..c8e9dff 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -812,18 +812,44 @@ impl Node { } } -/// A wildcard node that was skipped during a tree search. +/// A backtrack record captured during a tree search. /// -/// Contains the state necessary to backtrack to the given node. -struct Skipped<'node, 'path, T> { - // The node that was skipped. - node: &'node Node, +/// Contains the state necessary to resume the search from an alternative branch. +enum Skipped<'node, 'path, T> { + /// A wildcard node that was skipped in favor of a matching static child. + /// + /// We may end up needing to backtrack to the wildcard if the static branch + /// does not lead to a match. + Wild { + // The node that was skipped. + node: &'node Node, - /// The path at the time we skipped this node. - path: &'path [u8], + /// The path at the time we skipped this node. + path: &'path [u8], - // The number of parameters that were present. - params: usize, + // The number of parameters that were present. + params: usize, + }, + + /// A non-terminal catch-all node that committed to a particular boundary. + /// + /// The catch-all greedily captures up to the largest boundary that plausibly + /// allows a child to match. If that boundary leads to a dead end, we need to + /// resume here and try the next-shorter boundary. + CatchAllBoundary { + /// The catch-all node. + node: &'node Node, + + /// The full path at the catch-all node, including the captured segment. + path: &'path [u8], + + // The number of parameters that were present before the catch-all capture. + params: usize, + + /// The boundary that was committed to. The next attempt must find a + /// boundary strictly less than this. + boundary: usize, + }, } impl Node { @@ -881,7 +907,7 @@ impl Node { // We may end up needing to backtrack later in case we do not find a // match. if node.wild_child { - skipped.push(Skipped { + skipped.push(Skipped::Wild { node, path: previous, params: params.len(), @@ -1026,12 +1052,23 @@ impl Node { // A non-terminal catch-all must leave "enough" path // for one of its children to match. The child prefix // marks the point where the catch-all capture stops. - if let Some((child, boundary)) = node.children.iter().find_map(|child| { - find_wildcard_boundary(path, child) - // Empty catch-all parameters do not match. - .filter(|&boundary| boundary != 0) - .map(|boundary| (child, boundary)) - }) { + // + // The greediest plausible boundary may still lead to a + // dead end (the one-byte child heuristic is not exact), + // so we record a backtrack entry to resume here and try + // the next-shorter boundary if the chosen branch fails. + if let Some((child, boundary)) = + find_wildcard_boundary(path, node, usize::MAX) + { + // Record a way to resume at this catch-all and try + // a strictly shorter boundary on failure. + skipped.push(Skipped::CatchAllBoundary { + node, + path, + params: params.len(), + boundary, + }); + // Capture everything before the chosen boundary, // then resume matching at the child node with the // boundary still present in `path`. @@ -1050,16 +1087,53 @@ impl Node { } } - // Try backtracking to any matching wildcard nodes that we skipped while + // Try backtracking to any alternative branches that we skipped while // traversing the tree. - while let Some(skipped) = skipped.pop() { - if skipped.path.ends_with(path) { - // Found a matching node, restore the search state. - path = skipped.path; - node = skipped.node; - backtracking = true; - params.truncate(skipped.params); - continue 'backtrack; + while let Some(skip) = skipped.pop() { + match skip { + Skipped::Wild { + node: skip_node, + path: skip_path, + params: skip_params, + } => { + if skip_path.ends_with(path) { + // Found a matching node, restore the search state. + path = skip_path; + node = skip_node; + backtracking = true; + params.truncate(skip_params); + continue 'backtrack; + } + } + + Skipped::CatchAllBoundary { + node: skip_node, + path: skip_path, + params: skip_params, + boundary, + } => { + // Try the next-shorter boundary at this catch-all node. + if let Some((child, next_boundary)) = + find_wildcard_boundary(skip_path, skip_node, boundary) + { + // Restore state to the catch-all and commit to the + // shorter boundary, recording the next attempt. + params.truncate(skip_params); + skipped.push(Skipped::CatchAllBoundary { + node: skip_node, + path: skip_path, + params: skip_params, + boundary: next_boundary, + }); + + let key = &skip_node.prefix[2..skip_node.prefix.len() - 1]; + params.push(key, &skip_path[..next_boundary]); + path = &skip_path[next_boundary..]; + node = child; + backtracking = false; + continue 'backtrack; + } + } } } @@ -1089,32 +1163,51 @@ impl Node { /// Finds the position where a non-terminal catch-all should stop capturing. /// -/// `child` is the route node that follows the catch-all. Its prefix is the boundary marker, -/// such as `/blobs/` in `/v2/{*name}/blobs/{digest}`. The search proceeds from the end -/// of `path` so captures are greedy, but a boundary is only accepted if the child can +/// `node` is the catch-all node; each of its children's prefixes is a candidate boundary +/// marker, such as `/blobs/` in `/v2/{*name}/blobs/{digest}`. The search proceeds from the +/// end of `path` so captures are greedy, but a boundary is only accepted if the child can /// plausibly continue matching after its prefix. That avoids stopping at an earlier repeated /// marker in paths like `/v2/a/blobs/b/blobs/sha256:abc`. -fn find_wildcard_boundary(path: &[u8], child: &Node) -> Option { - let boundary = child.prefix.as_ref(); - - if boundary.is_empty() || path.len() < boundary.len() { - return None; - } +/// +/// The one-byte plausibility check is a heuristic, not a full subtree match, so the greediest +/// candidate may still lead to a dead end. `max_boundary` lets callers resume the search: +/// only boundaries strictly less than `max_boundary` are considered, so a failed branch can +/// be retried with the next-shorter boundary. Pass `usize::MAX` for the initial (greediest) +/// search. +/// +/// Returns the matching child together with the chosen boundary, where the boundary is the +/// number of bytes the catch-all captures (never zero, as empty captures do not match). +fn find_wildcard_boundary<'node, T>( + path: &[u8], + node: &'node Node, + max_boundary: usize, +) -> Option<(&'node Node, usize)> { + node.children.iter().find_map(|child| { + let boundary = child.prefix.as_ref(); - (0..=path.len() - boundary.len()).rev().find(|&i| { - if !path[i..].starts_with(&boundary) { - return false; + if boundary.is_empty() || path.len() < boundary.len() { + return None; } - // Do not accept a repeated boundary unless the child could match what follows it. - // This keeps `/v2/{*name}/blobs/{digest}` greedy for paths containing `blobs` - // inside `{*name}`. - let rest = &path[i + boundary.len()..]; - rest.is_empty() && child.value.is_some() - || rest - .first() - .map_or(false, |next| child.indices.contains(next)) - || !rest.is_empty() && child.wild_child + let start = (path.len() - boundary.len()).min(max_boundary.saturating_sub(1)); + + (1..=start).rev().find_map(|i| { + if !path[i..].starts_with(&boundary) { + return None; + } + + // Do not accept a repeated boundary unless the child could match what follows it. + // This keeps `/v2/{*name}/blobs/{digest}` greedy for paths containing `blobs` + // inside `{*name}`. + let rest = &path[i + boundary.len()..]; + let plausible = rest.is_empty() && child.value.is_some() + || rest + .first() + .map_or(false, |next| child.indices.contains(next)) + || !rest.is_empty() && child.wild_child; + + plausible.then_some((child, i)) + }) }) } diff --git a/tests/match.rs b/tests/match.rs index 6995cd2..d251caa 100644 --- a/tests/match.rs +++ b/tests/match.rs @@ -598,6 +598,95 @@ fn non_terminal_catchalls() { .run(); } +// A non-terminal catch-all followed by sibling literals must be able to backtrack across boundary +// positions when the greediest boundary leads to a dead end. +#[test] +fn non_terminal_catchall_boundary_backtracking() { + let mut router = Router::new(); + router + .insert("/v2/{repository}/{*image}/blobs/{digest}", "blob") + .unwrap(); + router + .insert( + "/v2/{repository}/{*image}/manifests/{reference}", + "manifest", + ) + .unwrap(); + router + .insert("/v2/{repository}/{*image}/tags/list", "tags") + .unwrap(); + router + .insert("/v2/{repository}/{*image}/referrers/{digest}", "referrers") + .unwrap(); + + // The shared `/` node has children with indices b, m, t, r. A final `manifests/{tag}` segment + // must route regardless of the tag's first character. + for tag in [ + "latest", + "v1", + "test", + "main", + "master", + "beta", + "release", + "rc1", + "bookworm", + "tags", + "blobs", + "referrers", + "manifests", + ] { + let path = format!("/v2/repo/myimg/manifests/{tag}"); + let m = router + .at(&path) + .unwrap_or_else(|e| panic!("{e} for {path}")); + assert_eq!(*m.value, "manifest", "value for {path}"); + assert_eq!(m.params.get("image"), Some("myimg"), "image for {path}"); + assert_eq!(m.params.get("reference"), Some(tag), "reference for {path}"); + } + + // Multi-segment image is still captured greedily. + let m = router.at("/v2/repo/team/sub/manifests/latest").unwrap(); + assert_eq!(*m.value, "manifest"); + assert_eq!(m.params.get("image"), Some("team/sub")); + assert_eq!(m.params.get("reference"), Some("latest")); + + // An image name that itself contains a sibling literal: the boundary must + // be the LAST occurrence, leaving the trailing literal to route. + let m = router + .at("/v2/repo/team/manifests/blobs/sha256abc") + .unwrap(); + assert_eq!(*m.value, "blob"); + assert_eq!(m.params.get("image"), Some("team/manifests")); + assert_eq!(m.params.get("digest"), Some("sha256abc")); + + let m = router.at("/v2/repo/team/blobs/manifests/v1").unwrap(); + assert_eq!(*m.value, "manifest"); + assert_eq!(m.params.get("image"), Some("team/blobs")); + assert_eq!(m.params.get("reference"), Some("v1")); + + // Other sibling routes still work. + let m = router.at("/v2/repo/myimg/tags/list").unwrap(); + assert_eq!(*m.value, "tags"); + assert_eq!(m.params.get("image"), Some("myimg")); + + let m = router.at("/v2/repo/myimg/blobs/sha256abc").unwrap(); + assert_eq!(*m.value, "blob"); + assert_eq!(m.params.get("image"), Some("myimg")); + assert_eq!(m.params.get("digest"), Some("sha256abc")); + + let m = router.at("/v2/repo/myimg/referrers/sha256abc").unwrap(); + assert_eq!(*m.value, "referrers"); + assert_eq!(m.params.get("image"), Some("myimg")); + assert_eq!(m.params.get("digest"), Some("sha256abc")); + + // A genuinely unmatched path still returns NotFound. + assert_eq!( + router.at("/v2/repo/img/bogus/x").unwrap_err(), + MatchError::NotFound, + ); +} + #[test] fn multiple_non_terminal_catchalls() { MatchTest { @@ -634,6 +723,73 @@ fn multiple_non_terminal_catchalls() { .run(); } +// With sibling literals after a non-terminal catch-all, boundary backtracking must pick the correct +// branch even when the final segment's first character collides with a sibling literal's first +// character. Here `manifests` and `tags` share no first character, but `m` (from a tag like `main`) +// collides with `manifests`, and `t` (from `test`) collides with `tags`. +#[test] +fn non_terminal_catchall_sibling_literal_collisions() { + MatchTest { + routes: vec![ + "/v2/{*image}/blobs/{digest}", + "/v2/{*image}/manifests/{reference}", + "/v2/{*image}/tags/list", + "/v2/{*image}/referrers/{digest}", + ], + matches: vec![ + ( + "/v2/myimg/manifests/test", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "test" }, + ), + ( + "/v2/myimg/manifests/main", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "main" }, + ), + ( + "/v2/myimg/manifests/bookworm", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "bookworm" }, + ), + ( + "/v2/myimg/manifests/rc1", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "rc1" }, + ), + ( + "/v2/myimg/manifests/release", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "release" }, + ), + ( + "/v2/myimg/manifests/tags", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "tags" }, + ), + ( + "/v2/myimg/manifests/blobs", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "myimg", "reference" => "blobs" }, + ), + // Image name itself contains a sibling literal segment. + ( + "/v2/team/manifests/blobs/sha256abc", + "/v2/{*image}/blobs/{digest}", + p! { "image" => "team/manifests", "digest" => "sha256abc" }, + ), + ( + "/v2/team/blobs/manifests/v1", + "/v2/{*image}/manifests/{reference}", + p! { "image" => "team/blobs", "reference" => "v1" }, + ), + // No matching route after the catch-all. + ("/v2/img/bogus/x", "", Err(())), + ], + } + .run(); +} + #[test] fn multiple_non_terminal_catchalls_with_terminal_catchall() { MatchTest {