Improve error handling

src/main.rs

@@ -5,7 +5,7 @@ pub use comment::*;
 mod database;
 pub use database::Database;
 
-use actix_web::{get, post, web, App, HttpRequest, HttpResponse, HttpServer, Responder};
+use actix_web::{get, post, web, App, HttpRequest, HttpResponse, HttpServer};
 use clap::Parser;
 use sanitize_html::{errors::SanitizeError, rules::predefined::DEFAULT, sanitize_str};
 use scraper::{Html, Selector};
@@ -19,69 +19,68 @@ struct AppState {
     arguments: Arguments,
 }
 
-// TODO: Make error handling system not bad.
-// Currently, it's a horrible mix of custom error types a direct HttpResponses,
-// due to not being able to pass HttpResponse out of web::block...
-// Well, it works, at least.
-
-enum DatabaseAccessError {
-    BadOrigin,
-    AccessError,
-    DatabaseError,
-}
-
-enum CommentCreationError {
-    DatabaseAccessError(DatabaseAccessError),
-    BadParent,
-}
-
-impl DatabaseAccessError {
-    fn to_http_response(&self) -> HttpResponse {
-        match self {
-            Self::BadOrigin => HttpResponse::BadRequest().reason("bad origin").finish(),
-            Self::AccessError => HttpResponse::InternalServerError()
-                .reason("database access error")
-                .finish(), // e.g. PoisonError
-            Self::DatabaseError => HttpResponse::InternalServerError()
-                .reason("database error")
-                .finish(),
-        }
-    }
+enum Error {
+    InvalidOrigin,
+    InvalidBody,
+    InvalidUrl,
+    InvalidFields,
+    InvalidContentId,
+    InvalidParent,
+    EmailRequired,
+    NameRequired,
+    DatabaseAccessError,
+    DatabaseInternalError,
+    SanitizationError,
+    PageFetchError,
 }
 
-impl CommentCreationError {
+impl Error {
     fn to_http_response(&self) -> HttpResponse {
         match self {
-            Self::DatabaseAccessError(error) => error.to_http_response(),
-            Self::BadParent => HttpResponse::BadRequest()
-                .reason("invalid comment parent")
-                .finish(),
-        }
+            Self::InvalidOrigin |
+            Self::InvalidBody |
+            Self::InvalidUrl |
+            Self::InvalidFields |
+            Self::InvalidContentId |
+            Self::InvalidParent |
+            Self::EmailRequired |
+            Self::NameRequired => HttpResponse::BadRequest(),
+            Self::DatabaseAccessError |
+            Self::DatabaseInternalError |
+            Self::SanitizationError |
+            Self::PageFetchError => HttpResponse::InternalServerError(),
+        }.reason(match self {
+            Self::InvalidOrigin => "invalid request origin",
+            Self::InvalidBody => "invalid request body",
+            Self::InvalidUrl => "invalid request url",
+            Self::InvalidFields => "invalid request field",
+            Self::InvalidContentId => "invalid request content id",
+            Self::InvalidParent => "invalid comment parent",
+            Self::EmailRequired => "comment email required",
+            Self::NameRequired => "comment name required",
+            Self::DatabaseAccessError => "database access error",
+            Self::DatabaseInternalError => "database internal error",
+            Self::SanitizationError => "comment sanitization error",
+            Self::PageFetchError => "page fetch error",
+        }).finish()
     }
 }
 
 impl AppState {
     fn get_db<'a>(
-        &'a self,
-        request: &HttpRequest,
-    ) -> Result<MutexGuard<'a, Database>, DatabaseAccessError> {
-        self.get_db_with_origin(get_request_origin(request))
-    }
-
-    fn get_db_with_origin<'a>(
         &'a self,
         origin: Option<String>,
-    ) -> Result<MutexGuard<'a, Database>, DatabaseAccessError> {
+    ) -> Result<MutexGuard<'a, Database>, Error> {
         let origin = match origin {
             Some(origin) => origin,
-            None => return Err(DatabaseAccessError::BadOrigin),
+            None => return Err(Error::InvalidOrigin),
         };
         match self.databases.get(&origin) {
             Some(database) => Ok(match database.lock() {
                 Ok(database) => database,
-                Err(_) => return Err(DatabaseAccessError::AccessError),
+                Err(_) => return Err(Error::DatabaseAccessError),
             }),
-            None => return Err(DatabaseAccessError::AccessError),
+            None => return Err(Error::InvalidOrigin),
         }
     }
 }
@@ -124,35 +123,40 @@ struct Arguments {
     email_required: bool,
 }
 
-#[get("/{content_id}")]
-async fn get_comments(
+async fn _get_comments(
     data: web::Data<AppState>,
     request: HttpRequest,
     content_id: web::Path<String>,
-) -> impl Responder {
+) -> Result<Vec<Comment>, Error> {
     let origin = get_request_origin(&request);
-    let comments = match web::block(move || {
+    match web::block(move || {
         Ok(
-            match match data.get_db_with_origin(origin) {
+            match match data.get_db(origin) {
                 Ok(database) => database,
                 Err(err) => return Err(err),
             }
             .get_comments(&content_id)
             {
                 Ok(comments) => comments,
-                Err(_) => return Err(DatabaseAccessError::DatabaseError),
+                Err(_) => return Err(Error::DatabaseInternalError),
             },
         )
-    })
-    .await
-    {
-        Ok(comments) => match comments {
-            Ok(comments) => comments,
-            Err(err) => return err.to_http_response(),
-        },
-        Err(_) => return DatabaseAccessError::AccessError.to_http_response(),
-    };
-    HttpResponse::Ok().json(comments)
+    }).await {
+        Ok(result) => result,
+        Err(_) => Err(Error::DatabaseAccessError),
+    }
+}
+
+#[get("/{content_id}")]
+async fn get_comments(
+    data: web::Data<AppState>,
+    request: HttpRequest,
+    content_id: web::Path<String>,
+) -> HttpResponse {
+    match _get_comments(data, request, content_id).await {
+        Ok(comments) => HttpResponse::Ok().json(comments),
+        Err(err) => err.to_http_response(),
+    }
 }
 
 #[derive(Deserialize)]
@@ -161,12 +165,11 @@ struct PostCommentsRequest {
     comment: Comment,
 }
 
-#[post("/")]
-async fn post_comment(
+async fn _post_comment(
     data: web::Data<AppState>,
     request: HttpRequest,
     bytes: web::Bytes,
-) -> impl Responder {
+) -> Result<(), Error> {
     match String::from_utf8(bytes.to_vec()) {
         Ok(text) => {
             let PostCommentsRequest { url, comment } =
@@ -181,32 +184,26 @@ async fn post_comment(
                             Ok(())
                         };
                         if let Err(_) = sanitize_req() {
-                            return HttpResponse::InternalServerError()
-                                .reason("failed to sanitize request")
-                                .finish();
+                            return Err(Error::SanitizationError);
                         }
                         req
                     }
                     Err(_) => {
-                        return HttpResponse::BadRequest()
-                            .reason("invalid request body")
-                            .finish()
+                        return Err(Error::InvalidBody);
                     }
                 };
             if comment.validate().is_err() {
-                return HttpResponse::BadRequest()
-                    .reason("invalid comment field(s)")
-                    .finish();
+                return Err(Error::InvalidFields);
             }
             if comment.author.is_none() && data.arguments.name_required {
-                return HttpResponse::BadRequest().reason("name required").finish();
+                return Err(Error::NameRequired);
             }
             if comment.email.is_none() && data.arguments.email_required {
-                return HttpResponse::BadRequest().reason("email required").finish();
+                return Err(Error::EmailRequired);
             }
             let origin = match get_request_origin(&request) {
                 Some(origin) => origin,
-                None => return HttpResponse::BadRequest().reason("bad origin").finish(),
+                None => return Err(Error::InvalidOrigin),
             };
             // Check to see if provided URL is in scope.
             // This is to prevent malicious requests that try to get server to fetch external websites.
@@ -218,31 +215,25 @@ async fn post_comment(
                         break 'outer;
                     }
                 }
-                return HttpResponse::BadRequest()
-                    .reason("url out of scope")
-                    .finish();
+                return Err(Error::InvalidUrl);
             }
             match get_page_data(&url).await {
                 Ok(page_data_option) => match page_data_option {
                     Some(page_data) => {
                         if page_data.content_id != comment.content_id {
-                            return HttpResponse::BadRequest()
-                                .reason("content ids don't match")
-                                .finish();
+                            return Err(Error::InvalidContentId)
                         }
                     }
-                    None => return HttpResponse::BadRequest().reason("url invalid").finish(), // e.g. 404
+                    None => return Err(Error::InvalidUrl), // e.g. 404
                 },
                 Err(_) => {
-                    return HttpResponse::InternalServerError()
-                        .reason("failed to get page data")
-                        .finish()
+                    return Err(Error::PageFetchError);
                 }
             };
             match web::block(move || {
-                let database = match data.get_db_with_origin(Some(origin)) {
+                let database = match data.get_db(Some(origin)) {
                     Ok(database) => database,
-                    Err(err) => return Err(CommentCreationError::DatabaseAccessError(err)),
+                    Err(err) => return Err(err),
                 };
                 if let Some(parent) = comment.parent {
                     'outer2: loop {
@@ -256,35 +247,38 @@ async fn post_comment(
                                         break;
                                     }
                                 }
-                            }
+                            },
                             Err(_) => {
-                                return Err(CommentCreationError::DatabaseAccessError(
-                                    DatabaseAccessError::DatabaseError,
-                                ))
+                                return Err(Error::DatabaseInternalError);
                             }
                         };
-                        return Err(CommentCreationError::BadParent);
+                        return Err(Error::InvalidParent);
                     }
                 }
                 if let Err(_) = database.create_comment(&comment) {
-                    return Err(CommentCreationError::DatabaseAccessError(
-                        DatabaseAccessError::DatabaseError,
-                    ));
+                    return Err(Error::DatabaseInternalError);
                 }
                 Ok(())
             })
             .await
             {
-                Ok(result) => match result {
-                    Ok(_) => HttpResponse::Ok().into(),
-                    Err(error) => error.to_http_response(),
-                },
-                Err(_) => DatabaseAccessError::AccessError.to_http_response(),
+                Ok(result) => result,
+                Err(_) => Err(Error::DatabaseAccessError),
             }
         }
-        Err(_) => HttpResponse::BadRequest()
-            .reason("failed to parse request body")
-            .finish(),
+        Err(_) => Err(Error::InvalidBody),
+    }
+}
+
+#[post("/")]
+async fn post_comment(
+    data: web::Data<AppState>,
+    request: HttpRequest,
+    bytes: web::Bytes,
+) -> HttpResponse {
+    match _post_comment(data, request, bytes).await {
+        Ok(_) => HttpResponse::Ok().finish(),
+        Err(err) => err.to_http_response(),
     }
 }