From 287f2914d0d8c91c80e286e49dc1b504ea0508ef Mon Sep 17 00:00:00 2001 From: Wim Looman Date: Tue, 2 Jun 2020 09:34:56 +0200 Subject: [PATCH 1/2] Only chain to different handlers for 404 errors --- src/web/mod.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/web/mod.rs b/src/web/mod.rs index 930e93e51..646ab08d1 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -138,19 +138,24 @@ impl CratesfyiHandler { impl Handler for CratesfyiHandler { fn handle(&self, req: &mut Request) -> IronResult { + fn if_404( + e: IronError, + handle: impl FnOnce() -> IronResult, + ) -> IronResult { + if e.response.status == Some(status::NotFound) { + handle() + } else { + Err(e) + } + }; + // try serving shared rustdoc resources first, then router, then db/static file handler // return 404 if none of them return Ok self.shared_resource_handler .handle(req) - .or_else(|e| self.router_handler.handle(req).or(Err(e))) - .or_else(|e| { - // if router fails try to serve files from database first - self.database_file_handler.handle(req).or(Err(e)) - }) - .or_else(|e| { - // and then try static handler. if all of them fails, return 404 - self.static_handler.handle(req).or(Err(e)) - }) + .or_else(|e| if_404(e, || self.router_handler.handle(req))) + .or_else(|e| if_404(e, || self.database_file_handler.handle(req))) + .or_else(|e| if_404(e, || self.static_handler.handle(req))) .or_else(|e| { let err = if let Some(err) = e.error.downcast::() { *err From 75fba109354828b690419a3365eb01d18eafd028 Mon Sep 17 00:00:00 2001 From: Wim Looman Date: Tue, 2 Jun 2020 10:14:09 +0200 Subject: [PATCH 2/2] Log and return 500 for other errors --- src/web/error.rs | 9 +++++++++ src/web/mod.rs | 17 ++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/web/error.rs b/src/web/error.rs index 2a29404ef..11cbab701 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -10,6 +10,7 @@ pub enum Nope { ResourceNotFound, CrateNotFound, NoResults, + InternalServerError, } impl fmt::Display for Nope { @@ -18,6 +19,7 @@ impl fmt::Display for Nope { Nope::ResourceNotFound => "Requested resource not found", Nope::CrateNotFound => "Requested crate not found", Nope::NoResults => "Search yielded no results", + Nope::InternalServerError => "Internal server error", }) } } @@ -59,6 +61,13 @@ impl Handler for Nope { .to_resp("releases") } } + Nope::InternalServerError => { + // something went wrong, details should have been logged + Page::new("internal server error".to_owned()) + .set_status(status::InternalServerError) + .title("Internal server error") + .to_resp("error") + } } } } diff --git a/src/web/mod.rs b/src/web/mod.rs index 646ab08d1..32172068d 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -159,10 +159,21 @@ impl Handler for CratesfyiHandler { .or_else(|e| { let err = if let Some(err) = e.error.downcast::() { *err - } else if e.error.downcast::().is_some() { + } else if e.error.downcast::().is_some() + || e.response.status == Some(status::NotFound) + { error::Nope::ResourceNotFound + } else if e.response.status == Some(status::InternalServerError) { + log::error!("internal server error: {}", e.error); + error::Nope::InternalServerError } else { - panic!("all cratesfyi errors should be of type Nope"); + log::error!( + "No error page for status {:?}; {}", + e.response.status, + e.error + ); + // TODO: add in support for other errors that are actually used + error::Nope::InternalServerError }; if let error::Nope::ResourceNotFound = err { @@ -186,7 +197,7 @@ impl Handler for CratesfyiHandler { } } - debug!("Path not found: {}", DebugPath(&req.url)); + debug!("Path not found: {}; {}", DebugPath(&req.url), e.error); } Self::chain(self.pool.clone(), err).handle(req)