Skip to content

Commit

Permalink
Don't eagerly merge configs. (pantsbuild#20459)
Browse files Browse the repository at this point in the history 
Merging the config values is incorrect for list- and
dict-valued options, where we want to concatenate
edits, not stomp them.

For example, if for some list-valued option the default
value is [0,1], config1 has -[0] and config2 has +[2,3]
then the resulting value should be [1,2,3], but before
this change it would have been [0,1,2,3].

Instead, we preserve each config file as a separate
source, and do the appropriate stomping/merging
at option value get time.
  • Loading branch information
@benjyw
benjyw authored Jan 25, 2024
1 parent 24d6675 commit b47af8a
Show file tree
Hide file tree
Showing 5 changed files with 180 additions and 136 deletions.
6 changes: 4 additions & 2 deletions src/rust/engine/options/src/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,11 @@ impl OptionsSource for Args {
self.get_list(id, parse_string_list)
}

fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> {
fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
match self.find_flag(Self::arg_names(id, Negate::False))? {
Some((name, ref value, _)) => parse_dict(value).map(Some).map_err(|e| e.render(name)),
Some((name, ref value, _)) => parse_dict(value)
.map(|e| Some(vec![e]))
.map_err(|e| e.render(name)),
None => Ok(None),
}
}
Expand Down
250 changes: 133 additions & 117 deletions src/rust/engine/options/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use std::collections::{HashMap, HashSet};
use std::ffi::OsString;
use std::fs;
use std::mem;
use std::path::Path;

use lazy_static::lazy_static;
Expand Down Expand Up @@ -217,21 +217,132 @@ fn toml_table_to_dict(table: &Value) -> HashMap<String, Val> {
}

#[derive(Clone)]
pub(crate) struct Config {
struct ConfigSource {
#[allow(dead_code)]
path: Option<OsString>,
config: Value,
}

impl ConfigSource {
fn option_name(id: &OptionId) -> String {
id.name("_", NameTransform::None)
}

fn get_value(&self, id: &OptionId) -> Option<&Value> {
self.config
.get(id.scope())
.and_then(|table| table.get(Self::option_name(id)))
}

fn get_list<T: FromValue>(
&self,
id: &OptionId,
parse_list: fn(&str) -> Result<Vec<ListEdit<T>>, ParseError>,
) -> Result<Vec<ListEdit<T>>, String> {
let mut list_edits = vec![];
if let Some(table) = self.config.get(id.scope()) {
let option_name = Self::option_name(id);
if let Some(value) = table.get(&option_name) {
match value {
Value::Table(sub_table) => {
if sub_table.is_empty()
|| !sub_table.keys().collect::<HashSet<_>>().is_subset(
&["add".to_owned(), "remove".to_owned()]
.iter()
.collect::<HashSet<_>>(),
)
{
return Err(format!(
"Expected {option_name} to contain an 'add' element, a 'remove' element or both but found: {sub_table:?}"
));
}
if let Some(add) = sub_table.get("add") {
list_edits.push(ListEdit {
action: ListEditAction::Add,
items: T::extract_list(&format!("{option_name}.add"), add)?,
})
}
if let Some(remove) = sub_table.get("remove") {
list_edits.push(ListEdit {
action: ListEditAction::Remove,
items: T::extract_list(&format!("{option_name}.remove"), remove)?,
})
}
}
Value::String(v) => {
list_edits.extend(parse_list(v).map_err(|e| e.render(option_name))?);
}
value => list_edits.push(ListEdit {
action: ListEditAction::Replace,
items: T::extract_list(&option_name, value)?,
}),
}
}
}
Ok(list_edits)
}

fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> {
if let Some(table) = self.config.get(id.scope()) {
let option_name = Self::option_name(id);
if let Some(value) = table.get(&option_name) {
match value {
Value::Table(sub_table) => {
if let Some(add) = sub_table.get("add") {
if sub_table.len() == 1 && add.is_table() {
return Ok(Some(DictEdit {
action: DictEditAction::Add,
items: toml_table_to_dict(add),
}));
}
}
return Ok(Some(DictEdit {
action: DictEditAction::Replace,
items: toml_table_to_dict(value),
}));
}
Value::String(v) => {
return Ok(Some(parse_dict(v).map_err(|e| e.render(option_name))?));
}
_ => {
return Err(format!(
"Expected {option_name} to be a toml table or Python dict, but given {value}."
));
}
}
}
}
Ok(None)
}
}

#[derive(Clone)]
pub(crate) struct Config {
sources: Vec<ConfigSource>,
}

impl Config {
pub(crate) fn default() -> Config {
pub(crate) fn parse<P: AsRef<Path>>(
files: &[P],
seed_values: &InterpolationMap,
) -> Result<Config, String> {
let mut sources = vec![];
for file in files {
sources.push(Self::parse_source(file, seed_values)?);
}
Ok(Config { sources })
}

pub(crate) fn merge(self, other: Config) -> Config {
Config {
config: Value::Table(Table::new()),
sources: self.sources.into_iter().chain(other.sources).collect(),
}
}

pub(crate) fn parse<P: AsRef<Path>>(
fn parse_source<P: AsRef<Path>>(
file: P,
seed_values: &InterpolationMap,
) -> Result<Config, String> {
) -> Result<ConfigSource, String> {
let config_contents = fs::read_to_string(&file).map_err(|e| {
format!(
"Failed to read config file {}: {}",
Expand Down Expand Up @@ -308,101 +419,29 @@ impl Config {
};

let new_table = Table::from_iter(new_sections?);
Ok(Config {
Ok(ConfigSource {
path: Some(file.as_ref().as_os_str().into()),
config: Value::Table(new_table),
})
}

pub(crate) fn merged<P: AsRef<Path>>(
files: &[P],
seed_values: &InterpolationMap,
) -> Result<Config, String> {
files
.iter()
.map(|f| Config::parse(f, seed_values))
.try_fold(Config::default(), |config, parse_result| {
parse_result.map(|parsed| config.merge(parsed))
})
}

fn option_name(id: &OptionId) -> String {
id.name("_", NameTransform::None)
}

fn get_value(&self, id: &OptionId) -> Option<&Value> {
self.config
.get(id.scope())
.and_then(|table| table.get(Self::option_name(id)))
self.sources
.iter()
.rev()
.find_map(|source| source.get_value(id))
}

fn get_list<T: FromValue>(
&self,
id: &OptionId,
parse_list: fn(&str) -> Result<Vec<ListEdit<T>>, ParseError>,
) -> Result<Option<Vec<ListEdit<T>>>, String> {
if let Some(table) = self.config.get(id.scope()) {
let option_name = Self::option_name(id);
let mut list_edits = vec![];
if let Some(value) = table.get(&option_name) {
match value {
Value::Table(sub_table) => {
if sub_table.is_empty()
|| !sub_table.keys().collect::<HashSet<_>>().is_subset(
&["add".to_owned(), "remove".to_owned()]
.iter()
.collect::<HashSet<_>>(),
)
{
return Err(format!(
"Expected {option_name} to contain an 'add' element, a 'remove' element or both but found: {sub_table:?}"
));
}
if let Some(add) = sub_table.get("add") {
list_edits.push(ListEdit {
action: ListEditAction::Add,
items: T::extract_list(&format!("{option_name}.add"), add)?,
})
}
if let Some(remove) = sub_table.get("remove") {
list_edits.push(ListEdit {
action: ListEditAction::Remove,
items: T::extract_list(&format!("{option_name}.remove"), remove)?,
})
}
}
Value::String(v) => {
list_edits.extend(parse_list(v).map_err(|e| e.render(option_name))?);
}
value => list_edits.push(ListEdit {
action: ListEditAction::Replace,
items: T::extract_list(&option_name, value)?,
}),
}
}
if !list_edits.is_empty() {
return Ok(Some(list_edits));
}
}
Ok(None)
}

pub(crate) fn merge(mut self, mut other: Config) -> Config {
let mut map = mem::take(self.config.as_table_mut().unwrap());
let mut other = mem::take(other.config.as_table_mut().unwrap());
// Merge overlapping sections.
for (scope, table) in &mut map {
if let Some(mut other_table) = other.remove(scope) {
table
.as_table_mut()
.unwrap()
.extend(mem::take(other_table.as_table_mut().unwrap()));
}
}
// And then extend non-overlapping sections.
map.extend(other);
Config {
config: Value::Table(map),
let mut edits: Vec<ListEdit<T>> = vec![];
for source in self.sources.iter() {
edits.append(&mut source.get_list(id, parse_list)?);
}
Ok(Some(edits))
}
}

Expand Down Expand Up @@ -443,36 +482,13 @@ impl OptionsSource for Config {
self.get_list(id, parse_string_list)
}

fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> {
if let Some(table) = self.config.get(id.scope()) {
let option_name = Self::option_name(id);
if let Some(value) = table.get(&option_name) {
match value {
Value::Table(sub_table) => {
if let Some(add) = sub_table.get("add") {
if sub_table.len() == 1 && add.is_table() {
return Ok(Some(DictEdit {
action: DictEditAction::Add,
items: toml_table_to_dict(add),
}));
}
}
return Ok(Some(DictEdit {
action: DictEditAction::Replace,
items: toml_table_to_dict(value),
}));
}
Value::String(v) => {
return Ok(Some(parse_dict(v).map_err(|e| e.render(option_name))?));
}
_ => {
return Err(format!(
"Expected {option_name} to be a toml table or Python dict, but given {value}."
));
}
}
fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
let mut edits = vec![];
for source in self.sources.iter() {
if let Some(edit) = source.get_dict(id)? {
edits.push(edit);
}
}
Ok(None)
Ok(if edits.is_empty() { None } else { Some(edits) })
}
}
46 changes: 34 additions & 12 deletions src/rust/engine/options/src/config_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn maybe_config<I: IntoIterator<Item = &'static str>>(file_contents: I) -> Resul
path
})
.collect::<Vec<_>>();
Config::merged(
Config::parse(
&files,
&HashMap::from([
("seed1".to_string(), "seed1val".to_string()),
Expand Down Expand Up @@ -56,24 +56,46 @@ fn test_display() {
}

#[test]
fn test_section_overlap() {
// Two files with the same section should result in merged content for that section.
fn test_multiple_sources() {
let config = config([
"[section]\n\
field1 = 'something'\n",
name = 'first'\n\
field1 = 'something'\n\
list='+[0,1]'",
"[section]\n\
field2 = 'something else'\n",
name = 'second'\n\
field2 = 'something else'\n\
list='-[0],+[2, 3]'",
]);

let assert_string = |expected: &str, id: OptionId| {
assert_eq!(
expected.to_owned(),
config.get_string(&id).unwrap().unwrap()
)
let assert_string = |expected: &str, id: &OptionId| {
assert_eq!(expected.to_owned(), config.get_string(id).unwrap().unwrap())
};

assert_string("something", option_id!(["section"], "field1"));
assert_string("something else", option_id!(["section"], "field2"));
assert_string("second", &option_id!(["section"], "name"));
assert_string("something", &option_id!(["section"], "field1"));
assert_string("something else", &option_id!(["section"], "field2"));

assert_eq!(
vec![
ListEdit {
action: ListEditAction::Add,
items: vec![0, 1]
},
ListEdit {
action: ListEditAction::Remove,
items: vec![0]
},
ListEdit {
action: ListEditAction::Add,
items: vec![2, 3]
},
],
config
.get_int_list(&option_id!(["section"], "list"))
.unwrap()
.unwrap()
)
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions src/rust/engine/options/src/env.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,10 @@ impl OptionsSource for Env {
self.get_list(id, parse_string_list)
}

fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> {
fn get_dict(&self, id: &OptionId) -> Result<Option<Vec<DictEdit>>, String> {
if let Some(value) = self.get_string(id)? {
parse_dict(&value)
.map(Some)
.map(|e| Some(vec![e]))
.map_err(|e| e.render(self.display(id)))
} else {
Ok(None)
Expand Down
Loading

0 comments on commit b47af8a

Please sign in to comment.