Refactor returning comments to the user

Ensure that we're actually intending to send a message and not just
blindly push error messages out to the user.

src/config.rs

@@ -1,6 +1,7 @@
 use crate::github::GithubClient;
 use failure::Error;
 use std::collections::HashMap;
+use std::fmt;
 use std::sync::{Arc, RwLock};
 use std::time::{Duration, Instant};
 
@@ -8,7 +9,8 @@ static CONFIG_FILE_NAME: &str = "triagebot.toml";
 const REFRESH_EVERY: Duration = Duration::from_secs(2 * 60); // Every two minutes
 
 lazy_static::lazy_static! {
-    static ref CONFIG_CACHE: RwLock<HashMap<String, (Arc<Config>, Instant)>> =
+    static ref CONFIG_CACHE:
+        RwLock<HashMap<String, (Result<Arc<Config>, ConfigurationError>, Instant)>> =
         RwLock::new(HashMap::new());
 }
 
@@ -31,17 +33,22 @@ pub(crate) struct RelabelConfig {
     pub(crate) allow_unauthenticated: Vec<String>,
 }
 
-pub(crate) async fn get(gh: &GithubClient, repo: &str) -> Result<Arc<Config>, Error> {
+pub(crate) async fn get(gh: &GithubClient, repo: &str) -> Result<Arc<Config>, ConfigurationError> {
     if let Some(config) = get_cached_config(repo) {
         log::trace!("returning config for {} from cache", repo);
-        Ok(config)
+        config
     } else {
         log::trace!("fetching fresh config for {}", repo);
-        get_fresh_config(gh, repo).await
+        let res = get_fresh_config(gh, repo).await;
+        CONFIG_CACHE
+            .write()
+            .unwrap()
+            .insert(repo.to_string(), (res.clone(), Instant::now()));
+        res
     }
 }
 
-fn get_cached_config(repo: &str) -> Option<Arc<Config>> {
+fn get_cached_config(repo: &str) -> Option<Result<Arc<Config>, ConfigurationError>> {
     let cache = CONFIG_CACHE.read().unwrap();
     cache.get(repo).and_then(|(config, fetch_time)| {
         if fetch_time.elapsed() < REFRESH_EVERY {
@@ -52,21 +59,43 @@ fn get_cached_config(repo: &str) -> Option<Arc<Config>> {
     })
 }
 
-async fn get_fresh_config(gh: &GithubClient, repo: &str) -> Result<Arc<Config>, Error> {
+async fn get_fresh_config(
+    gh: &GithubClient,
+    repo: &str,
+) -> Result<Arc<Config>, ConfigurationError> {
     let contents = gh
         .raw_file(repo, "master", CONFIG_FILE_NAME)
-        .await?
-        .ok_or_else(|| {
-            failure::err_msg(
-                "This repository is not enabled to use triagebot.\n\
-                 Add a `triagebot.toml` in the root of the master branch to enable it.",
-            )
-        })?;
-    let config = Arc::new(toml::from_slice::<Config>(&contents)?);
+        .await
+        .map_err(|e| ConfigurationError::Http(Arc::new(e)))?
+        .ok_or(ConfigurationError::Missing)?;
+    let config = Arc::new(toml::from_slice::<Config>(&contents).map_err(ConfigurationError::Toml)?);
     log::debug!("fresh configuration for {}: {:?}", repo, config);
-    CONFIG_CACHE
-        .write()
-        .unwrap()
-        .insert(repo.to_string(), (config.clone(), Instant::now()));
     Ok(config)
 }
+
+#[derive(Clone, Debug)]
+pub enum ConfigurationError {
+    Missing,
+    Toml(toml::de::Error),
+    Http(Arc<Error>),
+}
+
+impl std::error::Error for ConfigurationError {}
+
+impl fmt::Display for ConfigurationError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            ConfigurationError::Missing => write!(
+                f,
+                "This repository is not enabled to use triagebot.\n\
+                 Add a `triagebot.toml` in the root of the master branch to enable it."
+            ),
+            ConfigurationError::Toml(e) => {
+                write!(f, "Malformed `triagebot.toml` in master branch.\n{}", e)
+            }
+            ConfigurationError::Http(_) => {
+                write!(f, "Failed to query configuration for this repository.")
+            }
+        }
+    }
+}

src/handlers.rs

@@ -1,23 +1,55 @@
+use crate::config::{self, ConfigurationError};
 use crate::github::{Event, GithubClient};
 use failure::Error;
 use futures::future::BoxFuture;
+use std::fmt;
+
+#[derive(Debug)]
+pub enum HandlerError {
+    Message(String),
+    Other(Error),
+}
+
+impl std::error::Error for HandlerError {}
+
+impl fmt::Display for HandlerError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            HandlerError::Message(msg) => write!(f, "{}", msg),
+            HandlerError::Other(_) => write!(f, "An internal error occurred."),
+        }
+    }
+}
 
 macro_rules! handlers {
     ($($name:ident = $handler:expr,)*) => {
         $(mod $name;)*
 
-        pub async fn handle(ctx: &Context, event: &Event) -> Result<(), Error> {
-            let config = crate::config::get(&ctx.github, event.repo_name()).await?;
-            $(if let Some(input) = Handler::parse_input(&$handler, ctx, event)? {
+        pub async fn handle(ctx: &Context, event: &Event) -> Result<(), HandlerError> {
+            $(
+            if let Some(input) = Handler::parse_input(
+                    &$handler, ctx, event).map_err(HandlerError::Message)? {
+                let config = match config::get(&ctx.github, event.repo_name()).await {
+                    Ok(config) => config,
+                    Err(e @ ConfigurationError::Missing) => {
+                        return Err(HandlerError::Message(e.to_string()));
+                    }
+                    Err(e @ ConfigurationError::Toml(_)) => {
+                        return Err(HandlerError::Message(e.to_string()));
+                    }
+                    Err(e @ ConfigurationError::Http(_)) => {
+                        return Err(HandlerError::Other(e.into()));
+                    }
+                };
                 if let Some(config) = &config.$name {
-                    Handler::handle_input(&$handler, ctx, config, event, input).await?;
+                    Handler::handle_input(&$handler, ctx, config, event, input).await.map_err(HandlerError::Other)?;
                 } else {
-                    failure::bail!(
+                    return Err(HandlerError::Message(format!(
                         "The feature `{}` is not enabled in this repository.\n\
                          To enable it add its section in the `triagebot.toml` \
                          in the root of the repository.",
                         stringify!($name)
-                    );
+                    )));
                 }
             })*
             Ok(())
@@ -40,7 +72,7 @@ pub trait Handler: Sync + Send {
     type Input;
     type Config;
 
-    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, Error>;
+    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, String>;
 
     fn handle_input<'a>(
         &self,

src/handlers/assign.rs

@@ -33,7 +33,7 @@ impl Handler for AssignmentHandler {
     type Input = AssignCommand;
     type Config = AssignConfig;
 
-    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, Error> {
+    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, String> {
         let body = if let Some(b) = event.comment_body() {
             b
         } else {
@@ -54,11 +54,11 @@ impl Handler for AssignmentHandler {
         match input.parse_command() {
             Command::Assign(Ok(command)) => Ok(Some(command)),
             Command::Assign(Err(err)) => {
-                failure::bail!(
+                return Err(format!(
                     "Parsing assign command in [comment]({}) failed: {}",
                     event.html_url().expect("has html url"),
                     err
-                );
+                ));
             }
             _ => Ok(None),
         }

src/handlers/relabel.rs

@@ -25,7 +25,7 @@ impl Handler for RelabelHandler {
     type Input = RelabelCommand;
     type Config = RelabelConfig;
 
-    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, Error> {
+    fn parse_input(&self, ctx: &Context, event: &Event) -> Result<Option<Self::Input>, String> {
         let body = if let Some(b) = event.comment_body() {
             b
         } else {
@@ -45,11 +45,11 @@ impl Handler for RelabelHandler {
         match input.parse_command() {
             Command::Relabel(Ok(command)) => Ok(Some(command)),
             Command::Relabel(Err(err)) => {
-                failure::bail!(
+                return Err(format!(
                     "Parsing label command in [comment]({}) failed: {}",
                     event.html_url().expect("has html url"),
                     err
-                );
+                ));
             }
             _ => Ok(None),
         }

src/lib.rs

@@ -2,6 +2,7 @@
 #![allow(clippy::new_without_default)]
 
 use failure::{Error, ResultExt};
+use handlers::HandlerError;
 use interactions::ErrorComment;
 use std::fmt;
 
@@ -61,7 +62,7 @@ pub async fn webhook(
     payload: String,
     ctx: &handlers::Context,
 ) -> Result<(), WebhookError> {
-    match event {
+    let event = match event {
         EventName::IssueComment => {
             let payload = deserialize_payload::<github::IssueCommentEvent>(&payload)
                 .context("IssueCommentEvent failed to deserialize")
@@ -69,14 +70,7 @@ pub async fn webhook(
 
             log::info!("handling issue comment {:?}", payload);
 
-            let event = github::Event::IssueComment(payload);
-            if let Err(err) = handlers::handle(&ctx, &event).await {
-                if let Some(issue) = event.issue() {
-                    let cmnt = ErrorComment::new(issue, err.to_string());
-                    cmnt.post(&ctx.github).await?;
-                }
-                return Err(err.into());
-            }
+            github::Event::IssueComment(payload)
         }
         EventName::Issue => {
             let payload = deserialize_payload::<github::IssuesEvent>(&payload)
@@ -85,17 +79,28 @@ pub async fn webhook(
 
             log::info!("handling issue event {:?}", payload);
 
-            let event = github::Event::Issue(payload);
-            if let Err(err) = handlers::handle(&ctx, &event).await {
+            github::Event::Issue(payload)
+        }
+        // Other events need not be handled
+        EventName::Other => {
+            return Ok(());
+        }
+    };
+    if let Err(err) = handlers::handle(&ctx, &event).await {
+        match err {
+            HandlerError::Message(message) => {
                 if let Some(issue) = event.issue() {
-                    let cmnt = ErrorComment::new(issue, err.to_string());
+                    let cmnt = ErrorComment::new(issue, message);
                     cmnt.post(&ctx.github).await?;
                 }
-                return Err(err.into());
+            }
+            HandlerError::Other(err) => {
+                log::error!("handling event failed: {:?}", err);
+                return Err(WebhookError(failure::err_msg(
+                    "handling failed, error logged",
+                )));
             }
         }
-        // Other events need not be handled
-        EventName::Other => {}
     }
     Ok(())
 }