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.

Sign up for GitHub

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

Jump to bottom

Remove Trailing Slash Redirects #36

Merged
merged 5 commits into from
Feb 3, 2024
Merged

Remove Trailing Slash Redirects #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f7976b
remove trailing slash redirects
ibraheemdev Aug 2, 2023
ba4efdf
update benchmark results
ibraheemdev Aug 2, 2023
7ff2ca9
remove missing backtrack case
ibraheemdev Aug 2, 2023
6d703bf
add trailing slash tests
ibraheemdev Feb 3, 2024
eff698a
merge into master
ibraheemdev Feb 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ in under 200 nanoseconds, an order of magnitude faster than most other routers.

```text
Compare Routers/matchit
time: [197.57 ns 198.74 ns 199.83 ns]
time: [175.96 ns 176.39 ns 176.84 ns]

Compare Routers/actix
time: [26.805 us 26.811 us 26.816 us]
Expand Down
32 changes: 1 addition & 31 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/// Represents errors that can occur when inserting a new route.
#[non_exhaustive]
#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub enum InsertError {

Check warning on line 8 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

item name ends with its containing module's name

Check warning on line 8 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

item name ends with its containing module's name
/// Attempted to insert a path that conflicts with an existing route.
Conflict {
/// The existing route that the insertion is conflicting with.
Expand All @@ -23,7 +23,7 @@
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Conflict { with } => {
write!(

Check warning on line 26 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

variables can be used directly in the `format!` string

Check warning on line 26 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

variables can be used directly in the `format!` string
f,
"insertion failed due to conflict with previously registered route: {}",
with
Expand All @@ -47,7 +47,7 @@
if prefix == current.prefix {
let mut route = route.to_owned();
denormalize_params(&mut route, &current.param_remapping);
return InsertError::Conflict {

Check warning on line 50 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

unnecessary structure name repetition

Check warning on line 50 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

unnecessary structure name repetition
with: String::from_utf8(route).unwrap(),
};
}
Expand All @@ -71,7 +71,7 @@

denormalize_params(&mut route, &last.param_remapping);

InsertError::Conflict {

Check warning on line 74 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

unnecessary structure name repetition

Check warning on line 74 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

unnecessary structure name repetition
with: String::from_utf8(route).unwrap(),
}
}
Expand All @@ -86,16 +86,6 @@
/// router.insert("/home", "Welcome!")?;
/// router.insert("/blog/", "Our blog.")?;
///
/// // a route exists without the trailing slash
/// if let Err(err) = router.at("/home/") {
/// assert_eq!(err, MatchError::ExtraTrailingSlash);
/// }
///
/// // a route exists with a trailing slash
/// if let Err(err) = router.at("/blog") {
/// assert_eq!(err, MatchError::MissingTrailingSlash);
/// }
///
/// // no routes match
/// if let Err(err) = router.at("/foobar") {
/// assert_eq!(err, MatchError::NotFound);
Expand All @@ -103,34 +93,14 @@
/// # Ok(())
/// # }
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum MatchError {

Check warning on line 96 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

item name ends with its containing module's name

Check warning on line 96 in src/error.rs

View workflow job for this annotation

GitHub Actions / Clippy Lints 

item name ends with its containing module's name
/// The path was missing a trailing slash.
MissingTrailingSlash,
/// The path had an extra trailing slash.
ExtraTrailingSlash,
/// No matching route was found.
NotFound,
}

impl MatchError {
pub(crate) fn unsure(full_path: &[u8]) -> Self {
if full_path[full_path.len() - 1] == b'/' {
MatchError::ExtraTrailingSlash
} else {
MatchError::MissingTrailingSlash
}
}
}

impl fmt::Display for MatchError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let msg = match self {
MatchError::MissingTrailingSlash => "match error: expected trailing slash",
MatchError::ExtraTrailingSlash => "match error: found extra trailing slash",
MatchError::NotFound => "match error: route not found",
};

write!(f, "{}", msg)
write!(f, "matching route not found")
}
}

Expand Down
78 changes: 5 additions & 73 deletions src/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,14 +363,6 @@
});
}

// child won't match because of an extra trailing slash
if path == b"/"
&& current.children[i].prefix != b"/"
&& current.value.is_some()
{
return Err(MatchError::ExtraTrailingSlash);
}

// continue with the child node
current = &current.children[i];
continue 'walk;
Expand All @@ -379,15 +371,8 @@

// we didn't find a match and there are no children with wildcards, there is no match
if !current.wild_child {
// extra trailing slash
if path == b"/" && current.value.is_some() {
return Err(MatchError::ExtraTrailingSlash);
}

// try backtracking
if path != b"/" {
try_backtrack!();
}
try_backtrack!();

// nothing found
return Err(MatchError::NotFound);
Expand All @@ -404,14 +389,6 @@
let (param, rest) = path.split_at(i);

if let [child] = current.children.as_slice() {
// child won't match because of an extra trailing slash
if rest == b"/"
&& child.prefix != b"/"
&& current.value.is_some()
{
return Err(MatchError::ExtraTrailingSlash);
}

// store the parameter value
params.push(&current.prefix[1..], param);

Expand All @@ -422,16 +399,8 @@
continue 'walk;
}

// this node has no children yet the path has more segments...
// either the path has an extra trailing slash or there is no match
if path.len() == i + 1 {
return Err(MatchError::ExtraTrailingSlash);
}

// try backtracking
if path != b"/" {
try_backtrack!();
}
try_backtrack!();

return Err(MatchError::NotFound);
}
Expand All @@ -450,22 +419,8 @@
return Ok((value, params));
}

// check the child node in case the path is missing a trailing slash
if let [child] = current.children.as_slice() {
current = child;

if (current.prefix == b"/" && current.value.is_some())
|| (current.prefix.is_empty()
&& current.indices == b"/")
{
return Err(MatchError::MissingTrailingSlash);
}

// no match, try backtracking
if path != b"/" {
try_backtrack!();
}
}
// no match, try backtracking
try_backtrack!();

// this node doesn't have the value, no match
return Err(MatchError::NotFound);
Expand Down Expand Up @@ -506,34 +461,11 @@
// nope, try backtracking
try_backtrack!();

// TODO: does this *always* means there is an extra trailing slash?
if path == b"/" && current.wild_child && current.node_type != NodeType::Root {
return Err(MatchError::unsure(full_path));
}

if !backtracking {
// check if the path is missing a trailing slash
if let Some(i) = current.indices.iter().position(|&c| c == b'/') {
current = &current.children[i];

if current.prefix.len() == 1 && current.value.is_some() {
return Err(MatchError::MissingTrailingSlash);
}
}
}

return Err(MatchError::NotFound);
}

// nothing matches, check for a missing trailing slash
if current.prefix.split_last() == Some((&b'/', path)) && current.value.is_some() {
return Err(MatchError::MissingTrailingSlash);
}

// last chance, try backtracking
if path != b"/" {
try_backtrack!();
}
try_backtrack!();

return Err(MatchError::NotFound);
}
Expand Down Expand Up @@ -590,7 +522,7 @@
wildcard_index += start;

// normalize the parameter
let removed = path.splice(

Check warning on line 525 in src/tree.rs

View workflow job for this annotation

GitHub Actions / Cargo Check 

cannot borrow `path` as mutable because it is also borrowed as immutable

Check warning on line 525 in src/tree.rs

View workflow job for this annotation

GitHub Actions / Cargo Check 

cannot borrow `path` as mutable because it is also borrowed as immutable
(wildcard_index)..(wildcard_index + wildcard.len()),
vec![b':', next],
);
Expand Down Expand Up @@ -628,7 +560,7 @@
};

// denormalize this parameter
route.splice(

Check warning on line 563 in src/tree.rs

View workflow job for this annotation

GitHub Actions / Cargo Check 

cannot borrow `*route` as mutable because it is also borrowed as immutable

Check warning on line 563 in src/tree.rs

View workflow job for this annotation

GitHub Actions / Cargo Check 

cannot borrow `*route` as mutable because it is also borrowed as immutable
(wildcard_index)..(wildcard_index + wildcard.len()),
next.clone(),
);
Expand Down
Loading
Loading