Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: reduce boilplate for some endpoint checkings; keep error same style. #5047

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions core/src/services/alluxio/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,9 @@ impl Builder for AlluxioBuilder {
let root = normalize_root(&self.config.root.clone().unwrap_or_default());
debug!("backend use root {}", &root);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Alluxio)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR, but I don't want to optimize code in this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is similiar to #5037 (comment)

.map_err(|err| err.with_operation("Builder::build"))?;
debug!("backend use endpoint {}", &endpoint);

let client = if let Some(client) = self.http_client {
Expand Down
9 changes: 3 additions & 6 deletions core/src/services/azdls/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,9 @@ impl Builder for AzdlsBuilder {
}?;
debug!("backend use filesystem {}", &filesystem);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Azdls)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)
.map_err(|err| err.with_operation("Builder::build"))?;
debug!("backend use endpoint {}", &endpoint);

let client = if let Some(client) = self.http_client {
Expand Down
9 changes: 3 additions & 6 deletions core/src/services/azfile/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,12 +148,9 @@ impl Builder for AzfileBuilder {
let root = normalize_root(&self.config.root.unwrap_or_default());
debug!("backend use root {}", root);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Azfile)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)
.map_err(|err| err.with_operation("Builder::build"))?;
debug!("backend use endpoint {}", &endpoint);

let client = if let Some(client) = self.http_client {
Expand Down
20 changes: 8 additions & 12 deletions core/src/services/cos/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,22 +157,18 @@ impl Builder for CosBuilder {

let bucket = match &self.config.bucket {
Some(bucket) => Ok(bucket.to_string()),
None => Err(
Error::new(ErrorKind::ConfigInvalid, "The bucket is misconfigured")
.with_context("service", Scheme::Cos),
),
None => Err(Error::error_kind_config_invalid(
Self::SCHEME,
"The bucket is misconfigured",
)),
}?;
debug!("backend use bucket {}", &bucket);

let uri = match &self.config.endpoint {
Some(endpoint) => endpoint.parse::<Uri>().map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
.with_context("service", Scheme::Cos)
.with_context("endpoint", endpoint)
.set_source(err)
}),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Cos)),
Some(endpoint) => endpoint
.parse::<Uri>()
.map_err(|err| Error::empty_endpoint_err(Self::SCHEME).set_source(err)),
None => Err(Error::empty_endpoint_err(Self::SCHEME)),
}?;

let scheme = match uri.scheme_str() {
Expand Down
9 changes: 3 additions & 6 deletions core/src/services/dbfs/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,9 @@ impl Builder for DbfsBuilder {
let root = normalize_root(&self.config.root.unwrap_or_default());
debug!("backend use root {}", root);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Dbfs)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)
.map_err(|err| err.with_operation("Builder::build"))?;
debug!("backend use endpoint: {}", &endpoint);

let token = match self.config.token {
Expand Down
7 changes: 2 additions & 5 deletions core/src/services/ftp/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,8 @@ impl Builder for FtpBuilder {

fn build(self) -> Result<impl Access> {
debug!("ftp backend build started: {:?}", &self);
let endpoint = match &self.config.endpoint {
None => return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")),
Some(v) => v,
};

let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
let endpoint_uri = match endpoint.parse::<Uri>() {
Err(e) => {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
Expand Down
11 changes: 2 additions & 9 deletions core/src/services/http/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,8 @@ impl Builder for HttpBuilder {

fn build(self) -> Result<impl Access> {
debug!("backend build started: {:?}", &self);

let endpoint = match &self.config.endpoint {
Some(v) => v,
None => {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Http))
}
};

let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
let root = normalize_root(&self.config.root.unwrap_or_default());
debug!("backend use root {}", root);

Expand Down
8 changes: 2 additions & 6 deletions core/src/services/ipfs/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,8 @@ impl Builder for IpfsBuilder {
}
debug!("backend use root {}", root);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Ipfs)
.with_context("root", &root)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
debug!("backend use endpoint {}", &endpoint);

let client = if let Some(client) = self.http_client {
Expand Down
12 changes: 4 additions & 8 deletions core/src/services/koofr/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,9 @@ impl Builder for KoofrBuilder {
let root = normalize_root(&self.config.root.clone().unwrap_or_default());
debug!("backend use root {}", &root);

if self.config.endpoint.is_empty() {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Koofr));
}

debug!("backend use endpoint {}", &self.config.endpoint);
let endpoint =
Error::ensure_endpoint_not_empty(Some(self.config.endpoint.as_str()), Self::SCHEME)?;
debug!("backend use endpoint {}", endpoint);

if self.config.email.is_empty() {
return Err(Error::new(ErrorKind::ConfigInvalid, "email is empty")
Expand Down Expand Up @@ -177,7 +173,7 @@ impl Builder for KoofrBuilder {
Ok(KoofrBackend {
core: Arc::new(KoofrCore {
root,
endpoint: self.config.endpoint.clone(),
endpoint,
email: self.config.email.clone(),
password,
mount_id: OnceCell::new(),
Expand Down
6 changes: 2 additions & 4 deletions core/src/services/memcached/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,8 @@ impl Builder for MemcachedBuilder {
type Config = MemcachedConfig;

fn build(self) -> Result<impl Access> {
let endpoint = self.config.endpoint.clone().ok_or_else(|| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Memcached)
})?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
let uri = http::Uri::try_from(&endpoint).map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
.with_context("service", Scheme::Memcached)
Expand Down
6 changes: 2 additions & 4 deletions core/src/services/obs/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,10 @@ impl Builder for ObsBuilder {

let uri = match &self.config.endpoint {
Some(endpoint) => endpoint.parse::<Uri>().map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
.with_context("service", Scheme::Obs)
Error::error_kind_config_invalid(Self::SCHEME, "endpoint is invalid")
.set_source(err)
}),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Obs)),
None => Err(Error::empty_endpoint_err(Self::SCHEME)),
}?;

let scheme = match uri.scheme_str() {
Expand Down
22 changes: 8 additions & 14 deletions core/src/services/oss/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,11 @@ impl OssBuilder {
fn parse_endpoint(&self, endpoint: &Option<String>, bucket: &str) -> Result<(String, String)> {
let (endpoint, host) = match endpoint.clone() {
Some(ep) => {
let uri = ep.parse::<Uri>().map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
.with_context("service", Scheme::Oss)
.with_context("endpoint", &ep)
.set_source(err)
})?;
let uri = ep
.parse::<Uri>()
.map_err(|err| Error::empty_endpoint_err(Self::SCHEME).set_source(err))?;
let host = uri.host().ok_or_else(|| {
Error::new(ErrorKind::ConfigInvalid, "endpoint host is empty")
.with_context("service", Scheme::Oss)
Error::error_kind_config_invalid(Self::SCHEME, "endpoint host is empty")
.with_context("endpoint", &ep)
})?;
let full_host = if let Some(port) = uri.port_u16() {
Expand All @@ -174,20 +170,18 @@ impl OssBuilder {
Some(scheme_str) => match scheme_str {
"http" | "https" => format!("{scheme_str}://{full_host}"),
_ => {
return Err(Error::new(
ErrorKind::ConfigInvalid,
return Err(Error::error_kind_config_invalid(
Self::SCHEME,
"endpoint protocol is invalid",
)
.with_context("service", Scheme::Oss));
));
}
},
None => format!("https://{full_host}"),
};
(endpoint, full_host)
}
None => {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Oss));
return Err(Error::empty_endpoint_err(Self::SCHEME));
}
};
Ok((endpoint, host))
Expand Down
12 changes: 4 additions & 8 deletions core/src/services/pcloud/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,9 @@ impl Builder for PcloudBuilder {
debug!("backend use root {}", &root);

// Handle endpoint.
if self.config.endpoint.is_empty() {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Pcloud));
}

debug!("backend use endpoint {}", &self.config.endpoint);
let endpoint =
Error::ensure_endpoint_not_empty(Some(self.config.endpoint.as_str()), Self::SCHEME)?;
debug!("backend use endpoint {}", endpoint);

let username = match &self.config.username {
Some(username) => Ok(username.clone()),
Expand Down Expand Up @@ -171,7 +167,7 @@ impl Builder for PcloudBuilder {
Ok(PcloudBackend {
core: Arc::new(PcloudCore {
root,
endpoint: self.config.endpoint.clone(),
endpoint,
username,
password,
client,
Expand Down
8 changes: 2 additions & 6 deletions core/src/services/seafile/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,8 @@ impl Builder for SeafileBuilder {

debug!("backend use repo_name {}", &self.config.repo_name);

let endpoint = match &self.config.endpoint {
Some(endpoint) => Ok(endpoint.clone()),
None => Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_operation("Builder::build")
.with_context("service", Scheme::Seafile)),
}?;
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;

let username = match &self.config.username {
Some(username) => Ok(username.clone()),
Expand Down
8 changes: 4 additions & 4 deletions core/src/services/sftp/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,10 @@ impl Builder for SftpBuilder {

fn build(self) -> Result<impl Access> {
debug!("sftp backend build started: {:?}", &self);
let endpoint = match self.config.endpoint.clone() {
Some(v) => v,
None => return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")),
};
// Handle endpoint.
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
debug!("backend use endpoint {}", endpoint);

let user = self.config.user.clone();

Expand Down
16 changes: 7 additions & 9 deletions core/src/services/webdav/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,13 @@ impl Builder for WebdavBuilder {
fn build(self) -> Result<impl Access> {
debug!("backend build started: {:?}", &self);

let endpoint = match &self.config.endpoint {
Some(v) => v,
None => {
return Err(Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", Scheme::Webdav));
}
};
// Handle endpoint.
let endpoint =
Error::ensure_endpoint_not_empty(self.config.endpoint.as_deref(), Self::SCHEME)?;
debug!("backend use endpoint {}", endpoint);

// Some services might return the path with suffix `/remote.php/webdav/`, we need to trim them.
let server_path = http::Uri::from_str(endpoint)
let server_path = http::Uri::from_str(&endpoint)
.map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is invalid")
.with_context("service", Scheme::Webdav)
Expand Down Expand Up @@ -176,7 +174,7 @@ impl Builder for WebdavBuilder {
}

let core = Arc::new(WebdavCore {
endpoint: endpoint.to_string(),
endpoint,
server_path,
authorization,
disable_copy: self.config.disable_copy,
Expand Down
31 changes: 31 additions & 0 deletions core/src/types/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ use std::fmt::Display;
use std::fmt::Formatter;
use std::io;

use crate::Scheme;

/// Result that is a wrapper of `Result<T, opendal::Error>`
pub type Result<T, E = Error> = std::result::Result<T, E>;

Expand Down Expand Up @@ -400,6 +402,35 @@ impl Error {
pub fn is_temporary(&self) -> bool {
self.status == ErrorStatus::Temporary
}

/** Useful helper functions for Builder::build */

#[inline]
#[allow(unused)]
pub(crate) fn error_kind_config_invalid(scheme: Scheme, message: impl Into<String>) -> Self {
// Since all ErrorKind::ConfigInvalid occur in Builder::build,
// we always call `with_operation` and `with_context`.
Self::new(ErrorKind::ConfigInvalid, message)
.with_operation("Builder::build")
.with_context("service", scheme)
}

#[inline]
#[allow(unused)]
pub(crate) fn empty_endpoint_err(ctx_schema: Scheme) -> Self {
Self::error_kind_config_invalid(ctx_schema, "endpoint is empty")
}

#[inline]
#[allow(unused)]
pub(crate) fn ensure_endpoint_not_empty(
endpoint: Option<&str>,
ctx_schema: Scheme,
) -> Result<String, Self> {
let endpoint = endpoint.ok_or_else(|| Self::empty_endpoint_err(ctx_schema))?;

Ok(endpoint.to_string())
}
}

impl From<Error> for io::Error {
Expand Down
Loading