refa(common): better ApiError constructors & getters, feature gate tracing (#2045)

* refa(common): better ApiError constructors & getters, feature gate tracing

* axum status code fallback

* use display for error types

* clippy

Author: jonaro00

api-client/src/util.rs

@@ -32,10 +32,13 @@ impl ToJson for reqwest::Response {
 
             let res: ApiError = match serde_json::from_str(&string) {
                 Ok(res) => res,
-                _ => ApiError {
-                    message: format!("Failed to parse response from the server:\n{}", string),
-                    status_code: status_code.as_u16(),
-                },
+                _ => ApiError::new(
+                    format!(
+                        "Failed to parse error response from the server:\n{}",
+                        string
+                    ),
+                    status_code,
+                ),
             };
 
             Err(res.into())

cargo-shuttle/src/lib.rs

@@ -611,8 +611,8 @@ impl Shuttle {
                 // If API error contains message regarding format of error name, print that error and prompt again
                 if let Ok(api_error) = e.downcast::<ApiError>() {
                     // If the returned error string changes, this could break
-                    if api_error.message.contains("Invalid project name") {
-                        eprintln!("{}", api_error.message.yellow());
+                    if api_error.message().contains("Invalid project name") {
+                        eprintln!("{}", api_error.message().yellow());
                         eprintln!("{}", "Try a different name.".yellow());
                         return false;
                     }

common/Cargo.toml

@@ -32,18 +32,19 @@ rstest = "0.24.0"
 
 [features]
 # main features
-models = ["chrono/serde", "dep:tracing"]
+models = ["chrono/serde"]
 config = ["anyhow", "dirs", "toml"]
 
 # additional sub-features
-axum = ["dep:axum", "dep:tracing"]
+axum = ["dep:axum"]
 display = ["chrono/clock", "dep:crossterm"]
 tables = ["models", "display", "dep:comfy-table"]
+tracing-in-errors = ["dep:tracing"]
 unknown-variants = [] # add fallback to Unknown variant on enum model deser
 utoipa = ["dep:utoipa"] # derive OpenAPI definitions for models
 
 # internal / utility features
-integration-tests = []
+integration-tests = ["dep:tracing"]
 
 [package.metadata.docs.rs]
 features = [

common/src/models/error.rs

@@ -1,226 +1,166 @@
-use std::fmt::{Display, Formatter};
+use std::fmt::Display;
 
-use http::StatusCode;
+use http::{status::InvalidStatusCode, StatusCode};
 use serde::{Deserialize, Serialize};
 
-#[cfg(feature = "display")]
-use crossterm::style::Stylize;
-
 #[cfg(feature = "axum")]
 impl axum::response::IntoResponse for ApiError {
     fn into_response(self) -> axum::response::Response {
+        #[cfg(feature = "tracing-in-errors")]
         tracing::warn!("{}", self.message);
 
-        (self.status(), axum::Json(self)).into_response()
+        (
+            self.status().unwrap_or(StatusCode::INTERNAL_SERVER_ERROR),
+            axum::Json(self),
+        )
+            .into_response()
     }
 }
 
 #[derive(Serialize, Deserialize, Debug)]
 #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))]
 #[typeshare::typeshare]
 pub struct ApiError {
-    pub message: String,
-    pub status_code: u16,
+    message: String,
+    status_code: u16,
 }
 
 impl ApiError {
-    pub fn internal(message: &str) -> Self {
+    #[inline(always)]
+    pub fn new(message: impl Display, status_code: StatusCode) -> Self {
         Self {
             message: message.to_string(),
-            status_code: StatusCode::INTERNAL_SERVER_ERROR.as_u16(),
+            status_code: status_code.as_u16(),
         }
     }
+    #[inline(always)]
+    pub fn status(&self) -> Result<StatusCode, InvalidStatusCode> {
+        StatusCode::from_u16(self.status_code)
+    }
+    #[inline(always)]
+    pub fn message(&self) -> &str {
+        self.message.as_str()
+    }
+
+    /// Create a one-off internal error with a string message exposed to the user.
+    #[inline(always)]
+    pub fn internal(message: impl AsRef<str>) -> Self {
+        #[cfg(feature = "tracing-in-errors")]
+        {
+            /// Dummy wrapper to allow logging a string `as &dyn std::error::Error`
+            #[derive(Debug)]
+            struct InternalError(String);
+            impl std::error::Error for InternalError {}
+            impl std::fmt::Display for InternalError {
+                fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                    f.write_str(self.0.as_str())
+                }
+            }
+
+            tracing::error!(
+                error = &InternalError(message.as_ref().to_owned()) as &dyn std::error::Error,
+                "Internal API Error"
+            );
+        }
+
+        Self::_internal(message.as_ref())
+    }
 
     /// Creates an internal error without exposing sensitive information to the user.
     #[inline(always)]
     #[allow(unused_variables)]
-    pub fn internal_safe<E>(message: &str, error: E) -> Self
-    where
-        E: std::error::Error + 'static,
-    {
-        tracing::error!(error = &error as &dyn std::error::Error, "{message}");
+    pub fn internal_safe<E: std::error::Error + 'static>(safe_msg: impl Display, error: E) -> Self {
+        #[cfg(feature = "tracing-in-errors")]
+        tracing::error!(error = &error as &dyn std::error::Error, "{}", safe_msg);
 
         // Return the raw error during debug builds
         #[cfg(debug_assertions)]
         {
-            ApiError::internal(&error.to_string())
+            Self::_internal(error)
         }
         // Return the safe message during release builds
         #[cfg(not(debug_assertions))]
         {
-            ApiError::internal(message)
+            Self::_internal(safe_msg)
         }
     }
 
-    pub fn unavailable(error: impl std::error::Error) -> Self {
-        Self {
-            message: error.to_string(),
-            status_code: StatusCode::SERVICE_UNAVAILABLE.as_u16(),
-        }
+    // 5xx
+    #[inline(always)]
+    fn _internal(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::INTERNAL_SERVER_ERROR)
     }
-
-    pub fn bad_request(error: impl std::error::Error) -> Self {
-        Self {
-            message: error.to_string(),
-            status_code: StatusCode::BAD_REQUEST.as_u16(),
-        }
+    #[inline(always)]
+    pub fn service_unavailable(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::SERVICE_UNAVAILABLE)
     }
-
-    pub fn unauthorized() -> Self {
-        Self {
-            message: "Unauthorized".to_string(),
-            status_code: StatusCode::UNAUTHORIZED.as_u16(),
-        }
+    // 4xx
+    #[inline(always)]
+    pub fn bad_request(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::BAD_REQUEST)
     }
-
-    pub fn forbidden() -> Self {
-        Self {
-            message: "Forbidden".to_string(),
-            status_code: StatusCode::FORBIDDEN.as_u16(),
-        }
+    #[inline(always)]
+    pub fn unauthorized(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::UNAUTHORIZED)
     }
-
-    pub fn status(&self) -> StatusCode {
-        StatusCode::from_u16(self.status_code).unwrap_or(StatusCode::INTERNAL_SERVER_ERROR)
+    #[inline(always)]
+    pub fn forbidden(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::FORBIDDEN)
+    }
+    #[inline(always)]
+    pub fn not_found(error: impl Display) -> Self {
+        Self::new(error.to_string(), StatusCode::NOT_FOUND)
     }
 }
 
 pub trait ErrorContext<T> {
     /// Make a new internal server error with the given message.
-    #[inline(always)]
-    fn context_internal_error(self, message: &str) -> Result<T, ApiError>
-    where
-        Self: Sized,
-    {
-        self.with_context_internal_error(move || message.to_string())
-    }
+    fn context_internal_error(self, message: impl Display) -> Result<T, ApiError>;
 
     /// Make a new internal server error using the given function to create the message.
-    fn with_context_internal_error(self, message: impl FnOnce() -> String) -> Result<T, ApiError>;
-
-    /// Make a new bad request error with the given message.
-    #[inline(always)]
-    fn context_bad_request(self, message: &str) -> Result<T, ApiError>
-    where
-        Self: Sized,
-    {
-        self.with_context_bad_request(move || message.to_string())
-    }
-
-    /// Make a new bad request error using the given function to create the message.
-    fn with_context_bad_request(self, message: impl FnOnce() -> String) -> Result<T, ApiError>;
-
-    /// Make a new not found error with the given message.
     #[inline(always)]
-    fn context_not_found(self, message: &str) -> Result<T, ApiError>
+    fn with_context_internal_error(self, message: impl FnOnce() -> String) -> Result<T, ApiError>
     where
         Self: Sized,
     {
-        self.with_context_not_found(move || message.to_string())
+        self.context_internal_error(message())
     }
-
-    /// Make a new not found error using the given function to create the message.
-    fn with_context_not_found(self, message: impl FnOnce() -> String) -> Result<T, ApiError>;
 }
 
 impl<T, E> ErrorContext<T> for Result<T, E>
 where
     E: std::error::Error + 'static,
 {
     #[inline(always)]
-    fn with_context_internal_error(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Ok(value) => Ok(value),
-            Err(error) => Err(ApiError::internal_safe(message().as_ref(), error)),
-        }
-    }
-
-    #[inline(always)]
-    fn with_context_bad_request(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Ok(value) => Ok(value),
-            Err(error) => Err({
-                let message = message();
-                tracing::warn!(
-                    error = &error as &dyn std::error::Error,
-                    "bad request: {message}"
-                );
-
-                ApiError {
-                    message,
-                    status_code: StatusCode::BAD_REQUEST.as_u16(),
-                }
-            }),
-        }
-    }
-
-    #[inline(always)]
-    fn with_context_not_found(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Ok(value) => Ok(value),
-            Err(error) => Err({
-                let message = message();
-                tracing::warn!(
-                    error = &error as &dyn std::error::Error,
-                    "not found: {message}"
-                );
-
-                ApiError {
-                    message,
-                    status_code: StatusCode::NOT_FOUND.as_u16(),
-                }
-            }),
-        }
-    }
-}
-
-impl<T> ErrorContext<T> for Option<T> {
-    #[inline]
-    fn with_context_internal_error(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Some(value) => Ok(value),
-            None => Err(ApiError::internal(message().as_ref())),
-        }
-    }
-
-    #[inline]
-    fn with_context_bad_request(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Some(value) => Ok(value),
-            None => Err({
-                ApiError {
-                    message: message(),
-                    status_code: StatusCode::BAD_REQUEST.as_u16(),
-                }
-            }),
-        }
-    }
-
-    #[inline]
-    fn with_context_not_found(self, message: impl FnOnce() -> String) -> Result<T, ApiError> {
-        match self {
-            Some(value) => Ok(value),
-            None => Err({
-                ApiError {
-                    message: message(),
-                    status_code: StatusCode::NOT_FOUND.as_u16(),
-                }
-            }),
-        }
+    fn context_internal_error(self, message: impl Display) -> Result<T, ApiError> {
+        self.map_err(|error| ApiError::internal_safe(message, error))
     }
 }
 
-impl Display for ApiError {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        #[cfg(feature = "display")]
-        return write!(
+impl std::fmt::Display for ApiError {
+    #[cfg(feature = "display")]
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use crossterm::style::Stylize;
+        write!(
             f,
             "{}\nMessage: {}",
-            self.status().to_string().bold(),
+            self.status()
+                .map(|s| s.to_string())
+                .unwrap_or("Unknown".to_owned())
+                .bold(),
             self.message.to_string().red()
-        );
-        #[cfg(not(feature = "display"))]
-        return write!(f, "{}\nMessage: {}", self.status(), self.message);
+        )
+    }
+    #[cfg(not(feature = "display"))]
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "{}\nMessage: {}",
+            self.status()
+                .map(|s| s.to_string())
+                .unwrap_or("Unknown".to_owned()),
+            self.message,
+        )
     }
 }