From f609afc5eda78af6f2047b91688c46cbc97f9c3b Mon Sep 17 00:00:00 2001 From: Antoine Gersant Date: Mon, 21 Nov 2022 15:33:50 -0800 Subject: [PATCH] Structured errors continued --- src/app/config.rs | 16 ++++------ src/app/ddns.rs | 38 ++++++++++++++++-------- src/app/index/query.rs | 16 +++++----- src/app/index/update/inserter.rs | 13 ++++---- src/app/playlist.rs | 6 +++- src/app/settings.rs | 4 ++- src/app/user.rs | 22 +++++++------- src/app/vfs.rs | 37 +++++++++++++++++------ src/db.rs | 51 +++++++++++++++++++++----------- src/service/actix/api.rs | 5 +--- src/service/error.rs | 47 +++++++++++++++++++++++++++-- 11 files changed, 170 insertions(+), 85 deletions(-) diff --git a/src/app/config.rs b/src/app/config.rs index ba03657..f50a52d 100644 --- a/src/app/config.rs +++ b/src/app/config.rs @@ -6,18 +6,14 @@ use crate::app::{ddns, settings, user, vfs}; #[derive(thiserror::Error, Debug)] pub enum Error { + #[error(transparent)] + Ddns(#[from] ddns::Error), #[error(transparent)] Settings(#[from] settings::Error), #[error(transparent)] User(#[from] user::Error), - #[error("Unspecified")] - Unspecified, -} - -impl From for Error { - fn from(_: anyhow::Error) -> Self { - Error::Unspecified - } + #[error(transparent)] + Vfs(#[from] vfs::Error), } #[derive(Default, Deserialize)] @@ -82,9 +78,7 @@ impl Manager { .iter() .filter(|old_user| !users.iter().any(|u| u.name == old_user.name)) { - self.user_manager - .delete(&old_user.name) - .map_err(|_| Error::Unspecified)?; + self.user_manager.delete(&old_user.name)?; } // Insert new users diff --git a/src/app/ddns.rs b/src/app/ddns.rs index dc6d3d5..32f5e46 100644 --- a/src/app/ddns.rs +++ b/src/app/ddns.rs @@ -1,14 +1,31 @@ -use anyhow::bail; use diesel::prelude::*; use log::{error, info}; use serde::{Deserialize, Serialize}; use std::thread; use std::time; -use crate::db::{ddns_config, DB}; +use crate::db::{self, ddns_config, DB}; const DDNS_UPDATE_URL: &str = "https://ydns.io/api/v1/update/"; +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("DDNS update query failed with HTTP status code `{0}`")] + UpdateQueryFailed(u16), + #[error(transparent)] + DatabaseConnection(#[from] db::Error), + #[error(transparent)] + Database(#[from] diesel::result::Error), + #[error("Unspecified")] + Unspecified, +} + +impl From for Error { + fn from(_: anyhow::Error) -> Self { + Error::Unspecified + } +} + #[derive(Clone, Debug, Deserialize, Insertable, PartialEq, Eq, Queryable, Serialize)] #[diesel(table_name = ddns_config)] pub struct Config { @@ -27,7 +44,7 @@ impl Manager { Self { db } } - fn update_my_ip(&self) -> anyhow::Result<()> { + fn update_my_ip(&self) -> Result<(), Error> { let config = self.config()?; if config.host.is_empty() || config.username.is_empty() { info!("Skipping DDNS update because credentials are missing"); @@ -39,17 +56,14 @@ impl Manager { .auth(&config.username, &config.password) .call(); - if !response.ok() { - bail!( - "DDNS update query failed with status code: {}", - response.status() - ); + if response.ok() { + Ok(()) + } else { + Err(Error::UpdateQueryFailed(response.status())) } - - Ok(()) } - pub fn config(&self) -> anyhow::Result { + pub fn config(&self) -> Result { use crate::db::ddns_config::dsl::*; let mut connection = self.db.connect()?; Ok(ddns_config @@ -57,7 +71,7 @@ impl Manager { .get_result(&mut connection)?) } - pub fn set_config(&self, new_config: &Config) -> anyhow::Result<()> { + pub fn set_config(&self, new_config: &Config) -> Result<(), Error> { use crate::db::ddns_config::dsl::*; let mut connection = self.db.connect()?; diesel::update(ddns_config) diff --git a/src/app/index/query.rs b/src/app/index/query.rs index 4a7011c..85e1186 100644 --- a/src/app/index/query.rs +++ b/src/app/index/query.rs @@ -5,12 +5,14 @@ use diesel::sql_types; use std::path::Path; use super::*; -use crate::db::{directories, songs}; +use crate::db::{self, directories, songs}; #[derive(thiserror::Error, Debug)] pub enum QueryError { - #[error("VFS path not found")] - VFSPathNotFound, + #[error(transparent)] + DatabaseConnection(#[from] db::Error), + #[error(transparent)] + Vfs(#[from] vfs::Error), #[error("Unspecified")] Unspecified, } @@ -47,9 +49,7 @@ impl Index { output.extend(virtual_directories.map(CollectionFile::Directory)); } else { // Browse sub-directory - let real_path = vfs - .virtual_to_real(virtual_path) - .map_err(|_| QueryError::VFSPathNotFound)?; + let real_path = vfs.virtual_to_real(virtual_path)?; let real_path_string = real_path.as_path().to_string_lossy().into_owned(); let real_directories: Vec = directories::table @@ -83,9 +83,7 @@ impl Index { let mut connection = self.db.connect()?; let real_songs: Vec = if virtual_path.as_ref().parent().is_some() { - let real_path = vfs - .virtual_to_real(virtual_path) - .map_err(|_| QueryError::VFSPathNotFound)?; + let real_path = vfs.virtual_to_real(virtual_path)?; let song_path_filter = { let mut path_buf = real_path; path_buf.push("%"); diff --git a/src/app/index/update/inserter.rs b/src/app/index/update/inserter.rs index 6fffd15..4c73e44 100644 --- a/src/app/index/update/inserter.rs +++ b/src/app/index/update/inserter.rs @@ -1,4 +1,3 @@ -use anyhow::Error; use crossbeam_channel::Receiver; use diesel::prelude::*; use log::error; @@ -87,26 +86,26 @@ impl Inserter { } fn flush_directories(&mut self) { - let res = self.db.connect().and_then(|mut connection| { + let res = self.db.connect().ok().and_then(|mut connection| { diesel::insert_into(directories::table) .values(&self.new_directories) .execute(&mut *connection) // TODO https://github.com/diesel-rs/diesel/issues/1822 - .map_err(Error::new) + .ok() }); - if res.is_err() { + if res.is_none() { error!("Could not insert new directories in database"); } self.new_directories.clear(); } fn flush_songs(&mut self) { - let res = self.db.connect().and_then(|mut connection| { + let res = self.db.connect().ok().and_then(|mut connection| { diesel::insert_into(songs::table) .values(&self.new_songs) .execute(&mut *connection) // TODO https://github.com/diesel-rs/diesel/issues/1822 - .map_err(Error::new) + .ok() }); - if res.is_err() { + if res.is_none() { error!("Could not insert new songs in database"); } self.new_songs.clear(); diff --git a/src/app/playlist.rs b/src/app/playlist.rs index 0a4b1a1..b3cba61 100644 --- a/src/app/playlist.rs +++ b/src/app/playlist.rs @@ -7,14 +7,18 @@ use std::path::Path; use crate::app::index::Song; use crate::app::vfs; -use crate::db::{playlist_songs, playlists, users, DB}; +use crate::db::{self, playlist_songs, playlists, users, DB}; #[derive(thiserror::Error, Debug)] pub enum Error { + #[error(transparent)] + DatabaseConnection(#[from] db::Error), #[error("User not found")] UserNotFound, #[error("Playlist not found")] PlaylistNotFound, + #[error(transparent)] + Vfs(#[from] vfs::Error), #[error("Unspecified")] Unspecified, } diff --git a/src/app/settings.rs b/src/app/settings.rs index 6e54e13..cef3c9f 100644 --- a/src/app/settings.rs +++ b/src/app/settings.rs @@ -4,12 +4,14 @@ use serde::Deserialize; use std::convert::TryInto; use std::time::Duration; -use crate::db::{misc_settings, DB}; +use crate::db::{self, misc_settings, DB}; #[derive(thiserror::Error, Debug)] pub enum Error { #[error("Missing auth secret")] AuthSecretNotFound, + #[error(transparent)] + DatabaseConnection(#[from] db::Error), #[error("Auth secret does not have the expected format")] InvalidAuthSecret, #[error("Missing settings")] diff --git a/src/app/user.rs b/src/app/user.rs index 063b795..00ee929 100644 --- a/src/app/user.rs +++ b/src/app/user.rs @@ -7,10 +7,12 @@ use serde::{Deserialize, Serialize}; use std::time::{SystemTime, UNIX_EPOCH}; use crate::app::settings::AuthSecret; -use crate::db::{users, DB}; +use crate::db::{self, users, DB}; -#[derive(thiserror::Error, Debug, PartialEq, Eq)] +#[derive(thiserror::Error, Debug)] pub enum Error { + #[error(transparent)] + DatabaseConnection(#[from] db::Error), #[error("Cannot use empty username")] EmptyUsername, #[error("Cannot use empty password")] @@ -387,10 +389,10 @@ mod test { password: TEST_PASSWORD.to_owned(), admin: false, }; - assert_eq!( + assert!(matches!( ctx.user_manager.create(&new_user).unwrap_err(), Error::EmptyUsername - ); + )); } #[test] @@ -401,10 +403,10 @@ mod test { password: "".to_owned(), admin: false, }; - assert_eq!( + assert!(matches!( ctx.user_manager.create(&new_user).unwrap_err(), Error::EmptyPassword - ); + )); } #[test] @@ -455,12 +457,12 @@ mod test { }; ctx.user_manager.create(&new_user).unwrap(); - assert_eq!( + assert!(matches!( ctx.user_manager .login(TEST_USERNAME, "not the password") .unwrap_err(), Error::IncorrectPassword - ) + )); } #[test] @@ -539,9 +541,9 @@ mod test { let authorization = ctx .user_manager .authenticate(&token, AuthorizationScope::PolarisAuth); - assert_eq!( + assert!(matches!( authorization.unwrap_err(), Error::IncorrectAuthorizationScope - ) + )); } } diff --git a/src/app/vfs.rs b/src/app/vfs.rs index cd69d79..1a88206 100644 --- a/src/app/vfs.rs +++ b/src/app/vfs.rs @@ -1,11 +1,30 @@ -use anyhow::{bail, Result}; use core::ops::Deref; use diesel::prelude::*; use regex::Regex; use serde::{Deserialize, Serialize}; use std::path::{self, Path, PathBuf}; -use crate::db::{mount_points, DB}; +use crate::db::{self, mount_points, DB}; + +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("The following real path could not be mapped to a virtual path: `{0}`")] + CouldNotMapToVirtualPath(PathBuf), + #[error("The following virtual path could not be mapped to a real path: `{0}`")] + CouldNotMapToRealPath(PathBuf), + #[error(transparent)] + DatabaseConnection(#[from] db::Error), + #[error(transparent)] + Database(#[from] diesel::result::Error), + #[error("Unspecified")] + Unspecified, +} + +impl From for Error { + fn from(_: anyhow::Error) -> Self { + Error::Unspecified + } +} #[derive(Clone, Debug, Deserialize, Insertable, PartialEq, Eq, Queryable, Serialize)] #[diesel(table_name = mount_points)] @@ -44,7 +63,7 @@ impl VFS { VFS { mounts } } - pub fn real_to_virtual>(&self, real_path: P) -> Result { + pub fn real_to_virtual>(&self, real_path: P) -> Result { for mount in &self.mounts { if let Ok(p) = real_path.as_ref().strip_prefix(&mount.source) { let mount_path = Path::new(&mount.name); @@ -55,10 +74,10 @@ impl VFS { }; } } - bail!("Real path has no match in VFS") + Err(Error::CouldNotMapToVirtualPath(real_path.as_ref().into())) } - pub fn virtual_to_real>(&self, virtual_path: P) -> Result { + pub fn virtual_to_real>(&self, virtual_path: P) -> Result { for mount in &self.mounts { let mount_path = Path::new(&mount.name); if let Ok(p) = virtual_path.as_ref().strip_prefix(mount_path) { @@ -69,7 +88,7 @@ impl VFS { }; } } - bail!("Virtual path has no match in VFS") + Err(Error::CouldNotMapToRealPath(virtual_path.as_ref().into())) } pub fn mounts(&self) -> &Vec { @@ -87,13 +106,13 @@ impl Manager { Self { db } } - pub fn get_vfs(&self) -> Result { + pub fn get_vfs(&self) -> Result { let mount_dirs = self.mount_dirs()?; let mounts = mount_dirs.into_iter().map(|p| p.into()).collect(); Ok(VFS::new(mounts)) } - pub fn mount_dirs(&self) -> Result> { + pub fn mount_dirs(&self) -> Result, Error> { use self::mount_points::dsl::*; let mut connection = self.db.connect()?; let mount_dirs: Vec = mount_points @@ -102,7 +121,7 @@ impl Manager { Ok(mount_dirs) } - pub fn set_mount_dirs(&self, mount_dirs: &[MountDir]) -> Result<()> { + pub fn set_mount_dirs(&self, mount_dirs: &[MountDir]) -> Result<(), Error> { let mut connection = self.db.connect()?; connection .transaction::<_, diesel::result::Error, _>(|connection| { diff --git a/src/db.rs b/src/db.rs index c1c046e..4f9fb36 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,10 +1,9 @@ -use anyhow::{bail, Error, Result}; use diesel::r2d2::{self, ConnectionManager, PooledConnection}; use diesel::sqlite::SqliteConnection; use diesel::RunQueryDsl; use diesel_migrations::EmbeddedMigrations; use diesel_migrations::MigrationHarness; -use std::path::Path; +use std::path::{Path, PathBuf}; mod schema; @@ -12,6 +11,18 @@ pub use self::schema::*; const MIGRATIONS: EmbeddedMigrations = embed_migrations!("migrations"); +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error("Could not initialize database connection pool")] + ConnectionPoolBuild, + #[error("Could not acquire database connection from pool")] + ConnectionPool, + #[error("Filesystem error for `{0}`: `{1}`")] + Io(PathBuf, std::io::Error), + #[error("Could not apply database migrations")] + Migration, +} + #[derive(Clone)] pub struct DB { pool: r2d2::Pool>, @@ -39,34 +50,38 @@ impl diesel::r2d2::CustomizeConnection } impl DB { - pub fn new(path: &Path) -> Result { - std::fs::create_dir_all(path.parent().unwrap())?; + pub fn new(path: &Path) -> Result { + let directory = path.parent().unwrap(); + std::fs::create_dir_all(directory).map_err(|e| Error::Io(directory.to_owned(), e))?; let manager = ConnectionManager::::new(path.to_string_lossy()); let pool = diesel::r2d2::Pool::builder() .connection_customizer(Box::new(ConnectionCustomizer {})) - .build(manager)?; + .build(manager) + .or(Err(Error::ConnectionPoolBuild))?; let db = DB { pool }; db.migrate_up()?; Ok(db) } - pub fn connect(&self) -> Result>> { - self.pool.get().map_err(Error::new) + pub fn connect(&self) -> Result>, Error> { + self.pool.get().or(Err(Error::ConnectionPool)) } - #[allow(dead_code)] - fn migrate_down(&self) -> Result<()> { - let mut connection = self.connect().unwrap(); - if let Err(e) = connection.revert_all_migrations(MIGRATIONS) { - bail!(e); - } - Ok(()) + #[cfg(test)] + fn migrate_down(&self) -> Result<(), Error> { + let mut connection = self.connect()?; + connection + .revert_all_migrations(MIGRATIONS) + .and(Ok(())) + .or(Err(Error::Migration)) } - fn migrate_up(&self) -> Result<()> { - let mut connection = self.connect().unwrap(); - connection.run_pending_migrations(MIGRATIONS).unwrap(); - Ok(()) + fn migrate_up(&self) -> Result<(), Error> { + let mut connection = self.connect()?; + connection + .run_pending_migrations(MIGRATIONS) + .and(Ok(())) + .or(Err(Error::Migration)) } } diff --git a/src/service/actix/api.rs b/src/service/actix/api.rs index d591f71..0bed30b 100644 --- a/src/service/actix/api.rs +++ b/src/service/actix/api.rs @@ -504,7 +504,6 @@ async fn get_audio( let vfs = vfs_manager.get_vfs()?; let path = percent_decode_str(&path).decode_utf8_lossy(); vfs.virtual_to_real(Path::new(path.as_ref())) - .map_err(|_| APIError::VFSPathNotFound) }) .await?; @@ -525,9 +524,7 @@ async fn get_thumbnail( let thumbnail_path = block(move || { let vfs = vfs_manager.get_vfs()?; let path = percent_decode_str(&path).decode_utf8_lossy(); - let image_path = vfs - .virtual_to_real(Path::new(path.as_ref())) - .map_err(|_| APIError::VFSPathNotFound)?; + let image_path = vfs.virtual_to_real(Path::new(path.as_ref()))?; thumbnails_manager .get_thumbnail(&image_path, &options) .map_err(|_| APIError::Unspecified) diff --git a/src/service/error.rs b/src/service/error.rs index 4b9ed5d..68198f0 100644 --- a/src/service/error.rs +++ b/src/service/error.rs @@ -1,7 +1,8 @@ use thiserror::Error; use crate::app::index::QueryError; -use crate::app::{config, playlist, settings, user}; +use crate::app::{config, ddns, playlist, settings, user, vfs}; +use crate::db; #[derive(Error, Debug)] pub enum APIError { @@ -48,9 +49,10 @@ impl From for APIError { impl From for APIError { fn from(error: config::Error) -> APIError { match error { + config::Error::Ddns(e) => e.into(), config::Error::Settings(e) => e.into(), config::Error::User(e) => e.into(), - config::Error::Unspecified => APIError::Unspecified, + config::Error::Vfs(e) => e.into(), } } } @@ -58,8 +60,10 @@ impl From for APIError { impl From for APIError { fn from(error: playlist::Error) -> APIError { match error { + playlist::Error::DatabaseConnection(e) => e.into(), playlist::Error::PlaylistNotFound => APIError::PlaylistNotFound, playlist::Error::UserNotFound => APIError::UserNotFound, + playlist::Error::Vfs(e) => e.into(), playlist::Error::Unspecified => APIError::Unspecified, } } @@ -68,7 +72,8 @@ impl From for APIError { impl From for APIError { fn from(error: QueryError) -> APIError { match error { - QueryError::VFSPathNotFound => APIError::VFSPathNotFound, + QueryError::DatabaseConnection(e) => e.into(), + QueryError::Vfs(e) => e.into(), QueryError::Unspecified => APIError::Unspecified, } } @@ -78,6 +83,7 @@ impl From for APIError { fn from(error: settings::Error) -> APIError { match error { settings::Error::AuthSecretNotFound => APIError::Internal, + settings::Error::DatabaseConnection(e) => e.into(), settings::Error::InvalidAuthSecret => APIError::Internal, settings::Error::MiscSettingsNotFound => APIError::Internal, settings::Error::IndexAlbumArtPatternInvalid => APIError::Internal, @@ -90,6 +96,7 @@ impl From for APIError { impl From for APIError { fn from(error: user::Error) -> APIError { match error { + user::Error::DatabaseConnection(e) => e.into(), user::Error::EmptyUsername => APIError::EmptyUsername, user::Error::EmptyPassword => APIError::EmptyPassword, user::Error::IncorrectUsername => APIError::IncorrectCredentials, @@ -100,3 +107,37 @@ impl From for APIError { } } } + +impl From for APIError { + fn from(error: vfs::Error) -> APIError { + match error { + vfs::Error::CouldNotMapToVirtualPath(_) => APIError::VFSPathNotFound, + vfs::Error::CouldNotMapToRealPath(_) => APIError::VFSPathNotFound, + vfs::Error::Database(_) => APIError::Internal, + vfs::Error::DatabaseConnection(e) => e.into(), + vfs::Error::Unspecified => APIError::Unspecified, + } + } +} + +impl From for APIError { + fn from(error: ddns::Error) -> APIError { + match error { + ddns::Error::DatabaseConnection(e) => e.into(), + ddns::Error::UpdateQueryFailed(_) => APIError::Internal, + ddns::Error::Database(_) => APIError::Internal, + ddns::Error::Unspecified => APIError::Unspecified, + } + } +} + +impl From for APIError { + fn from(error: db::Error) -> APIError { + match error { + db::Error::ConnectionPoolBuild => APIError::Internal, + db::Error::ConnectionPool => APIError::Internal, + db::Error::Io(_, _) => APIError::Internal, + db::Error::Migration => APIError::Internal, + } + } +}