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

Signed-off-by: tison <wander4096@gmail.com>
apache · Jul 20, 2024 · 1ad5b7a · 1ad5b7a
1 parent 6f7ed13
commit 1ad5b7a
Show file tree

Hide file tree

Showing 10 changed files with 41 additions and 39 deletions.
diff --git a/core/src/docs/internals/accessor.rs b/core/src/docs/internals/accessor.rs
@@ -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 })

diff --git a/core/src/layers/immutable_index.rs b/core/src/layers/immutable_index.rs
@@ -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();

diff --git a/core/src/layers/retry.rs b/core/src/layers/retry.rs
@@ -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(),

diff --git a/core/src/services/alluxio/backend.rs b/core/src/services/alluxio/backend.rs
@@ -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!(

diff --git a/core/src/types/builder.rs b/core/src/types/builder.rs
@@ -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(())
     }

diff --git a/core/src/types/operator/builder.rs b/core/src/types/operator/builder.rs
@@ -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))
     }
 

diff --git a/integrations/object_store/README.md b/integrations/object_store/README.md
@@ -53,7 +53,7 @@ async fn main() {
         ]
         .into_iter()
         .collect(),
-    );
+    ).unwrap();
 
     // Create a new operator
     let operator = Operator::new(builder).unwrap().finish();

diff --git a/integrations/object_store/examples/basic.rs b/integrations/object_store/examples/basic.rs
@@ -18,7 +18,8 @@ async fn main() {
         ]
         .into_iter()
         .collect(),
-    );
+    )
+    .unwrap();
 
     // Create a new operator
     let operator = Operator::new(builder).unwrap().finish();

diff --git a/integrations/object_store/src/lib.rs b/integrations/object_store/src/lib.rs
@@ -40,7 +40,7 @@
 //!         ]
 //!         .into_iter()
 //!         .collect(),
-//!     );
+//!     ).unwrap();
 //!
 //!     // Create a new operator
 //!     let operator = Operator::new(builder).unwrap().finish();

diff --git a/integrations/object_store/src/store.rs b/integrations/object_store/src/store.rs
@@ -68,7 +68,7 @@ use std::sync::Arc;
 ///         ]
 ///         .into_iter()
 ///         .collect(),
-///     );
+///     ).unwrap();
 ///
 ///     // Create a new operator
 ///     let operator = Operator::new(builder).unwrap().finish();