Improved error handling.

Based on seanmonstar/warp#458 , an
error is not a rejection, but an error in a web server should
impl Reply, and then `Result<impl Repy, Error>` should do so too.

Author: kaj

Cargo.toml

@@ -26,7 +26,8 @@ slug = "0.1.4"
 structopt = { version = "0.3.2", features = ["wrap_help"] }
 tokio = { version = "0.2", features = ["macros"] }
 tokio-diesel = "0.3.0"
-warp = { version = "0.2.0", default-features = false }
+#warp = { version = "0.2.0", default-features = false }
+warp = { git = "https://github.com/cjbassi/warp", branch="error", default-features = false }
 roxmltree = "0.13.0"
 
 [dependencies.diesel]

src/server/covers.rs

@@ -1,4 +1,4 @@
-use super::{custom, redirect, PgPool};
+use super::{redirect, PgPool, ServerError};
 use crate::schema::covers::dsl as c;
 use crate::schema::issues::dsl as i;
 use crate::templates::statics::xcover_jpg;
@@ -8,36 +8,36 @@ use mime::IMAGE_JPEG;
 use std::str::FromStr;
 use tokio_diesel::{AsyncRunQueryDsl, OptionalExtension};
 use warp::http::header::{CONTENT_TYPE, EXPIRES};
-use warp::http::{Response, StatusCode};
-use warp::reject::not_found;
-use warp::{Rejection, Reply};
+use warp::http::{response::Builder, StatusCode};
+use warp::reply::Response;
+use warp::Reply;
 
-#[allow(clippy::needless_pass_by_value)]
 pub async fn cover_image(
     db: PgPool,
     issue: CoverRef,
-) -> Result<impl Reply, Rejection> {
+) -> Result<Response, ServerError> {
     let data = i::issues
         .inner_join(c::covers)
         .select(c::image)
         .filter(i::year.eq(issue.year))
         .filter(i::number.eq(issue.number))
         .first_async::<Vec<u8>>(&db)
         .await
-        .optional()
-        .map_err(custom)?;
+        .optional()?;
 
     if let Some(data) = data {
         let medium_expires = Utc::now() + Duration::days(90);
-        Ok(Response::builder()
+        Ok(Builder::new()
             .header(CONTENT_TYPE, IMAGE_JPEG.as_ref())
             .header(EXPIRES, medium_expires.to_rfc2822())
-            .body(data))
+            .body(data.into())
+            .unwrap())
     } else {
-        Ok(Response::builder()
+        Ok(Builder::new()
             .status(StatusCode::NOT_FOUND)
             .header(CONTENT_TYPE, xcover_jpg.mime.as_ref())
-            .body(xcover_jpg.content.to_vec()))
+            .body(xcover_jpg.content.to_vec().into())
+            .unwrap())
     }
 }
 
@@ -66,18 +66,17 @@ pub async fn redirect_cover(
     db: PgPool,
     year: CYear,
     issue: SIssue,
-) -> Result<impl Reply, Rejection> {
+) -> Result<impl Reply, ServerError> {
     let exists = i::issues
         .filter(i::year.eq(year.0))
         .filter(i::number.eq(issue.0))
         .count()
         .get_result_async::<i64>(&db)
-        .await
-        .map_err(custom)?;
+        .await?;
     if exists > 0 {
-        redirect(&format!("/c/f{}-{}.jpg", year.0, issue.0))
+        Ok(redirect(&format!("/c/f{}-{}.jpg", year.0, issue.0)))
     } else {
-        Err(not_found())
+        Err(ServerError::not_found())
     }
 }
 

src/server/creators.rs

@@ -1,4 +1,4 @@
-use super::{custom, custom_or_404, goh, redirect};
+use super::{goh, redirect, OptionalExtension, ServerError};
 use super::{FullArticle, FullEpisode, OtherContribs, PgFilter, PgPool};
 use crate::models::creator_contributions::CreatorContributions;
 use crate::models::{
@@ -20,20 +20,20 @@ use crate::schema::titles::dsl as t;
 use crate::templates::{self, RenderRucte};
 use diesel::dsl::{any, min};
 use diesel::prelude::*;
-use tokio_diesel::{AsyncError, AsyncRunQueryDsl, OptionalExtension};
+use tokio_diesel::{AsyncError, AsyncRunQueryDsl};
 use warp::filters::BoxedFilter;
 use warp::http::response::Builder;
 use warp::reply::Response;
-use warp::{self, Filter, Rejection, Reply};
+use warp::{self, Filter, Reply};
 
 pub fn routes(s: PgFilter) -> BoxedFilter<(impl Reply,)> {
     use warp::path::{end, param};
-    let list = goh().and(end()).and(s.clone()).and_then(list_creators);
-    let one = goh().and(s).and(param()).and(end()).and_then(one_creator);
+    let list = goh().and(end()).and(s.clone()).map_async(list_creators);
+    let one = goh().and(s).and(param()).and(end()).map_async(one_creator);
     list.or(one).unify().boxed()
 }
 
-async fn list_creators(db: PgPool) -> Result<Response, Rejection> {
+async fn list_creators(db: PgPool) -> Result<Response, ServerError> {
     use crate::models::creator_contributions::creator_contributions::dsl as cc;
     let all = cc::creator_contributions
         .select((
@@ -45,21 +45,20 @@ async fn list_creators(db: PgPool) -> Result<Response, Rejection> {
             cc::latest_issue,
         ))
         .load_async::<CreatorContributions>(&db)
-        .await
-        .map_err(custom)?;
-    Builder::new().html(|o| templates::creators(o, &all))
+        .await?;
+    Ok(Builder::new().html(|o| templates::creators(o, &all))?)
 }
 
 async fn one_creator(
     db: PgPool,
     slug: String,
-) -> Result<Response, Rejection> {
+) -> Result<Response, ServerError> {
     let creator = c::creators
         .filter(c::slug.eq(slug.clone()))
         .first_async::<Creator>(&db)
         .await
-        .optional()
-        .map_err(custom)?;
+        .optional()?;
+
     let creator = if let Some(creator) = creator {
         creator
     } else {
@@ -69,7 +68,7 @@ async fn one_creator(
             .replace(".html", "");
         log::info!("Looking for creator fallback {:?} -> {:?}", slug, target);
         if target == "anderas%eriksson" || target == "andreas%erikssson" {
-            return redirect("/who/andreas-eriksson");
+            return Ok(redirect("/who/andreas-eriksson"));
         }
         let found = ca::creator_aliases
             .inner_join(c::creators)
@@ -78,9 +77,9 @@ async fn one_creator(
             .select(c::slug)
             .first_async::<String>(&db)
             .await
-            .map_err(custom_or_404)?;
+            .or_404()?;
         log::debug!("Found replacement: {:?}", found);
-        return redirect(&format!("/who/{}", found));
+        return Ok(redirect(&format!("/who/{}", found)));
     };
 
     let about_raw = a::articles
@@ -92,23 +91,16 @@ async fn one_creator(
         .order(min(i::magic))
         .group_by(a::articles::all_columns())
         .load_async::<Article>(&db)
-        .await
-        .map_err(custom)?;
+        .await?;
     let mut about = Vec::with_capacity(about_raw.len());
     for article in about_raw.into_iter() {
         let published = i::issues
             .inner_join(p::publications)
             .select((i::year, (i::number, i::number_str)))
             .filter(p::article_id.eq(article.id))
             .load_async::<IssueRef>(&db)
-            .await
-            .map_err(custom)?;
-        about.push((
-            FullArticle::load_async(article, &db)
-                .await
-                .map_err(custom)?,
-            published,
-        ));
+            .await?;
+        about.push((FullArticle::load_async(article, &db).await?, published));
     }
 
     let e_t_columns = (t::titles::all_columns(), e::episodes::all_columns());
@@ -125,13 +117,10 @@ async fn one_creator(
         .order(min(i::magic))
         .group_by(e_t_columns)
         .load_async::<(Title, Episode)>(&db)
-        .await
-        .map_err(custom)?;
+        .await?;
     let mut main_episodes = Vec::with_capacity(main_episodes_raw.len());
     for (t, ep) in main_episodes_raw.into_iter() {
-        let e = FullEpisode::load_details_async(ep, &db)
-            .await
-            .map_err(custom)?;
+        let e = FullEpisode::load_details_async(ep, &db).await?;
         main_episodes.push((t, e));
     }
 
@@ -143,31 +132,23 @@ async fn one_creator(
         .order(min(i::magic))
         .group_by(a::articles::all_columns())
         .load_async::<Article>(&db)
-        .await
-        .map_err(custom)?;
+        .await?;
     let mut articles_by = Vec::with_capacity(articles_by_raw.len());
     for article in articles_by_raw.into_iter() {
         let published = i::issues
             .inner_join(p::publications)
             .select((i::year, (i::number, i::number_str)))
             .filter(p::article_id.eq(article.id))
             .load_async::<IssueRef>(&db)
-            .await
-            .map_err(custom)?;
-        articles_by.push((
-            FullArticle::load_async(article, &db)
-                .await
-                .map_err(custom)?,
-            published,
-        ))
+            .await?;
+        articles_by
+            .push((FullArticle::load_async(article, &db).await?, published))
     }
 
-    let covers = CoverSet::by(&creator, &db).await.map_err(custom)?;
-    let others = OtherContribs::for_creator(&creator, &db)
-        .await
-        .map_err(custom)?;
+    let covers = CoverSet::by(&creator, &db).await?;
+    let others = OtherContribs::for_creator(&creator, &db).await?;
 
-    Builder::new().html(|o| {
+    Ok(Builder::new().html(|o| {
         templates::creator(
             o,
             &creator,
@@ -177,7 +158,7 @@ async fn one_creator(
             &articles_by,
             &others,
         )
-    })
+    })?)
 }
 
 pub struct CoverSet {

src/server/error.rs

@@ -0,0 +1,96 @@
+use std::fmt::{self, Display};
+use tokio_diesel::AsyncError;
+use warp::http::StatusCode;
+use warp::reply::Response;
+use warp::{Rejection, Reply};
+
+#[derive(Debug, Clone)]
+pub struct ServerError {
+    code: StatusCode,
+}
+
+impl ServerError {
+    pub fn not_found() -> Self {
+        ServerError {
+            code: StatusCode::NOT_FOUND,
+        }
+    }
+}
+
+impl Reply for ServerError {
+    fn into_response(self) -> Response {
+        use super::templates::{error, notfound, RenderRucte};
+        use warp::http::response::Builder;
+        log::error!("{}", self);
+        let res = Builder::new().status(self.code);
+        if self.code == StatusCode::NOT_FOUND {
+            res.html(|o| notfound(o, StatusCode::NOT_FOUND)).unwrap()
+        } else {
+            res.html(|o| error(o, self.code)).unwrap()
+        }
+    }
+}
+impl std::error::Error for ServerError {}
+
+impl Display for ServerError {
+    fn fmt(&self, out: &mut fmt::Formatter) -> fmt::Result {
+        write!(out, "Error: {}", self.code)
+    }
+}
+
+impl From<Rejection> for ServerError {
+    fn from(err: Rejection) -> ServerError {
+        log::error!("Reject {:?}", err);
+        let code = if err.is_not_found() {
+            StatusCode::NOT_FOUND
+        } else if let Some(_) = err.find::<warp::reject::MethodNotAllowed>() {
+            StatusCode::METHOD_NOT_ALLOWED
+        } else {
+            StatusCode::INTERNAL_SERVER_ERROR
+        };
+        ServerError { code }
+    }
+}
+
+impl From<AsyncError> for ServerError {
+    fn from(err: AsyncError) -> ServerError {
+        let code = match err {
+            AsyncError::Checkout(e) => {
+                log::error!("Pool error: {}", e);
+                StatusCode::SERVICE_UNAVAILABLE
+            }
+            AsyncError::Error(e) => {
+                log::error!("Error: {}", e);
+                StatusCode::INTERNAL_SERVER_ERROR
+            }
+        };
+        ServerError { code }
+    }
+}
+/*impl From<warp::http::Error> for ServerError {
+    fn from(err: warp::http::Error) -> ServerError {
+        ServerError {
+            code: err.into(),
+        }
+    }
+}*/
+
+pub trait OptionalExtension<T>: tokio_diesel::OptionalExtension<T> {
+    fn or_404(self) -> Result<T, ServerError>
+    where
+        Self: Sized,
+    {
+        match tokio_diesel::OptionalExtension::optional(self)? {
+            Some(t) => Ok(t),
+            None => Err(ServerError::not_found()),
+        }
+    }
+    fn optional(self) -> Result<Option<T>, ServerError>
+    where
+        Self: Sized,
+    {
+        Ok(tokio_diesel::OptionalExtension::optional(self)?)
+    }
+}
+
+impl<T> OptionalExtension<T> for Result<T, AsyncError> {}