From 2681e92d4eb3d30542d9168bd70a35e765d2c8f9 Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Tue, 27 Sep 2022 22:58:25 +0800 Subject: [PATCH 1/5] Support MEILI_CONFIG_FILE_PATH to define config file path Close #2800 This is an alternative to the `--config-file-path` option. If both `--config-file-path` and `MEILI_CONFIG_FILE_PATH` are present, `--config-file-path` takes precedence according to the "Priority order" section of #2558. --- meilisearch-http/src/option.rs | 48 ++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/meilisearch-http/src/option.rs b/meilisearch-http/src/option.rs index a47a2b211..ca5fa7c1d 100644 --- a/meilisearch-http/src/option.rs +++ b/meilisearch-http/src/option.rs @@ -1,3 +1,4 @@ +use std::env; use std::fs; use std::io::{BufReader, Read}; use std::path::PathBuf; @@ -51,6 +52,7 @@ const MEILI_LOG_LEVEL: &str = "MEILI_LOG_LEVEL"; #[cfg(feature = "metrics")] const MEILI_ENABLE_METRICS_ROUTE: &str = "MEILI_ENABLE_METRICS_ROUTE"; +const DEFAULT_CONFIG_FILE_PATH: &str = "./config.yml"; const DEFAULT_DB_PATH: &str = "./data.ms"; const DEFAULT_HTTP_ADDR: &str = "127.0.0.1:7700"; const DEFAULT_ENV: &str = "development"; @@ -261,33 +263,33 @@ impl Opt { // Parse the args to get the config_file_path. let mut opts = Opt::parse(); let mut config_read_from = None; - if let Some(config_file_path) = opts + let config_file_path = opts .config_file_path .clone() - .or_else(|| Some(PathBuf::from("./config.toml"))) - { - match std::fs::read(&config_file_path) { - Ok(config) => { - // If the file is successfully read, we deserialize it with `toml`. - let opt_from_config = toml::from_slice::(&config)?; - // Return an error if config file contains 'config_file_path' - // Using that key in the config file doesn't make sense bc it creates a logical loop (config file referencing itself) - if opt_from_config.config_file_path.is_some() { - anyhow::bail!("`config_file_path` is not supported in config file") - } - // We inject the values from the toml in the corresponding env vars if needs be. Doing so, we respect the priority toml < env vars < cli args. - opt_from_config.export_to_env(); - // Once injected we parse the cli args once again to take the new env vars into scope. - opts = Opt::parse(); - config_read_from = Some(config_file_path); + .or_else(|| env::var("MEILI_CONFIG_FILE_PATH").map(PathBuf::from).ok()) + .unwrap_or_else(|| PathBuf::from(DEFAULT_CONFIG_FILE_PATH)); + + match std::fs::read(&config_file_path) { + Ok(config) => { + // If the file is successfully read, we deserialize it with `toml`. + let opt_from_config = toml::from_slice::(&config)?; + // Return an error if config file contains 'config_file_path' + // Using that key in the config file doesn't make sense bc it creates a logical loop (config file referencing itself) + if opt_from_config.config_file_path.is_some() { + anyhow::bail!("`config_file_path` is not supported in config file") } - // If we have an error while reading the file defined by the user. - Err(_) if opts.config_file_path.is_some() => anyhow::bail!( - "unable to open or read the {:?} configuration file.", - opts.config_file_path.unwrap().display().to_string() - ), - _ => (), + // We inject the values from the toml in the corresponding env vars if needs be. Doing so, we respect the priority toml < env vars < cli args. + opt_from_config.export_to_env(); + // Once injected we parse the cli args once again to take the new env vars into scope. + opts = Opt::parse(); + config_read_from = Some(config_file_path); } + // If we have an error while reading the file defined by the user. + Err(_) if opts.config_file_path.is_some() => anyhow::bail!( + "unable to open or read the {:?} configuration file.", + opts.config_file_path.unwrap().display().to_string() + ), + _ => (), } Ok((opts, config_read_from)) From 91accc0194eb077cdc3d61fb9855a21a606fb553 Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Tue, 11 Oct 2022 21:35:07 +0800 Subject: [PATCH 2/5] Fix default config file path typo --- meilisearch-http/src/option.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meilisearch-http/src/option.rs b/meilisearch-http/src/option.rs index ca5fa7c1d..fcd0f4529 100644 --- a/meilisearch-http/src/option.rs +++ b/meilisearch-http/src/option.rs @@ -52,7 +52,7 @@ const MEILI_LOG_LEVEL: &str = "MEILI_LOG_LEVEL"; #[cfg(feature = "metrics")] const MEILI_ENABLE_METRICS_ROUTE: &str = "MEILI_ENABLE_METRICS_ROUTE"; -const DEFAULT_CONFIG_FILE_PATH: &str = "./config.yml"; +const DEFAULT_CONFIG_FILE_PATH: &str = "./config.toml"; const DEFAULT_DB_PATH: &str = "./data.ms"; const DEFAULT_HTTP_ADDR: &str = "127.0.0.1:7700"; const DEFAULT_ENV: &str = "development"; From 3c3ae3ff9838cd7cd56b19b4c358696db5069352 Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Wed, 12 Oct 2022 11:58:28 +0800 Subject: [PATCH 3/5] Impeove invalid config_file_path handling 1. Besides opt.config_file_path, also consider MEILI_CONFIG_FILE_PATH in the Err path because they are both user input. 2. Print out the incorrect file path in error message. 3. Add tests https://github.com/meilisearch/meilisearch/pull/2804#discussion_r991999888 --- Cargo.lock | 10 ++++++++ meilisearch-http/Cargo.toml | 1 + meilisearch-http/src/option.rs | 46 ++++++++++++++++++++++++++++------ 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b26636a01..5e32e9afa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2095,6 +2095,7 @@ dependencies = [ "static-files", "sysinfo", "tar", + "temp-env", "tempfile", "thiserror", "time", @@ -3451,6 +3452,15 @@ dependencies = [ "xattr", ] +[[package]] +name = "temp-env" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a30d48359f77fbb6af3d7b928cc2d092e1dc90b44f397e979ef08ae15733ed65" +dependencies = [ + "once_cell", +] + [[package]] name = "tempfile" version = "3.3.0" diff --git a/meilisearch-http/Cargo.toml b/meilisearch-http/Cargo.toml index cabb6ed48..11d142eb6 100644 --- a/meilisearch-http/Cargo.toml +++ b/meilisearch-http/Cargo.toml @@ -89,6 +89,7 @@ manifest-dir-macros = "0.1.16" maplit = "1.0.2" urlencoding = "2.1.2" yaup = "0.2.1" +temp-env = "0.3.1" [features] default = ["analytics", "meilisearch-lib/default", "mini-dashboard"] diff --git a/meilisearch-http/src/option.rs b/meilisearch-http/src/option.rs index fcd0f4529..ddae59c5a 100644 --- a/meilisearch-http/src/option.rs +++ b/meilisearch-http/src/option.rs @@ -263,10 +263,12 @@ impl Opt { // Parse the args to get the config_file_path. let mut opts = Opt::parse(); let mut config_read_from = None; - let config_file_path = opts + let user_specified_config_file_path = opts .config_file_path .clone() - .or_else(|| env::var("MEILI_CONFIG_FILE_PATH").map(PathBuf::from).ok()) + .or_else(|| env::var("MEILI_CONFIG_FILE_PATH").map(PathBuf::from).ok()); + let config_file_path = user_specified_config_file_path + .clone() .unwrap_or_else(|| PathBuf::from(DEFAULT_CONFIG_FILE_PATH)); match std::fs::read(&config_file_path) { @@ -284,12 +286,17 @@ impl Opt { opts = Opt::parse(); config_read_from = Some(config_file_path); } - // If we have an error while reading the file defined by the user. - Err(_) if opts.config_file_path.is_some() => anyhow::bail!( - "unable to open or read the {:?} configuration file.", - opts.config_file_path.unwrap().display().to_string() - ), - _ => (), + Err(e) => { + match user_specified_config_file_path { + // If we have an error while reading the file defined by the user. + Some(path) => anyhow::bail!( + "unable to open or read the {:?} configuration file: {}.", + path, + e, + ), + None => (), + } + } } Ok((opts, config_read_from)) @@ -529,4 +536,27 @@ mod test { fn test_valid_opt() { assert!(Opt::try_parse_from(Some("")).is_ok()); } + + #[test] + fn test_meilli_config_file_path_valid() { + temp_env::with_vars( + vec![("MEILI_CONFIG_FILE_PATH", Some("../config.toml"))], // Relative path in meilisearch_http package + || { + assert!(Opt::try_build().is_ok()); + }, + ); + } + + #[test] + fn test_meilli_config_file_path_invalid() { + temp_env::with_vars( + vec![("MEILI_CONFIG_FILE_PATH", Some("../configgg.toml"))], + || { + assert_eq!( + Opt::try_build().unwrap_err().to_string(), + "unable to open or read the \"../configgg.toml\" configuration file: No such file or directory (os error 2)." + ); + }, + ); + } } From 9ebc73e6ac4b4b21b5bca0332f641b5eb66276de Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Fri, 14 Oct 2022 14:16:10 +0800 Subject: [PATCH 4/5] Comply with Clippy rule single-match --- meilisearch-http/src/option.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/meilisearch-http/src/option.rs b/meilisearch-http/src/option.rs index ddae59c5a..b02b33758 100644 --- a/meilisearch-http/src/option.rs +++ b/meilisearch-http/src/option.rs @@ -287,14 +287,13 @@ impl Opt { config_read_from = Some(config_file_path); } Err(e) => { - match user_specified_config_file_path { + if let Some(path) = user_specified_config_file_path { // If we have an error while reading the file defined by the user. - Some(path) => anyhow::bail!( + anyhow::bail!( "unable to open or read the {:?} configuration file: {}.", path, e, - ), - None => (), + ) } } } From 53e5229b4aefccbc691ac13fc768347463d65eb7 Mon Sep 17 00:00:00 2001 From: Lawrence Chou Date: Fri, 14 Oct 2022 14:49:31 +0800 Subject: [PATCH 5/5] Assert error message for Windows besides *nix The 'Tests on windows-latest' now failed with error message below ---- option::test::test_meilli_config_file_path_invalid stdout ---- thread 'option::test::test_meilli_config_file_path_invalid' panicked at 'assertion failed: left: `"unable to open or read the \"../configgg.toml\" configuration file: The system cannot find the file specified. (os error 2)."`, right: `"unable to open or read the \"../configgg.toml\" configuration file: No such file or directory (os error 2)."`', meilisearch-http\src\option.rs:555:17 https://github.com/meilisearch/meilisearch/actions/runs/3231941308/jobs/5291998750 --- meilisearch-http/src/option.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/meilisearch-http/src/option.rs b/meilisearch-http/src/option.rs index b02b33758..c23c3e69a 100644 --- a/meilisearch-http/src/option.rs +++ b/meilisearch-http/src/option.rs @@ -529,6 +529,7 @@ fn default_log_level() -> String { #[cfg(test)] mod test { + use super::*; #[test] @@ -551,9 +552,16 @@ mod test { temp_env::with_vars( vec![("MEILI_CONFIG_FILE_PATH", Some("../configgg.toml"))], || { - assert_eq!( - Opt::try_build().unwrap_err().to_string(), - "unable to open or read the \"../configgg.toml\" configuration file: No such file or directory (os error 2)." + let possible_error_messages = [ + "unable to open or read the \"../configgg.toml\" configuration file: No such file or directory (os error 2).", + "unable to open or read the \"../configgg.toml\" configuration file: The system cannot find the file specified. (os error 2).", // Windows + ]; + let error_message = Opt::try_build().unwrap_err().to_string(); + assert!( + possible_error_messages.contains(&error_message.as_str()), + "Expected onf of {:?}, got {:?}.", + possible_error_messages, + error_message ); }, );