From ac0a77bae228a6adf7437dcc9144d81264116645 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Fri, 7 Apr 2023 10:46:53 -0700 Subject: [PATCH] Allow dynamic parameters to match empty segments. The net effect of this commit is three-fold: * A request to `/` now matches `/`. `/foo/` matches `//`. * A segment matched to a dynamic parameter may be empty. * A request to `/foo/` no longer matches `/foo` or `/`. Instead, such a request would match `/foo/` or `/foo/`. The `&str` and `String` parameter guards were updated to reflect this change: they now error, with a newly introduced error type `Empty` in the `rocket::error` module, when the parameter is empty. As this was the only built-in parameter guard that would be effected by this change (all other guards already required nonempty parameters to succeed), the majority of applications will see no effect as a result. For applications wanting the previous functionality, a new `AdHoc::uri_normalizer()` fairing was introduced. --- .../ui-fail-nightly/typed-uri-bad-type.stderr | 4 ++-- .../typed-uris-bad-params.stderr | 4 ++-- .../ui-fail-stable/typed-uri-bad-type.stderr | 4 ++-- .../typed-uris-bad-params.stderr | 4 ++-- core/lib/src/error.rs | 18 ++++++++++++++++++ core/lib/src/request/from_param.rs | 14 +++++++++++--- core/lib/src/request/request.rs | 6 ++++-- core/lib/src/route/uri.rs | 2 -- core/lib/src/router/collider.rs | 14 ++++++++------ core/lib/src/router/matcher.rs | 9 ++------- core/lib/src/router/router.rs | 7 ++++--- examples/hello/src/main.rs | 4 ---- 12 files changed, 55 insertions(+), 35 deletions(-) diff --git a/core/codegen/tests/ui-fail-nightly/typed-uri-bad-type.stderr b/core/codegen/tests/ui-fail-nightly/typed-uri-bad-type.stderr index 9d41bd6b..af0c2e50 100644 --- a/core/codegen/tests/ui-fail-nightly/typed-uri-bad-type.stderr +++ b/core/codegen/tests/ui-fail-nightly/typed-uri-bad-type.stderr @@ -2,13 +2,13 @@ error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-nightly/typed-uri-bad-type.rs:22:37 | 22 | fn optionals(id: Option, name: Result) { } - | ^^^^^^^^^^^^^^^^^^^^ expected `Infallible`, found `&str` + | ^^^^^^^^^^^^^^^^^^^^ expected `Empty`, found `&str` error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-nightly/typed-uri-bad-type.rs:22:37 | 22 | fn optionals(id: Option, name: Result) { } - | ^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `Infallible` + | ^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `Empty` error[E0277]: the trait bound `usize: FromUriParam` is not satisfied --> tests/ui-fail-nightly/typed-uri-bad-type.rs:45:22 diff --git a/core/codegen/tests/ui-fail-nightly/typed-uris-bad-params.stderr b/core/codegen/tests/ui-fail-nightly/typed-uris-bad-params.stderr index 4afd2f50..536db778 100644 --- a/core/codegen/tests/ui-fail-nightly/typed-uris-bad-params.stderr +++ b/core/codegen/tests/ui-fail-nightly/typed-uris-bad-params.stderr @@ -285,10 +285,10 @@ error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-nightly/typed-uris-bad-params.rs:15:37 | 15 | fn optionals(id: Option, name: Result) { } - | ^^^^^^^^^^^^^^^^^^^^ expected `Infallible`, found `&str` + | ^^^^^^^^^^^^^^^^^^^^ expected `Empty`, found `&str` error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-nightly/typed-uris-bad-params.rs:15:37 | 15 | fn optionals(id: Option, name: Result) { } - | ^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `Infallible` + | ^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `Empty` diff --git a/core/codegen/tests/ui-fail-stable/typed-uri-bad-type.stderr b/core/codegen/tests/ui-fail-stable/typed-uri-bad-type.stderr index 70fba713..218e523a 100644 --- a/core/codegen/tests/ui-fail-stable/typed-uri-bad-type.stderr +++ b/core/codegen/tests/ui-fail-stable/typed-uri-bad-type.stderr @@ -2,13 +2,13 @@ error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-stable/typed-uri-bad-type.rs:22:37 | 22 | fn optionals(id: Option, name: Result) { } - | ^^^^^^ expected enum `Infallible`, found `&str` + | ^^^^^^ expected struct `Empty`, found `&str` error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-stable/typed-uri-bad-type.rs:22:37 | 22 | fn optionals(id: Option, name: Result) { } - | ^^^^^^ expected `&str`, found enum `Infallible` + | ^^^^^^ expected `&str`, found struct `Empty` error[E0277]: the trait bound `usize: FromUriParam` is not satisfied --> tests/ui-fail-stable/typed-uri-bad-type.rs:45:22 diff --git a/core/codegen/tests/ui-fail-stable/typed-uris-bad-params.stderr b/core/codegen/tests/ui-fail-stable/typed-uris-bad-params.stderr index 87e3ed4d..227923a0 100644 --- a/core/codegen/tests/ui-fail-stable/typed-uris-bad-params.stderr +++ b/core/codegen/tests/ui-fail-stable/typed-uris-bad-params.stderr @@ -276,10 +276,10 @@ error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-stable/typed-uris-bad-params.rs:15:37 | 15 | fn optionals(id: Option, name: Result) { } - | ^^^^^^ expected enum `Infallible`, found `&str` + | ^^^^^^ expected struct `Empty`, found `&str` error[E0271]: type mismatch resolving `>::Error == &str` --> tests/ui-fail-stable/typed-uris-bad-params.rs:15:37 | 15 | fn optionals(id: Option, name: Result) { } - | ^^^^^^ expected `&str`, found enum `Infallible` + | ^^^^^^ expected `&str`, found struct `Empty` diff --git a/core/lib/src/error.rs b/core/lib/src/error.rs index 0fcb91c4..314cf2c8 100644 --- a/core/lib/src/error.rs +++ b/core/lib/src/error.rs @@ -98,6 +98,10 @@ pub enum ErrorKind { ), } +/// An error that occurs when a value was unexpectedly empty. +#[derive(Clone, Copy, Default, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub struct Empty; + impl From for Error { fn from(kind: ErrorKind) -> Self { Error::new(kind) @@ -259,3 +263,17 @@ impl Drop for Error { } } } + +impl fmt::Debug for Empty { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("empty parameter") + } +} + +impl fmt::Display for Empty { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("empty parameter") + } +} + +impl StdError for Empty { } diff --git a/core/lib/src/request/from_param.rs b/core/lib/src/request/from_param.rs index 3eca0084..ffd73e34 100644 --- a/core/lib/src/request/from_param.rs +++ b/core/lib/src/request/from_param.rs @@ -1,6 +1,7 @@ use std::str::FromStr; use std::path::PathBuf; +use crate::error::Empty; use crate::http::uri::{Segments, error::PathError, fmt::Path}; /// Trait to convert a dynamic path segment string to a concrete value. @@ -184,20 +185,27 @@ pub trait FromParam<'a>: Sized { } impl<'a> FromParam<'a> for &'a str { - type Error = std::convert::Infallible; + type Error = Empty; #[inline(always)] fn from_param(param: &'a str) -> Result<&'a str, Self::Error> { + if param.is_empty() { + return Err(Empty); + } + Ok(param) } } impl<'a> FromParam<'a> for String { - type Error = std::convert::Infallible; + type Error = Empty; #[inline(always)] fn from_param(param: &'a str) -> Result { - // TODO: Tell the user they're being inefficient? + if param.is_empty() { + return Err(Empty); + } + Ok(param.to_string()) } } diff --git a/core/lib/src/request/request.rs b/core/lib/src/request/request.rs index 7365ec9e..625c4715 100644 --- a/core/lib/src/request/request.rs +++ b/core/lib/src/request/request.rs @@ -802,6 +802,8 @@ impl<'r> Request<'r> { /// ```rust /// # let c = rocket::local::blocking::Client::debug_with(vec![]).unwrap(); /// # let get = |uri| c.get(uri); + /// use rocket::error::Empty; + /// /// assert_eq!(get("/a/b/c").param(0), Some(Ok("a"))); /// assert_eq!(get("/a/b/c").param(1), Some(Ok("b"))); /// assert_eq!(get("/a/b/c").param(2), Some(Ok("c"))); @@ -811,7 +813,7 @@ impl<'r> Request<'r> { /// assert!(get("/1/b/3").param::(1).unwrap().is_err()); /// assert_eq!(get("/1/b/3").param(2), Some(Ok(3))); /// - /// assert_eq!(get("/").param::<&str>(0), None); + /// assert_eq!(get("/").param::<&str>(0), Some(Err(Empty))); /// ``` #[inline] pub fn param<'a, T>(&'a self, n: usize) -> Option> @@ -940,7 +942,7 @@ impl<'r> Request<'r> { /// codegen. #[inline] pub fn routed_segment(&self, n: usize) -> Option<&str> { - self.routed_segments(0..).get(n).filter(|p| !p.is_empty()) + self.routed_segments(0..).get(n) } /// Get the segments beginning at the `n`th, 0-indexed, after the mount diff --git a/core/lib/src/route/uri.rs b/core/lib/src/route/uri.rs index a0c885b7..7d0f0f01 100644 --- a/core/lib/src/route/uri.rs +++ b/core/lib/src/route/uri.rs @@ -127,8 +127,6 @@ impl<'a> RouteUri<'a> { .into_normalized() .into_owned(); - dbg!(&base, &origin, &compiled_uri, &uri); - let source = uri.to_string().into(); let metadata = Metadata::from(&base, &uri); diff --git a/core/lib/src/router/collider.rs b/core/lib/src/router/collider.rs index 757a624d..13840774 100644 --- a/core/lib/src/router/collider.rs +++ b/core/lib/src/router/collider.rs @@ -19,7 +19,9 @@ impl Collide for Route { /// * If route doesn't specify a format, it gets requests for any format. /// /// Because query parsing is lenient, and dynamic query parameters can be - /// missing, queries do not impact whether two routes collide. + /// missing, the particularities of a query string do not impact whether two + /// routes collide. The query effects the route's color, however, which + /// effects its rank. fn collides_with(&self, other: &Route) -> bool { self.method == other.method && self.rank == other.rank @@ -64,9 +66,7 @@ impl Collide for RouteUri<'_> { impl Collide for Segment { fn collides_with(&self, other: &Self) -> bool { - self.dynamic && !other.value.is_empty() - || other.dynamic && !self.value.is_empty() - || self.value == other.value + self.dynamic || other.dynamic || self.value == other.value } } @@ -136,7 +136,6 @@ mod tests { #[test] fn non_collisions() { - assert_no_collision!("/", "/"); assert_no_collision!("/a", "/b"); assert_no_collision!("/a/b", "/a"); assert_no_collision!("/a/b", "/a/c"); @@ -152,7 +151,6 @@ mod tests { assert_no_collision!("/hello", "/hello/"); assert_no_collision!("/hello/there", "/hello/there/"); - assert_no_collision!("/hello/", "/hello/"); assert_no_collision!("/a?", "/b"); assert_no_collision!("/a/b", "/a?"); @@ -194,6 +192,8 @@ mod tests { assert_no_collision!("/a", "/aaa"); assert_no_collision!("/", "/a"); + assert_no_collision!(ranked "/", "/"); + assert_no_collision!(ranked "/hello/", "/hello/"); assert_no_collision!(ranked "/", "/?a"); assert_no_collision!(ranked "/", "/?"); assert_no_collision!(ranked "/a/", "/a/?d"); @@ -201,9 +201,11 @@ mod tests { #[test] fn collisions() { + assert_collision!("/", "/"); assert_collision!("/a", "/a"); assert_collision!("/hello", "/hello"); assert_collision!("/hello/there/how/ar", "/hello/there/how/ar"); + assert_collision!("/hello/", "/hello/"); assert_collision!("/", "/"); assert_collision!("/", "/b"); diff --git a/core/lib/src/router/matcher.rs b/core/lib/src/router/matcher.rs index 496efac0..521d96c9 100644 --- a/core/lib/src/router/matcher.rs +++ b/core/lib/src/router/matcher.rs @@ -32,9 +32,6 @@ impl Catcher { /// * It is a default catcher _or_ has a code of `status`. /// * Its base is a prefix of the normalized/decoded `req.path()`. pub(crate) fn matches(&self, status: Status, req: &Request<'_>) -> bool { - dbg!(self.base.path().segments()); - dbg!(req.uri().path().segments()); - self.code.map_or(true, |code| code == status.code) && self.base.path().segments().prefix_of(req.uri().path().segments()) } @@ -70,10 +67,6 @@ fn paths_match(route: &Route, req: &Request<'_>) -> bool { return true; } - if route_seg.dynamic && req_seg.is_empty() { - return false; - } - if !route_seg.dynamic && route_seg.value != req_seg { return false; } @@ -161,6 +154,8 @@ mod tests { assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?c=foo&")); assert!(req_matches_route("/a/b?c=foo&d=z", "/a/b?d=z&")); + assert!(req_matches_route("/", "/")); + assert!(req_matches_route("/a", "/")); assert!(req_matches_route("/a", "/a")); assert!(req_matches_route("/a/", "/a/")); diff --git a/core/lib/src/router/router.rs b/core/lib/src/router/router.rs index d74e6d3b..88d3a293 100644 --- a/core/lib/src/router/router.rs +++ b/core/lib/src/router/router.rs @@ -185,6 +185,7 @@ mod test { assert!(rankless_route_collisions(&["/<_..>", "/<_>"])); assert!(rankless_route_collisions(&["/<_>/b", "/a/b"])); assert!(rankless_route_collisions(&["/", "/"])); + assert!(rankless_route_collisions(&["/<_>", "/"])); } #[test] @@ -232,13 +233,13 @@ mod test { assert!(!rankless_route_collisions(&["/a/b", "/a/b/c"])); assert!(!rankless_route_collisions(&["/a/b/c/d", "/a/b/c//e"])); assert!(!rankless_route_collisions(&["/a/d/", "/a/b/c"])); - assert!(!rankless_route_collisions(&["/<_>", "/"])); assert!(!rankless_route_collisions(&["/a/<_>", "/a"])); assert!(!rankless_route_collisions(&["/a/<_>", "/<_>"])); } #[test] fn test_no_collision_when_ranked() { + assert!(!default_rank_route_collisions(&["/<_>", "/"])); assert!(!default_rank_route_collisions(&["/", "/hello"])); assert!(!default_rank_route_collisions(&["/hello/bob", "/hello/"])); assert!(!default_rank_route_collisions(&["/a/b/c/d", "///c/d"])); @@ -298,6 +299,7 @@ mod test { assert!(route(&router, Get, "/hello").is_some()); let router = router_with_routes(&["/"]); + assert!(route(&router, Get, "/").is_some()); assert!(route(&router, Get, "/hello").is_some()); assert!(route(&router, Get, "/hi").is_some()); assert!(route(&router, Get, "/bobbbbbbbbbby").is_some()); @@ -307,6 +309,7 @@ mod test { assert!(route(&router, Get, "/hello/hi").is_some()); assert!(route(&router, Get, "/i/a").is_some()); assert!(route(&router, Get, "/jdlk/asdij").is_some()); + assert!(route(&router, Get, "/a/").is_some()); let mut router = Router::new(); router.add_route(Route::new(Put, "/hello", dummy_handler)); @@ -347,7 +350,6 @@ mod test { assert!(route(&router, Put, "/hello").is_none()); assert!(route(&router, Post, "/hello").is_none()); assert!(route(&router, Options, "/hello").is_none()); - assert!(route(&router, Get, "/").is_none()); assert!(route(&router, Get, "/hello/").is_none()); assert!(route(&router, Get, "/hello/there/").is_none()); assert!(route(&router, Get, "/hello/there/").is_none()); @@ -355,7 +357,6 @@ mod test { let router = router_with_routes(&["//"]); assert!(route(&router, Get, "/a/b/c").is_none()); assert!(route(&router, Get, "/a").is_none()); - assert!(route(&router, Get, "/a/").is_none()); assert!(route(&router, Get, "/a/b/c/d").is_none()); assert!(route(&router, Get, "/a/b/").is_none()); assert!(route(&router, Put, "/hello/hi").is_none()); diff --git a/examples/hello/src/main.rs b/examples/hello/src/main.rs index e9c1081a..0f8c55cb 100644 --- a/examples/hello/src/main.rs +++ b/examples/hello/src/main.rs @@ -74,10 +74,6 @@ fn hello(lang: Option, opt: Options<'_>) -> String { #[launch] fn rocket() -> _ { - // FIXME: Check docs corresponding to normalization/matching/colliding. - // FUZZ: If rand_req1.matches(foo) && rand_req2.matches(bar) => - // rand_req1.collides_with(rand_req2) - rocket::build() .mount("/", routes![hello]) .mount("/hello", routes![world, mir])