From da97c4ded2ce778ec0f4df9e3ff14252f00764b2 Mon Sep 17 00:00:00 2001 From: SabrinaJewson Date: Wed, 15 Jun 2022 14:07:40 +0100 Subject: [PATCH 1/5] Remove `ModelError` in favour of strongly-typed errors --- rspotify-model/src/auth.rs | 176 ++++++++++++++++++++++++++++++++---- rspotify-model/src/error.rs | 11 --- src/client_creds.rs | 3 +- src/clients/oauth.rs | 5 +- src/lib.rs | 4 +- 5 files changed, 168 insertions(+), 31 deletions(-) diff --git a/rspotify-model/src/auth.rs b/rspotify-model/src/auth.rs index 53c7586e..d8cb627c 100644 --- a/rspotify-model/src/auth.rs +++ b/rspotify-model/src/auth.rs @@ -1,14 +1,11 @@ //! All objects related to the auth flows defined by Spotify API -use crate::{ - custom_serde::{duration_second, space_separated_scopes}, - ModelResult, -}; +use crate::custom_serde::{duration_second, space_separated_scopes}; use std::{ collections::{HashMap, HashSet}, - fs, - io::{Read, Write}, + fmt::{self, Display, Formatter}, + fs, io, path::Path, }; @@ -56,22 +53,22 @@ impl Default for Token { impl Token { /// Tries to initialize the token from a cache file. - pub fn from_cache>(path: T) -> ModelResult { - let mut file = fs::File::open(path)?; - let mut tok_str = String::new(); - file.read_to_string(&mut tok_str)?; - let tok = serde_json::from_str::(&tok_str)?; + pub fn from_cache>(path: T) -> Result { + let json = read_file(path.as_ref()).map_err(ReadTokenCacheError::Reading)?; + + let tok = + serde_json::from_slice::(&*json).map_err(ReadTokenCacheError::Deserializing)?; Ok(tok) } /// Saves the token information into its cache file. - pub fn write_cache>(&self, path: T) -> ModelResult<()> { - let token_info = serde_json::to_string(&self)?; + pub fn write_cache>(&self, path: T) -> Result<(), WriteTokenCacheError> { + // `serde_json::to_vec` only errors if our `Serialize` implementation fails, which it + // shouldn’t, or we try to serialize a map with non-string keys, which we don’t. + let json = serde_json::to_vec(&self).unwrap(); - let mut file = fs::OpenOptions::new().write(true).create(true).open(path)?; - file.set_len(0)?; - file.write_all(token_info.as_bytes())?; + write_file(path.as_ref(), &*json).map_err(|inner| WriteTokenCacheError { inner })?; Ok(()) } @@ -95,6 +92,153 @@ impl Token { } } +/// An error reading a cached [`Token`]. +#[derive(Debug)] +pub enum ReadTokenCacheError { + /// There was an error reading the cache file into memory. + Reading(ReadFileError), + + /// There was an error deserializing the contents of the cache file. + Deserializing(serde_json::Error), +} + +impl Display for ReadTokenCacheError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str("failed to read token from cache") + } +} + +impl std::error::Error for ReadTokenCacheError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(match self { + Self::Reading(e) => e, + Self::Deserializing(e) => e, + }) + } +} + +/// An error writing a [`Token`] to its cache. +#[derive(Debug)] +#[non_exhaustive] +pub struct WriteTokenCacheError { + /// The underlying error in writing the [`Token`] file cache. + pub inner: WriteFileError, +} + +impl Display for WriteTokenCacheError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + f.write_str("failed to write token to cache") + } +} + +impl std::error::Error for WriteTokenCacheError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.inner) + } +} + +fn read_file(path: &Path) -> Result, ReadFileError> { + fs::read(path).map_err(|inner| ReadFileError { + inner, + path: Box::from(path), + }) +} + +/// An error reading a file. +#[derive(Debug)] +pub struct ReadFileError { + // Intentionally not exposed to allow future API evolution, e.g. moving this to a enum variants + // `Open(io::Error)` and `Read(io::Error)` + inner: io::Error, + path: Box, +} + +impl ReadFileError { + /// Returns a shared reference to the underlying I/O that caused this. + #[must_use] + pub fn io(&self) -> &io::Error { + &self.inner + } + + /// Consumes this error type, returning the underlying inner I/O error. + /// + /// It is not recommended to use this method just to unify error types, as you will lose + /// valuable information in the error message. + #[must_use] + pub fn into_io(self) -> io::Error { + self.inner + } + + /// Returns a shared reference to the path that could not be read. + #[must_use] + pub fn path(&self) -> &Path { + &*self.path + } +} + +impl Display for ReadFileError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "failed to read file {}", self.path.display()) + } +} + +impl std::error::Error for ReadFileError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.io()) + } +} + +fn write_file(path: &Path, bytes: &[u8]) -> Result<(), WriteFileError> { + fs::write(path, bytes).map_err(|inner| WriteFileError { + inner, + path: Box::from(path), + }) +} + +/// An error writing a file. +#[derive(Debug)] +pub struct WriteFileError { + // Intentionally not exposed to allow future API evolution, e.g. moving this to an enum variant + // `Open(io::Error)` and `Write(io::Error)` + inner: io::Error, + path: Box, +} + +impl WriteFileError { + /// Returns a shared reference to the underlying I/O that caused this. + #[must_use] + pub fn io(&self) -> &io::Error { + &self.inner + } + + /// Consumes this error type, returning the underlying inner I/O error. + /// + /// It is not recommended to use this method just to unify error types, as you will lose + /// valuable information in the error message. + #[must_use] + pub fn into_io(self) -> io::Error { + self.inner + } + + /// Returns a shared reference to the path that could not be written to. + #[must_use] + pub fn path(&self) -> &Path { + &*self.path + } +} + +impl Display for WriteFileError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "failed to write file {}", self.path.display()) + } +} + +impl std::error::Error for WriteFileError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.io()) + } +} + #[cfg(test)] mod test { use crate::Token; diff --git a/rspotify-model/src/error.rs b/rspotify-model/src/error.rs index faf9a9b8..a8f068bb 100644 --- a/rspotify-model/src/error.rs +++ b/rspotify-model/src/error.rs @@ -2,7 +2,6 @@ use serde::Deserialize; use thiserror::Error; pub type ApiResult = Result; -pub type ModelResult = Result; /// Matches errors that are returned from the Spotfiy /// API as part of the JSON response object. @@ -22,13 +21,3 @@ pub enum ApiError { reason: String, }, } - -/// Groups up the kinds of errors that may happen in this crate. -#[derive(Debug, Error)] -pub enum ModelError { - #[error("json parse error: {0}")] - ParseJson(#[from] serde_json::Error), - - #[error("input/output error: {0}")] - Io(#[from] std::io::Error), -} diff --git a/src/client_creds.rs b/src/client_creds.rs index c31ea5ee..c8fdede0 100644 --- a/src/client_creds.rs +++ b/src/client_creds.rs @@ -1,6 +1,7 @@ use crate::{ clients::{mutex::Mutex, BaseClient}, http::{Form, HttpClient}, + model::ReadTokenCacheError, params, ClientResult, Config, Credentials, Token, }; @@ -93,7 +94,7 @@ impl ClientCredsSpotify { /// * The read token is expired /// * The cached token is disabled in the config #[maybe_async] - pub async fn read_token_cache(&self) -> ClientResult> { + pub async fn read_token_cache(&self) -> Result, ReadTokenCacheError> { if !self.get_config().token_cached { log::info!("Token cache read ignored (not configured)"); return Ok(None); diff --git a/src/clients/oauth.rs b/src/clients/oauth.rs index 4f02d31b..e0ef9f8a 100644 --- a/src/clients/oauth.rs +++ b/src/clients/oauth.rs @@ -56,7 +56,10 @@ pub trait OAuthClient: BaseClient { /// the application re-authenticate. /// /// [`ClientCredsSpotify::read_token_cache`]: crate::client_creds::ClientCredsSpotify::read_token_cache - async fn read_token_cache(&self, allow_expired: bool) -> ClientResult> { + async fn read_token_cache( + &self, + allow_expired: bool, + ) -> Result, ReadTokenCacheError> { if !self.get_config().token_cached { log::info!("Auth token cache read ignored (not configured)"); return Ok(None); diff --git a/src/lib.rs b/src/lib.rs index 2394ef08..d3e5f26f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -212,8 +212,8 @@ pub enum ClientError { #[error("cache file error: {0}")] CacheFile(String), - #[error("model error: {0}")] - Model(#[from] model::ModelError), + #[error("error updating token cache: {0}")] + UpdateTokenCache(#[from] model::WriteTokenCacheError), } // The conversion has to be done manually because it's in a `Box` From e6324963fa2041352524a2136d0e5c03866c2d23 Mon Sep 17 00:00:00 2001 From: SabrinaJewson Date: Wed, 15 Jun 2022 14:13:57 +0100 Subject: [PATCH 2/5] Remove `ClientError::CacheFile` --- src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d3e5f26f..3e9a8bfb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -209,9 +209,6 @@ pub enum ClientError { #[error("cli error: {0}")] Cli(String), - #[error("cache file error: {0}")] - CacheFile(String), - #[error("error updating token cache: {0}")] UpdateTokenCache(#[from] model::WriteTokenCacheError), } From 0b1f78ca556ac13a2823ae00cf59c879101be74c Mon Sep 17 00:00:00 2001 From: SabrinaJewson Date: Wed, 15 Jun 2022 14:19:16 +0100 Subject: [PATCH 3/5] Document `ModelError` removal in changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7701e92..97084de8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## Unreleased + +**Breaking changes**: +- ([#327](https://github.com/ramsayleung/rspotify/pull/327)) `ModelError` has been replaced with the more precise error types `ReadTokenCacheError` and `WriteTokenCacheError` + ## 0.11.5 (2022.03.28) **Breaking changes**: From 5678cf110e0637eb9ab6013569fb98988d2fd1fe Mon Sep 17 00:00:00 2001 From: SabrinaJewson Date: Wed, 15 Jun 2022 15:22:51 +0100 Subject: [PATCH 4/5] Use `derive(Error)` instead of manual impls --- rspotify-model/src/auth.rs | 71 ++++++++------------------------------ 1 file changed, 14 insertions(+), 57 deletions(-) diff --git a/rspotify-model/src/auth.rs b/rspotify-model/src/auth.rs index d8cb627c..0c25f882 100644 --- a/rspotify-model/src/auth.rs +++ b/rspotify-model/src/auth.rs @@ -11,6 +11,7 @@ use std::{ use chrono::{DateTime, Duration, Utc}; use serde::{Deserialize, Serialize}; +use thiserror::Error; /// Spotify access token information /// @@ -93,50 +94,26 @@ impl Token { } /// An error reading a cached [`Token`]. -#[derive(Debug)] +#[derive(Debug, Error)] +#[error("failed to read token from cache")] pub enum ReadTokenCacheError { /// There was an error reading the cache file into memory. - Reading(ReadFileError), + Reading(#[source] ReadFileError), /// There was an error deserializing the contents of the cache file. - Deserializing(serde_json::Error), -} - -impl Display for ReadTokenCacheError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - f.write_str("failed to read token from cache") - } -} - -impl std::error::Error for ReadTokenCacheError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(match self { - Self::Reading(e) => e, - Self::Deserializing(e) => e, - }) - } + Deserializing(#[source] serde_json::Error), } /// An error writing a [`Token`] to its cache. -#[derive(Debug)] +#[derive(Debug, Error)] #[non_exhaustive] +#[error("failed to write token to cache")] pub struct WriteTokenCacheError { /// The underlying error in writing the [`Token`] file cache. + #[source] pub inner: WriteFileError, } -impl Display for WriteTokenCacheError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - f.write_str("failed to write token to cache") - } -} - -impl std::error::Error for WriteTokenCacheError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(&self.inner) - } -} - fn read_file(path: &Path) -> Result, ReadFileError> { fs::read(path).map_err(|inner| ReadFileError { inner, @@ -145,10 +122,12 @@ fn read_file(path: &Path) -> Result, ReadFileError> { } /// An error reading a file. -#[derive(Debug)] +#[derive(Debug, Error)] +#[error("failed to read file {}", path.display())] pub struct ReadFileError { // Intentionally not exposed to allow future API evolution, e.g. moving this to a enum variants // `Open(io::Error)` and `Read(io::Error)` + #[source] inner: io::Error, path: Box, } @@ -176,18 +155,6 @@ impl ReadFileError { } } -impl Display for ReadFileError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "failed to read file {}", self.path.display()) - } -} - -impl std::error::Error for ReadFileError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(self.io()) - } -} - fn write_file(path: &Path, bytes: &[u8]) -> Result<(), WriteFileError> { fs::write(path, bytes).map_err(|inner| WriteFileError { inner, @@ -196,10 +163,12 @@ fn write_file(path: &Path, bytes: &[u8]) -> Result<(), WriteFileError> { } /// An error writing a file. -#[derive(Debug)] +#[derive(Debug, Error)] +#[error("failed to write file {}", path.display())] pub struct WriteFileError { // Intentionally not exposed to allow future API evolution, e.g. moving this to an enum variant // `Open(io::Error)` and `Write(io::Error)` + #[source] inner: io::Error, path: Box, } @@ -227,18 +196,6 @@ impl WriteFileError { } } -impl Display for WriteFileError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "failed to write file {}", self.path.display()) - } -} - -impl std::error::Error for WriteFileError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(self.io()) - } -} - #[cfg(test)] mod test { use crate::Token; From 48a3e6d2e49a36c9bdec775bfd0d9b03515ffe61 Mon Sep 17 00:00:00 2001 From: SabrinaJewson Date: Wed, 15 Jun 2022 17:20:42 +0100 Subject: [PATCH 5/5] Use American English and wrap comments --- rspotify-model/src/auth.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/rspotify-model/src/auth.rs b/rspotify-model/src/auth.rs index 0c25f882..adb5ea85 100644 --- a/rspotify-model/src/auth.rs +++ b/rspotify-model/src/auth.rs @@ -65,8 +65,9 @@ impl Token { /// Saves the token information into its cache file. pub fn write_cache>(&self, path: T) -> Result<(), WriteTokenCacheError> { - // `serde_json::to_vec` only errors if our `Serialize` implementation fails, which it - // shouldn’t, or we try to serialize a map with non-string keys, which we don’t. + // `serde_json::to_vec` only errors if our `Serialize` implementation + // fails, which it shouldn’t, or we try to serialize a map with + // non-string keys, which we don’t. let json = serde_json::to_vec(&self).unwrap(); write_file(path.as_ref(), &*json).map_err(|inner| WriteTokenCacheError { inner })?; @@ -125,8 +126,8 @@ fn read_file(path: &Path) -> Result, ReadFileError> { #[derive(Debug, Error)] #[error("failed to read file {}", path.display())] pub struct ReadFileError { - // Intentionally not exposed to allow future API evolution, e.g. moving this to a enum variants - // `Open(io::Error)` and `Read(io::Error)` + // Intentionally not exposed to allow future API evolution, e.g., moving + // this to a enum variants `Open(io::Error)` and `Read(io::Error)` #[source] inner: io::Error, path: Box, @@ -141,8 +142,8 @@ impl ReadFileError { /// Consumes this error type, returning the underlying inner I/O error. /// - /// It is not recommended to use this method just to unify error types, as you will lose - /// valuable information in the error message. + /// It is not recommended to use this method just to unify error types, as + /// you will lose valuable information in the error message. #[must_use] pub fn into_io(self) -> io::Error { self.inner @@ -166,8 +167,8 @@ fn write_file(path: &Path, bytes: &[u8]) -> Result<(), WriteFileError> { #[derive(Debug, Error)] #[error("failed to write file {}", path.display())] pub struct WriteFileError { - // Intentionally not exposed to allow future API evolution, e.g. moving this to an enum variant - // `Open(io::Error)` and `Write(io::Error)` + // Intentionally not exposed to allow future API evolution, e.g., moving + // this to an enum variant `Open(io::Error)` and `Write(io::Error)` #[source] inner: io::Error, path: Box, @@ -182,8 +183,8 @@ impl WriteFileError { /// Consumes this error type, returning the underlying inner I/O error. /// - /// It is not recommended to use this method just to unify error types, as you will lose - /// valuable information in the error message. + /// It is not recommended to use this method just to unify error types, as + /// you will lose valuable information in the error message. #[must_use] pub fn into_io(self) -> io::Error { self.inner