Implement better errors using thiserror

Cargo.toml

@@ -30,3 +30,4 @@ serial_test = "3.0.0"
 time = "0.3.34"
 once_cell = "1.19.0"
 dotenvy = "0.15.7"
+thiserror = "1.0"

src/bot/commands/subject.rs

@@ -5,7 +5,7 @@ use sqlx::{MySql, Pool};
 use tracing::info;
 
 use crate::{
-    bot::{checks, Context, Error},
+    bot::{checks, Context, Error, Result},
     mysql_lib::{self, DatabaseSubject},
 };
 
@@ -17,7 +17,7 @@ use crate::{
     subcommands("add", "remove", "show", "manage"),
     subcommand_required
 )]
-pub async fn subject(_ctx: Context<'_>) -> Result<(), Error> {
+pub async fn subject(_ctx: Context<'_>) -> Result<()> {
     unimplemented!()
 }
 
@@ -28,7 +28,7 @@ pub async fn add(
     #[description = "list of subject names or id's from \"subject show\""] names_or_ids: Vec<
         String,
     >,
-) -> Result<(), Error> {
+) -> Result<()> {
     if !checks::is_user_verified(ctx).await {
         ctx.reply("You need to be verified for this command").await?;
         return Ok(());
@@ -48,7 +48,7 @@ pub async fn add(
         user_roles,
         names_or_ids,
     )
-    .await;
+    .await?;
 
     let roles: Vec<_> = requested_subjects
         .into_iter()
@@ -88,7 +88,7 @@ pub async fn remove(
     #[description = "list of subject names or id's from \"subject show\""] names_or_ids: Vec<
         String,
     >,
-) -> Result<(), Error> {
+) -> Result<()> {
     if !checks::is_user_verified(ctx).await {
         ctx.reply("You need to be verified for this command").await?;
         return Ok(());
@@ -108,7 +108,7 @@ pub async fn remove(
         user_roles,
         names_or_ids,
     )
-    .await;
+    .await?;
 
     let roles: Vec<_> = requested_subjects
         .into_iter()
@@ -146,7 +146,7 @@ pub async fn remove(
 
 /// Show available subjects for a user
 #[poise::command(slash_command, prefix_command)]
-pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
+pub async fn show(ctx: Context<'_>) -> Result<()> {
     if !checks::is_user_verified(ctx).await {
         ctx.reply("You need to be verified for this command").await?;
         return Ok(());
@@ -156,7 +156,7 @@ pub async fn show(ctx: Context<'_>) -> Result<(), Error> {
     let guild = ctx.guild_id().unwrap();
     let user_roles = &ctx.author_member().await.unwrap().roles;
 
-    let available_subjects = get_available_subjects(db, guild, user_roles).await;
+    let available_subjects = get_available_subjects(db, guild, user_roles).await?;
 
     let formatted_subjects = format_subjects(&available_subjects);
 
@@ -185,42 +185,43 @@ async fn get_available_subjects(
     db: &Pool<MySql>,
     guild: GuildId,
     user_roles: &[RoleId],
-) -> Vec<DatabaseSubject> {
+) -> Result<Vec<DatabaseSubject>> {
     // objective: Get all semester study groups below or at the user's
     info!("Getting all available subjects for a user");
 
     // get all semester study groups for guild (they have an associated discord role)
     let guild_semester_study_groups = mysql_lib::get_semester_study_groups_in_guild(db, guild)
         .await
-        .unwrap();
+        .ok_or(Error::Database)?;
 
     info!(?guild_semester_study_groups, "All semester study groups in the guild");
 
     // find out which semester study group user is in using that role
     let user_semester_study_group = guild_semester_study_groups
         .iter()
         .find(|group| user_roles.contains(&group.role))
-        .expect("User is in none of the guild's semester study groups");
+        .ok_or(Error::UserIsMissingSemesterStudyGroup)?;
 
     // get all related semester study groups
     let related_semester_study_groups = mysql_lib::get_semester_study_groups_in_study_group(
         db,
         user_semester_study_group.study_group_id,
     )
     .await
-    .unwrap();
+    .ok_or(Error::Database)?;
 
     info!(?related_semester_study_groups, "All semester study groups for the users study group");
 
     // filter out to only those whose semester <= user's semester
-    let valid_semester_study_groups = related_semester_study_groups
+    let valid_semester_study_groups: Vec<_> = related_semester_study_groups
         .into_iter()
-        .filter(|sem_study_group| sem_study_group.semester <= user_semester_study_group.semester);
+        .filter(|sem_study_group| sem_study_group.semester <= user_semester_study_group.semester)
+        .collect();
 
     info!(?valid_semester_study_groups, "All semester study groups below or at the users semester");
 
     // get all subjects for all those semester study groups
-    let subjects: Vec<_> = valid_semester_study_groups
+    let subjects: Vec<_> = valid_semester_study_groups.into_iter()
         .map(|sem_study_group| {
             mysql_lib::get_subjects_for_semester_study_group(db, sem_study_group.role)
         })
@@ -239,47 +240,43 @@ async fn get_available_subjects(
 
     info!(?subjects, "All subjects that the user could potentially be in");
 
-    subjects
+    Ok(subjects)
 }
 
 /// Parses subject names/ids from user input, then
 /// gets the appropriate database objects for it.
 /// Needs the user's roles because those dictate which
 /// subjects are available to them.
-///
-/// TODO: Handle failures correctly
 async fn get_subjects_from_user_input(
     db: &Pool<MySql>,
     guild: GuildId,
     user_roles: &[RoleId],
     names_or_ids: Vec<String>,
-) -> Vec<DatabaseSubject> {
-    let available_subjects = get_available_subjects(db, guild, user_roles).await;
+) -> Result<Vec<DatabaseSubject>> {
+    let available_subjects = get_available_subjects(db, guild, user_roles).await?;
 
     names_or_ids
-        .into_iter()
-        .map(|name_or_id| {
-            if let Ok(subject_id) = name_or_id.parse::<i32>() {
-                // user specified a subject id
-                available_subjects.iter().find(|subject| subject.id == Some(subject_id)).unwrap().clone()
-            } else {
-                // user specified a name
-                let subject_name = name_or_id;
-
-                let matching_subject = available_subjects
-                    .iter()
-                    .find(|db_subject| db_subject.name == subject_name);
-
-                if let Some(subject) = matching_subject {
-                    subject.clone()
+            .into_iter()
+            .map(|name_or_id| {
+                if let Ok(subject_id) = name_or_id.parse::<i32>() {
+                    // user specified a subject id
+                    let subject = available_subjects.iter()
+                        .find(|subject| subject.id == Some(subject_id))
+                        .ok_or(Error::InvalidSubjectId(subject_id))?
+                        .clone();
+
+                    Ok(subject)
                 } else {
-                    todo!(
-                        "User has specified subject name which does not exist / they can't access"
-                    )
-                }
-            }
-        })
-        .collect()
+                    // user specified a name
+                    let subject_name = name_or_id;
+
+                    available_subjects
+                        .iter()
+                        .find(|db_subject| db_subject.name == subject_name)
+                        .cloned()
+                        .ok_or(Error::InvalidSubjectName(subject_name))                }
+            })
+            .collect()
 }
 
 /// Admin commands for creating/deleting subjects
@@ -289,13 +286,13 @@ async fn get_subjects_from_user_input(
     subcommand_required,
     required_permissions = "ADMINISTRATOR"
 )]
-pub async fn manage(_ctx: Context<'_>) -> Result<(), Error> {
+pub async fn manage(_ctx: Context<'_>) -> Result<()> {
     unimplemented!()
 }
 
 /// Creates a new subject, together with role and text channel
 #[poise::command(prefix_command, required_permissions = "ADMINISTRATOR")]
-pub async fn create(ctx: Context<'_>, name: String) -> Result<(), Error> {
+pub async fn create(ctx: Context<'_>, name: String) -> Result<()> {
     let guild_id = ctx.guild_id().unwrap();
     let db = &ctx.data().database_pool;
     let discord_http = ctx.http();
@@ -358,7 +355,7 @@ pub async fn create(ctx: Context<'_>, name: String) -> Result<(), Error> {
 
 /// Deletes the subject, it's role, and it's text channel
 #[poise::command(prefix_command, required_permissions = "ADMINISTRATOR")]
-pub async fn delete(ctx: Context<'_>, role: RoleId) -> Result<(), Error> {
+pub async fn delete(ctx: Context<'_>, role: RoleId) -> Result<()> {
     let guild_id = ctx.guild_id().unwrap();
     let db = &ctx.data().database_pool;
 

src/bot/error.rs

@@ -0,0 +1,28 @@
+use poise::serenity_prelude as serenity;
+use thiserror::Error;
+
+pub type Result<T> = std::result::Result<T, Error>;
+
+#[derive(Error, Debug)]
+pub enum Error {
+    /// TODO: Actually put a meaningful error in here.
+    /// needs integration with the mysql_lib module.
+    #[error("Error with the database, check logs")]
+    Database,
+    #[error("Serenity error")]
+    Serenity(#[from] serenity::Error),
+    #[error("You are not part of a semester study group")]
+    UserIsMissingSemesterStudyGroup,
+    #[error("Invalid subject id: {0}")]
+    InvalidSubjectId(i32),
+    #[error("Invalid subject name: {0}")]
+    InvalidSubjectName(String),
+}
+
+impl Error {
+    /// This can match on specific error types, so that they can be formatted using
+    /// discord-specific formatting options.
+    pub fn create_reply(&self) -> poise::CreateReply {
+        poise::CreateReply::default().content(self.to_string()).reply(true)
+    }
+}

src/bot/mod.rs

@@ -2,17 +2,20 @@ use std::collections::HashMap;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
-use poise::serenity_prelude as serenity;
 use poise::serenity_prelude::GuildId;
+use poise::{serenity_prelude as serenity, FrameworkError};
 use redis::Client;
 use sqlx::{MySql, Pool};
 use tracing::info;
 
-use crate::{env, logging};
 use crate::mysql_lib::NewGuild;
+use crate::{env, logging};
 
 mod checks;
 mod commands;
+mod error;
+
+pub use error::{Error, Result};
 
 /// User data, which is stored and accessible in all command invocations
 #[derive(Debug)]
@@ -24,7 +27,6 @@ pub struct Data {
     setup_in_progress: Mutex<HashMap<GuildId, NewGuild>>,
 }
 
-pub type Error = Box<dyn std::error::Error + Send + Sync>;
 pub type Context<'a> = poise::Context<'a, Data, Error>;
 pub type Framework = poise::Framework<Data, Error>;
 
@@ -35,6 +37,7 @@ pub async fn entrypoint(database_pool: Pool<MySql>, redis_client: Client) {
     let db_clone = database_pool.clone();
     let framework = Framework::builder()
         .options(poise::FrameworkOptions {
+            on_error: |err| Box::pin(handle_error(err)),
             commands: vec![
                 commands::ping(),
                 commands::logger_pipe(),
@@ -97,3 +100,42 @@ pub async fn entrypoint(database_pool: Pool<MySql>, redis_client: Client) {
 
     client.start().await.expect("Err running poise client");
 }
+
+async fn handle_error(framework_error: FrameworkError<'_, Data, Error>) {
+    use FrameworkError::*;
+    match framework_error {
+        Command { error, ctx, .. } => {
+            let command = ctx.invocation_string();
+            let username = &ctx.author().name;
+            let discriminator = ctx.author().discriminator;
+            let user_id = ctx.author().id.get();
+            tracing::warn!(
+                ?error,
+                command_string = command,
+                username,
+                discriminator,
+                user_id,
+                "Error occured in a poise command handler"
+            );
+            let _ = ctx.send(error.create_reply()).await;
+        }
+        CommandPanic { payload, ctx, .. } => {
+            let command = ctx.invocation_string();
+            let username = &ctx.author().name;
+            let discriminator = ctx.author().discriminator;
+            let user_id = ctx.author().id.get();
+            tracing::error!(
+                payload,
+                command_string = command,
+                username,
+                discriminator,
+                user_id,
+                "Command handler caused a panic"
+            );
+            let _ = ctx.reply(
+                "An internal error occured while executing this command. Try again later, or contact the administrators."
+            ).await;
+        }
+        _ => {}
+    }
+}