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

feat(core)!: from_map is now fallible #4917

Merged
merged 1 commit into from
Jul 20, 2024
Merged
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
24 changes: 14 additions & 10 deletions core/src/docs/internals/accessor.rs
Original file line number Diff line number Diff line change
@@ -214,15 +214,22 @@
//! /// Ok(())
//! /// }
//! /// ```
//! #[derive(Default, Serialize, Deserialize, Clone, PartialEq, Eq)]
//! #[serde(default)]
//! #[non_exhaustive]
//! pub struct DuckConfig {
//! pub root: Option<String>,
//! }
//!
//! #[derive(Default, Clone)]
//! pub struct DuckBuilder {
//! root: Option<String>,
//! config: DuckConfig,
//! }
//! ```
//!
//! Note that `DuckBuilder` is part of our public API, so it needs to be
//! documented. And any changes you make will directly affect users, so
//! please take it seriously. Otherwise you will be hunted down by many
//! please take it seriously. Otherwise, you will be hunted down by many
//! angry ducks.
//!
//! Then, we can implement required APIs for `DuckBuilder`:
@@ -233,7 +240,7 @@
//! ///
//! /// All operations will happen under this root.
//! pub fn root(&mut self, root: &str) -> &mut Self {
//! self.root = if root.is_empty() {
//! self.config.root = if root.is_empty() {
//! None
//! } else {
//! Some(root.to_string())
@@ -246,19 +253,16 @@
//! impl Builder for DuckBuilder {
//! const SCHEME: Scheme = Scheme::Duck;
//! type Accessor = DuckBackend;
//! type Config = DuckConfig;
//!
//! fn from_map(map: HashMap<String, String>) -> Self {
//! let mut builder = DuckBuilder::default();
//!
//! map.get("root").map(|v| builder.root(v));
//!
//! builder
//! fn from_config(config: Self::Config) -> Self {
//! DuckBuilder { config }
//! }
//!
//! fn build(&mut self) -> Result<Self::Accessor> {
//! debug!("backend build started: {:?}", &self);
//!
//! let root = normalize_root(&self.root.clone().unwrap_or_default());
//! let root = normalize_root(&self.config.root.clone().unwrap_or_default());
//! debug!("backend use root {}", &root);
//!
//! Ok(DuckBackend { root })
24 changes: 12 additions & 12 deletions core/src/layers/immutable_index.rs
Original file line number Diff line number Diff line change
@@ -264,12 +264,12 @@ mod tests {
iil.insert(i.to_string())
}

let op = Operator::new(Http::from_map({
let op = Http::from_map({
let mut map = HashMap::new();
map.insert("endpoint".to_string(), "https://xuanwo.io".to_string());

map
}))?
})
.and_then(Operator::new)?
.layer(LoggingLayer::default())
.layer(iil)
.finish();
@@ -302,12 +302,12 @@ mod tests {
iil.insert(i.to_string())
}

let op = Operator::new(Http::from_map({
let op = Http::from_map({
let mut map = HashMap::new();
map.insert("endpoint".to_string(), "https://xuanwo.io".to_string());

map
}))?
})
.and_then(Operator::new)?
.layer(LoggingLayer::default())
.layer(iil)
.finish();
@@ -346,12 +346,12 @@ mod tests {
iil.insert(i.to_string())
}

let op = Operator::new(Http::from_map({
let op = Http::from_map({
let mut map = HashMap::new();
map.insert("endpoint".to_string(), "https://xuanwo.io".to_string());

map
}))?
})
.and_then(Operator::new)?
.layer(LoggingLayer::default())
.layer(iil)
.finish();
@@ -404,12 +404,12 @@ mod tests {
iil.insert(i.to_string())
}

let op = Operator::new(Http::from_map({
let op = Http::from_map({
let mut map = HashMap::new();
map.insert("endpoint".to_string(), "https://xuanwo.io".to_string());

map
}))?
})
.and_then(Operator::new)?
.layer(LoggingLayer::default())
.layer(iil)
.finish();
5 changes: 0 additions & 5 deletions core/src/layers/retry.rs
Original file line number Diff line number Diff line change
@@ -756,7 +756,6 @@ impl<P: oio::BlockingList, I: RetryInterceptor> oio::BlockingList for RetryWrapp

#[cfg(test)]
mod tests {
use std::collections::HashMap;
use std::sync::Arc;
use std::sync::Mutex;

@@ -782,10 +781,6 @@ mod tests {
Self::default()
}

fn from_map(_: HashMap<String, String>) -> Self {
Self::default()
}

fn build(&mut self) -> Result<Self::Accessor> {
Ok(MockService {
attempt: self.attempt.clone(),
2 changes: 1 addition & 1 deletion core/src/services/alluxio/backend.rs
Original file line number Diff line number Diff line change
@@ -259,7 +259,7 @@ mod test {
map.insert("root".to_string(), "/".to_string());
map.insert("endpoint".to_string(), "http://127.0.0.1:39999".to_string());

let builder = AlluxioBuilder::from_map(map);
let builder = AlluxioBuilder::from_map(map).unwrap();

assert_eq!(builder.config.root, Some("/".to_string()));
assert_eq!(
14 changes: 8 additions & 6 deletions core/src/types/builder.rs
Original file line number Diff line number Diff line change
@@ -48,10 +48,14 @@ pub trait Builder: Default {
fn from_config(config: Self::Config) -> Self;

/// Construct a builder from given map which contains several parameters needed by underlying service.
fn from_map(map: HashMap<String, String>) -> Self {
Self::Config::deserialize(ConfigDeserializer::new(map))
.map(Self::from_config)
.expect("config deserialize must succeed")
fn from_map(map: HashMap<String, String>) -> Result<Self> {
match Self::Config::deserialize(ConfigDeserializer::new(map)) {
Ok(config) => Ok(Self::from_config(config)),
Err(err) => Err(
Error::new(ErrorKind::ConfigInvalid, "failed to deserialize config")
.set_source(err),
),
}
}

/// Consume the accessor builder to build a service.
@@ -68,8 +72,6 @@ impl Builder for () {

fn from_config(_: Self::Config) -> Self {}

fn from_map(_: HashMap<String, String>) -> Self {}

fn build(&mut self) -> Result<Self::Accessor> {
Ok(())
}
2 changes: 1 addition & 1 deletion core/src/types/operator/builder.rs
Original file line number Diff line number Diff line change
@@ -112,7 +112,7 @@ impl Operator {
pub fn from_map<B: Builder>(
map: HashMap<String, String>,
) -> Result<OperatorBuilder<impl Access>> {
let acc = B::from_map(map).build()?;
let acc = B::from_map(map)?.build()?;
Ok(OperatorBuilder::new(acc))
}

2 changes: 1 addition & 1 deletion integrations/object_store/README.md
Original file line number Diff line number Diff line change
@@ -53,7 +53,7 @@ async fn main() {
]
.into_iter()
.collect(),
);
).unwrap();

// Create a new operator
let operator = Operator::new(builder).unwrap().finish();
3 changes: 2 additions & 1 deletion integrations/object_store/examples/basic.rs
Original file line number Diff line number Diff line change
@@ -18,7 +18,8 @@ async fn main() {
]
.into_iter()
.collect(),
);
)
.unwrap();

// Create a new operator
let operator = Operator::new(builder).unwrap().finish();
2 changes: 1 addition & 1 deletion integrations/object_store/src/lib.rs
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@
//! ]
//! .into_iter()
//! .collect(),
//! );
//! ).unwrap();
//!
//! // Create a new operator
//! let operator = Operator::new(builder).unwrap().finish();
2 changes: 1 addition & 1 deletion integrations/object_store/src/store.rs
Original file line number Diff line number Diff line change
@@ -68,7 +68,7 @@ use std::sync::Arc;
/// ]
/// .into_iter()
/// .collect(),
/// );
/// ).unwrap();
///
/// // Create a new operator
/// let operator = Operator::new(builder).unwrap().finish();