Improve our handling of user facing errors

src/agenda.rs

@@ -1,8 +1,8 @@
 use axum::response::Html;
 
 use crate::actions::{Action, Query, QueryKind, QueryMap, Step};
+use crate::errors::AppError;
 use crate::github;
-use crate::utils::AppError;
 use std::sync::Arc;
 
 pub async fn lang_http() -> axum::response::Result<Html<String>, AppError> {

src/bors.rs

@@ -2,7 +2,7 @@ use std::sync::Arc;
 
 use axum::{Json, extract::State};
 
-use crate::{db, handlers::Context, utils::AppError};
+use crate::{db, errors::AppError, handlers::Context};
 
 pub async fn bors_commit_list(
     State(ctx): State<Arc<Context>>,

src/errors.rs

@@ -0,0 +1,119 @@
+//! Errors handling
+
+use std::fmt;
+
+use crate::interactions::REPORT_TO;
+
+use axum::{
+    http::StatusCode,
+    response::{IntoResponse, Response},
+};
+
+/// Represent a user error.
+///
+/// The message will be shown to the user via comment posted by this bot.
+#[derive(Debug)]
+pub enum UserError {
+    /// Simple message
+    Message(String),
+    /// Unknown labels
+    UnknownLabels { labels: Vec<String> },
+    /// Invalid assignee
+    InvalidAssignee,
+}
+
+impl std::error::Error for UserError {}
+
+// NOTE: This is used to post the Github comment; make sure it's valid markdown.
+impl fmt::Display for UserError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            UserError::Message(msg) => f.write_str(msg),
+            UserError::UnknownLabels { labels } => {
+                write!(f, "Unknown labels: {}", labels.join(", "))
+            }
+            UserError::InvalidAssignee => write!(f, "invalid assignee"),
+        }
+    }
+}
+
+/// Creates a [`UserError`] with message.
+///
+/// Should be used when an handler is in error due to the user action's (not a PR,
+/// not a issue, not authorized, ...).
+///
+/// Should be used like this `return user_error!("My error message.");`.
+macro_rules! user_error {
+    ($err:expr $(,)?) => {
+        anyhow::Result::Err(anyhow::anyhow!(crate::errors::UserError::Message(
+            $err.into()
+        )))
+    };
+}
+
+// export the macro
+pub(crate) use user_error;
+
+/// Represent a application error.
+///
+/// Useful for returning a error via the API
+pub struct AppError(anyhow::Error);
+
+impl IntoResponse for AppError {
+    fn into_response(self) -> Response {
+        tracing::error!("{:?}", &self.0);
+        (
+            StatusCode::INTERNAL_SERVER_ERROR,
+            format!("Something went wrong: {}\n\n{REPORT_TO}", self.0),
+        )
+            .into_response()
+    }
+}
+
+impl<E> From<E> for AppError
+where
+    E: Into<anyhow::Error>,
+{
+    fn from(err: E) -> Self {
+        AppError(err.into())
+    }
+}
+
+/// Represent an error when trying to assign someone
+#[derive(Debug)]
+pub enum AssignmentError {
+    InvalidAssignee,
+    Other(anyhow::Error),
+}
+
+// NOTE: This is used to post the Github comment; make sure it's valid markdown.
+impl fmt::Display for AssignmentError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            AssignmentError::InvalidAssignee => write!(f, "invalid assignee"),
+            AssignmentError::Other(err) => write!(f, "{err}"),
+        }
+    }
+}
+
+impl From<AssignmentError> for anyhow::Error {
+    fn from(a: AssignmentError) -> Self {
+        match a {
+            AssignmentError::InvalidAssignee => UserError::InvalidAssignee.into(),
+            AssignmentError::Other(err) => err.context("assignment error"),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn display_labels() {
+        let x = UserError::UnknownLabels {
+            labels: vec!["A-bootstrap".into(), "xxx".into()],
+        };
+        assert_eq!(x.to_string(), "Unknown labels: A-bootstrap, xxx");
+    }
+}

src/gh_changes_since.rs

@@ -7,11 +7,7 @@ use axum::{
 };
 use hyper::StatusCode;
 
-use crate::{
-    github,
-    handlers::Context,
-    utils::{AppError, is_repo_autorized},
-};
+use crate::{errors::AppError, github, handlers::Context, utils::is_repo_autorized};
 
 /// Redirects to either `/gh-range-diff` (when the base changed) or to GitHub's compare
 /// page (when the base is the same).

src/gh_range_diff.rs

@@ -23,7 +23,7 @@ use unicode_segmentation::UnicodeSegmentation;
 
 use crate::github::GithubCompare;
 use crate::utils::is_repo_autorized;
-use crate::{github, handlers::Context, utils::AppError};
+use crate::{errors::AppError, github, handlers::Context};
 
 static MARKER_RE: LazyLock<Regex> =
     LazyLock::new(|| Regex::new(r"@@ -[\d]+,[\d]+ [+][\d]+,[\d]+ @@").unwrap());

src/gha_logs.rs

@@ -1,7 +1,8 @@
+use crate::errors::AppError;
 use crate::github::{self, WorkflowRunJob};
 use crate::handlers::Context;
 use crate::interactions::REPORT_TO;
-use crate::utils::{AppError, is_repo_autorized};
+use crate::utils::is_repo_autorized;
 use anyhow::Context as _;
 use axum::extract::{Path, State};
 use axum::http::HeaderValue;

src/github.rs

@@ -1,3 +1,4 @@
+use crate::errors::{AssignmentError, UserError};
 use crate::team_data::TeamClient;
 use anyhow::Context;
 use async_trait::async_trait;
@@ -546,30 +547,13 @@ where
     }
 }
 
-#[derive(Debug)]
-pub enum AssignmentError {
-    InvalidAssignee,
-    Http(anyhow::Error),
-}
-
 #[derive(Debug)]
 pub enum Selection<'a, T: ?Sized> {
     All,
     One(&'a T),
     Except(&'a T),
 }
 
-impl fmt::Display for AssignmentError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            AssignmentError::InvalidAssignee => write!(f, "invalid assignee"),
-            AssignmentError::Http(e) => write!(f, "cannot assign: {e}"),
-        }
-    }
-}
-
-impl std::error::Error for AssignmentError {}
-
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct IssueRepository {
     pub organization: String,
@@ -612,20 +596,6 @@ impl IssueRepository {
     }
 }
 
-#[derive(Debug)]
-pub(crate) struct UnknownLabels {
-    labels: Vec<String>,
-}
-
-// NOTE: This is used to post the Github comment; make sure it's valid markdown.
-impl fmt::Display for UnknownLabels {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "Unknown labels: {}", &self.labels.join(", "))
-    }
-}
-
-impl std::error::Error for UnknownLabels {}
-
 impl Issue {
     pub fn to_zulip_github_reference(&self) -> ZulipGitHubReference {
         ZulipGitHubReference {
@@ -867,7 +837,7 @@ impl Issue {
         }
 
         if !unknown_labels.is_empty() {
-            return Err(UnknownLabels {
+            return Err(UserError::UnknownLabels {
                 labels: unknown_labels,
             }
             .into());
@@ -929,12 +899,14 @@ impl Issue {
         struct AssigneeReq<'a> {
             assignees: &'a [&'a str],
         }
+
         client
             .send_req(client.delete(&url).json(&AssigneeReq {
                 assignees: &assignees[..],
             }))
             .await
-            .map_err(AssignmentError::Http)?;
+            .map_err(AssignmentError::Other)?;
+
         Ok(())
     }
 
@@ -958,7 +930,8 @@ impl Issue {
         let result: Issue = client
             .json(client.post(&url).json(&AssigneeReq { assignees: &[user] }))
             .await
-            .map_err(AssignmentError::Http)?;
+            .map_err(AssignmentError::Other)?;
+
         // Invalid assignees are silently ignored. We can just check if the user is now
         // contained in the assignees list.
         let success = result
@@ -3272,16 +3245,3 @@ impl Submodule {
         client.repository(fullname).await
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn display_labels() {
-        let x = UnknownLabels {
-            labels: vec!["A-bootstrap".into(), "xxx".into()],
-        };
-        assert_eq!(x.to_string(), "Unknown labels: A-bootstrap, xxx");
-    }
-}

src/handlers.rs

@@ -10,18 +10,6 @@ use std::fmt;
 use std::sync::Arc;
 use tracing as log;
 
-/// Creates a [`UserError`] with message.
-///
-/// Should be used when an handler is in error due to the user action's (not a PR,
-/// not a issue, not authorized, ...).
-///
-/// Should be used like this `return user_error!("My error message.");`.
-macro_rules! user_error {
-    ($err:expr $(,)?) => {
-        anyhow::Result::Err(anyhow::anyhow!(crate::handlers::UserError($err.into())))
-    };
-}
-
 mod assign;
 mod autolabel;
 mod backport;
@@ -274,11 +262,15 @@ macro_rules! issue_handlers {
                                 if let Some(config) = &config.$name {
                                     $name::handle_input(ctx, config, event, input)
                                         .await
-                                        .map_err(|e| {
-                                            HandlerError::Other(e.context(format!(
-                                                "error when processing {} handler",
-                                                stringify!($name)
-                                            )))
+                                        .map_err(|mut e| {
+                                            if let Some(err) = e.downcast_mut::<crate::errors::UserError>() {
+                                                HandlerError::Message(err.to_string())
+                                            } else {
+                                                HandlerError::Other(e.context(format!(
+                                                    "error when processing {} handler",
+                                                    stringify!($name)
+                                                )))
+                                            }
                                         })
                                 } else {
                                     Err(HandlerError::Message(format!(
@@ -419,8 +411,8 @@ macro_rules! command_handlers {
                             $name::handle_command(ctx, config, event, command)
                                 .await
                                 .unwrap_or_else(|mut err| {
-                                    if let Some(err) = err.downcast_mut::<UserError>() {
-                                        errors.push(HandlerError::Message(std::mem::take(&mut err.0)));
+                                    if let Some(err) = err.downcast_mut::<crate::errors::UserError>() {
+                                        errors.push(HandlerError::Message(err.to_string()));
                                     } else {
                                         errors.push(HandlerError::Message(format!(
                                             "`{}` handler unexpectedly failed in [this comment]({}): {err}",
@@ -490,17 +482,3 @@ impl fmt::Display for HandlerError {
         }
     }
 }
-
-/// Represent a user error.
-///
-/// The message will be shown to the user via comment posted by this bot.
-#[derive(Debug)]
-pub struct UserError(String);
-
-impl std::error::Error for UserError {}
-
-impl fmt::Display for UserError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.write_str(&self.0)
-    }
-}

src/handlers/assign.rs

@@ -22,6 +22,7 @@
 
 use crate::db::issue_data::IssueData;
 use crate::db::review_prefs::{RotationMode, get_review_prefs_batch};
+use crate::errors::{self, AssignmentError, user_error};
 use crate::github::UserId;
 use crate::handlers::pr_tracking::ReviewerWorkqueue;
 use crate::{
@@ -554,8 +555,8 @@ pub(super) async fn handle_command(
                         .add_labels(&ctx.github, vec![github::Label { name: t_label }])
                         .await
                     {
-                        if let Some(github::UnknownLabels { .. }) = err.downcast_ref() {
-                            log::warn!("Error assigning label: {err}");
+                        if let Some(errors::UserError::UnknownLabels { .. }) = err.downcast_ref() {
+                            log::warn!("error assigning team label: {err}");
                         } else {
                             return Err(err);
                         }
@@ -655,11 +656,8 @@ pub(super) async fn handle_command(
                 e.apply(&ctx.github, String::new()).await?;
                 return Ok(());
             } // we are done
-            Err(github::AssignmentError::InvalidAssignee) => {
-                issue
-                    .set_assignee(&ctx.github, &ctx.username)
-                    .await
-                    .context("self-assignment failed")?;
+            Err(AssignmentError::InvalidAssignee) => {
+                issue.set_assignee(&ctx.github, &ctx.username).await?;
                 let cmt_body = format!(
                     "This issue has been assigned to @{to_assign} via [this comment]({}).",
                     event.html_url().unwrap()