From da6a21eac969c96005f2b5b4deb0b1ea91926951 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 14 May 2024 10:22:56 +0100 Subject: [PATCH] refactor: [#855] show toml file location in Figment errors Before this commit we loaded configuration in Figment always using a Toml string even if the configuration cames from a toml file. WHen there is an error Figment does not show the file location and that's one of the main advantages of using Figment. All errors point to the primary source of the configuration option. This commit fixes that problem leting Figment to load the configuration from the file when the user provides a file. Sample error: ``` Loading configuration from default configuration file: `./share/default/config/tracker.development.sqlite3.toml` ... thread 'main' panicked at src/bootstrap/config.rs:45:32: called `Result::unwrap()` on an `Err` value: ConfigError { source: LocatedError { source: Error { tag: Tag(Default, 2), profile: Some(Profile(Uncased { string: "default" })), metadata: Some(Metadata { name: "TOML file", source: Some(File("/home/developer/torrust/torrust-tracker/./share/default/config/tracker.development.sqlite3.toml")), provide_location: Some(Location { file: "packages/configuration/src/v1/mod.rs", line: 330, col: 18 }), interpolater: }), path: [], kind: Message("TOML parse error at line 2, column 15\n |\n2 | enabled = truee\n | ^\nexpected newline, `#`\n"), prev: None }, location: Location { file: "packages/configuration/src/v1/mod.rs", line: 334, col: 41 } } } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` Notice how the file path is included is the error: `/home/developer/torrust/torrust-tracker/./share/default/config/tracker.development.sqlite3.toml` --- packages/configuration/src/lib.rs | 47 ++++++++---------- packages/configuration/src/v1/mod.rs | 71 +++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 39 deletions(-) diff --git a/packages/configuration/src/lib.rs b/packages/configuration/src/lib.rs index c393623d..9a00e6bb 100644 --- a/packages/configuration/src/lib.rs +++ b/packages/configuration/src/lib.rs @@ -7,8 +7,8 @@ pub mod v1; use std::collections::HashMap; +use std::env; use std::sync::Arc; -use std::{env, fs}; use camino::Utf8PathBuf; use derive_more::Constructor; @@ -38,7 +38,8 @@ pub struct TrackerPolicy { /// Information required for loading config #[derive(Debug, Default, Clone)] pub struct Info { - tracker_toml: String, + config_toml: Option, + config_toml_path: String, api_admin_token: Option, } @@ -51,38 +52,30 @@ impl Info { /// #[allow(clippy::needless_pass_by_value)] pub fn new( - env_var_config: String, - env_var_path_config: String, - default_path_config: String, + env_var_config_toml: String, + env_var_config_toml_path: String, + default_config_toml_path: String, env_var_api_admin_token: String, ) -> Result { - let tracker_toml = if let Ok(tracker_toml) = env::var(&env_var_config) { - println!("Loading configuration from env var {env_var_config} ..."); + let config_toml = if let Ok(config_toml) = env::var(env_var_config_toml) { + println!("Loading configuration from environment variable {config_toml} ..."); + Some(config_toml) + } else { + None + }; - tracker_toml + let config_toml_path = if let Ok(config_toml_path) = env::var(env_var_config_toml_path) { + println!("Loading configuration from file: `{config_toml_path}` ..."); + config_toml_path } else { - let config_path = if let Ok(config_path) = env::var(env_var_path_config) { - println!("Loading configuration file: `{config_path}` ..."); - - config_path - } else { - println!("Loading default configuration file: `{default_path_config}` ..."); - - default_path_config - }; - - fs::read_to_string(config_path) - .map_err(|e| Error::UnableToLoadFromConfigFile { - source: (Arc::new(e) as DynError).into(), - })? - .parse() - .map_err(|_e: std::convert::Infallible| Error::Infallible)? + println!("Loading configuration from default configuration file: `{default_config_toml_path}` ..."); + default_config_toml_path }; - let api_admin_token = env::var(env_var_api_admin_token).ok(); Ok(Self { - tracker_toml, - api_admin_token, + config_toml, + config_toml_path, + api_admin_token: env::var(env_var_api_admin_token).ok(), }) } } diff --git a/packages/configuration/src/v1/mod.rs b/packages/configuration/src/v1/mod.rs index d19fedc8..8e15d65c 100644 --- a/packages/configuration/src/v1/mod.rs +++ b/packages/configuration/src/v1/mod.rs @@ -250,6 +250,11 @@ use self::tracker_api::HttpApi; use self::udp_tracker::UdpTracker; use crate::{Error, Info}; +/// Prefix for env vars that overwrite configuration options. +const CONFIG_OVERRIDE_PREFIX: &str = "TORRUST_TRACKER_CONFIG_OVERRIDE_"; +/// Path separator in env var names for nested values in configuration. +const CONFIG_OVERRIDE_SEPARATOR: &str = "__"; + /// Core configuration for the tracker. #[derive(Serialize, Deserialize, PartialEq, Eq, Debug)] pub struct Configuration { @@ -315,9 +320,16 @@ impl Configuration { /// /// Will return `Err` if the environment variable does not exist or has a bad configuration. pub fn load(info: &Info) -> Result { - let figment = Figment::from(Serialized::defaults(Configuration::default())) - .merge(Toml::string(&info.tracker_toml)) - .merge(Env::prefixed("TORRUST_TRACKER__").split("__")); + let figment = if let Some(config_toml) = &info.config_toml { + // Config in env var has priority over config file path + Figment::from(Serialized::defaults(Configuration::default())) + .merge(Toml::string(config_toml)) + .merge(Env::prefixed(CONFIG_OVERRIDE_PREFIX).split(CONFIG_OVERRIDE_SEPARATOR)) + } else { + Figment::from(Serialized::defaults(Configuration::default())) + .merge(Toml::file(&info.config_toml_path)) + .merge(Env::prefixed(CONFIG_OVERRIDE_PREFIX).split(CONFIG_OVERRIDE_SEPARATOR)) + }; let mut config: Configuration = figment.extract()?; @@ -449,11 +461,14 @@ mod tests { #[test] fn configuration_should_use_the_default_values_when_an_empty_configuration_is_provided_by_the_user() { - figment::Jail::expect_with(|_jail| { + figment::Jail::expect_with(|jail| { + jail.create_file("tracker.toml", "")?; + let empty_configuration = String::new(); let info = Info { - tracker_toml: empty_configuration, + config_toml: Some(empty_configuration), + config_toml_path: "tracker.toml".to_string(), api_admin_token: None, }; @@ -466,28 +481,59 @@ mod tests { } #[test] - fn configuration_should_be_loaded_from_a_toml_config_file() { + fn default_configuration_could_be_overwritten_from_a_single_env_var_with_toml_contents() { figment::Jail::expect_with(|_jail| { + let config_toml = r#" + db_path = "OVERWRITTEN DEFAULT DB PATH" + "# + .to_string(); + let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(config_toml), + config_toml_path: String::new(), api_admin_token: None, }; let configuration = Configuration::load(&info).expect("Could not load configuration from file"); - assert_eq!(configuration, Configuration::default()); + assert_eq!(configuration.core.db_path, "OVERWRITTEN DEFAULT DB PATH".to_string()); + + Ok(()) + }); + } + + #[test] + fn default_configuration_could_be_overwritten_from_a_toml_config_file() { + figment::Jail::expect_with(|jail| { + jail.create_file( + "tracker.toml", + r#" + db_path = "OVERWRITTEN DEFAULT DB PATH" + "#, + )?; + + let info = Info { + config_toml: None, + config_toml_path: "tracker.toml".to_string(), + api_admin_token: None, + }; + + let configuration = Configuration::load(&info).expect("Could not load configuration from file"); + + assert_eq!(configuration.core.db_path, "OVERWRITTEN DEFAULT DB PATH".to_string()); Ok(()) }); } #[test] - fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_env_var() { + fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_an_env_var() { figment::Jail::expect_with(|jail| { - jail.set_env("TORRUST_TRACKER__HTTP_API__ACCESS_TOKENS__ADMIN", "NewToken"); + jail.set_env("TORRUST_TRACKER_CONFIG_OVERRIDE_HTTP_API__ACCESS_TOKENS__ADMIN", "NewToken"); let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(default_config_toml()), + config_toml_path: String::new(), api_admin_token: None, }; @@ -506,7 +552,8 @@ mod tests { fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_the_deprecated_env_var_name() { figment::Jail::expect_with(|_jail| { let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(default_config_toml()), + config_toml_path: String::new(), api_admin_token: Some("NewToken".to_owned()), };