diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs index 2668b42de..40d686e3a 100644 --- a/src/api/admin/error.rs +++ b/src/api/admin/error.rs @@ -1,3 +1,5 @@ +use std::convert::TryFrom; + use err_derive::Error; use hyper::header::HeaderValue; use hyper::{HeaderMap, StatusCode}; @@ -38,6 +40,19 @@ where } } +/// FIXME: helper errors are transformed into their corresponding variants +/// in the Error struct, but in many case a helper error should be considered +/// an internal error. +impl From for Error { + fn from(err: HelperError) -> Error { + match CommonError::try_from(err) { + Ok(ce) => Self::Common(ce), + Err(HelperError::NoSuchAccessKey(k)) => Self::NoSuchAccessKey(k), + Err(_) => unreachable!(), + } + } +} + impl CommonErrorDerivative for Error {} impl Error { diff --git a/src/api/common_error.rs b/src/api/common_error.rs index c47555d40..0c8006dc2 100644 --- a/src/api/common_error.rs +++ b/src/api/common_error.rs @@ -1,3 +1,5 @@ +use std::convert::TryFrom; + use err_derive::Error; use hyper::StatusCode; @@ -97,18 +99,39 @@ impl CommonError { } } -impl From for CommonError { - fn from(err: HelperError) -> Self { +impl TryFrom for CommonError { + type Error = HelperError; + + fn try_from(err: HelperError) -> Result { match err { - HelperError::Internal(i) => Self::InternalError(i), - HelperError::BadRequest(b) => Self::BadRequest(b), - HelperError::InvalidBucketName(n) => Self::InvalidBucketName(n), - HelperError::NoSuchBucket(n) => Self::NoSuchBucket(n), - e => Self::bad_request(format!("{}", e)), + HelperError::Internal(i) => Ok(Self::InternalError(i)), + HelperError::BadRequest(b) => Ok(Self::BadRequest(b)), + HelperError::InvalidBucketName(n) => Ok(Self::InvalidBucketName(n)), + HelperError::NoSuchBucket(n) => Ok(Self::NoSuchBucket(n)), + e => Err(e), } } } +/// This function converts HelperErrors into CommonErrors, +/// for variants that exist in CommonError. +/// This is used for helper functions that might return InvalidBucketName +/// or NoSuchBucket for instance, and we want to pass that error +/// up to our caller. +pub(crate) fn pass_helper_error(err: HelperError) -> CommonError { + match CommonError::try_from(err) { + Ok(e) => e, + Err(e) => panic!("Helper error `{}` should hot have happenned here", e), + } +} + +pub(crate) fn helper_error_as_internal(err: HelperError) -> CommonError { + match err { + HelperError::Internal(e) => CommonError::InternalError(e), + e => CommonError::InternalError(GarageError::Message(e.to_string())), + } +} + pub trait CommonErrorDerivative: From { fn internal_error(msg: M) -> Self { Self::from(CommonError::InternalError(GarageError::Message( diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index f2a3942ed..de6e5f067 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -90,11 +90,13 @@ impl ApiHandler for K2VApiServer { let bucket_id = garage .bucket_helper() .resolve_bucket(&bucket_name, &api_key) - .await?; + .await + .map_err(pass_helper_error)?; let bucket = garage .bucket_helper() .get_existing_bucket(bucket_id) - .await?; + .await + .map_err(helper_error_as_internal)?; let bucket_params = bucket.state.into_option().unwrap(); let allowed = match endpoint.authorization_type() { diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs index 02b7ae8bf..e4d0b0e53 100644 --- a/src/api/k2v/batch.rs +++ b/src/api/k2v/batch.rs @@ -4,12 +4,12 @@ use serde::{Deserialize, Serialize}; use garage_table::{EnumerationOrder, TableSchema}; -use garage_model::k2v::causality::*; use garage_model::k2v::item_table::*; use crate::helpers::*; use crate::k2v::api_server::{ReqBody, ResBody}; use crate::k2v::error::*; +use crate::k2v::item::parse_causality_token; use crate::k2v::range::read_range; pub async fn handle_insert_batch( @@ -23,7 +23,7 @@ pub async fn handle_insert_batch( let mut items2 = vec![]; for it in items { - let ct = it.ct.map(|s| CausalContext::parse_helper(&s)).transpose()?; + let ct = it.ct.map(|s| parse_causality_token(&s)).transpose()?; let v = match it.v { Some(vs) => DvvsValue::Value( BASE64_STANDARD @@ -281,7 +281,8 @@ pub(crate) async fn handle_poll_range( query.seen_marker, timeout_msec, ) - .await?; + .await + .map_err(pass_helper_error)?; if let Some((items, seen_marker)) = resp { let resp = PollRangeResponse { diff --git a/src/api/k2v/error.rs b/src/api/k2v/error.rs index 164792270..dbe4be2c0 100644 --- a/src/api/k2v/error.rs +++ b/src/api/k2v/error.rs @@ -3,6 +3,7 @@ use hyper::header::HeaderValue; use hyper::{HeaderMap, StatusCode}; use crate::common_error::CommonError; +pub(crate) use crate::common_error::{helper_error_as_internal, pass_helper_error}; pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError}; use crate::generic_server::ApiError; use crate::helpers::*; @@ -28,6 +29,10 @@ pub enum Error { #[error(display = "Invalid base64: {}", _0)] InvalidBase64(#[error(source)] base64::DecodeError), + /// Invalid causality token + #[error(display = "Invalid causality token")] + InvalidCausalityToken, + /// The client asked for an invalid return format (invalid Accept header) #[error(display = "Not acceptable: {}", _0)] NotAcceptable(String), @@ -72,6 +77,7 @@ impl Error { Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed", Error::InvalidBase64(_) => "InvalidBase64", Error::InvalidUtf8Str(_) => "InvalidUtf8String", + Error::InvalidCausalityToken => "CausalityToken", } } } @@ -85,7 +91,8 @@ impl ApiError for Error { Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, Error::AuthorizationHeaderMalformed(_) | Error::InvalidBase64(_) - | Error::InvalidUtf8Str(_) => StatusCode::BAD_REQUEST, + | Error::InvalidUtf8Str(_) + | Error::InvalidCausalityToken => StatusCode::BAD_REQUEST, } } diff --git a/src/api/k2v/item.rs b/src/api/k2v/item.rs index af3af4e4a..873717276 100644 --- a/src/api/k2v/item.rs +++ b/src/api/k2v/item.rs @@ -18,6 +18,10 @@ pub enum ReturnFormat { Either, } +pub(crate) fn parse_causality_token(s: &str) -> Result { + CausalContext::parse(s).ok_or(Error::InvalidCausalityToken) +} + impl ReturnFormat { pub fn from(req: &Request) -> Result { let accept = match req.headers().get(header::ACCEPT) { @@ -136,7 +140,7 @@ pub async fn handle_insert_item( .get(X_GARAGE_CAUSALITY_TOKEN) .map(|s| s.to_str()) .transpose()? - .map(CausalContext::parse_helper) + .map(parse_causality_token) .transpose()?; let body = http_body_util::BodyExt::collect(req.into_body()) @@ -176,7 +180,7 @@ pub async fn handle_delete_item( .get(X_GARAGE_CAUSALITY_TOKEN) .map(|s| s.to_str()) .transpose()? - .map(CausalContext::parse_helper) + .map(parse_causality_token) .transpose()?; let value = DvvsValue::Deleted; diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 1737af331..f9dafa106 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -150,7 +150,8 @@ impl ApiHandler for S3ApiServer { let bucket_id = garage .bucket_helper() .resolve_bucket(&bucket_name, &api_key) - .await?; + .await + .map_err(pass_helper_error)?; let bucket = garage .bucket_helper() .get_existing_bucket(bucket_id) diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs index e375a7145..b67ace884 100644 --- a/src/api/s3/copy.rs +++ b/src/api/s3/copy.rs @@ -655,7 +655,8 @@ async fn get_copy_source(ctx: &ReqCtx, req: &Request) -> Result for Error { + fn from(err: HelperError) -> Error { + Error::Common(helper_error_as_internal(err)) + } +} + impl CommonErrorDerivative for Error {} impl From for Error { diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs index 725f38470..5279ec6a8 100644 --- a/src/api/s3/post_object.rs +++ b/src/api/s3/post_object.rs @@ -107,7 +107,8 @@ pub async fn handle_post_object( let bucket_id = garage .bucket_helper() .resolve_bucket(&bucket_name, &api_key) - .await?; + .await + .map_err(pass_helper_error)?; if !api_key.allow_write(&bucket_id) { return Err(Error::forbidden("Operation is not allowed for this key.")); diff --git a/src/model/k2v/causality.rs b/src/model/k2v/causality.rs index c80ebd39b..7d311ede4 100644 --- a/src/model/k2v/causality.rs +++ b/src/model/k2v/causality.rs @@ -16,8 +16,6 @@ use serde::{Deserialize, Serialize}; use garage_util::data::*; -use crate::helper::error::{Error as HelperError, OkOrBadRequest}; - /// Node IDs used in K2V are u64 integers that are the abbreviation /// of full Garage node IDs which are 256-bit UUIDs. pub type K2VNodeId = u64; @@ -99,10 +97,6 @@ impl CausalContext { Some(ret) } - pub fn parse_helper(s: &str) -> Result { - Self::parse(s).ok_or_bad_request("Invalid causality token") - } - /// Check if this causal context contains newer items than another one pub fn is_newer_than(&self, other: &Self) -> bool { vclock_gt(&self.vector_clock, &other.vector_clock)