diff --git a/src/rust/engine/options/src/args.rs b/src/rust/engine/options/src/args.rs index c3b14b36fbc..faa29f746b8 100644 --- a/src/rust/engine/options/src/args.rs +++ b/src/rust/engine/options/src/args.rs @@ -228,17 +228,22 @@ impl OptionsSource for Args { self.get_list::(id) } - fn get_dict(&self, id: &OptionId) -> Result, String> { - // We iterate in reverse so that the rightmost arg wins in case an option - // is specified multiple times. - for arg in self.args.iter().rev() { + fn get_dict(&self, id: &OptionId) -> Result>, String> { + let mut edits = vec![]; + for arg in self.args.iter() { if arg.matches(id) { - return expand_to_dict(arg.value.clone().ok_or_else(|| { - format!("Expected list option {} to have a value.", self.display(id)) - })?) - .map_err(|e| e.render(&arg.flag)); + let value = arg.value.clone().ok_or_else(|| { + format!("Expected dict option {} to have a value.", self.display(id)) + })?; + if let Some(es) = expand_to_dict(value).map_err(|e| e.render(&arg.flag))? { + edits.extend(es); + } } } - Ok(None) + if edits.is_empty() { + Ok(None) + } else { + Ok(Some(edits)) + } } } diff --git a/src/rust/engine/options/src/args_tests.rs b/src/rust/engine/options/src/args_tests.rs index 9074d0b0b28..fe81a7c0f53 100644 --- a/src/rust/engine/options/src/args_tests.rs +++ b/src/rust/engine/options/src/args_tests.rs @@ -237,6 +237,22 @@ fn test_list_fromfile() { }], "fromfile.txt", ); + do_test( + "+[-42]", + &[ListEdit { + action: ListEditAction::Add, + items: vec![-42], + }], + "fromfile.txt", + ); + do_test( + "[-42]", + &[ListEdit { + action: ListEditAction::Replace, + items: vec![-42], + }], + "fromfile.txt", + ); do_test( "[10, 12]", &[ListEdit { @@ -258,20 +274,31 @@ fn test_list_fromfile() { #[test] fn test_dict_fromfile() { fn do_test(content: &str, filename: &str) { - let expected = DictEdit { - action: DictEditAction::Replace, - items: hashmap! { - "FOO".to_string() => Val::Dict(hashmap! { - "BAR".to_string() => Val::Float(3.14), - "BAZ".to_string() => Val::Dict(hashmap! { - "QUX".to_string() => Val::Bool(true), - "QUUX".to_string() => Val::List(vec![ Val::Int(1), Val::Int(2)]) - }) - }),}, - }; + let expected = vec![ + DictEdit { + action: DictEditAction::Replace, + items: hashmap! { + "FOO".to_string() => Val::Dict(hashmap! { + "BAR".to_string() => Val::Float(3.14), + "BAZ".to_string() => Val::Dict(hashmap! { + "QUX".to_string() => Val::Bool(true), + "QUUX".to_string() => Val::List(vec![ Val::Int(1), Val::Int(2)]) + }) + }),}, + }, + DictEdit { + action: DictEditAction::Add, + items: hashmap! { + "KEY".to_string() => Val::String("VALUE".to_string()), + }, + }, + ]; let (_tmpdir, fromfile_path) = write_fromfile(filename, content); - let args = Args::new(vec![format!("--foo=@{}", &fromfile_path.display())]); + let args = Args::new(vec![ + format!("--foo=@{}", &fromfile_path.display()), + "--foo=+{'KEY':'VALUE'}".to_string(), + ]); let actual = args.get_dict(&option_id!("foo")).unwrap().unwrap(); assert_eq!(expected, actual) } @@ -296,6 +323,19 @@ fn test_dict_fromfile() { "#, "fromfile.yaml", ); + + // Test adding, rather than replacing, from a raw text fromfile. + let expected_add = vec![DictEdit { + action: DictEditAction::Add, + items: hashmap! {"FOO".to_string() => Val::Int(42)}, + }]; + + let (_tmpdir, fromfile_path) = write_fromfile("fromfile.txt", "+{'FOO':42}"); + let args = Args::new(vec![format!("--foo=@{}", &fromfile_path.display())]); + assert_eq!( + expected_add, + args.get_dict(&option_id!("foo")).unwrap().unwrap() + ) } #[test] diff --git a/src/rust/engine/options/src/config.rs b/src/rust/engine/options/src/config.rs index ae06e3ab66e..0e5c0800de0 100644 --- a/src/rust/engine/options/src/config.rs +++ b/src/rust/engine/options/src/config.rs @@ -414,7 +414,7 @@ impl OptionsSource for Config { self.get_list::(id) } - fn get_dict(&self, id: &OptionId) -> Result, String> { + fn get_dict(&self, id: &OptionId) -> Result>, String> { if let Some(table) = self.value.get(id.scope.name()) { let option_name = Self::option_name(id); if let Some(value) = table.get(&option_name) { @@ -422,16 +422,16 @@ impl OptionsSource for Config { Value::Table(sub_table) => { if let Some(add) = sub_table.get("add") { if sub_table.len() == 1 && add.is_table() { - return Ok(Some(DictEdit { + return Ok(Some(vec![DictEdit { action: DictEditAction::Add, items: toml_table_to_dict(add), - })); + }])); } } - return Ok(Some(DictEdit { + return Ok(Some(vec![DictEdit { action: DictEditAction::Replace, items: toml_table_to_dict(value), - })); + }])); } Value::String(v) => { return expand_to_dict(v.to_owned()) diff --git a/src/rust/engine/options/src/config_tests.rs b/src/rust/engine/options/src/config_tests.rs index 2465c97a17e..6362dd79409 100644 --- a/src/rust/engine/options/src/config_tests.rs +++ b/src/rust/engine/options/src/config_tests.rs @@ -144,13 +144,13 @@ fn test_interpolate_config() { ); assert_eq!( - DictEdit { + vec![DictEdit { action: DictEditAction::Replace, items: HashMap::from([ ("fruit".to_string(), Val::String("strawberry".to_string())), ("spice".to_string(), Val::String("black pepper".to_string())) ]) - }, + }], conf.get_dict(&option_id!(["groceries"], "inline_table")) .unwrap() .unwrap() @@ -230,7 +230,7 @@ fn test_list_fromfile() { #[test] fn test_dict_fromfile() { fn do_test(content: &str, filename: &str) { - let expected = DictEdit { + let expected = vec![DictEdit { action: DictEditAction::Replace, items: hashmap! { "FOO".to_string() => Val::Dict(hashmap! { @@ -240,7 +240,7 @@ fn test_dict_fromfile() { "QUUX".to_string() => Val::List(vec![ Val::Int(1), Val::Int(2)]) }) }),}, - }; + }]; let (_tmpdir, fromfile_path) = write_fromfile(filename, content); let conf = config(format!("[GLOBAL]\nfoo = '@{}'\n", fromfile_path.display()).as_str()); diff --git a/src/rust/engine/options/src/env.rs b/src/rust/engine/options/src/env.rs index 8cc5a557d29..ac12db1156b 100644 --- a/src/rust/engine/options/src/env.rs +++ b/src/rust/engine/options/src/env.rs @@ -127,7 +127,7 @@ impl OptionsSource for Env { self.get_list::(id) } - fn get_dict(&self, id: &OptionId) -> Result, String> { + fn get_dict(&self, id: &OptionId) -> Result>, String> { for env_var_name in &Self::env_var_names(id) { if let Some(value) = self.env.get(env_var_name) { return expand_to_dict(value.to_owned()).map_err(|e| e.render(self.display(id))); diff --git a/src/rust/engine/options/src/env_tests.rs b/src/rust/engine/options/src/env_tests.rs index 2487005eb4f..a1d5e84b8b2 100644 --- a/src/rust/engine/options/src/env_tests.rs +++ b/src/rust/engine/options/src/env_tests.rs @@ -266,7 +266,7 @@ fn test_list_fromfile() { #[test] fn test_dict_fromfile() { fn do_test(content: &str, filename: &str) { - let expected = DictEdit { + let expected = vec![DictEdit { action: DictEditAction::Replace, items: hashmap! { "FOO".to_string() => Val::Dict(hashmap! { @@ -276,7 +276,7 @@ fn test_dict_fromfile() { "QUUX".to_string() => Val::List(vec![ Val::Int(1), Val::Int(2)]) }) }),}, - }; + }]; let (_tmpdir, fromfile_path) = write_fromfile(filename, content); let env = env([( diff --git a/src/rust/engine/options/src/lib.rs b/src/rust/engine/options/src/lib.rs index df65ae55199..47400d05ec2 100644 --- a/src/rust/engine/options/src/lib.rs +++ b/src/rust/engine/options/src/lib.rs @@ -182,7 +182,7 @@ pub(crate) trait OptionsSource { /// Get the dict option identified by `id` from this source. /// Errors when this source has an option value for `id` but that value is not a dict. /// - fn get_dict(&self, id: &OptionId) -> Result, String>; + fn get_dict(&self, id: &OptionId) -> Result>, String>; } #[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] @@ -227,7 +227,7 @@ pub struct ListOptionValue { #[derive(Debug)] pub struct DictOptionValue { - pub derivation: Option>, + pub derivation: Option)>>, // The highest-priority source that provided edits for this value. pub source: Source, pub value: HashMap, @@ -582,25 +582,27 @@ impl OptionParser { if self.include_derivation { let mut derivations = vec![( Source::Default, - DictEdit { + vec![DictEdit { action: DictEditAction::Replace, items: dict.clone(), - }, + }], )]; for (source_type, source) in self.sources.iter() { - if let Some(dict_edit) = source.get_dict(id)? { - derivations.push((source_type.clone(), dict_edit)); + if let Some(dict_edits) = source.get_dict(id)? { + derivations.push((source_type.clone(), dict_edits)); } } derivation = Some(derivations); } let mut highest_priority_source = Source::Default; for (source_type, source) in self.sources.iter() { - if let Some(dict_edit) = source.get_dict(id)? { + if let Some(dict_edits) = source.get_dict(id)? { highest_priority_source = source_type.clone(); - match dict_edit.action { - DictEditAction::Replace => dict = dict_edit.items, - DictEditAction::Add => dict.extend(dict_edit.items), + for dict_edit in dict_edits { + match dict_edit.action { + DictEditAction::Replace => dict = dict_edit.items, + DictEditAction::Add => dict.extend(dict_edit.items), + } } } } diff --git a/src/rust/engine/options/src/parse.rs b/src/rust/engine/options/src/parse.rs index 0b15398be6f..8540fbba70c 100644 --- a/src/rust/engine/options/src/parse.rs +++ b/src/rust/engine/options/src/parse.rs @@ -435,16 +435,16 @@ pub(crate) fn expand_to_list( } } -pub(crate) fn expand_to_dict(value: String) -> Result, ParseError> { +pub(crate) fn expand_to_dict(value: String) -> Result>, ParseError> { let (path_opt, value_opt) = maybe_expand(value)?; if let Some(value) = value_opt { if let Some(items) = try_deserialize(&value, path_opt)? { - Ok(Some(DictEdit { + Ok(Some(vec![DictEdit { action: DictEditAction::Replace, items, - })) + }])) } else { - parse_dict(&value).map(Some) + parse_dict(&value).map(|x| Some(vec![x])) } } else { Ok(None) diff --git a/src/rust/engine/options/src/parse_tests.rs b/src/rust/engine/options/src/parse_tests.rs index c2123b48b8d..8a569d58f47 100644 --- a/src/rust/engine/options/src/parse_tests.rs +++ b/src/rust/engine/options/src/parse_tests.rs @@ -843,6 +843,13 @@ fn test_expand_fromfile_to_dict() { "{prefix}{}", _tmpdir.path().join(filename).display() )) + .map(|x| { + if let Some(des) = x { + des.into_iter().next() + } else { + None + } + }) } fn do_test(content: &str, expected: &DictEdit, filename: &str) { diff --git a/src/rust/engine/options/src/tests.rs b/src/rust/engine/options/src/tests.rs index 68d20431f73..2de7ba1ace5 100644 --- a/src/rust/engine/options/src/tests.rs +++ b/src/rust/engine/options/src/tests.rs @@ -340,7 +340,7 @@ fn test_parse_dict_options() { fn check( expected: HashMap<&str, Val>, - expected_derivation: Vec<(Source, DictEdit)>, + expected_derivation: Vec<(Source, Vec)>, args: Vec<&'static str>, env: Vec<(&'static str, &'static str)>, config: &'static str, @@ -359,18 +359,31 @@ fn test_parse_dict_options() { }); } - fn replace(items: HashMap<&str, Val>) -> DictEdit { - DictEdit { + fn replace(items: HashMap<&str, Val>) -> Vec { + vec![DictEdit { action: DictEditAction::Replace, items: with_owned_keys(items), - } + }] } - fn add(items: HashMap<&str, Val>) -> DictEdit { - DictEdit { + fn add(items: HashMap<&str, Val>) -> Vec { + vec![DictEdit { action: DictEditAction::Add, items: with_owned_keys(items), - } + }] + } + + fn add2(items0: HashMap<&str, Val>, items1: HashMap<&str, Val>) -> Vec { + vec![ + DictEdit { + action: DictEditAction::Add, + items: with_owned_keys(items0), + }, + DictEdit { + action: DictEditAction::Add, + items: with_owned_keys(items1), + }, + ] } let default_derivation = ( @@ -383,6 +396,7 @@ fn test_parse_dict_options() { "key1" => Val::Int(1), "key2" => Val::String("val2".to_string()), "key3" => Val::Int(3), + "key3a" => Val::String("3a".to_string()), "key4" => Val::Float(4.0), "key5" => Val::Bool(true), "key6" => Val::Int(6), @@ -392,9 +406,15 @@ fn test_parse_dict_options() { (config_source(), add(hashmap! {"key5" => Val::Bool(true)})), (extra_config_source(), add(hashmap! {"key6" => Val::Int(6)})), (Source::Env, add(hashmap! {"key4" => Val::Float(4.0)})), - (Source::Flag, add(hashmap! {"key3" => Val::Int(3)})), + ( + Source::Flag, + add2( + hashmap! {"key3" => Val::Int(3)}, + hashmap! {"key3a" => Val::String("3a".to_string())}, + ), + ), ], - vec!["--scope-foo=+{'key3': 3}"], + vec!["--scope-foo=+{'key3': 3}", "--scope-foo=+{'key3a': '3a'}"], vec![("PANTS_SCOPE_FOO", "+{'key4': 4.0}")], "[scope]\nfoo = \"+{ 'key5': true }\"", "[scope]\nfoo = \"+{ 'key6': 6 }\"",