Skip to content

Commit c6b8549

Browse files
authored
Merge pull request #2210 from Urgau/better-user-error
Improve our handling of user facing errors
2 parents 5424b88 + 87d1c44 commit c6b8549

20 files changed

+172
-159
lines changed

src/agenda.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use axum::response::Html;
22

33
use crate::actions::{Action, Query, QueryKind, QueryMap, Step};
4+
use crate::errors::AppError;
45
use crate::github;
5-
use crate::utils::AppError;
66
use std::sync::Arc;
77

88
pub async fn lang_http() -> axum::response::Result<Html<String>, AppError> {

src/bors.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::sync::Arc;
22

33
use axum::{Json, extract::State};
44

5-
use crate::{db, handlers::Context, utils::AppError};
5+
use crate::{db, errors::AppError, handlers::Context};
66

77
pub async fn bors_commit_list(
88
State(ctx): State<Arc<Context>>,

src/errors.rs

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
//! Errors handling
2+
3+
use std::fmt;
4+
5+
use crate::interactions::REPORT_TO;
6+
7+
use axum::{
8+
http::StatusCode,
9+
response::{IntoResponse, Response},
10+
};
11+
12+
/// Represent a user error.
13+
///
14+
/// The message will be shown to the user via comment posted by this bot.
15+
#[derive(Debug)]
16+
pub enum UserError {
17+
/// Simple message
18+
Message(String),
19+
/// Unknown labels
20+
UnknownLabels { labels: Vec<String> },
21+
/// Invalid assignee
22+
InvalidAssignee,
23+
}
24+
25+
impl std::error::Error for UserError {}
26+
27+
// NOTE: This is used to post the Github comment; make sure it's valid markdown.
28+
impl fmt::Display for UserError {
29+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
30+
match self {
31+
UserError::Message(msg) => f.write_str(msg),
32+
UserError::UnknownLabels { labels } => {
33+
write!(f, "Unknown labels: {}", labels.join(", "))
34+
}
35+
UserError::InvalidAssignee => write!(f, "invalid assignee"),
36+
}
37+
}
38+
}
39+
40+
/// Creates a [`UserError`] with message.
41+
///
42+
/// Should be used when an handler is in error due to the user action's (not a PR,
43+
/// not a issue, not authorized, ...).
44+
///
45+
/// Should be used like this `return user_error!("My error message.");`.
46+
macro_rules! user_error {
47+
($err:expr $(,)?) => {
48+
anyhow::Result::Err(anyhow::anyhow!(crate::errors::UserError::Message(
49+
$err.into()
50+
)))
51+
};
52+
}
53+
54+
// export the macro
55+
pub(crate) use user_error;
56+
57+
/// Represent a application error.
58+
///
59+
/// Useful for returning a error via the API
60+
pub struct AppError(anyhow::Error);
61+
62+
impl IntoResponse for AppError {
63+
fn into_response(self) -> Response {
64+
tracing::error!("{:?}", &self.0);
65+
(
66+
StatusCode::INTERNAL_SERVER_ERROR,
67+
format!("Something went wrong: {}\n\n{REPORT_TO}", self.0),
68+
)
69+
.into_response()
70+
}
71+
}
72+
73+
impl<E> From<E> for AppError
74+
where
75+
E: Into<anyhow::Error>,
76+
{
77+
fn from(err: E) -> Self {
78+
AppError(err.into())
79+
}
80+
}
81+
82+
/// Represent an error when trying to assign someone
83+
#[derive(Debug)]
84+
pub enum AssignmentError {
85+
InvalidAssignee,
86+
Other(anyhow::Error),
87+
}
88+
89+
// NOTE: This is used to post the Github comment; make sure it's valid markdown.
90+
impl fmt::Display for AssignmentError {
91+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
92+
match self {
93+
AssignmentError::InvalidAssignee => write!(f, "invalid assignee"),
94+
AssignmentError::Other(err) => write!(f, "{err}"),
95+
}
96+
}
97+
}
98+
99+
impl From<AssignmentError> for anyhow::Error {
100+
fn from(a: AssignmentError) -> Self {
101+
match a {
102+
AssignmentError::InvalidAssignee => UserError::InvalidAssignee.into(),
103+
AssignmentError::Other(err) => err.context("assignment error"),
104+
}
105+
}
106+
}
107+
108+
#[cfg(test)]
109+
mod tests {
110+
use super::*;
111+
112+
#[test]
113+
fn display_labels() {
114+
let x = UserError::UnknownLabels {
115+
labels: vec!["A-bootstrap".into(), "xxx".into()],
116+
};
117+
assert_eq!(x.to_string(), "Unknown labels: A-bootstrap, xxx");
118+
}
119+
}

src/gh_changes_since.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,7 @@ use axum::{
77
};
88
use hyper::StatusCode;
99

10-
use crate::{
11-
github,
12-
handlers::Context,
13-
utils::{AppError, is_repo_autorized},
14-
};
10+
use crate::{errors::AppError, github, handlers::Context, utils::is_repo_autorized};
1511

1612
/// Redirects to either `/gh-range-diff` (when the base changed) or to GitHub's compare
1713
/// page (when the base is the same).

src/gh_range_diff.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use unicode_segmentation::UnicodeSegmentation;
2323

2424
use crate::github::GithubCompare;
2525
use crate::utils::is_repo_autorized;
26-
use crate::{github, handlers::Context, utils::AppError};
26+
use crate::{errors::AppError, github, handlers::Context};
2727

2828
static MARKER_RE: LazyLock<Regex> =
2929
LazyLock::new(|| Regex::new(r"@@ -[\d]+,[\d]+ [+][\d]+,[\d]+ @@").unwrap());

src/gha_logs.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1+
use crate::errors::AppError;
12
use crate::github::{self, WorkflowRunJob};
23
use crate::handlers::Context;
34
use crate::interactions::REPORT_TO;
4-
use crate::utils::{AppError, is_repo_autorized};
5+
use crate::utils::is_repo_autorized;
56
use anyhow::Context as _;
67
use axum::extract::{Path, State};
78
use axum::http::HeaderValue;

src/github.rs

Lines changed: 7 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use crate::errors::{AssignmentError, UserError};
12
use crate::team_data::TeamClient;
23
use anyhow::Context;
34
use async_trait::async_trait;
@@ -546,30 +547,13 @@ where
546547
}
547548
}
548549

549-
#[derive(Debug)]
550-
pub enum AssignmentError {
551-
InvalidAssignee,
552-
Http(anyhow::Error),
553-
}
554-
555550
#[derive(Debug)]
556551
pub enum Selection<'a, T: ?Sized> {
557552
All,
558553
One(&'a T),
559554
Except(&'a T),
560555
}
561556

562-
impl fmt::Display for AssignmentError {
563-
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
564-
match self {
565-
AssignmentError::InvalidAssignee => write!(f, "invalid assignee"),
566-
AssignmentError::Http(e) => write!(f, "cannot assign: {e}"),
567-
}
568-
}
569-
}
570-
571-
impl std::error::Error for AssignmentError {}
572-
573557
#[derive(Debug, Clone, PartialEq, Eq)]
574558
pub struct IssueRepository {
575559
pub organization: String,
@@ -612,20 +596,6 @@ impl IssueRepository {
612596
}
613597
}
614598

615-
#[derive(Debug)]
616-
pub(crate) struct UnknownLabels {
617-
labels: Vec<String>,
618-
}
619-
620-
// NOTE: This is used to post the Github comment; make sure it's valid markdown.
621-
impl fmt::Display for UnknownLabels {
622-
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
623-
write!(f, "Unknown labels: {}", &self.labels.join(", "))
624-
}
625-
}
626-
627-
impl std::error::Error for UnknownLabels {}
628-
629599
impl Issue {
630600
pub fn to_zulip_github_reference(&self) -> ZulipGitHubReference {
631601
ZulipGitHubReference {
@@ -867,7 +837,7 @@ impl Issue {
867837
}
868838

869839
if !unknown_labels.is_empty() {
870-
return Err(UnknownLabels {
840+
return Err(UserError::UnknownLabels {
871841
labels: unknown_labels,
872842
}
873843
.into());
@@ -929,12 +899,14 @@ impl Issue {
929899
struct AssigneeReq<'a> {
930900
assignees: &'a [&'a str],
931901
}
902+
932903
client
933904
.send_req(client.delete(&url).json(&AssigneeReq {
934905
assignees: &assignees[..],
935906
}))
936907
.await
937-
.map_err(AssignmentError::Http)?;
908+
.map_err(AssignmentError::Other)?;
909+
938910
Ok(())
939911
}
940912

@@ -958,7 +930,8 @@ impl Issue {
958930
let result: Issue = client
959931
.json(client.post(&url).json(&AssigneeReq { assignees: &[user] }))
960932
.await
961-
.map_err(AssignmentError::Http)?;
933+
.map_err(AssignmentError::Other)?;
934+
962935
// Invalid assignees are silently ignored. We can just check if the user is now
963936
// contained in the assignees list.
964937
let success = result
@@ -3272,16 +3245,3 @@ impl Submodule {
32723245
client.repository(fullname).await
32733246
}
32743247
}
3275-
3276-
#[cfg(test)]
3277-
mod tests {
3278-
use super::*;
3279-
3280-
#[test]
3281-
fn display_labels() {
3282-
let x = UnknownLabels {
3283-
labels: vec!["A-bootstrap".into(), "xxx".into()],
3284-
};
3285-
assert_eq!(x.to_string(), "Unknown labels: A-bootstrap, xxx");
3286-
}
3287-
}

src/handlers.rs

Lines changed: 11 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,6 @@ use std::fmt;
1010
use std::sync::Arc;
1111
use tracing as log;
1212

13-
/// Creates a [`UserError`] with message.
14-
///
15-
/// Should be used when an handler is in error due to the user action's (not a PR,
16-
/// not a issue, not authorized, ...).
17-
///
18-
/// Should be used like this `return user_error!("My error message.");`.
19-
macro_rules! user_error {
20-
($err:expr $(,)?) => {
21-
anyhow::Result::Err(anyhow::anyhow!(crate::handlers::UserError($err.into())))
22-
};
23-
}
24-
2513
mod assign;
2614
mod autolabel;
2715
mod backport;
@@ -274,11 +262,15 @@ macro_rules! issue_handlers {
274262
if let Some(config) = &config.$name {
275263
$name::handle_input(ctx, config, event, input)
276264
.await
277-
.map_err(|e| {
278-
HandlerError::Other(e.context(format!(
279-
"error when processing {} handler",
280-
stringify!($name)
281-
)))
265+
.map_err(|mut e| {
266+
if let Some(err) = e.downcast_mut::<crate::errors::UserError>() {
267+
HandlerError::Message(err.to_string())
268+
} else {
269+
HandlerError::Other(e.context(format!(
270+
"error when processing {} handler",
271+
stringify!($name)
272+
)))
273+
}
282274
})
283275
} else {
284276
Err(HandlerError::Message(format!(
@@ -419,8 +411,8 @@ macro_rules! command_handlers {
419411
$name::handle_command(ctx, config, event, command)
420412
.await
421413
.unwrap_or_else(|mut err| {
422-
if let Some(err) = err.downcast_mut::<UserError>() {
423-
errors.push(HandlerError::Message(std::mem::take(&mut err.0)));
414+
if let Some(err) = err.downcast_mut::<crate::errors::UserError>() {
415+
errors.push(HandlerError::Message(err.to_string()));
424416
} else {
425417
errors.push(HandlerError::Message(format!(
426418
"`{}` handler unexpectedly failed in [this comment]({}): {err}",
@@ -490,17 +482,3 @@ impl fmt::Display for HandlerError {
490482
}
491483
}
492484
}
493-
494-
/// Represent a user error.
495-
///
496-
/// The message will be shown to the user via comment posted by this bot.
497-
#[derive(Debug)]
498-
pub struct UserError(String);
499-
500-
impl std::error::Error for UserError {}
501-
502-
impl fmt::Display for UserError {
503-
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
504-
f.write_str(&self.0)
505-
}
506-
}

src/handlers/assign.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
2323
use crate::db::issue_data::IssueData;
2424
use crate::db::review_prefs::{RotationMode, get_review_prefs_batch};
25+
use crate::errors::{self, AssignmentError, user_error};
2526
use crate::github::UserId;
2627
use crate::handlers::pr_tracking::ReviewerWorkqueue;
2728
use crate::{
@@ -554,8 +555,8 @@ pub(super) async fn handle_command(
554555
.add_labels(&ctx.github, vec![github::Label { name: t_label }])
555556
.await
556557
{
557-
if let Some(github::UnknownLabels { .. }) = err.downcast_ref() {
558-
log::warn!("Error assigning label: {err}");
558+
if let Some(errors::UserError::UnknownLabels { .. }) = err.downcast_ref() {
559+
log::warn!("error assigning team label: {err}");
559560
} else {
560561
return Err(err);
561562
}
@@ -655,11 +656,8 @@ pub(super) async fn handle_command(
655656
e.apply(&ctx.github, String::new()).await?;
656657
return Ok(());
657658
} // we are done
658-
Err(github::AssignmentError::InvalidAssignee) => {
659-
issue
660-
.set_assignee(&ctx.github, &ctx.username)
661-
.await
662-
.context("self-assignment failed")?;
659+
Err(AssignmentError::InvalidAssignee) => {
660+
issue.set_assignee(&ctx.github, &ctx.username).await?;
663661
let cmt_body = format!(
664662
"This issue has been assigned to @{to_assign} via [this comment]({}).",
665663
event.html_url().unwrap()

0 commit comments

Comments
 (0)