From 8033bdb0b4577133cff7d4d90a811ed8f3e13365 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 19:36:17 +0200 Subject: [PATCH] More precisions in errors & small refactoring --- src/api/admin/bucket.rs | 9 ++++----- src/api/admin/error.rs | 16 +++++++++------- src/api/admin/key.rs | 25 ++++++------------------- src/api/common_error.rs | 16 ++++++++-------- src/api/k2v/error.rs | 6 ++++-- src/api/s3/error.rs | 6 ++++-- 6 files changed, 35 insertions(+), 43 deletions(-) diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index c5518e4e..00450319 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -248,11 +248,10 @@ pub async fn handle_create_bucket( } let key = garage - .key_table - .get(&EmptyKey, &la.access_key_id) - .await? - .ok_or(Error::NoSuchAccessKey)?; - let state = key.state.as_option().ok_or(Error::NoSuchAccessKey)?; + .key_helper() + .get_existing_key(&la.access_key_id) + .await?; + let state = key.state.as_option().unwrap(); if matches!(state.local_aliases.get(&la.alias), Some(_)) { return Err(Error::bad_request("Local alias already exists")); } diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs index 38dfe5b6..cd7e6af7 100644 --- a/src/api/admin/error.rs +++ b/src/api/admin/error.rs @@ -18,8 +18,8 @@ pub enum Error { // Category: cannot process /// The API access key does not exist - #[error(display = "Access key not found")] - NoSuchAccessKey, + #[error(display = "Access key not found: {}", _0)] + NoSuchAccessKey(String), } impl From for Error @@ -38,9 +38,11 @@ impl From for Error { match err { HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), - HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), - HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), - HelperError::NoSuchAccessKey(_) => Self::NoSuchAccessKey, + HelperError::InvalidBucketName(n) => { + Self::CommonError(CommonError::InvalidBucketName(n)) + } + HelperError::NoSuchBucket(n) => Self::CommonError(CommonError::NoSuchBucket(n)), + HelperError::NoSuchAccessKey(n) => Self::NoSuchAccessKey(n), } } } @@ -49,7 +51,7 @@ impl Error { fn code(&self) -> &'static str { match self { Error::CommonError(c) => c.aws_code(), - Error::NoSuchAccessKey => "NoSuchAccessKey", + Error::NoSuchAccessKey(_) => "NoSuchAccessKey", } } } @@ -59,7 +61,7 @@ impl ApiError for Error { fn http_status_code(&self) -> StatusCode { match self { Error::CommonError(c) => c.http_status_code(), - Error::NoSuchAccessKey => StatusCode::NOT_FOUND, + Error::NoSuchAccessKey(_) => StatusCode::NOT_FOUND, } } diff --git a/src/api/admin/key.rs b/src/api/admin/key.rs index 8060bf1a..1e910d52 100644 --- a/src/api/admin/key.rs +++ b/src/api/admin/key.rs @@ -50,17 +50,12 @@ pub async fn handle_get_key_info( search: Option, ) -> Result, Error> { let key = if let Some(id) = id { - garage - .key_table - .get(&EmptyKey, &id) - .await? - .ok_or(Error::NoSuchAccessKey)? + garage.key_helper().get_existing_key(&id).await? } else if let Some(search) = search { garage .key_helper() .get_existing_matching_key(&search) - .await - .map_err(|_| Error::NoSuchAccessKey)? + .await? } else { unreachable!(); }; @@ -92,13 +87,9 @@ pub async fn handle_update_key( ) -> Result, Error> { let req = parse_json_body::(req).await?; - let mut key = garage - .key_table - .get(&EmptyKey, &id) - .await? - .ok_or(Error::NoSuchAccessKey)?; + let mut key = garage.key_helper().get_existing_key(&id).await?; - let key_state = key.state.as_option_mut().ok_or(Error::NoSuchAccessKey)?; + let key_state = key.state.as_option_mut().unwrap(); if let Some(new_name) = req.name { key_state.name.update(new_name); @@ -127,13 +118,9 @@ struct UpdateKeyRequest { } pub async fn handle_delete_key(garage: &Arc, id: String) -> Result, Error> { - let mut key = garage - .key_table - .get(&EmptyKey, &id) - .await? - .ok_or(Error::NoSuchAccessKey)?; + let mut key = garage.key_helper().get_existing_key(&id).await?; - key.state.as_option().ok_or(Error::NoSuchAccessKey)?; + key.state.as_option().unwrap(); garage.key_helper().delete_key(&mut key).await?; diff --git a/src/api/common_error.rs b/src/api/common_error.rs index b6dbf059..20f9f266 100644 --- a/src/api/common_error.rs +++ b/src/api/common_error.rs @@ -32,8 +32,8 @@ pub enum CommonError { // These have to be error codes referenced in the S3 spec here: // https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList /// The bucket requested don't exists - #[error(display = "Bucket not found")] - NoSuchBucket, + #[error(display = "Bucket not found: {}", _0)] + NoSuchBucket(String), /// Tried to create a bucket that already exist #[error(display = "Bucket already exists")] @@ -45,8 +45,8 @@ pub enum CommonError { // Category: bad request /// Bucket name is not valid according to AWS S3 specs - #[error(display = "Invalid bucket name")] - InvalidBucketName, + #[error(display = "Invalid bucket name: {}", _0)] + InvalidBucketName(String), } impl CommonError { @@ -62,9 +62,9 @@ impl CommonError { } CommonError::BadRequest(_) => StatusCode::BAD_REQUEST, CommonError::Forbidden(_) => StatusCode::FORBIDDEN, - CommonError::NoSuchBucket => StatusCode::NOT_FOUND, + CommonError::NoSuchBucket(_) => StatusCode::NOT_FOUND, CommonError::BucketNotEmpty | CommonError::BucketAlreadyExists => StatusCode::CONFLICT, - CommonError::InvalidBucketName => StatusCode::BAD_REQUEST, + CommonError::InvalidBucketName(_) => StatusCode::BAD_REQUEST, } } @@ -80,10 +80,10 @@ impl CommonError { "InternalError" } CommonError::BadRequest(_) => "InvalidRequest", - CommonError::NoSuchBucket => "NoSuchBucket", + CommonError::NoSuchBucket(_) => "NoSuchBucket", CommonError::BucketAlreadyExists => "BucketAlreadyExists", CommonError::BucketNotEmpty => "BucketNotEmpty", - CommonError::InvalidBucketName => "InvalidBucketName", + CommonError::InvalidBucketName(_) => "InvalidBucketName", } } diff --git a/src/api/k2v/error.rs b/src/api/k2v/error.rs index 85d5de9d..dbd9a5f5 100644 --- a/src/api/k2v/error.rs +++ b/src/api/k2v/error.rs @@ -59,8 +59,10 @@ impl From for Error { match err { HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), - HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), - HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), + HelperError::InvalidBucketName(n) => { + Self::CommonError(CommonError::InvalidBucketName(n)) + } + HelperError::NoSuchBucket(n) => Self::CommonError(CommonError::NoSuchBucket(n)), e => Self::CommonError(CommonError::BadRequest(format!("{}", e))), } } diff --git a/src/api/s3/error.rs b/src/api/s3/error.rs index 4edff3a1..f2096bea 100644 --- a/src/api/s3/error.rs +++ b/src/api/s3/error.rs @@ -91,8 +91,10 @@ impl From for Error { match err { HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), - HelperError::InvalidBucketName(_) => Self::CommonError(CommonError::InvalidBucketName), - HelperError::NoSuchBucket(_) => Self::CommonError(CommonError::NoSuchBucket), + HelperError::InvalidBucketName(n) => { + Self::CommonError(CommonError::InvalidBucketName(n)) + } + HelperError::NoSuchBucket(n) => Self::CommonError(CommonError::NoSuchBucket(n)), e => Self::bad_request(format!("{}", e)), } }