From 7e69f872a72962bd516729b5387445343cce95cc Mon Sep 17 00:00:00 2001 From: David Rousselie Date: Mon, 14 Feb 2022 23:42:20 +0100 Subject: [PATCH] Refactor error handling --- .pre-commit-config.yaml | 7 ++ Cargo.lock | 28 ++++++++ Cargo.toml | 2 + src/contextswitch/api.rs | 44 +++++++++++-- src/contextswitch/taskwarrior.rs | 109 +++++++++++++++++++++---------- src/routes/tasks.rs | 29 ++++++-- 6 files changed, 173 insertions(+), 46 deletions(-) create mode 100644 .pre-commit-config.yaml diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..34d0d19 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,7 @@ +repos: + - repo: https://github.com/doublify/pre-commit-rust + rev: v1.0 + hooks: + - id: fmt + - id: cargo-check + - id: clippy diff --git a/Cargo.lock b/Cargo.lock index 44f4cef..db3b3a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -216,6 +216,12 @@ dependencies = [ "winapi", ] +[[package]] +name = "anyhow" +version = "1.0.53" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94a45b455c14666b85fc40a019e8ab9eb75e3a124e05494f5397122bc9eb06e0" + [[package]] name = "autocfg" version = "1.0.1" @@ -325,6 +331,7 @@ version = "0.1.0" dependencies = [ "actix-http", "actix-web", + "anyhow", "chrono", "configparser", "contextswitch-types", @@ -337,6 +344,7 @@ dependencies = [ "rstest", "serde", "serde_json", + "thiserror", "tokio", "tracing", "tracing-actix-web", @@ -1406,6 +1414,26 @@ dependencies = [ "winapi", ] +[[package]] +name = "thiserror" +version = "1.0.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "854babe52e4df1653706b98fcfc05843010039b406875930a70e4d9644e5c417" +dependencies = [ + "thiserror-impl", +] + +[[package]] +name = "thiserror-impl" +version = "1.0.30" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa32fd3f627f367fe16f893e2597ae3c05020f8bba2666a4e6ea73d377e5714b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "thread_local" version = "1.1.3" diff --git a/Cargo.toml b/Cargo.toml index 01fe6be..5e6f574 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,8 @@ tracing-actix-web = "=0.5.0-beta.9" regex = "1.5.0" lazy_static = "1.4.0" tracing-bunyan-formatter = "0.3.2" +thiserror = "1.0.30" +anyhow = "1.0.53" [dev-dependencies] reqwest = { version = "0.11.0", features = ["json"] } diff --git a/src/contextswitch/api.rs b/src/contextswitch/api.rs index 267f0fe..c80b6c5 100644 --- a/src/contextswitch/api.rs +++ b/src/contextswitch/api.rs @@ -1,10 +1,42 @@ use crate::contextswitch::taskwarrior; use contextswitch_types::Task; -use std::io::Error; +use serde_json; + +fn error_chain_fmt( + e: &impl std::error::Error, + f: &mut std::fmt::Formatter<'_>, +) -> std::fmt::Result { + writeln!(f, "{}\n", e)?; + let mut current = e.source(); + while let Some(cause) = current { + writeln!(f, "Caused by:\n\t{}", cause)?; + current = cause.source(); + } + Ok(()) +} + +impl std::fmt::Debug for ContextswitchError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + error_chain_fmt(self, f) + } +} + +#[derive(thiserror::Error)] +pub enum ContextswitchError { + #[error("Invalid Contextswitch metadata: {metadata}")] + InvalidMetadataError { + #[source] + source: serde_json::Error, + metadata: String, + }, + #[error(transparent)] + UnexpectedError(#[from] anyhow::Error), +} #[tracing::instrument(level = "debug")] -pub fn list_tasks(filters: Vec<&str>) -> Result, Error> { - let tasks: Result, Error> = taskwarrior::list_tasks(filters)? +pub fn list_tasks(filters: Vec<&str>) -> Result, ContextswitchError> { + let tasks: Result, ContextswitchError> = taskwarrior::list_tasks(filters) + .map_err(|e| ContextswitchError::UnexpectedError(e.into()))? .iter() .map(Task::try_from) .collect(); @@ -12,7 +44,9 @@ pub fn list_tasks(filters: Vec<&str>) -> Result, Error> { } #[tracing::instrument(level = "debug")] -pub async fn add_task(add_args: Vec<&str>) -> Result { - let taskwarrior_task = taskwarrior::add_task(add_args).await?; +pub async fn add_task(add_args: Vec<&str>) -> Result { + let taskwarrior_task = taskwarrior::add_task(add_args) + .await + .map_err(|e| ContextswitchError::UnexpectedError(e.into()))?; (&taskwarrior_task).try_into() } diff --git a/src/contextswitch/taskwarrior.rs b/src/contextswitch/taskwarrior.rs index 4678610..08172ce 100644 --- a/src/contextswitch/taskwarrior.rs +++ b/src/contextswitch/taskwarrior.rs @@ -1,3 +1,5 @@ +use crate::contextswitch::ContextswitchError; +use anyhow::{anyhow, Context}; use chrono::{DateTime, Utc}; use configparser::ini::Ini; use contextswitch_types::{ContextSwitchMetadata, Task, TaskId}; @@ -6,7 +8,6 @@ use serde::{Deserialize, Serialize}; use serde_json; use std::env; use std::fmt; -use std::io::{Error, ErrorKind}; use std::path::Path; use std::process::Command; use std::str; @@ -60,12 +61,26 @@ pub struct TaskwarriorTask { skip_serializing_if = "Option::is_none", with = "contextswitch_types::opt_tw_date_format" )] + pub start: Option>, + #[serde( + default, + skip_serializing_if = "Option::is_none", + with = "contextswitch_types::opt_tw_date_format" + )] pub end: Option>, + #[serde( + default, + skip_serializing_if = "Option::is_none", + with = "contextswitch_types::opt_tw_date_format" + )] + pub wait: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] pub parent: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub project: Option, #[serde(default, skip_serializing_if = "Option::is_none")] + pub priority: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] pub recur: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub tags: Option>, @@ -74,16 +89,21 @@ pub struct TaskwarriorTask { } impl TryFrom<&TaskwarriorTask> for Task { - type Error = std::io::Error; + type Error = ContextswitchError; fn try_from(task: &TaskwarriorTask) -> Result { let cs_metadata = task.contextswitch.as_ref().map_or( Ok(None), - |cs_string| -> Result, serde_json::Error> { + |cs_string| -> Result, ContextswitchError> { if cs_string.is_empty() || cs_string == "{}" { Ok(None) } else { - Some(serde_json::from_str(cs_string)).transpose() + Some(serde_json::from_str(cs_string)) + .transpose() + .map_err(|e| ContextswitchError::InvalidMetadataError { + source: e, + metadata: cs_string.to_string(), + }) } }, )?; @@ -96,9 +116,12 @@ impl TryFrom<&TaskwarriorTask> for Task { description: task.description.clone(), urgency: task.urgency, due: task.due, + start: task.start, end: task.end, + wait: task.wait, parent: task.parent, project: task.project.clone(), + priority: task.priority, recur: task.recur, tags: task.tags.clone(), contextswitch: cs_metadata, @@ -106,6 +129,20 @@ impl TryFrom<&TaskwarriorTask> for Task { } } +#[derive(thiserror::Error, Debug)] +pub enum TaskwarriorError { + #[error("Error while executing Taskwarrior")] + ExecutionError(#[from] std::io::Error), + #[error("Error while parsing Taskwarrior output")] + OutputParsingError { + #[source] + source: serde_json::Error, + output: String, + }, + #[error(transparent)] + UnexpectedError(#[from] anyhow::Error), +} + fn write_default_config(data_location: &str) -> String { let mut taskrc = Ini::new(); taskrc.setstr("default", "data.location", Some(data_location)); @@ -159,33 +196,39 @@ pub fn load_config(task_data_location: Option<&str>) -> String { } #[tracing::instrument(level = "debug")] -pub fn list_tasks(filters: Vec<&str>) -> Result, Error> { +pub fn list_tasks(filters: Vec<&str>) -> Result, TaskwarriorError> { let args = [filters, vec!["export"]].concat(); - let export_output = Command::new("task").args(args).output()?; + let export_output = Command::new("task") + .args(args) + .output() + .map_err(TaskwarriorError::ExecutionError)?; - let tasks: Vec = serde_json::from_slice(&export_output.stdout)?; + let output = + String::from_utf8(export_output.stdout).context("Failed to read Taskwarrior output")?; + + let tasks: Vec = serde_json::from_str(&output) + .map_err(|e| TaskwarriorError::OutputParsingError { source: e, output })?; Ok(tasks) } #[tracing::instrument(level = "debug")] -pub fn get_task_by_local_id(id: &TaskwarriorTaskLocalId) -> Result, Error> { +pub fn get_task_by_local_id( + id: &TaskwarriorTaskLocalId, +) -> Result, TaskwarriorError> { let mut tasks: Vec = list_tasks(vec![&id.to_string()])?; if tasks.len() > 1 { - return Err(Error::new( - ErrorKind::Other, - format!( - "Found more than 1 task when searching for task with local ID {}", - id - ), - )); + return Err(TaskwarriorError::UnexpectedError(anyhow!( + "Found more than 1 task when searching for task with local ID {}", + id + ))); } Ok(tasks.pop()) } #[tracing::instrument(level = "debug")] -pub async fn add_task(add_args: Vec<&str>) -> Result { +pub async fn add_task(add_args: Vec<&str>) -> Result { lazy_static! { static ref RE: Regex = Regex::new(r"Created task (?P\d+).").unwrap(); static ref LOCK: Mutex = Mutex::new(0); @@ -194,35 +237,31 @@ pub async fn add_task(add_args: Vec<&str>) -> Result { let mut args = vec!["add"]; args.extend(add_args); - let add_output = Command::new("task").args(args).output()?; - let output = String::from_utf8(add_output.stdout).unwrap(); - let task_id_capture = RE.captures(&output).ok_or_else(|| { - Error::new( - ErrorKind::Other, - format!("Cannot extract task ID from: {}", &output), - ) - })?; + let add_output = Command::new("task") + .args(args) + .output() + .map_err(TaskwarriorError::ExecutionError)?; + let output = + String::from_utf8(add_output.stdout).context("Failed to read Taskwarrior output")?; + let task_id_capture = RE + .captures(&output) + .ok_or_else(|| anyhow!("Cannot extract task ID from: {}", &output))?; let task_id_str = task_id_capture .name("id") - .ok_or_else(|| { - Error::new( - ErrorKind::Other, - format!("Cannot extract task ID value from: {}", &output), - ) - })? + .ok_or_else(|| anyhow!("Cannot extract task ID value from: {}", &output))? .as_str(); let task_id = TaskwarriorTaskLocalId( task_id_str .parse::() - .map_err(|_| Error::new(ErrorKind::Other, "Cannot parse task ID value"))?, + .context("Cannot parse task ID value")?, ); let task = get_task_by_local_id(&task_id)?; task.ok_or_else(|| { - Error::new( - ErrorKind::Other, - format!("Newly created task with ID {} was not found", task_id), - ) + TaskwarriorError::UnexpectedError(anyhow!( + "Newly created task with ID {} was not found", + task_id + )) }) } diff --git a/src/routes/tasks.rs b/src/routes/tasks.rs index 25216c0..d2da379 100644 --- a/src/routes/tasks.rs +++ b/src/routes/tasks.rs @@ -1,16 +1,31 @@ use crate::contextswitch; -use actix_web::{web, HttpResponse}; +use actix_web::{http::StatusCode, web, HttpResponse, ResponseError}; +use anyhow::Context; use contextswitch_types::{NewTask, Task}; use serde::Deserialize; -use std::io::Error; #[derive(Deserialize)] pub struct TaskQuery { filter: Option, } +impl ResponseError for contextswitch::ContextswitchError { + fn status_code(&self) -> StatusCode { + match self { + contextswitch::ContextswitchError::InvalidMetadataError { .. } => { + StatusCode::INTERNAL_SERVER_ERROR + } + contextswitch::ContextswitchError::UnexpectedError(_) => { + StatusCode::INTERNAL_SERVER_ERROR + } + } + } +} + #[tracing::instrument(level = "debug", skip_all, fields(filter = %task_query.filter.as_ref().unwrap_or(&"".to_string())))] -pub async fn list_tasks(task_query: web::Query) -> Result { +pub async fn list_tasks( + task_query: web::Query, +) -> Result { let filter = task_query .filter .as_ref() @@ -19,16 +34,18 @@ pub async fn list_tasks(task_query: web::Query) -> Result) -> Result { +pub async fn add_task( + task: web::Json, +) -> Result { let task: Task = contextswitch::add_task(task.definition.split(' ').collect()).await?; Ok(HttpResponse::Ok() .content_type("application/json") - .body(serde_json::to_string(&task)?)) + .body(serde_json::to_string(&task).context("Cannot serialize Contextswitch task")?)) } #[tracing::instrument(level = "debug")]