Skip to content

Commit

Permalink
refactor: [#855] show toml file location in Figment errors
Browse files Browse the repository at this point in the history
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`
  • Loading branch information
josecelano committed May 14, 2024
1 parent 92408bc commit da6a21e
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 39 deletions.
47 changes: 20 additions & 27 deletions packages/configuration/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String>,
config_toml_path: String,

Check warning on line 42 in packages/configuration/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

packages/configuration/src/lib.rs#L41-L42

Added lines #L41 - L42 were not covered by tests
api_admin_token: Option<String>,
}

Expand All @@ -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<Self, Error> {
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)

Check warning on line 62 in packages/configuration/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

packages/configuration/src/lib.rs#L61-L62

Added lines #L61 - L62 were not covered by tests
} 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

Check warning on line 69 in packages/configuration/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

packages/configuration/src/lib.rs#L68-L69

Added lines #L68 - L69 were not covered by tests
} 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(),
})
}
}
Expand Down
71 changes: 59 additions & 12 deletions packages/configuration/src/v1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<Configuration, Error> {
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()?;

Expand Down Expand Up @@ -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,
};

Expand All @@ -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,
};

Expand All @@ -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()),
};

Expand Down

0 comments on commit da6a21e

Please sign in to comment.