From 9160483554ef49586802bab66e112598efd8a509 Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Mon, 27 Mar 2017 03:52:26 -0700 Subject: [PATCH] A route with unspecified query parameters accepts any. This is a breaking change. It modifies collisions with respect to query parameters as well as the default ranking of routes. A route that does not specify query parameters will now match against requests with _and without_ query parameters, assuming all other elements of the route match as well. A route that _does_ specify query parameters will only match requests with query parameters; this remains true. To accommodate this change in the most natural manner possible, the default rankings of routes have changed as illustrated below: |-------------+-------+----------+---------------| | static path | query | new rank | previous rank | |-------------+-------+----------+---------------| | yes | yes | -4 | 0 | | yes | no | -3 | 0 | | no | yes | -2 | 1 | | no | no | -1 | 1 | |-------------+-------+----------+---------------| In other words, the most specific routes, with preference for paths over queries, are ranked highest (lower number). --- examples/static_files/src/tests.rs | 6 ++ lib/src/http/parse/accept.rs | 4 +- lib/src/router/collider.rs | 97 ++++++++++++++++++++---------- lib/src/router/mod.rs | 47 +++++++++++++++ lib/src/router/route.rs | 19 ++++-- 5 files changed, 134 insertions(+), 39 deletions(-) diff --git a/examples/static_files/src/tests.rs b/examples/static_files/src/tests.rs index 85626ee4..4b28b41f 100644 --- a/examples/static_files/src/tests.rs +++ b/examples/static_files/src/tests.rs @@ -34,20 +34,26 @@ fn read_file_content(path: &str) -> Vec { #[test] fn test_index_html() { test_query_file("/", "static/index.html", Status::Ok); + test_query_file("/?v=1", "static/index.html", Status::Ok); + test_query_file("/?this=should&be=ignored", "static/index.html", Status::Ok); } #[test] fn test_hidden_file() { test_query_file("/hidden/hi.txt", "static/hidden/hi.txt", Status::Ok); + test_query_file("/hidden/hi.txt?v=1", "static/hidden/hi.txt", Status::Ok); + test_query_file("/hidden/hi.txt?v=1&a=b", "static/hidden/hi.txt", Status::Ok); } #[test] fn test_icon_file() { test_query_file("/rocket-icon.jpg", "static/rocket-icon.jpg", Status::Ok); + test_query_file("/rocket-icon.jpg", "static/rocket-icon.jpg", Status::Ok); } #[test] fn test_invalid_path() { test_query_file("/thou_shalt_not_exist", None, Status::NotFound); test_query_file("/thou/shalt/not/exist", None, Status::NotFound); + test_query_file("/thou/shalt/not/exist?a=b&c=d", None, Status::NotFound); } diff --git a/lib/src/http/parse/accept.rs b/lib/src/http/parse/accept.rs index be6aa0f8..d01a01b9 100644 --- a/lib/src/http/parse/accept.rs +++ b/lib/src/http/parse/accept.rs @@ -35,8 +35,8 @@ pub fn parse_accept(mut input: &str) -> Result> { #[cfg(test)] mod test { - use http::{Accept, MediaType, WeightedMediaType}; - use super::{ParseResult, parse_accept}; + use http::MediaType; + use super::parse_accept; macro_rules! assert_no_parse { ($string:expr) => ({ diff --git a/lib/src/router/collider.rs b/lib/src/router/collider.rs index 58a03485..6404207b 100644 --- a/lib/src/router/collider.rs +++ b/lib/src/router/collider.rs @@ -50,12 +50,9 @@ impl<'a> Collider for &'a str { } } +// This _only_ checks the `path` component of the URI. impl<'a, 'b> Collider> for URI<'a> { fn collides_with(&self, other: &URI<'b>) -> bool { - if self.query().is_some() != other.query().is_some() { - return false; - } - for (seg_a, seg_b) in self.segments().zip(other.segments()) { if seg_a.ends_with("..>") || seg_b.ends_with("..>") { return true; @@ -83,8 +80,13 @@ impl Collider for ContentType { // This implementation is used at initialization to check if two user routes // collide before launching. Format collisions works like this: -// * If route a specifies format, it only gets requests for that format. -// * If a route doesn't specify format, it gets requests for any format. +// * If route specifies format, it only gets requests for that format. +// * If route doesn't specify format, it gets requests for any format. +// Query collisions work like this: +// * If route specifies qeury, it only gets request that have queries. +// * If route doesn't specify qeury, requests with and without queries match. +// As a result, as long as everything else collides, whether a route has a query +// or not is irrelevant: it will collide. impl Collider for Route { fn collides_with(&self, b: &Route) -> bool { self.method == b.method @@ -101,12 +103,16 @@ impl Collider for Route { // This implementation is used at runtime to check if a given request is // intended for this Route. Format collisions works like this: -// * If route a specifies format, it only gets requests for that format. -// * If a route doesn't specify format, it gets requests for any format. +// * If route specifies format, it only gets requests for that format. +// * If route doesn't specify format, it gets requests for any format. +// Query collisions work like this: +// * If route specifies a query, it only gets request that have queries. +// * If route doesn't specify query, requests with & without queries collide. impl<'r> Collider> for Route { fn collides_with(&self, req: &Request<'r>) -> bool { self.method == req.method() - && req.uri().collides_with(&self.path) + && self.path.collides_with(req.uri()) + && self.path.query().map_or(true, |_| req.uri().query().is_some()) // FIXME: On payload requests, check Content-Type, else Accept. && match (req.content_type().as_ref(), self.format.as_ref()) { (Some(ct_a), Some(ct_b)) => ct_a.collides_with(ct_b), @@ -198,7 +204,6 @@ mod tests { assert!(unranked_collide("/", "///a///")); } - #[test] fn query_collisions() { assert!(unranked_collide("/?", "/?")); @@ -207,6 +212,11 @@ mod tests { assert!(unranked_collide("/?", "/?")); assert!(unranked_collide("/a/b/c?", "/a/b/c?")); assert!(unranked_collide("//b/c?", "/a/b/?")); + assert!(unranked_collide("/?", "/")); + assert!(unranked_collide("/a?", "/a")); + assert!(unranked_collide("/a?", "/a")); + assert!(unranked_collide("/a/b?", "/a/b")); + assert!(unranked_collide("/a/b", "/a/b?")); } #[test] @@ -231,12 +241,11 @@ mod tests { #[test] fn query_non_collisions() { - assert!(!unranked_collide("/?", "/")); + assert!(!unranked_collide("/a?", "/b")); + assert!(!unranked_collide("/a/b", "/a?")); + assert!(!unranked_collide("/a/b/c?", "/a/b/c/d")); + assert!(!unranked_collide("/a/hello", "/a/?")); assert!(!unranked_collide("/?", "/hi")); - assert!(!unranked_collide("/?", "/a")); - assert!(!unranked_collide("/a?", "/a")); - assert!(!unranked_collide("/a/b?", "/a/b")); - assert!(!unranked_collide("/a/b", "/a/b/?")); } #[test] @@ -353,7 +362,7 @@ mod tests { assert!(!r_ct_ct_collide(Get, "text/html", Get, "text/css")); } - fn req_route_collide(m1: Method, ct1: S1, m2: Method, ct2: S2) -> bool + fn req_route_ct_collide(m1: Method, ct1: S1, m2: Method, ct2: S2) -> bool where S1: Into>, S2: Into> { let mut req = Request::new(m1, "/"); @@ -371,23 +380,49 @@ mod tests { #[test] fn test_req_route_ct_collisions() { - assert!(req_route_collide(Get, "application/json", Get, "application/json")); - assert!(req_route_collide(Get, "application/json", Get, "application/*")); - assert!(req_route_collide(Get, "application/json", Get, "*/json")); - assert!(req_route_collide(Get, "text/html", Get, "text/html")); - assert!(req_route_collide(Get, "text/html", Get, "*/*")); + assert!(req_route_ct_collide(Get, "application/json", Get, "application/json")); + assert!(req_route_ct_collide(Get, "application/json", Get, "application/*")); + assert!(req_route_ct_collide(Get, "application/json", Get, "*/json")); + assert!(req_route_ct_collide(Get, "text/html", Get, "text/html")); + assert!(req_route_ct_collide(Get, "text/html", Get, "*/*")); - assert!(req_route_collide(Get, "text/html", Get, None)); - assert!(req_route_collide(Get, None, Get, None)); - assert!(req_route_collide(Get, "application/json", Get, None)); - assert!(req_route_collide(Get, "x-custom/anything", Get, None)); + assert!(req_route_ct_collide(Get, "text/html", Get, None)); + assert!(req_route_ct_collide(Get, None, Get, None)); + assert!(req_route_ct_collide(Get, "application/json", Get, None)); + assert!(req_route_ct_collide(Get, "x-custom/anything", Get, None)); - assert!(!req_route_collide(Get, "application/json", Get, "text/html")); - assert!(!req_route_collide(Get, "application/json", Get, "text/*")); - assert!(!req_route_collide(Get, "application/json", Get, "*/xml")); + assert!(!req_route_ct_collide(Get, "application/json", Get, "text/html")); + assert!(!req_route_ct_collide(Get, "application/json", Get, "text/*")); + assert!(!req_route_ct_collide(Get, "application/json", Get, "*/xml")); - assert!(!req_route_collide(Get, None, Get, "text/html")); - assert!(!req_route_collide(Get, None, Get, "*/*")); - assert!(!req_route_collide(Get, None, Get, "application/json")); + assert!(!req_route_ct_collide(Get, None, Get, "text/html")); + assert!(!req_route_ct_collide(Get, None, Get, "*/*")); + assert!(!req_route_ct_collide(Get, None, Get, "application/json")); + } + + fn req_route_path_collide(a: &'static str, b: &'static str) -> bool { + let req = Request::new(Get, a.to_string()); + let route = Route::ranked(0, Get, b.to_string(), dummy_handler); + route.collides_with(&req) + } + + #[test] + fn test_req_route_query_collisions() { + assert!(req_route_path_collide("/a/b?a=b", "/a/b?")); + assert!(req_route_path_collide("/a/b?a=b", "//b?")); + assert!(req_route_path_collide("/a/b?a=b", "//?")); + assert!(req_route_path_collide("/a/b?a=b", "/a/?")); + assert!(req_route_path_collide("/?b=c", "/?")); + + assert!(req_route_path_collide("/a/b?a=b", "/a/b")); + assert!(req_route_path_collide("/a/b", "/a/b")); + assert!(req_route_path_collide("/a/b/c/d?", "/a/b/c/d")); + assert!(req_route_path_collide("/a/b/c/d?v=1&v=2", "/a/b/c/d")); + + assert!(!req_route_path_collide("/a/b", "/a/b?")); + assert!(!req_route_path_collide("/a/b/c", "/a/b?")); + assert!(!req_route_path_collide("/a?b=c", "/a/b?")); + assert!(!req_route_path_collide("/?b=c", "/a/b?")); + assert!(!req_route_path_collide("/?b=c", "/a?")); } } diff --git a/lib/src/router/mod.rs b/lib/src/router/mod.rs index b5c4e9de..48495630 100644 --- a/lib/src/router/mod.rs +++ b/lib/src/router/mod.rs @@ -245,6 +245,14 @@ mod test { assert_ranked_routes!(&["//b", "/hi/c"], "/hi/c", "/hi/c"); assert_ranked_routes!(&["//", "/hi/a"], "/hi/c", "//"); assert_ranked_routes!(&["/hi/a", "/hi/"], "/hi/c", "/hi/"); + assert_ranked_routes!(&["/a", "/a?"], "/a?b=c", "/a?"); + assert_ranked_routes!(&["/a", "/a?"], "/a", "/a"); + assert_ranked_routes!(&["/a", "/", "/a?", "/?"], "/a", "/a"); + assert_ranked_routes!(&["/a", "/", "/a?", "/?"], "/b", "/"); + assert_ranked_routes!(&["/a", "/", "/a?", "/?"], + "/b?v=1", "/?"); + assert_ranked_routes!(&["/a", "/", "/a?", "/?"], + "/a?b=c", "/a?"); } fn ranked_collisions(routes: &[(isize, &'static str)]) -> bool { @@ -341,6 +349,45 @@ mod test { ); } + macro_rules! assert_default_ranked_routing { + (to: $to:expr, with: $routes:expr, expect: $($want:expr),+) => ({ + let router = router_with_routes(&$routes); + let routed_to = matches(&router, Get, $to); + let expected = &[$($want),+]; + assert!(routed_to.len() == expected.len()); + for (got, expected) in routed_to.iter().zip(expected.iter()) { + assert_eq!(got.path.as_str() as &str, expected as &str); + } + }) + } + + #[test] + fn test_default_ranked_routing() { + assert_default_ranked_routing!( + to: "a/b?v=1", + with: ["a/", "a/b"], + expect: "a/b", "a/" + ); + + assert_default_ranked_routing!( + to: "a/b?v=1", + with: ["a/", "a/b", "a/b?"], + expect: "a/b?", "a/b", "a/" + ); + + assert_default_ranked_routing!( + to: "a/b?v=1", + with: ["a/", "a/b", "a/b?", "a/?"], + expect: "a/b?", "a/b", "a/?", "a/" + ); + + assert_default_ranked_routing!( + to: "a/b", + with: ["a/", "a/b", "a/b?", "a/?"], + expect: "a/b", "a/" + ); + } + fn match_params(router: &Router, path: &str, expected: &[&str]) -> bool { println!("Testing: {} (expect: {:?})", path, expected); route(router, Get, path).map_or(false, |route| { diff --git a/lib/src/router/route.rs b/lib/src/router/route.rs index 4796ef38..14a123ba 100644 --- a/lib/src/router/route.rs +++ b/lib/src/router/route.rs @@ -23,10 +23,16 @@ pub struct Route { pub format: Option, } -fn default_rank(path: &str) -> isize { - // The rank for a given path is 0 if it is a static route (it doesn't - // contain any dynamic ) or 1 if it is dynamic. - path.contains('<') as isize +#[inline(always)] +fn default_rank(uri: &URI) -> isize { + // static path, query = -4; static path, no query = -3 + // dynamic path, query = -2; dynamic path, no query = -1 + match (!uri.path().contains('<'), uri.query().is_some()) { + (true, true) => -4, + (true, false) => -3, + (false, true) => -2, + (false, false) => -1, + } } impl Route { @@ -37,11 +43,12 @@ impl Route { pub fn new(m: Method, path: S, handler: Handler) -> Route where S: AsRef { + let uri = URI::from(path.as_ref().to_string()); Route { method: m, handler: handler, - rank: default_rank(path.as_ref()), - path: URI::from(path.as_ref().to_string()), + rank: default_rank(&uri), + path: uri, format: None, } }