Skip to content

Commit

Permalink
[WIP] Passing the logic to the router and performance corrections
Browse files Browse the repository at this point in the history
  • Loading branch information
Drunpy committed Jun 23, 2020
1 parent 4c3e516 commit 13b6386
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 48 deletions.
76 changes: 42 additions & 34 deletions core/lib/src/rocket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,41 +288,49 @@ impl Rocket {
mut data: Data,
) -> handler::Outcome<'r> {
// Go through the list of matching routes until we fail or succeed.
let matches = self.router.route(request);
let mut counter: usize = 0;
for route in &matches {
counter += 1;

// Must pass HEAD requests foward
if (&request.method() != &Method::Head) && (&route.method != &request.method()) {
// Must make sure it consumed all list before fail
if &counter == &matches.len() {
error_!("No matching routes for {}.", request);
info_!(
"{} {}",
Paint::yellow("A similar route exists:").bold(),
route
);
return Outcome::Failure(Status::MethodNotAllowed);
} else {
continue;
}
} else {
// Retrieve and set the requests parameters.
info_!("Matched: {}", route);

request.set_route(route);

// Dispatch the request to the handler.
let outcome = route.handler.handle(request, data);
let method_matches = self.router.route(request, true);
if method_matches.len() > 0 {
for route in &method_matches {
// Retrieve and set the requests parameters.
info_!("Matched: {}", route);

request.set_route(route);

// Dispatch the request to the handler.
let outcome = route.handler.handle(request, data);

// Check if the request processing completed or if the request needs
// to be forwarded. If it does, continue the loop to try again.
info_!("{} {}", Paint::default("Outcome:").bold(), outcome);
match outcome {
o @ Outcome::Success(_) | o @ Outcome::Failure(_) => return o,
Outcome::Forward(unused_data) => data = unused_data,
};
}
}

// Check if the request processing completed or if the request needs
// to be forwarded. If it does, continue the loop to try again.
info_!("{} {}", Paint::default("Outcome:").bold(), outcome);
match outcome {
o @ Outcome::Success(_) | o @ Outcome::Failure(_) => return o,
Outcome::Forward(unused_data) => data = unused_data,
};
let match_any = self.router.route(request, false);
if match_any.len() > 0 {

let mut counter: usize = 0;
for route in &match_any {
counter += 1;

// Must pass HEAD requests foward
if (&request.method() != &Method::Head) {

This comment has been minimized.

Copy link
@igalic

igalic Jun 23, 2020

if we invert this condition, we can continue in the if, and then we don't need to nest a very complicated if in here

// Must make sure it consumed all list before fail
if &counter == &match_any.len() && !method_matches.iter().any(|item| route.collides_with(item)){

This comment has been minimized.

Copy link
@igalic

igalic Jun 23, 2020

this condition reads very confusing
can we make it any more readable or understandable?

error_!("No matching routes for {}.", request);
info_!(
"{} {}",
Paint::yellow("A similar route exists:").bold(),
route
);
return Outcome::Failure(Status::MethodNotAllowed);
} else {
continue;
}
}
}
}

Expand Down
18 changes: 14 additions & 4 deletions core/lib/src/router/collider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ impl Route {
&& formats_collide(self, other)
}

/// Determines if this route matches against the given request. This means
/// Determines if this route matches against the given request if . This means
/// that:
///
/// * The route's method matches that of the incoming request.
Expand All @@ -38,9 +38,19 @@ impl Route {
/// request query string, though in any position.
/// - If no query in route, requests with/without queries match.
#[doc(hidden)]
pub fn matches(&self, req: &Request<'_>) -> bool {
pub fn matches_by_method(&self, req: &Request<'_>) -> bool {
self.method == req.method()
&& paths_match(self, req)
&& queries_match(self, req)
&& formats_match(self, req)
}

/// Match agoinst any method.
#[doc(hidden)]
pub fn match_any(&self, req: &Request<'_>) -> bool {
paths_match(self, req) && queries_match(self, req) && formats_match(self, req)
}

}

fn paths_collide(route: &Route, other: &Route) -> bool {
Expand Down Expand Up @@ -413,7 +423,7 @@ mod tests {
route.format = Some(mt_str.parse::<MediaType>().unwrap());
}

route.matches(&req)
route.matches_by_method(&req)
}

#[test]
Expand Down Expand Up @@ -468,7 +478,7 @@ mod tests {
let rocket = Rocket::custom(Config::development());
let req = Request::new(&rocket, Get, Origin::parse(a).expect("valid URI"));
let route = Route::ranked(0, Get, b.to_string(), dummy_handler);
route.matches(&req)
route.matches_by_method(&req)
}

#[test]
Expand Down
20 changes: 12 additions & 8 deletions core/lib/src/router/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub struct Router {
routes: HashMap<Selector, Vec<Route>>,
}


impl Router {
pub fn new() -> Router {
Router { routes: HashMap::new() }
Expand All @@ -35,18 +36,21 @@ impl Router {
entries.insert(i, route);
}

pub fn route<'b>(&'b self, req: &Request<'_>) -> Vec<&'b Route> {
// TODO: Describe restric param
pub fn route<'b>(&'b self, req: &Request<'_>, restrict: bool) -> Vec<&'b Route> {
let mut matches = Vec::new();
for (_method, vec_route) in self.routes.iter() {
for _route in vec_route {
if _route.matches(req) {
for (_method, routes_vec) in self.routes.iter() {
for _route in routes_vec {
if _route.matches_by_method(req) {
matches.push(_route);
} else if !restrict && _route.match_any(req){
matches.push(_route);
}
}
}

trace_!("Routing the request: {}", req);
trace_!("All matches: {:?}", matches);
trace_!("Routing by method: {}", req);
trace_!("All matches by method: {:?}", matches);
matches
}

Expand Down Expand Up @@ -232,7 +236,7 @@ mod test {
fn route<'a>(router: &'a Router, method: Method, uri: &str) -> Option<&'a Route> {
let rocket = Rocket::custom(Config::development());
let request = Request::new(&rocket, method, Origin::parse(uri).unwrap());
let matches = router.route(&request);
let matches = router.route(&request, false);
if matches.len() > 0 {
Some(matches[0])
} else {
Expand All @@ -243,7 +247,7 @@ mod test {
fn matches<'a>(router: &'a Router, method: Method, uri: &str) -> Vec<&'a Route> {
let rocket = Rocket::custom(Config::development());
let request = Request::new(&rocket, method, Origin::parse(uri).unwrap());
router.route(&request)
router.route(&request, false)
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion core/lib/tests/fairing_before_head_strip-issue-546.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ mod fairing_before_head_strip {
assert_eq!(c.0.fetch_add(1, Ordering::SeqCst), 0);
}))
.attach(AdHoc::on_response("Check GET", |req, res| {
assert_eq!(req.method(), Method::Head);
assert_eq!(req.method(), Method::Get);
assert_eq!(res.body_string(), Some(RESPONSE_STRING.into()));
}));

Expand Down
2 changes: 1 addition & 1 deletion examples/errors/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn test_hello() {

#[test]
fn test_hello_invalid_age() {
for &(name, age) in &[("Ford", -129), ("Trillian", 128)] {
for &(name, age) in &[("Ford", "s"), ("Trillian", "f")] {
let uri = format!("/hello/{}/{}", name, age);
let body = format!("<p>Sorry, but '{}' is not a valid path!</p>
<p>Try visiting /hello/&lt;name&gt;/&lt;age&gt; instead.</p>",
Expand Down

0 comments on commit 13b6386

Please sign in to comment.