ibraheemdev / matchit

A high performance, zero-copy URL router.
https://docs.rs/matchit
MIT License
366 stars 37 forks source link

Overlapping wildcard and non-wildcard routes cause failing matches #31

Closed ColonelThirtyTwo closed 8 months ago

ColonelThirtyTwo commented 1 year ago

Example with matchit 0.7.0, rust 1.69.0:


use matchit::Router;

#[test]
fn fixed_route() {
    let mut router = Router::new();
    router.insert("/path/foo", "foo").unwrap();
    router.insert("/path/*rest", "wildcard").unwrap();

    assert_eq!(router.at("/path/foo").map(|m| *m.value), Ok("foo"));
    assert_eq!(router.at("/path/bar").map(|m| *m.value), Ok("wildcard"));
    //assert_eq!(router.at("/path/foo/").map(|m| *m.value), Ok("wildcard")); // Err(ExtraTrailingSlash)
}

#[test]
fn param_route() {
    let mut router = Router::new();
    router.insert("/path/foo/:arg", "foo").unwrap();
    router.insert("/path/*rest", "wildcard").unwrap();

    assert_eq!(router.at("/path/foo/myarg").map(|m| *m.value), Ok("foo"));
    //assert_eq!(router.at("/path/foo/myarg/").map(|m| *m.value), Ok("wildcard"));  // Err(ExtraTrailingSlash)
    //assert_eq!(router.at("/path/foo/myarg/bar/baz").map(|m| *m.value), Ok("wildcard"));  // Err(NotFound)
}

All of the commented out assertions fail, even though they should match the wildcard route.

ibraheemdev commented 1 year ago

Taking a look at this, your first two failing assertions seem to hit a an edge case where we would have to first backtrack, and restore the tsr state if backtracking fails. It brings me back to wondering whether tsr errors are even worth it over axum's approach (my registering both routes with and without the trailing slash).

Your last assertion is a clear bug where backtracking isn't taking place. At first glance seems similar to https://github.com/ibraheemdev/matchit/issues/12.

ColonelThirtyTwo commented 1 year ago

I personally find the trailing slash error weird. I feel that a path should match a route, or not match a route, and that the ExtraTrailingSlash is a weird quasi-matched state that apparently is causing issues. I don't believe adding a route twice - one with and one without a slash - is any more difficult than adding extra code to handle ExtraTrailingSlash.

ibraheemdev commented 1 year ago

Just released 0.7.2 which contains a partial fix (only covering your last assertion). The cases involving trailing slashes will be fixed as part of https://github.com/ibraheemdev/matchit/issues/35.

ibraheemdev commented 8 months ago

Trailing slashes are removed as part of 0.8, so this should no longer be an issue.