From 317cadaed861c117dd76c12c68bc25439d1a9b38 Mon Sep 17 00:00:00 2001 From: weisd Date: Mon, 23 Jun 2025 17:04:14 +0800 Subject: [PATCH] refactor(ecstore): simplify bucket metadata error handling by using ConfigNotFound - Remove specific bucket metadata error variants (BucketPolicyNotFound, etc.) - Unify all configuration-not-found errors to use StorageError::ConfigNotFound - Simplify error handling in bucket metadata system by removing redundant error conversions - Clean up error matching patterns in metadata getter methods - Remove debug print statement from get_bucket_targets_config - Update error handling in S3 API layer to use unified ConfigNotFound error This change improves code maintainability by reducing error type complexity while preserving the same error semantics for API consumers. --- ecstore/src/bucket/error.rs | 10 --- ecstore/src/bucket/metadata_sys.rs | 130 +++++------------------------ ecstore/src/error.rs | 23 +++-- rustfs/src/storage/ecfs.rs | 2 +- 4 files changed, 39 insertions(+), 126 deletions(-) diff --git a/ecstore/src/bucket/error.rs b/ecstore/src/bucket/error.rs index 44b2df1d..df098153 100644 --- a/ecstore/src/bucket/error.rs +++ b/ecstore/src/bucket/error.rs @@ -32,19 +32,9 @@ impl BucketMetadataError { } } -impl From for Error { - fn from(e: BucketMetadataError) -> Self { - match e { - BucketMetadataError::BucketPolicyNotFound => Error::BucketPolicyNotFound, - _ => Error::other(e), - } - } -} - impl From for BucketMetadataError { fn from(e: Error) -> Self { match e { - Error::BucketPolicyNotFound => BucketMetadataError::BucketPolicyNotFound, Error::Io(e) => e.into(), _ => BucketMetadataError::other(e), } diff --git a/ecstore/src/bucket/metadata_sys.rs b/ecstore/src/bucket/metadata_sys.rs index f06a49fb..5e3cf190 100644 --- a/ecstore/src/bucket/metadata_sys.rs +++ b/ecstore/src/bucket/metadata_sys.rs @@ -1,10 +1,4 @@ -use std::collections::HashSet; -use std::sync::OnceLock; -use std::time::Duration; -use std::{collections::HashMap, sync::Arc}; - use crate::StorageAPI; -use crate::bucket::error::BucketMetadataError; use crate::bucket::metadata::{BUCKET_LIFECYCLE_CONFIG, load_bucket_metadata_parse}; use crate::bucket::utils::{deserialize, is_meta_bucketname}; use crate::cmd::bucket_targets; @@ -18,6 +12,10 @@ use s3s::dto::{ BucketLifecycleConfiguration, NotificationConfiguration, ObjectLockConfiguration, ReplicationConfiguration, ServerSideEncryptionConfiguration, Tagging, VersioningConfiguration, }; +use std::collections::HashSet; +use std::sync::OnceLock; +use std::time::Duration; +use std::{collections::HashMap, sync::Arc}; use time::OffsetDateTime; use tokio::sync::RwLock; use tokio::time::sleep; @@ -400,85 +398,46 @@ impl BucketMetadataSys { } pub async fn get_bucket_policy(&self, bucket: &str) -> Result<(BucketPolicy, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - warn!("get_bucket_policy err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketPolicyNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.policy_config { Ok((config.clone(), bm.policy_config_updated_at)) } else { - Err(BucketMetadataError::BucketPolicyNotFound.into()) + Err(Error::ConfigNotFound) } } pub async fn get_tagging_config(&self, bucket: &str) -> Result<(Tagging, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - warn!("get_tagging_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::TaggingNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.tagging_config { Ok((config.clone(), bm.tagging_config_updated_at)) } else { - Err(BucketMetadataError::TaggingNotFound.into()) + Err(Error::ConfigNotFound) } } pub async fn get_object_lock_config(&self, bucket: &str) -> Result<(ObjectLockConfiguration, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketObjectLockConfigNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.object_lock_config { Ok((config.clone(), bm.object_lock_config_updated_at)) } else { - Err(BucketMetadataError::BucketObjectLockConfigNotFound.into()) + Err(Error::ConfigNotFound) } } pub async fn get_lifecycle_config(&self, bucket: &str) -> Result<(BucketLifecycleConfiguration, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - warn!("get_lifecycle_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketLifecycleNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.lifecycle_config { if config.rules.is_empty() { - Err(BucketMetadataError::BucketLifecycleNotFound.into()) + Err(Error::ConfigNotFound) } else { Ok((config.clone(), bm.lifecycle_config_updated_at)) } } else { - Err(BucketMetadataError::BucketLifecycleNotFound.into()) + Err(Error::ConfigNotFound) } } @@ -499,22 +458,12 @@ impl BucketMetadataSys { } pub async fn get_sse_config(&self, bucket: &str) -> Result<(ServerSideEncryptionConfiguration, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - warn!("get_sse_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketSSEConfigNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.sse_config { Ok((config.clone(), bm.encryption_config_updated_at)) } else { - Err(BucketMetadataError::BucketSSEConfigNotFound.into()) + Err(Error::ConfigNotFound) } } @@ -530,42 +479,17 @@ impl BucketMetadataSys { } pub async fn get_quota_config(&self, bucket: &str) -> Result<(BucketQuota, OffsetDateTime)> { - let bm = match self.get_config(bucket).await { - Ok((res, _)) => res, - Err(err) => { - warn!("get_quota_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketQuotaConfigNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, _) = self.get_config(bucket).await?; if let Some(config) = &bm.quota_config { Ok((config.clone(), bm.quota_config_updated_at)) } else { - Err(BucketMetadataError::BucketQuotaConfigNotFound.into()) + Err(Error::ConfigNotFound) } } pub async fn get_replication_config(&self, bucket: &str) -> Result<(ReplicationConfiguration, OffsetDateTime)> { - let (bm, reload) = match self.get_config(bucket).await { - Ok(res) => { - if res.0.replication_config.is_none() { - return Err(BucketMetadataError::BucketReplicationConfigNotFound.into()); - } - res - } - Err(err) => { - warn!("get_replication_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketReplicationConfigNotFound.into()) - } else { - Err(err) - }; - } - }; + let (bm, reload) = self.get_config(bucket).await?; if let Some(config) = &bm.replication_config { if reload { @@ -574,24 +498,12 @@ impl BucketMetadataSys { //println!("549 {:?}", config.clone()); Ok((config.clone(), bm.replication_config_updated_at)) } else { - Err(BucketMetadataError::BucketReplicationConfigNotFound.into()) + Err(Error::ConfigNotFound) } } pub async fn get_bucket_targets_config(&self, bucket: &str) -> Result { - let (bm, reload) = match self.get_config(bucket).await { - Ok(res) => res, - Err(err) => { - warn!("get_replication_config err {:?}", &err); - return if err == Error::ConfigNotFound { - Err(BucketMetadataError::BucketRemoteTargetNotFound.into()) - } else { - Err(err) - }; - } - }; - - println!("573"); + let (bm, reload) = self.get_config(bucket).await?; if let Some(config) = &bm.bucket_target_config { if reload { @@ -601,7 +513,7 @@ impl BucketMetadataSys { Ok(config.clone()) } else { - Err(BucketMetadataError::BucketRemoteTargetNotFound.into()) + Err(Error::ConfigNotFound) } } } diff --git a/ecstore/src/error.rs b/ecstore/src/error.rs index 4fb5c7ee..89e34ce8 100644 --- a/ecstore/src/error.rs +++ b/ecstore/src/error.rs @@ -1,5 +1,6 @@ use rustfs_utils::path::decode_dir_object; +use crate::bucket::error::BucketMetadataError; use crate::disk::error::DiskError; pub type Error = StorageError; @@ -164,9 +165,6 @@ pub enum StorageError { #[error("first disk wait")] FirstDiskWait, - #[error("Bucket policy not found")] - BucketPolicyNotFound, - #[error("Io error: {0}")] Io(std::io::Error), } @@ -255,6 +253,22 @@ impl From for DiskError { } } +impl From for Error { + fn from(e: BucketMetadataError) -> Self { + match e { + BucketMetadataError::TaggingNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketPolicyNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketObjectLockConfigNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketLifecycleNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketSSEConfigNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketQuotaConfigNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketReplicationConfigNotFound => Error::ConfigNotFound, + BucketMetadataError::BucketRemoteTargetNotFound => Error::ConfigNotFound, + _ => Error::other(e), + } + } +} + impl From for StorageError { fn from(e: std::io::Error) -> Self { match e.downcast::() { @@ -379,7 +393,6 @@ impl Clone for StorageError { StorageError::FirstDiskWait => StorageError::FirstDiskWait, StorageError::TooManyOpenFiles => StorageError::TooManyOpenFiles, StorageError::NoHealRequired => StorageError::NoHealRequired, - StorageError::BucketPolicyNotFound => StorageError::BucketPolicyNotFound, } } } @@ -442,7 +455,6 @@ impl StorageError { StorageError::ConfigNotFound => 0x35, StorageError::TooManyOpenFiles => 0x36, StorageError::NoHealRequired => 0x37, - StorageError::BucketPolicyNotFound => 0x38, } } @@ -507,7 +519,6 @@ impl StorageError { 0x35 => Some(StorageError::ConfigNotFound), 0x36 => Some(StorageError::TooManyOpenFiles), 0x37 => Some(StorageError::NoHealRequired), - 0x38 => Some(StorageError::BucketPolicyNotFound), _ => None, } } diff --git a/rustfs/src/storage/ecfs.rs b/rustfs/src/storage/ecfs.rs index 45e29e6f..15acaf3c 100644 --- a/rustfs/src/storage/ecfs.rs +++ b/rustfs/src/storage/ecfs.rs @@ -1601,7 +1601,7 @@ impl S3 for FS { let cfg = match PolicySys::get(&bucket).await { Ok(res) => res, Err(err) => { - if StorageError::BucketPolicyNotFound == err { + if StorageError::ConfigNotFound == err { return Err(s3_error!(NoSuchBucketPolicy)); } return Err(S3Error::with_message(S3ErrorCode::InternalError, err.to_string()));