Skip to content

Commit

Permalink
Merge pull request #201 from DuskSystems/196-prevent-touching-parameters
Browse files Browse the repository at this point in the history 
Prevent touching parameters.
  • Loading branch information
@CathalMullan
CathalMullan authored Nov 12, 2024
2 parents 00e7065 + fa5172b commit 0843802
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 89 deletions.
32 changes: 16 additions & 16 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,29 +404,29 @@ In a router of 130 routes, benchmark matching 4 paths.

| Library | Time | Alloc Count | Alloc Size | Dealloc Count | Dealloc Size |
|:-----------------|----------:|------------:|-----------:|--------------:|-------------:|
| wayfind | 399.06 ns | 4 | 265 B | 4 | 265 B |
| matchit | 471.85 ns | 4 | 416 B | 4 | 448 B |
| path-tree | 581.91 ns | 4 | 416 B | 4 | 448 B |
| xitca-router | 583.90 ns | 7 | 800 B | 7 | 832 B |
| ntex-router | 1.6998 µs | 18 | 1.248 KB | 18 | 1.28 KB |
| route-recognizer | 4.6455 µs | 160 | 8.505 KB | 160 | 8.537 KB |
| routefinder | 6.4037 µs | 67 | 5.024 KB | 67 | 5.056 KB |
| actix-router | 20.932 µs | 214 | 13.93 KB | 214 | 13.96 KB |
| wayfind | 394.86 ns | 4 | 265 B | 4 | 265 B |
| matchit | 493.32 ns | 4 | 416 B | 4 | 448 B |
| xitca-router | 574.70 ns | 7 | 800 B | 7 | 832 B |
| path-tree | 609.10 ns | 4 | 416 B | 4 | 448 B |
| ntex-router | 1.7254 µs | 18 | 1.248 KB | 18 | 1.28 KB |
| route-recognizer | 4.6767 µs | 160 | 8.505 KB | 160 | 8.537 KB |
| routefinder | 6.4543 µs | 67 | 5.024 KB | 67 | 5.056 KB |
| actix-router | 20.905 µs | 214 | 13.93 KB | 214 | 13.96 KB |

#### `path-tree` inspired benches

In a router of 320 routes, benchmark matching 80 paths.

| Library | Time | Alloc Count | Alloc Size | Dealloc Count | Dealloc Size |
|:-----------------|----------:|------------:|-----------:|--------------:|-------------:|
| wayfind | 5.7101 µs | 59 | 2.567 KB | 59 | 2.567 KB |
| path-tree | 8.8842 µs | 59 | 7.447 KB | 59 | 7.47 KB |
| matchit | 9.0232 µs | 140 | 17.81 KB | 140 | 17.83 KB |
| xitca-router | 10.734 µs | 209 | 25.51 KB | 209 | 25.53 KB |
| ntex-router | 29.890 µs | 201 | 19.54 KB | 201 | 19.56 KB |
| route-recognizer | 91.900 µs | 2872 | 191.7 KB | 2872 | 204.8 KB |
| routefinder | 100.51 µs | 525 | 48.4 KB | 525 | 48.43 KB |
| actix-router | 176.08 µs | 2201 | 128.8 KB | 2201 | 128.8 KB |
| wayfind | 5.8725 µs | 59 | 2.567 KB | 59 | 2.567 KB |
| path-tree | 9.0383 µs | 59 | 7.447 KB | 59 | 7.47 KB |
| matchit | 9.1017 µs | 140 | 17.81 KB | 140 | 17.83 KB |
| xitca-router | 10.762 µs | 209 | 25.51 KB | 209 | 25.53 KB |
| ntex-router | 30.119 µs | 201 | 19.54 KB | 201 | 19.56 KB |
| route-recognizer | 92.110 µs | 2872 | 191.7 KB | 2872 | 204.8 KB |
| routefinder | 101.03 µs | 525 | 48.4 KB | 525 | 48.43 KB |
| actix-router | 178.83 µs | 2201 | 128.8 KB | 2201 | 128.8 KB |

## License

Expand Down
50 changes: 50 additions & 0 deletions src/errors/route.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,39 @@ pub enum RouteError {
/// The length of the parameter (including braces).
length: usize,
},

/// Two parameters side by side were found in the route.
///
/// # Examples
///
/// ```rust
/// use wayfind::errors::RouteError;
///
/// let error = RouteError::TouchingParameters {
/// route: "/{a}{b}".to_string(),
/// start: 1,
/// length: 6,
/// };
///
/// let display = "
/// touching parameters
///
/// Route: /{a}{b}
/// ^^^^^^
///
/// tip: Touching parameters are not supported
/// ";
///
/// assert_eq!(error.to_string(), display.trim());
/// ```
TouchingParameters {
/// The route containing touching parameters.
route: String,
/// The position of the first opening brace.
start: usize,
/// The combined length of both parameters (including braces).
length: usize,
},
}

impl std::error::Error for RouteError {}
Expand Down Expand Up @@ -534,6 +567,23 @@ tip: Parameter names must be unique within a route"#
tip: Constraint names must not contain the characters: ':', '*', '{{', '}}', '(', ')', '/'"#
)
}

Self::TouchingParameters {
route,
start,
length,
} => {
let arrow = " ".repeat(*start) + &"^".repeat(*length);
write!(
f,
r#"touching parameters
Route: {route}
{arrow}
tip: Touching parameters are not supported"#
)
}
}
}
}
Expand Down
50 changes: 44 additions & 6 deletions src/parser.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::errors::{EncodingError, RouteError};
use rustc_hash::FxHashMap;
use smallvec::{smallvec, SmallVec};

/// Characters that are not allowed in parameter names or constraints.
const INVALID_PARAM_CHARS: [u8; 7] = [b':', b'*', b'{', b'}', b'(', b')', b'/'];
Expand Down Expand Up @@ -157,26 +157,42 @@ impl Parser {
let mut parts = vec![];
let mut cursor = 0;

let mut seen_parameters: FxHashMap<String, (usize, usize)> = FxHashMap::default();
// Parameters in order (name, start, length)
let mut seen_parameters: SmallVec<[(String, usize, usize); 4]> = smallvec![];

while cursor < raw.len() {
match raw[cursor] {
b'{' => {
let (part, next_cursor) = Self::parse_parameter_part(raw, cursor)?;

// Check for touching parameters.
if let Some((_, start, length)) = seen_parameters.last() {
if cursor == start + length {
return Err(RouteError::TouchingParameters {
route: String::from_utf8_lossy(raw).to_string(),
start: *start,
length: next_cursor - start,
});
}
}

// Check for duplicate names.
if let Part::Dynamic { name, .. } | Part::Wildcard { name, .. } = &part {
if let Some(&(first, first_length)) = seen_parameters.get(name) {
if let Some((_, start, length)) = seen_parameters
.iter()
.find(|(existing, _, _)| existing == name)
{
return Err(RouteError::DuplicateParameter {
route: String::from_utf8_lossy(raw).to_string(),
name: name.to_string(),
first,
first_length,
first: *start,
first_length: *length,
second: cursor,
second_length: next_cursor - cursor,
});
}

seen_parameters.insert(name.clone(), (cursor, next_cursor - cursor));
seen_parameters.push((name.clone(), cursor, next_cursor - cursor));
}

parts.push(part);
Expand Down Expand Up @@ -917,4 +933,26 @@ mod tests {
tip: Constraint names must not contain the characters: ':', '*', '{', '}', '(', ')', '/'
"#);
}

#[test]
fn test_parser_error_touching_parameters() {
let error = Parser::new(b"/users/{id}{name}").unwrap_err();
assert_eq!(
error,
RouteError::TouchingParameters {
route: "/users/{id}{name}".to_string(),
start: 7,
length: 10,
}
);

insta::assert_snapshot!(error, @r"
touching parameters
Route: /users/{id}{name}
^^^^^^^^^^
tip: Touching parameters are not supported
");
}
}
30 changes: 1 addition & 29 deletions tests/constraint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,35 +160,7 @@ fn test_constraint_builtin() -> Result<(), Box<dyn Error>> {
Ok(())
}

// FIXME: This feels like it could work. Might be better off erroring for all touching params.
#[test]
fn test_constraint_touching() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
router.constraint::<NameConstraint>()?;
router.insert("/users/{first:name}{second:u32}", 1)?;

insta::assert_snapshot!(router, @r"
/users/
╰─ {first:name}
╰─ {second:u32} [*]
");

let path = Path::new("/users/abc123")?;
let search = router.search(&path)?;
assert_eq!(search, None);

let path = Path::new("/users/abcdef")?;
let search = router.search(&path)?;
assert_eq!(search, None);

let path = Path::new("/users/123abc")?;
let search = router.search(&path)?;
assert_eq!(search, None);

Ok(())
}

// FIXME: Not really happy with this either. But no real way we could prevent unreachable routes at the constraint layer.
// NOTE: Not really happy with this. But no real way we could prevent unreachable routes at the constraint layer.
#[test]
fn test_constraint_unreachable() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
Expand Down
19 changes: 0 additions & 19 deletions tests/dynamic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,25 +184,6 @@ fn test_dynamic_greedy() -> Result<(), Box<dyn Error>> {
Ok(())
}

// FIXME: Should really be an error at insert time.
#[test]
fn test_dynamic_touching() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
router.insert("/{first}{second}", 1)?;

insta::assert_snapshot!(router, @r"
/
╰─ {first}
╰─ {second} [*]
");

let path = Path::new("/hello")?;
let search = router.search(&path)?;
assert_eq!(search, None);

Ok(())
}

#[test]
fn test_dynamic_priority() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
Expand Down
19 changes: 0 additions & 19 deletions tests/wildcard.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,25 +168,6 @@ fn test_wildcard_greedy() -> Result<(), Box<dyn Error>> {
Ok(())
}

// FIXME: Should really be an error at insert time.
#[test]
fn test_wildcard_touching() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
router.insert("/{*first}{*second}", 1)?;

insta::assert_snapshot!(router, @r"
/
╰─ {*first}
╰─ {*second} [*]
");

let path = Path::new("/hello")?;
let search = router.search(&path)?;
assert_eq!(search, None);

Ok(())
}

#[test]
fn test_wildcard_empty_segments() -> Result<(), Box<dyn Error>> {
let mut router = Router::new();
Expand Down

0 comments on commit 0843802

Please sign in to comment.