Skip to content

Commit

Permalink
chore(config): re-enable config pointers with a new whitelist
Browse files Browse the repository at this point in the history
commit-id:e4597e85
  • Loading branch information
Itay-Tsabary-Starkware committed Nov 3, 2024
1 parent bd3fad8 commit d9a8462
Show file tree
Hide file tree
Showing 9 changed files with 252 additions and 69 deletions.
116 changes: 100 additions & 16 deletions crates/papyrus_config/src/config_test.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};
use std::env;
use std::fs::File;
use std::path::PathBuf;
Expand Down Expand Up @@ -257,24 +257,41 @@ fn test_pointers_flow() {
const POINTING_PARAM_DESCRIPTION: &str = "This is a.";
const PUBLIC_POINTING_PARAM_NAME: &str = "public_a.a";
const PRIVATE_POINTING_PARAM_NAME: &str = "private_a.a";
const WHITELISTED_POINTING_PARAM_NAME: &str = "non_pointing.a";
const VALUE: usize = 5;

let config_map = BTreeMap::from([
ser_param(
PUBLIC_POINTING_PARAM_NAME,
&json!(5),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Public,
),
ser_param(
PRIVATE_POINTING_PARAM_NAME,
&json!(5),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
ser_param(
WHITELISTED_POINTING_PARAM_NAME,
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
]);
let pointers =
vec![ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION)];
let stored_map = combine_config_map_and_pointers(config_map, &pointers).unwrap();
let pointers = vec![(
ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION),
HashSet::from([
PUBLIC_POINTING_PARAM_NAME.to_string(),
PRIVATE_POINTING_PARAM_NAME.to_string(),
]),
)];
let non_pointer_params = HashSet::from([WHITELISTED_POINTING_PARAM_NAME.to_string()]);
let stored_map =
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();

// Assert the pointing parameters are correctly set.
assert_eq!(
stored_map[PUBLIC_POINTING_PARAM_NAME],
json!(SerializedParam {
Expand All @@ -291,6 +308,18 @@ fn test_pointers_flow() {
privacy: ParamPrivacy::Private,
})
);

// Assert the whitelisted parameter is correctly set.
assert_eq!(
stored_map[WHITELISTED_POINTING_PARAM_NAME],
json!(SerializedParam {
description: POINTING_PARAM_DESCRIPTION.to_owned(),
content: SerializedContent::DefaultValue(json!(VALUE)),
privacy: ParamPrivacy::Private,
})
);

// Assert the pointed parameter is correctly set as a required parameter.
assert_eq!(
stored_map[TARGET_PARAM_NAME],
json!(SerializedParam {
Expand All @@ -316,27 +345,43 @@ fn test_required_pointers_flow() {
const POINTING_PARAM_DESCRIPTION: &str = "This is b.";
const PUBLIC_POINTING_PARAM_NAME: &str = "public_b.b";
const PRIVATE_POINTING_PARAM_NAME: &str = "private_b.b";
const WHITELISTED_POINTING_PARAM_NAME: &str = "non_pointing.b";
const VALUE: usize = 6;

let config_map = BTreeMap::from([
ser_param(
PUBLIC_POINTING_PARAM_NAME,
&json!(6),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Public,
),
ser_param(
PRIVATE_POINTING_PARAM_NAME,
&json!(6),
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
ser_param(
WHITELISTED_POINTING_PARAM_NAME,
&json!(VALUE),
POINTING_PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
),
]);
let pointers = vec![ser_pointer_target_required_param(
REQUIRED_PARAM_NAME,
SerializationType::PositiveInteger,
REQUIRED_PARAM_DESCRIPTION,
let pointers = vec![(
ser_pointer_target_required_param(
REQUIRED_PARAM_NAME,
SerializationType::PositiveInteger,
REQUIRED_PARAM_DESCRIPTION,
),
HashSet::from([
PUBLIC_POINTING_PARAM_NAME.to_string(),
PRIVATE_POINTING_PARAM_NAME.to_string(),
]),
)];
let stored_map = combine_config_map_and_pointers(config_map, &pointers).unwrap();
let non_pointer_params = HashSet::from([WHITELISTED_POINTING_PARAM_NAME.to_string()]);
let stored_map =
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();

// Assert the pointing parameters are correctly set.
assert_eq!(
Expand All @@ -356,6 +401,16 @@ fn test_required_pointers_flow() {
})
);

// Assert the whitelisted parameter is correctly set.
assert_eq!(
stored_map[WHITELISTED_POINTING_PARAM_NAME],
json!(SerializedParam {
description: POINTING_PARAM_DESCRIPTION.to_owned(),
content: SerializedContent::DefaultValue(json!(VALUE)),
privacy: ParamPrivacy::Private,
})
);

// Assert the pointed parameter is correctly set as a required parameter.
assert_eq!(
stored_map[REQUIRED_PARAM_NAME],
Expand All @@ -367,6 +422,35 @@ fn test_required_pointers_flow() {
);
}

#[test]
#[should_panic(
expected = "The target param should_be_pointing.c should point to c, or to be whitelisted."
)]
fn test_missing_pointer_flow() {
const TARGET_PARAM_NAME: &str = "c";
const TARGET_PARAM_DESCRIPTION: &str = "This is common c.";
const PARAM_DESCRIPTION: &str = "This is c.";
const NON_POINTING_PARAM_NAME: &str = "should_be_pointing.c";

// Define a non-pointing parameter and a target pointer such that the parameter name matches the
// target.
let config_map = BTreeMap::from([ser_param(
NON_POINTING_PARAM_NAME,
&json!(7),
PARAM_DESCRIPTION,
ParamPrivacyInput::Private,
)]);
let pointers = vec![(
ser_pointer_target_param(TARGET_PARAM_NAME, &json!(10), TARGET_PARAM_DESCRIPTION),
HashSet::new(),
)];
// Do not whitelist the non-pointing parameter.
let non_pointer_params = HashSet::new();

// Attempt to combine the config map and pointers. This should panic.
combine_config_map_and_pointers(config_map, &pointers, &non_pointer_params).unwrap();
}

#[test]
fn test_replace_pointers() {
let (mut config_map, _) = split_values_and_types(BTreeMap::from([ser_param(
Expand Down Expand Up @@ -416,7 +500,7 @@ fn load_custom_config(args: Vec<&str>) -> CustomConfig {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config.json");
CustomConfig { param_path: "default value".to_owned(), seed: 5 }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

load_and_process_config::<CustomConfig>(
Expand Down Expand Up @@ -506,7 +590,7 @@ fn load_required_param_path(args: Vec<&str>) -> String {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config.json");
RequiredConfig { param_path: "default value".to_owned(), num: 3 }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

let loaded_config = load_and_process_config::<CustomConfig>(
Expand Down Expand Up @@ -600,7 +684,7 @@ fn deeply_nested_optionals() {
let dir = TempDir::new().unwrap();
let file_path = dir.path().join("config2.json");
Level0 { level0_value: 1, level1: None }
.dump_to_file(&vec![], file_path.to_str().unwrap())
.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap())
.unwrap();

let l0 = load_and_process_config::<Level0>(
Expand Down
66 changes: 48 additions & 18 deletions crates/papyrus_config/src/dumping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
//! }
//! ```
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};
use std::fs::File;
use std::io::{BufWriter, Write};

Expand All @@ -54,6 +54,9 @@ use crate::{
IS_NONE_MARK,
};

/// Detailing pointers in the config map.
pub type ConfigPointers = Vec<((ParamPath, SerializedParam), HashSet<ParamPath>)>;

/// Serialization for configs.
pub trait SerializeConfig {
/// Conversion of a configuration to a mapping of flattened parameters to their descriptions and
Expand All @@ -69,7 +72,7 @@ pub trait SerializeConfig {
/// # Example
///
/// ```
/// # use std::collections::BTreeMap;
/// # use std::collections::{BTreeMap, HashSet};
///
/// # use papyrus_config::dumping::{ser_param, SerializeConfig};
/// # use papyrus_config::{ParamPath, ParamPrivacyInput, SerializedParam};
Expand All @@ -94,15 +97,17 @@ pub trait SerializeConfig {
///
/// let dir = TempDir::new().unwrap();
/// let file_path = dir.path().join("config.json");
/// ConfigExample { key: 42 }.dump_to_file(&vec![], file_path.to_str().unwrap());
/// ConfigExample { key: 42 }.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap());
/// ```
/// Note, in the case of a None sub configs, its elements will not be included in the file.
fn dump_to_file(
&self,
config_pointers: &Vec<(ParamPath, SerializedParam)>,
config_pointers: &ConfigPointers,
non_pointer_params: &HashSet<ParamPath>,
file_path: &str,
) -> Result<(), ConfigError> {
let combined_map = combine_config_map_and_pointers(self.dump(), config_pointers)?;
let combined_map =
combine_config_map_and_pointers(self.dump(), config_pointers, non_pointer_params)?;

// Create file writer.
let file = File::create(file_path)?;
Expand Down Expand Up @@ -293,27 +298,52 @@ pub fn ser_pointer_target_required_param(
/// the target parameter paths to contain only the name of the target they point to.
pub(crate) fn combine_config_map_and_pointers(
mut config_map: BTreeMap<ParamPath, SerializedParam>,
pointers: &Vec<(ParamPath, SerializedParam)>,
pointers: &ConfigPointers,
non_pointer_params: &HashSet<ParamPath>,
) -> Result<Value, ConfigError> {
// Update config with target params.
for (target_param, serialized_pointer) in pointers {
for ((target_param, serialized_pointer), pointing_params_vec) in pointers {
// Insert target param.
config_map.insert(target_param.clone(), serialized_pointer.clone());

// Update config entries that match the target param as pointers.
config_map.iter_mut().for_each(|(param_path, serialized_param)| {
// Check if the param is the target param.
if param_path.ends_with(format!("{FIELD_SEPARATOR}{target_param}").as_str()) {
// Point to the target param.
*serialized_param = SerializedParam {
description: serialized_param.description.clone(),
// Update pointing params to point at the target param.
for pointing_param in pointing_params_vec {
let pointing_serialized_param =
config_map.get(pointing_param).ok_or(ConfigError::PointerSourceNotFound {
pointing_param: pointing_param.to_owned(),
})?;
config_map.insert(
pointing_param.to_owned(),
SerializedParam {
description: pointing_serialized_param.description.clone(),
content: SerializedContent::PointerTarget(target_param.to_owned()),
privacy: serialized_param.privacy.clone(),
};
}
});
privacy: pointing_serialized_param.privacy.clone(),
},
);
}
}

// Iterate over the config, check that all parameters whose name matches a pointer target either
// point at it or are in the whitelist.
config_map.iter().for_each(|(param_path, serialized_param)| {
for ((target_param, _), _) in pointers {
// Check if the param name matches a pointer target, and that it is not in the
// whitelist.
if param_path.ends_with(format!("{FIELD_SEPARATOR}{target_param}").as_str())
&& !non_pointer_params.contains(param_path)
{
// Check that the param points to the target param.
assert!(
serialized_param.content
== SerializedContent::PointerTarget(target_param.to_owned()),
"The target param {} should point to {}, or to be whitelisted.",
param_path,
target_param
);
};
}
});

Ok(json!(config_map))
}

Expand Down
4 changes: 2 additions & 2 deletions crates/papyrus_config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//! # Example
//!
//! ```
//! use std::collections::BTreeMap;
//! use std::collections::{BTreeMap, HashSet};
//! use std::fs::File;
//! use std::path::Path;
//!
Expand Down Expand Up @@ -36,7 +36,7 @@
//!
//! let dir = TempDir::new().unwrap();
//! let file_path = dir.path().join("config.json");
//! ConfigExample { key: 42 }.dump_to_file(&vec![], file_path.to_str().unwrap());
//! ConfigExample { key: 42 }.dump_to_file(&vec![], &HashSet::new(), file_path.to_str().unwrap());
//! let file = File::open(file_path).unwrap();
//! let loaded_config = load_and_process_config::<ConfigExample>(
//! file,
Expand Down
4 changes: 2 additions & 2 deletions crates/papyrus_node/src/bin/papyrus_dump_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#[cfg(feature = "rpc")]
use papyrus_config::dumping::SerializeConfig;
#[cfg(feature = "rpc")]
use papyrus_node::config::pointers::CONFIG_POINTERS;
use papyrus_node::config::pointers::{CONFIG_NON_POINTERS_WHITELIST, CONFIG_POINTERS};
#[cfg(feature = "rpc")]
use papyrus_node::config::{NodeConfig, DEFAULT_CONFIG_PATH};

Expand All @@ -15,7 +15,7 @@ use papyrus_node::config::{NodeConfig, DEFAULT_CONFIG_PATH};
fn main() {
#[cfg(feature = "rpc")]
NodeConfig::default()
.dump_to_file(&CONFIG_POINTERS, DEFAULT_CONFIG_PATH)
.dump_to_file(&CONFIG_POINTERS, &CONFIG_NON_POINTERS_WHITELIST, DEFAULT_CONFIG_PATH)
.expect("dump to file error");
// TODO(shahak): Try to find a way to remove this binary altogether when the feature rpc is
// turned off.
Expand Down
10 changes: 8 additions & 2 deletions crates/papyrus_node/src/config/config_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use tempfile::NamedTempFile;
use validator::Validate;

#[cfg(feature = "rpc")]
use crate::config::pointers::CONFIG_POINTERS;
use crate::config::pointers::{CONFIG_NON_POINTERS_WHITELIST, CONFIG_POINTERS};
use crate::config::{node_command, NodeConfig, DEFAULT_CONFIG_PATH};

// Returns the required and generated params in config/papyrus/default_config.json with the default
Expand Down Expand Up @@ -137,7 +137,13 @@ fn default_config_file_is_up_to_date() {
// Create a temporary file and dump the default config to it.
let mut tmp_file_path = env::temp_dir();
tmp_file_path.push("cfg.json");
NodeConfig::default().dump_to_file(&CONFIG_POINTERS, tmp_file_path.to_str().unwrap()).unwrap();
NodeConfig::default()
.dump_to_file(
&CONFIG_POINTERS,
&CONFIG_NON_POINTERS_WHITELIST,
tmp_file_path.to_str().unwrap(),
)
.unwrap();

// Read the dumped config from the file.
let from_code: serde_json::Value =
Expand Down
Loading

0 comments on commit d9a8462

Please sign in to comment.