Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ColonelThirtyTwo opened this issue May 8, 2023 · 4 comments
Closed
Labels
bug Something isn't working

Comments

@ColonelThirtyTwo
Copy link

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
Copy link
Owner

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 #12.

@ColonelThirtyTwo
Copy link
Author

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.

This was referenced Jul 28, 2023
@ibraheemdev ibraheemdev reopened this Aug 3, 2023
@ibraheemdev
Copy link
Owner

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 #35.

@ibraheemdev
Copy link
Owner

ibraheemdev commented Mar 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants