Merge pull request #689 from imobachgs/improve-error-handling

Improve error handling

rust/agama-cli/src/config.rs

@@ -34,7 +34,8 @@ pub enum ConfigAction {
 pub async fn run(subcommand: ConfigCommands, format: Format) -> anyhow::Result<()> {
     let store = SettingsStore::new(connection().await?).await?;
 
-    match parse_config_command(subcommand) {
+    let command = parse_config_command(subcommand)?;
+    match command {
         ConfigAction::Set(changes) => {
             let scopes = changes
                 .keys()
@@ -68,26 +69,56 @@ pub async fn run(subcommand: ConfigCommands, format: Format) -> anyhow::Result<(
     }
 }
 
-fn parse_config_command(subcommand: ConfigCommands) -> ConfigAction {
+fn parse_config_command(subcommand: ConfigCommands) -> Result<ConfigAction, CliError> {
     match subcommand {
-        ConfigCommands::Add { key, values } => ConfigAction::Add(key, parse_keys_values(values)),
-        ConfigCommands::Show => ConfigAction::Show,
-        ConfigCommands::Set { values } => ConfigAction::Set(parse_keys_values(values)),
-        ConfigCommands::Load { path } => ConfigAction::Load(path),
+        ConfigCommands::Add { key, values } => {
+            Ok(ConfigAction::Add(key, parse_keys_values(values)?))
+        }
+        ConfigCommands::Show => Ok(ConfigAction::Show),
+        ConfigCommands::Set { values } => Ok(ConfigAction::Set(parse_keys_values(values)?)),
+        ConfigCommands::Load { path } => Ok(ConfigAction::Load(path)),
     }
 }
 
-fn parse_keys_values(keys_values: Vec<String>) -> HashMap<String, String> {
-    keys_values
-        .iter()
-        .filter_map(|s| {
-            if let Some((key, value)) = s.split_once('=') {
-                Some((key.to_string(), value.to_string()))
-            } else {
-                None
-            }
-        })
-        .collect()
+/// Split the elements on '=' to make a hash of them.
+fn parse_keys_values(keys_values: Vec<String>) -> Result<HashMap<String, String>, CliError> {
+    let mut changes = HashMap::new();
+    for s in keys_values {
+        let Some((key, value)) = s.split_once('=') else {
+            return Err(CliError::MissingSeparator(s));
+        };
+        changes.insert(key.to_string(), value.to_string());
+    }
+    Ok(changes)
+}
+
+#[test]
+fn test_parse_keys_values() {
+    // happy path, make a hash out of the vec
+    let happy_in = vec!["one=first".to_string(), "two=second".to_string()];
+    let happy_out = HashMap::from([
+        ("one".to_string(), "first".to_string()),
+        ("two".to_string(), "second".to_string()),
+    ]);
+    let r = parse_keys_values(happy_in);
+    assert!(r.is_ok());
+    assert_eq!(r.unwrap(), happy_out);
+
+    // an empty list is fine
+    let empty_vec = Vec::<String>::new();
+    let empty_hash = HashMap::<String, String>::new();
+    let r = parse_keys_values(empty_vec);
+    assert!(r.is_ok());
+    assert_eq!(r.unwrap(), empty_hash);
+
+    // an empty member fails
+    let empty_string = vec!["".to_string(), "two=second".to_string()];
+    let r = parse_keys_values(empty_string);
+    assert!(r.is_err());
+    assert_eq!(
+        format!("{}", r.unwrap_err()),
+        "Missing the '=' separator in ''"
+    );
 }
 
 fn key_to_scope(key: &str) -> Result<Scope, Box<dyn Error>> {

rust/agama-cli/src/error.rs

@@ -8,4 +8,6 @@ pub enum CliError {
     ValidationError,
     #[error("Could not start the installation")]
     InstallationError,
+    #[error("Missing the '=' separator in '{0}'")]
+    MissingSeparator(String),
 }

rust/agama-derive/src/lib.rs

@@ -1,7 +1,37 @@
+//! Implements a derive macro to implement the Settings from the `agama_settings` crate.
+//!
+//! ```no_compile
+//! use agama_settings::{Settings, settings::Settings};
+//!
+//! #[derive(Default, Settings)]
+//! struct UserSettings {
+//!   name: Option<String>,
+//!   enabled: Option<bool>
+//! }
+//!
+//! #[derive(Default, Settings)]
+//! struct InstallSettings {
+//!   #[settings(nested, alias = "first_user")]
+//!   user: Option<UserSettings>,
+//!   reboot: Option<bool>
+//!   product: Option<String>,
+//!   #[settings(collection)]
+//!   packages: Vec<String>
+//! }
+//!
+//! ## Supported attributes
+//!
+//! * `nested`: the field is another struct that implements `Settings`.
+//! * `collection`: the attribute is a vector of elements of type T. You might need to implement
+//!   `TryFrom<SettingObject> for T` for your custom types.
+//! * `flatten`: the field is flatten (in serde jargon).
+//! * `alias`: and alternative name for the field. It can be specified several times.
+//! ```
+
 use proc_macro::TokenStream;
 use proc_macro2::TokenStream as TokenStream2;
 use quote::quote;
-use syn::{parse_macro_input, DeriveInput, Fields};
+use syn::{parse_macro_input, DeriveInput, Fields, LitStr};
 
 #[derive(Debug, Clone, Copy, PartialEq)]
 enum SettingKind {
@@ -20,6 +50,21 @@ struct SettingField {
     pub ident: syn::Ident,
     /// Setting kind (scalar, collection, struct).
     pub kind: SettingKind,
+    /// Whether it is a flatten (in serde jargon) value.
+    pub flatten: bool,
+    /// Aliases for the field (especially useful for flatten fields).
+    pub aliases: Vec<String>,
+}
+
+impl SettingField {
+    pub fn new(ident: syn::Ident) -> Self {
+        Self {
+            ident,
+            kind: SettingKind::Scalar,
+            flatten: false,
+            aliases: vec![],
+        }
+    }
 }
 
 /// List of setting fields
@@ -74,12 +119,13 @@ pub fn agama_attributes_derive(input: TokenStream) -> TokenStream {
 }
 
 fn expand_set_fn(settings: &SettingFieldsList) -> TokenStream2 {
-    if settings.is_empty() {
+    let scalar_fields = settings.by_type(SettingKind::Scalar);
+    let nested_fields = settings.by_type(SettingKind::Nested);
+    if scalar_fields.is_empty() && nested_fields.is_empty() {
         return quote! {};
     }
 
-    let mut scalar_handling = quote! {};
-    let scalar_fields = settings.by_type(SettingKind::Scalar);
+    let mut scalar_handling = quote! { Ok(()) };
     if !scalar_fields.is_empty() {
         let field_name = scalar_fields.iter().map(|s| s.ident.clone());
         scalar_handling = quote! {
@@ -89,19 +135,23 @@ fn expand_set_fn(settings: &SettingFieldsList) -> TokenStream2 {
                 })?,)*
                 _ => return Err(agama_settings::SettingsError::UnknownAttribute(attr.to_string()))
             }
+            Ok(())
         }
     }
 
     let mut nested_handling = quote! {};
-    let nested_fields = settings.by_type(SettingKind::Nested);
     if !nested_fields.is_empty() {
         let field_name = nested_fields.iter().map(|s| s.ident.clone());
+        let aliases = quote_fields_aliases(&nested_fields);
+        let attr = nested_fields
+            .iter()
+            .map(|s| if s.flatten { quote!(attr) } else { quote!(id) });
         nested_handling = quote! {
             if let Some((ns, id)) = attr.split_once('.') {
                 match ns {
-                    #(stringify!(#field_name) => {
+                    #(stringify!(#field_name) #aliases => {
                         let #field_name = self.#field_name.get_or_insert(Default::default());
-                        #field_name.set(id, value).map_err(|e| e.with_attr(attr))?
+                        #field_name.set(#attr, value).map_err(|e| e.with_attr(attr))?
                     })*
                     _ => return Err(agama_settings::SettingsError::UnknownAttribute(attr.to_string()))
                 }
@@ -114,7 +164,6 @@ fn expand_set_fn(settings: &SettingFieldsList) -> TokenStream2 {
          fn set(&mut self, attr: &str, value: agama_settings::SettingValue) -> Result<(), agama_settings::SettingsError> {
             #nested_handling
             #scalar_handling
-            Ok(())
          }
     }
 }
@@ -155,12 +204,13 @@ fn expand_merge_fn(settings: &SettingFieldsList) -> TokenStream2 {
 }
 
 fn expand_add_fn(settings: &SettingFieldsList) -> TokenStream2 {
-    if settings.is_empty() {
+    let collection_fields = settings.by_type(SettingKind::Collection);
+    let nested_fields = settings.by_type(SettingKind::Nested);
+    if collection_fields.is_empty() && nested_fields.is_empty() {
         return quote! {};
     }
 
-    let mut collection_handling = quote! {};
-    let collection_fields = settings.by_type(SettingKind::Collection);
+    let mut collection_handling = quote! { Ok(()) };
     if !collection_fields.is_empty() {
         let field_name = collection_fields.iter().map(|s| s.ident.clone());
         collection_handling = quote! {
@@ -173,11 +223,11 @@ fn expand_add_fn(settings: &SettingFieldsList) -> TokenStream2 {
                 },)*
                 _ => return Err(agama_settings::SettingsError::UnknownAttribute(attr.to_string()))
             }
+            Ok(())
         };
     }
 
     let mut nested_handling = quote! {};
-    let nested_fields = settings.by_type(SettingKind::Nested);
     if !nested_fields.is_empty() {
         let field_name = nested_fields.iter().map(|s| s.ident.clone());
         nested_handling = quote! {
@@ -193,12 +243,10 @@ fn expand_add_fn(settings: &SettingFieldsList) -> TokenStream2 {
             }
         }
     }
-
     quote! {
         fn add(&mut self, attr: &str, value: agama_settings::SettingObject) -> Result<(), agama_settings::SettingsError> {
             #nested_handling
             #collection_handling
-            Ok(())
         }
     }
 }
@@ -210,15 +258,13 @@ fn expand_add_fn(settings: &SettingFieldsList) -> TokenStream2 {
 fn parse_setting_fields(fields: Vec<&syn::Field>) -> SettingFieldsList {
     let mut settings = vec![];
     for field in fields {
-        let mut setting = SettingField {
-            ident: field.ident.clone().expect("to find a field ident"),
-            kind: SettingKind::Scalar,
-        };
-
+        let ident = field.ident.clone().expect("to find a field ident");
+        let mut setting = SettingField::new(ident);
         for attr in &field.attrs {
             if !attr.path().is_ident("settings") {
                 continue;
             }
+
             attr.parse_nested_meta(|meta| {
                 if meta.path.is_ident("collection") {
                     setting.kind = SettingKind::Collection;
@@ -228,6 +274,16 @@ fn parse_setting_fields(fields: Vec<&syn::Field>) -> SettingFieldsList {
                     setting.kind = SettingKind::Nested;
                 }
 
+                if meta.path.is_ident("flatten") {
+                    setting.flatten = true;
+                }
+
+                if meta.path.is_ident("alias") {
+                    let value = meta.value()?;
+                    let alias: LitStr = value.parse()?;
+                    setting.aliases.push(alias.value());
+                }
+
                 Ok(())
             })
             .expect("settings arguments do not follow the expected structure");
@@ -236,3 +292,17 @@ fn parse_setting_fields(fields: Vec<&syn::Field>) -> SettingFieldsList {
     }
     SettingFieldsList(settings)
 }
+
+fn quote_fields_aliases(nested_fields: &Vec<&SettingField>) -> Vec<TokenStream2> {
+    nested_fields
+        .iter()
+        .map(|f| {
+            let aliases = f.aliases.clone();
+            if aliases.is_empty() {
+                quote! {}
+            } else {
+                quote! { #(| #aliases)* }
+            }
+        })
+        .collect()
+}

rust/agama-lib/src/install_settings.rs

@@ -62,7 +62,7 @@ impl FromStr for Scope {
 #[serde(rename_all = "camelCase")]
 pub struct InstallSettings {
     #[serde(default, flatten)]
-    #[settings(nested)]
+    #[settings(nested, flatten, alias = "root")]
     pub user: Option<UserSettings>,
     #[serde(default)]
     #[settings(nested)]

rust/agama-lib/src/users/settings.rs

@@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize};
 #[serde(rename_all = "camelCase")]
 pub struct UserSettings {
     #[serde(rename = "user")]
-    #[settings(nested)]
+    #[settings(nested, alias = "user")]
     pub first_user: Option<FirstUserSettings>,
     #[settings(nested)]
     pub root: Option<RootUserSettings>,

rust/agama-settings/src/lib.rs

@@ -1,3 +1,60 @@
+//! This module offers a mechanism to easily map the values from the command
+//! line to proper installation settings.
+//!
+//! In Agama, the installation settings are modeled using structs with optional fields and vectors.
+//! To specify a value in the command line, the user needs to specify:
+//!
+//! * a setting ID (`"users.name"`, `"storage.lvm"`, and so on), that must be used to find the
+//!   setting.
+//! * a value, which is captured as a string (`"Foo Bar"`, `"true"`, etc.) and it should be
+//!   converted to the proper type.
+//!
+//! Implementing the [Settings](crate::settings::Settings) trait adds support for setting the value
+//! in an straightforward way, taking care of the conversions automatically. The newtype
+//! [SettingValue] takes care of such a conversion.
+//!
+//! ## Example
+//!
+//! The best way to understand how it works is to see it in action. In the example below, there is
+//! a simplified `InstallSettings` struct that is composed by the user settings, which is another
+//! struct, and a boolean field.
+//!
+//! In this case, the trait is automatically derived, implementing a `set` method that allows
+//! setting configuration value by specifying:
+//!
+//! * An ID, like `users.name`.
+//! * A string-based value, which is automatically converted to the corresponding type in the
+//! struct.
+//!
+//! ```
+//! use agama_settings::{Settings, settings::{SettingValue, Settings}};
+//!
+//! #[derive(Default, Settings)]
+//! struct UserSettings {
+//!   name: Option<String>,
+//!   enabled: Option<bool>
+//! }
+//!
+//! #[derive(Default, Settings)]
+//! struct InstallSettings {
+//!   #[settings(nested)]
+//!   user: Option<UserSettings>,
+//!   reboot: Option<bool>
+//! }
+//!
+//! let user = UserSettings { name: Some(String::from("foo")), enabled: Some(false) };
+//! let mut settings = InstallSettings { user: Some(user), reboot: None };
+//!
+//! settings.set("user.name", SettingValue("foo.bar".to_string()));
+//! settings.set("user.enabled", SettingValue("true".to_string()));
+//! settings.set("reboot", SettingValue("true".to_string()));
+//!
+//! let user = settings.user.unwrap();
+//! assert_eq!(user.name, Some("foo.bar".to_string()));
+//! assert_eq!(user.enabled, Some(true));
+//! assert_eq!(settings.reboot, Some(true));
+//! ```
+
 pub mod error;
 pub mod settings;