Improved errors and error handling

ALU-Schumacher · Aug 17, 2022 · f8717d3 · f8717d3
1 parent 1c4337c
commit f8717d3
Show file tree

Hide file tree

Showing 12 changed files with 159 additions and 39 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/auditor/Cargo.toml b/auditor/Cargo.toml
@@ -68,6 +68,7 @@ shell-words = "^1"
 opentelemetry-prometheus = "0.10"
 opentelemetry = "0.17"
 actix-web-opentelemetry = { version = "0.12", features = ["metrics"] }
+thiserror = "1"
 
 [dependencies.sqlx]
 version = "0.5.7"

diff --git a/auditor/src/domain/component.rs b/auditor/src/domain/component.rs
@@ -6,7 +6,7 @@
 // copied, modified, or distributed except according to those terms.
 
 use super::{Score, ScoreTest, ValidAmount, ValidName};
-use anyhow::Error;
+use anyhow::{Context, Error};
 use fake::{Dummy, Fake, Faker, StringFaker};
 use rand::Rng;
 use serde::{Deserialize, Serialize};
@@ -26,8 +26,9 @@ pub struct Component {
 impl Component {
     pub fn new<T: AsRef<str>>(name: T, amount: i64) -> Result<Self, Error> {
         Ok(Component {
-            name: ValidName::parse(name.as_ref().to_string())?,
-            amount: ValidAmount::parse(amount)?,
+            name: ValidName::parse(name.as_ref().to_string())
+                .context("Failed to parse component name.")?,
+            amount: ValidAmount::parse(amount).context("Failed to parse component amount.")?,
             scores: vec![],
         })
     }

diff --git a/auditor/src/domain/mod.rs b/auditor/src/domain/mod.rs
@@ -12,9 +12,33 @@ mod validamount;
 mod validfactor;
 mod validname;
 
+use actix_web::{http::StatusCode, ResponseError};
 pub use component::{Component, ComponentTest};
 pub use record::{Record, RecordAdd, RecordTest, RecordUpdate};
 pub use score::{Score, ScoreTest};
 pub use validamount::ValidAmount;
 pub use validfactor::ValidFactor;
 pub use validname::ValidName;
+
+use crate::error::error_chain_fmt;
+
+#[derive(thiserror::Error)]
+pub struct ValidationError(String);
+
+impl std::fmt::Debug for ValidationError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        error_chain_fmt(self, f)
+    }
+}
+
+impl std::fmt::Display for ValidationError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Validating input failed: {}", self.0)
+    }
+}
+
+impl ResponseError for ValidationError {
+    fn status_code(&self) -> StatusCode {
+        StatusCode::BAD_REQUEST
+    }
+}
diff --git a/auditor/src/domain/record.rs b/auditor/src/domain/record.rs
@@ -8,7 +8,7 @@
 //! Record related types used for deserializing HTTP requests and serializing HTTP responses.
 
 use super::{Component, ComponentTest, ScoreTest, ValidName};
-use anyhow::Error;
+use anyhow::{Context, Error};
 use chrono::{DateTime, Utc};
 use fake::{Dummy, Fake, Faker, StringFaker};
 #[cfg(test)]
@@ -71,10 +71,14 @@ impl RecordAdd {
         start_time: DateTime<Utc>,
     ) -> Result<Self, Error> {
         Ok(RecordAdd {
-            record_id: ValidName::parse(record_id.as_ref().to_string())?,
-            site_id: ValidName::parse(site_id.as_ref().to_string())?,
-            user_id: ValidName::parse(user_id.as_ref().to_string())?,
-            group_id: ValidName::parse(group_id.as_ref().to_string())?,
+            record_id: ValidName::parse(record_id.as_ref().to_string())
+                .context("Failed to parse record_id.")?,
+            site_id: ValidName::parse(site_id.as_ref().to_string())
+                .context("Failed to parse site_id.")?,
+            user_id: ValidName::parse(user_id.as_ref().to_string())
+                .context("Failed to parse user_id.")?,
+            group_id: ValidName::parse(group_id.as_ref().to_string())
+                .context("Failed to parse group_id.")?,
             components,
             start_time,
             stop_time: None,
@@ -285,10 +289,13 @@ impl TryFrom<Record> for RecordAdd {
 
     fn try_from(value: Record) -> Result<Self, Self::Error> {
         Ok(RecordAdd {
-            record_id: ValidName::parse(value.record_id)?,
-            site_id: ValidName::parse(value.site_id.unwrap_or_else(|| "".to_string()))?,
-            user_id: ValidName::parse(value.user_id.unwrap_or_else(|| "".to_string()))?,
-            group_id: ValidName::parse(value.group_id.unwrap_or_else(|| "".to_string()))?,
+            record_id: ValidName::parse(value.record_id).context("Failed to parse record_id.")?,
+            site_id: ValidName::parse(value.site_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse site_id.")?,
+            user_id: ValidName::parse(value.user_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse user_id.")?,
+            group_id: ValidName::parse(value.group_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse group_id.")?,
             components: value
                 .components
                 .unwrap_or_default()
@@ -333,10 +340,13 @@ impl TryFrom<Record> for RecordUpdate {
 
     fn try_from(value: Record) -> Result<Self, Self::Error> {
         Ok(RecordUpdate {
-            record_id: ValidName::parse(value.record_id)?,
-            site_id: ValidName::parse(value.site_id.unwrap_or_else(|| "".to_string()))?,
-            user_id: ValidName::parse(value.user_id.unwrap_or_else(|| "".to_string()))?,
-            group_id: ValidName::parse(value.group_id.unwrap_or_else(|| "".to_string()))?,
+            record_id: ValidName::parse(value.record_id).context("Failed to parse record_id.")?,
+            site_id: ValidName::parse(value.site_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse site_id.")?,
+            user_id: ValidName::parse(value.user_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse user_id.")?,
+            group_id: ValidName::parse(value.group_id.unwrap_or_else(|| "".to_string()))
+                .context("Failed to parse group_id.")?,
             components: value
                 .components
                 .unwrap_or_default()

diff --git a/auditor/src/domain/score.rs b/auditor/src/domain/score.rs
@@ -6,7 +6,7 @@
 // copied, modified, or distributed except according to those terms.
 
 use super::{ValidFactor, ValidName};
-use anyhow::Error;
+use anyhow::{Context, Error};
 use fake::{Dummy, Fake, Faker, StringFaker};
 use rand::Rng;
 use serde::{Deserialize, Serialize};
@@ -23,8 +23,9 @@ pub struct Score {
 impl Score {
     pub fn new<T: AsRef<str>>(name: T, factor: f64) -> Result<Self, Error> {
         Ok(Score {
-            name: ValidName::parse(name.as_ref().to_string())?,
-            factor: ValidFactor::parse(factor)?,
+            name: ValidName::parse(name.as_ref().to_string())
+                .context("Failed to parse score name.")?,
+            factor: ValidFactor::parse(factor).context("Failed to parse score factor.")?,
         })
     }
 }

diff --git a/auditor/src/domain/validamount.rs b/auditor/src/domain/validamount.rs
@@ -5,6 +5,8 @@
 // http://opensource.org/licenses/MIT>, at your option. This file may not be
 // copied, modified, or distributed except according to those terms.
 
+use crate::domain::ValidationError;
+use anyhow::Context;
 use sqlx::{postgres::PgTypeInfo, Postgres, Type};
 use std::fmt;
 
@@ -17,9 +19,9 @@ pub struct ValidAmount(i64);
 
 impl ValidAmount {
     /// Returns `ValidAmount` only if input satisfies validation criteria, otherwise panics.
-    pub fn parse(s: i64) -> Result<ValidAmount, anyhow::Error> {
+    pub fn parse(s: i64) -> Result<ValidAmount, ValidationError> {
         if s < 0 {
-            Err(anyhow::anyhow!("Invalid amount: {}", s))
+            Err(ValidationError(format!("Invalid amount: {}", s)))
         } else {
             Ok(Self(s))
         }
@@ -57,7 +59,9 @@ impl<'de> serde::Deserialize<'de> for ValidAmount {
         D: serde::Deserializer<'de>,
     {
         let buf = i64::deserialize(deserializer)?;
-        ValidAmount::parse(buf).map_err(serde::de::Error::custom)
+        ValidAmount::parse(buf)
+            .with_context(|| format!("Parsing '{}' failed.", buf))
+            .map_err(serde::de::Error::custom)
     }
 }
 

diff --git a/auditor/src/domain/validfactor.rs b/auditor/src/domain/validfactor.rs
@@ -5,6 +5,8 @@
 // http://opensource.org/licenses/MIT>, at your option. This file may not be
 // copied, modified, or distributed except according to those terms.
 
+use crate::domain::ValidationError;
+use anyhow::Context;
 use sqlx::{postgres::PgTypeInfo, Postgres, Type};
 use std::fmt;
 
@@ -17,9 +19,9 @@ pub struct ValidFactor(f64);
 
 impl ValidFactor {
     /// Returns `ValidFactor` only if input satisfies validation criteria, otherwise panics.
-    pub fn parse(s: f64) -> Result<ValidFactor, anyhow::Error> {
+    pub fn parse(s: f64) -> Result<ValidFactor, ValidationError> {
         if s < 0.0 {
-            Err(anyhow::anyhow!("Invalid factor: {}", s))
+            Err(ValidationError(format!("Invalid factor: {}", s)))
         } else {
             Ok(Self(s))
         }
@@ -57,7 +59,9 @@ impl<'de> serde::Deserialize<'de> for ValidFactor {
         D: serde::Deserializer<'de>,
     {
         let buf = f64::deserialize(deserializer)?;
-        ValidFactor::parse(buf).map_err(serde::de::Error::custom)
+        ValidFactor::parse(buf)
+            .with_context(|| format!("Parsing '{}' failed.", buf))
+            .map_err(serde::de::Error::custom)
     }
 }
 

diff --git a/auditor/src/domain/validname.rs b/auditor/src/domain/validname.rs
@@ -6,6 +6,8 @@
 // copied, modified, or distributed except according to those terms.
 
 use crate::constants::FORBIDDEN_CHARACTERS;
+use crate::domain::ValidationError;
+use anyhow::Context;
 use std::fmt;
 use unicode_segmentation::UnicodeSegmentation;
 
@@ -19,15 +21,15 @@ pub struct ValidName(String);
 
 impl ValidName {
     /// Returns `ValidName` only if input satisfies validation criteria, otherwise panics.
-    pub fn parse(s: String) -> Result<ValidName, anyhow::Error> {
+    pub fn parse(s: String) -> Result<ValidName, ValidationError> {
         // remove trailing whitespace and check if string is then empty
         let is_empty_or_whitespace = s.trim().is_empty();
         // count characters
         let is_too_long = s.graphemes(true).count() > 256;
         // check for forbidden characters
         let contains_forbidden_characters = s.chars().any(|g| FORBIDDEN_CHARACTERS.contains(&g));
         if is_empty_or_whitespace || is_too_long || contains_forbidden_characters {
-            Err(anyhow::anyhow!("Invalid Name: {}", s))
+            Err(ValidationError(format!("Invalid Name: {}", s)))
         } else {
             Ok(Self(s))
         }
@@ -55,7 +57,12 @@ impl<'de> serde::Deserialize<'de> for ValidName {
         D: serde::Deserializer<'de>,
     {
         let buf = String::deserialize(deserializer)?;
-        ValidName::parse(buf).map_err(serde::de::Error::custom)
+        // Aaaah I don't like this clone at all... If stuff is slow, figure this out.
+        // I could remove the context, but it's nice to inform the user what's wrong. On the other
+        // hand, if users use our clients, this parsing can't fail.
+        ValidName::parse(buf.clone())
+            .with_context(|| format!("Parsing '{}' failed", buf))
+            .map_err(serde::de::Error::custom)
     }
 }
 

diff --git a/auditor/src/error.rs b/auditor/src/error.rs
@@ -0,0 +1,19 @@
+// Copyright 2021-2022 AUDITOR developers
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT or
+// http://opensource.org/licenses/MIT>, at your option. This file may not be
+// copied, modified, or distributed except according to those terms.
+
+pub fn error_chain_fmt(
+    e: &impl std::error::Error,
+    f: &mut std::fmt::Formatter<'_>,
+) -> std::fmt::Result {
+    writeln!(f, "{}\n", e)?;
+    let mut current = e.source();
+    while let Some(cause) = current {
+        writeln!(f, "Caused by:\n\t{}", cause)?;
+        current = cause.source();
+    }
+    Ok(())
+}
diff --git a/auditor/src/lib.rs b/auditor/src/lib.rs
@@ -13,6 +13,7 @@ pub mod client;
 pub mod configuration;
 pub mod constants;
 pub mod domain;
+pub mod error;
 pub mod routes;
 pub mod startup;
 pub mod telemetry;
diff --git a/auditor/src/routes/add.rs b/auditor/src/routes/add.rs
@@ -6,25 +6,52 @@
 // copied, modified, or distributed except according to those terms.
 
 use crate::domain::RecordAdd;
-use actix_web::{web, HttpResponse};
+use crate::error::error_chain_fmt;
+use actix_web::{http::StatusCode, web, HttpResponse, ResponseError};
 use chrono::Utc;
 use sqlx;
 use sqlx::PgPool;
 
+#[derive(thiserror::Error)]
+pub enum AddError {
+    #[error("{0}")]
+    ValidationError(String),
+    #[error(transparent)]
+    UnexpectedError(#[from] anyhow::Error),
+}
+
+impl ResponseError for AddError {
+    fn status_code(&self) -> StatusCode {
+        match self {
+            AddError::ValidationError(_) => StatusCode::BAD_REQUEST,
+            AddError::UnexpectedError(_) => StatusCode::INTERNAL_SERVER_ERROR,
+        }
+    }
+}
+
+impl std::fmt::Debug for AddError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        error_chain_fmt(self, f)
+    }
+}
+
 #[tracing::instrument(
     name = "Adding a record to the database",
     skip(record, pool),
     fields(record_id = %record.record_id)
 )]
-pub async fn add(record: web::Json<RecordAdd>, pool: web::Data<PgPool>) -> HttpResponse {
-    match add_record(&record, &pool).await {
-        Ok(_) => HttpResponse::Ok().finish(),
-        Err(_) => HttpResponse::InternalServerError().finish(),
-    }
+pub async fn add(
+    record: web::Json<RecordAdd>,
+    pool: web::Data<PgPool>,
+) -> Result<HttpResponse, AddError> {
+    add_record(&record, &pool)
+        .await
+        .map_err(AddError::UnexpectedError)?;
+    Ok(HttpResponse::Ok().finish())
 }
 
 #[tracing::instrument(name = "Inserting record into database", skip(record, pool))]
-pub async fn add_record(record: &RecordAdd, pool: &PgPool) -> Result<(), sqlx::Error> {
+pub async fn add_record(record: &RecordAdd, pool: &PgPool) -> Result<(), anyhow::Error> {
     let runtime = match record.stop_time.as_ref() {
         Some(&stop) => Some((stop - record.start_time).num_seconds()),
         _ => None,
@@ -50,10 +77,30 @@ pub async fn add_record(record: &RecordAdd, pool: &PgPool) -> Result<(), sqlx::E
     )
     .execute(pool)
     .await
-    .map_err(|e| {
-        tracing::error!("Failed to execute query: {:?}", e);
-        e
-    })?;
+    .map_err(AddRecordError)?;
 
     Ok(())
 }
+
+pub struct AddRecordError(sqlx::Error);
+
+impl std::error::Error for AddRecordError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        Some(&self.0)
+    }
+}
+
+impl std::fmt::Debug for AddRecordError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        error_chain_fmt(self, f)
+    }
+}
+
+impl std::fmt::Display for AddRecordError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "A database error was encountered while trying to store a record."
+        )
+    }
+}