diff --git a/src/agenda.rs b/src/agenda.rs index 310cc51db..812add27b 100644 --- a/src/agenda.rs +++ b/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, AppError> { diff --git a/src/bors.rs b/src/bors.rs index 5878de27f..aad469709 100644 --- a/src/bors.rs +++ b/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>, diff --git a/src/errors.rs b/src/errors.rs new file mode 100644 index 000000000..d6192e453 --- /dev/null +++ b/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 }, + /// 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 From for AppError +where + E: Into, +{ + 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 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"); + } +} diff --git a/src/gh_changes_since.rs b/src/gh_changes_since.rs index 32c174fd2..2009ff5e8 100644 --- a/src/gh_changes_since.rs +++ b/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). diff --git a/src/gh_range_diff.rs b/src/gh_range_diff.rs index 686d138f5..9ff597989 100644 --- a/src/gh_range_diff.rs +++ b/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 = LazyLock::new(|| Regex::new(r"@@ -[\d]+,[\d]+ [+][\d]+,[\d]+ @@").unwrap()); diff --git a/src/gha_logs.rs b/src/gha_logs.rs index 8dacfe4f0..89424c22d 100644 --- a/src/gha_logs.rs +++ b/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; diff --git a/src/github.rs b/src/github.rs index 5c4c4e3a6..c3a8aa572 100644 --- a/src/github.rs +++ b/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,12 +547,6 @@ where } } -#[derive(Debug)] -pub enum AssignmentError { - InvalidAssignee, - Http(anyhow::Error), -} - #[derive(Debug)] pub enum Selection<'a, T: ?Sized> { All, @@ -559,17 +554,6 @@ pub enum Selection<'a, T: ?Sized> { 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, -} - -// 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"); - } -} diff --git a/src/handlers.rs b/src/handlers.rs index 0b5a79e8c..2f8bcf342 100644 --- a/src/handlers.rs +++ b/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::() { + 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::() { - errors.push(HandlerError::Message(std::mem::take(&mut err.0))); + if let Some(err) = err.downcast_mut::() { + 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) - } -} diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index ef5cfdd2b..85cb4e111 100644 --- a/src/handlers/assign.rs +++ b/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() diff --git a/src/handlers/autolabel.rs b/src/handlers/autolabel.rs index eaf9d357e..0b823597d 100644 --- a/src/handlers/autolabel.rs +++ b/src/handlers/autolabel.rs @@ -203,21 +203,11 @@ pub(super) async fn handle_input( event: &IssuesEvent, input: AutolabelInput, ) -> anyhow::Result<()> { - match event.issue.add_labels(&ctx.github, input.add).await { - Ok(()) => {} - Err(e) => { - use crate::github::UnknownLabels; - if let Some(err @ UnknownLabels { .. }) = e.downcast_ref() { - event - .issue - .post_comment(&ctx.github, &err.to_string()) - .await - .context("failed to post missing label comment")?; - return Ok(()); - } - return Err(e); - } - } + event + .issue + .add_labels(&ctx.github, input.add) + .await + .context("failed to add the labels to the issue")?; event .issue diff --git a/src/handlers/close.rs b/src/handlers/close.rs index 539f6653d..2d9f2dccc 100644 --- a/src/handlers/close.rs +++ b/src/handlers/close.rs @@ -1,6 +1,6 @@ //! Allows to close an issue or a PR -use crate::{config::CloseConfig, github::Event, handlers::Context}; +use crate::{config::CloseConfig, errors::user_error, github::Event, handlers::Context}; use parser::command::close::CloseCommand; pub(super) async fn handle_command( diff --git a/src/handlers/concern.rs b/src/handlers/concern.rs index f06fb74ba..a6b509eac 100644 --- a/src/handlers/concern.rs +++ b/src/handlers/concern.rs @@ -4,6 +4,7 @@ use anyhow::{Context as _, bail}; use crate::{ config::ConcernConfig, + errors::user_error, github::{Event, Label}, handlers::Context, interactions::EditIssueBody, diff --git a/src/handlers/major_change.rs b/src/handlers/major_change.rs index d42f9f73b..38ba92735 100644 --- a/src/handlers/major_change.rs +++ b/src/handlers/major_change.rs @@ -1,5 +1,6 @@ use std::fmt::Display; +use crate::errors::user_error; use crate::jobs::Job; use crate::zulip::api::Recipient; use crate::{ diff --git a/src/handlers/nominate.rs b/src/handlers/nominate.rs index cb3dac179..89321e3bc 100644 --- a/src/handlers/nominate.rs +++ b/src/handlers/nominate.rs @@ -2,6 +2,7 @@ use crate::{ config::NominateConfig, + errors::user_error, github::{self, Event}, handlers::Context, interactions::ErrorComment, diff --git a/src/handlers/ping.rs b/src/handlers/ping.rs index 9b0eb700e..921b023f9 100644 --- a/src/handlers/ping.rs +++ b/src/handlers/ping.rs @@ -8,6 +8,7 @@ use std::borrow::Cow; use crate::{ config::PingConfig, + errors::user_error, github::{self, Event}, handlers::Context, }; diff --git a/src/handlers/relabel.rs b/src/handlers/relabel.rs index d76725bc7..191e0a720 100644 --- a/src/handlers/relabel.rs +++ b/src/handlers/relabel.rs @@ -10,14 +10,15 @@ use std::collections::BTreeSet; +use crate::errors::user_error; use crate::github::Label; use crate::team_data::TeamClient; use crate::{ config::RelabelConfig, - github::UnknownLabels, github::{self, Event}, handlers::Context, }; +use anyhow::Context as _; use parser::command::relabel::{LabelDelta, RelabelCommand}; pub(super) async fn handle_command( @@ -54,26 +55,16 @@ pub(super) async fn handle_command( let (to_add, to_remove) = compute_label_deltas(&input.0); // Add labels - if let Err(e) = issue.add_labels(&ctx.github, to_add.clone()).await { - tracing::error!( - "failed to add {to_add:?} from issue {issue}: {e:?}", - issue = issue.global_id(), - ); - if let Some(err @ UnknownLabels { .. }) = e.downcast_ref() { - issue.post_comment(&ctx.github, &err.to_string()).await?; - } - - return Err(e); - } + issue + .add_labels(&ctx.github, to_add.clone()) + .await + .context("failed to add labels to the issue")?; // Remove labels - if let Err(e) = issue.remove_labels(&ctx.github, to_remove.clone()).await { - tracing::error!( - "failed to remove {to_remove:?} from issue {issue}: {e:?}", - issue = issue.global_id(), - ); - return Err(e); - } + issue + .remove_labels(&ctx.github, to_remove.clone()) + .await + .context("failed to remove labels from the issue")?; Ok(()) } diff --git a/src/handlers/shortcut.rs b/src/handlers/shortcut.rs index 6ef58cea9..78e9394f9 100644 --- a/src/handlers/shortcut.rs +++ b/src/handlers/shortcut.rs @@ -5,6 +5,7 @@ use crate::{ config::ShortcutConfig, db::issue_data::IssueData, + errors::user_error, github::{Event, Label}, handlers::Context, }; diff --git a/src/lib.rs b/src/lib.rs index c86699e95..cee362e31 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,6 +6,7 @@ pub mod bors; mod changelogs; mod config; pub mod db; +mod errors; pub mod gh_changes_since; pub mod gh_range_diff; pub mod gha_logs; diff --git a/src/notification_listing.rs b/src/notification_listing.rs index 006f0fcbf..86a94ba9f 100644 --- a/src/notification_listing.rs +++ b/src/notification_listing.rs @@ -9,7 +9,7 @@ use axum::{ use hyper::StatusCode; use serde::Deserialize; -use crate::{db::notifications::get_notifications, handlers::Context, utils::AppError}; +use crate::{db::notifications::get_notifications, errors::AppError, handlers::Context}; #[derive(Deserialize)] pub struct NotificationsQuery { diff --git a/src/utils.rs b/src/utils.rs index 9effca4c8..ab65d2ea6 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,10 +1,6 @@ -use crate::{handlers::Context, interactions::REPORT_TO}; +use crate::handlers::Context; use anyhow::Context as _; -use axum::{ - http::StatusCode, - response::{IntoResponse, Response}, -}; use std::borrow::Cow; /// Pluralize (add an 's' sufix) to `text` based on `count`. @@ -16,28 +12,6 @@ pub fn pluralize(text: &str, count: usize) -> Cow<'_, str> { } } -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 From for AppError -where - E: Into, -{ - fn from(err: E) -> Self { - AppError(err.into()) - } -} - pub(crate) async fn is_repo_autorized( ctx: &Context, owner: &str,