Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH 731 review #341

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cedb1b3
begining with test server_initializer_collected_params_combine_vlcs_p…
czarte Aug 23, 2023
32dd2cb
removed GatheredParams from server_initializer_collected_params, adde…
czarte Aug 28, 2023
690b099
add ComputedVcl struct and its implementation, add VclType enum, to b…
czarte Aug 29, 2023
0d7427b
back to VirtualCommandLine type without enum, passing different types…
czarte Aug 30, 2023
22e607b
fixed data flow with Evironment and Command line arguments, need to f…
czarte Aug 31, 2023
b9a6e85
changes in test to access config.file for various scenarios, add mach…
czarte Sep 4, 2023
08349e2
try to get rid of redundant MultiConfig construction in determine_use…
czarte Sep 13, 2023
8aead2a
handle computed arguments in multiconfig, closure for server_initiali…
czarte Sep 15, 2023
6167856
computing env_args and cmd_args in create_preorientation_args fn!
czarte Sep 21, 2023
7286ee8
fix passing test server_initializer_collected_params_can_read_paramet…
czarte Oct 4, 2023
fce6fe1
fixed VclArg problem with withdrawing data from VirtualCommandline, s…
czarte Oct 11, 2023
4dd0932
finalize preparing guts for multiconfig in server_initializer_collect…
czarte Oct 23, 2023
413ffc5
fix windows test of real_user soecified
czarte Oct 23, 2023
18d0cc1
renaming, adding TODOs, removing resolved TODOS
czarte Oct 25, 2023
efd3fbb
fixing tests for windows os
czarte Oct 30, 2023
cf6da59
fixed on windows
czarte Oct 31, 2023
34b8fea
formatting
czarte Nov 1, 2023
5895879
fixed real user for windows
czarte Nov 2, 2023
62e99e3
removed filling specified and unspecified boxes from server_initializ…
czarte Nov 1, 2023
8369ff9
last fixes of extract_values_and_fill_boxes, and test server_initiali…
czarte Nov 6, 2023
5b02822
fixed windows test for tilde
czarte Nov 8, 2023
0603f70
fix tilde test for windows
czarte Nov 8, 2023
2ec7e12
renaming of tests and functions
czarte Nov 8, 2023
fe491ab
remove GatheredParams (the goal), optimizing is_user_specified in Mul…
czarte Nov 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 141 additions & 12 deletions masq_lib/src/multi_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ macro_rules! values_m {
#[derive(Debug)]
pub struct MultiConfig<'a> {
arg_matches: ArgMatches<'a>,
computed_value_names: HashSet<String>,
}

impl<'a> MultiConfig<'a> {
Expand All @@ -63,9 +64,20 @@ impl<'a> MultiConfig<'a> {
) -> Result<MultiConfig<'a>, ConfiguratorError> {
let initial: Box<dyn VirtualCommandLine> =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this down so that it's next to where it's used? It'd be easier to read.

Box::new(CommandLineVcl::new(vec![String::new()]));
let merged: Box<dyn VirtualCommandLine> = vcls
let mut computed_value_names = HashSet::new();
vcls.iter().for_each(|vcl| {
vcl.vcl_args().iter().for_each(|vcl_arg| {
match vcl.is_computed() {
true => computed_value_names.insert(vcl_arg.name().to_string()),
false => computed_value_names.remove(&vcl_arg.name().to_string()),
};
})
});
// TODO pull this out to function to use in determine_user_specific_data
let merged = vcls
.into_iter()
.fold(initial, |so_far, vcl| merge(so_far, vcl));

let arg_matches = match schema
.clone()
.get_matches_from_safe(merged.args().into_iter())
Expand All @@ -78,7 +90,10 @@ impl<'a> MultiConfig<'a> {
_ => return Err(Self::make_configurator_error(e)),
},
};
Ok(MultiConfig { arg_matches })
Ok(MultiConfig {
arg_matches,
computed_value_names,
})
}

fn check_for_invalid_value_err(
Expand Down Expand Up @@ -139,6 +154,10 @@ impl<'a> MultiConfig<'a> {
ConfiguratorError::required("<unknown>", &format!("Unfamiliar message: {}", e.message))
}

pub fn is_user_specified(&self, value_name: &str) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the old code, there's a macro that looks like this:

#[macro_export]
macro_rules! value_user_specified_m {
    ($m:ident, $v:expr, $t:ty) => {{
        let user_specified = $m.occurrences_of($v) > 0;
        let matches = $m.arg_matches_ref();
        match value_t!(matches, $v, $t) {
            Ok(v) => (Some(v), user_specified),
            Err(_) => (None, user_specified),
        }
    }};
}

That seems incompatible with this code. Do the two pass the same tests?

Further observation: The macro above does not seem to be used anywhere in the codebase anymore. (Check to make sure I'm right about this.) If it's not, there's no need to keep it around, and once it's deleted its meaning can be redefined as desired.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value_user_specified_m! macro is definitely not comparable to is_user_specified method in MultiConfig. value_user_specified_m! macro is returning true even if the value is computed.

!self.computed_value_names.contains(value_name)
}

pub fn occurrences_of(&self, parameter: &str) -> u64 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I think we can remove occurrences_of() (at least as a public function) and replace all its occurrences with calls to is_user_specified() since everywhere it's used, the return value is compared to zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid it is not possible. is_user_specified is tracking only user_specific_data, that's means data_directory, real_user & config_file so far. If we want to scale it to all data, we should add more config variables to this process or make the ComputedVcl global for all variables. That should go to the new card in my point of view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next thing is is_user_specified fn in unprivileged_parse_args_configuration.rs - this function could be replaced with occurrences_of or we should consider the comment above and solve it within the new card. That depends on the purpose of usage is_user_specified in UnprivilegedParseArgsConfiguration::unprivileged_parse_args where we use it to fill up unprivileged_config .blockchain_bridge_config .blockchain_service_url_opt
and unprivileged_config.blockchain_bridge_config.gas_price

self.arg_matches.occurrences_of(parameter)
}
Expand All @@ -150,6 +169,7 @@ impl<'a> MultiConfig<'a> {

pub trait VclArg: Debug {
fn name(&self) -> &str;
fn value_opt(&self) -> Option<&str>;
fn to_args(&self) -> Vec<String>;
fn dup(&self) -> Box<dyn VclArg>;
}
Expand All @@ -175,7 +195,9 @@ impl VclArg for NameValueVclArg {
fn name(&self) -> &str {
&self.name
}

fn value_opt(&self) -> Option<&str> {
Some(self.value.as_str())
}
fn to_args(&self) -> Vec<String> {
vec![self.name.clone(), self.value.clone()]
}
Expand Down Expand Up @@ -203,7 +225,9 @@ impl VclArg for NameOnlyVclArg {
fn name(&self) -> &str {
&self.name
}

fn value_opt(&self) -> Option<&str> {
None
}
fn to_args(&self) -> Vec<String> {
vec![self.name.clone()]
}
Expand All @@ -224,6 +248,9 @@ impl NameOnlyVclArg {
pub trait VirtualCommandLine {
fn vcl_args(&self) -> Vec<&dyn VclArg>;
fn args(&self) -> Vec<String>;
fn is_computed(&self) -> bool {
false
}
}

impl Debug for dyn VirtualCommandLine {
Expand Down Expand Up @@ -263,6 +290,24 @@ pub fn merge(
})
}

pub struct ComputedVcl {
vcl_args: Vec<Box<dyn VclArg>>,
}

impl VirtualCommandLine for ComputedVcl {
fn vcl_args(&self) -> Vec<&dyn VclArg> {
vcl_args_to_vcl_args(&self.vcl_args)
czarte marked this conversation as resolved.
Show resolved Hide resolved
}

fn args(&self) -> Vec<String> {
vcl_args_to_args(&self.vcl_args)
}

fn is_computed(&self) -> bool {
true
}
}

pub struct CommandLineVcl {
vcl_args: Vec<Box<dyn VclArg>>,
}
Expand All @@ -283,6 +328,33 @@ impl From<Vec<Box<dyn VclArg>>> for CommandLineVcl {
}
}

impl ComputedVcl {
czarte marked this conversation as resolved.
Show resolved Hide resolved
pub fn new(mut args: Vec<String>) -> ComputedVcl {
args.remove(0); // remove command
let mut vcl_args = vec![];
while let Some(vcl_arg) = Self::next_vcl_arg(&mut args) {
vcl_args.push(vcl_arg);
}
ComputedVcl { vcl_args }
}

fn next_vcl_arg(args: &mut Vec<String>) -> Option<Box<dyn VclArg>> {
if args.is_empty() {
return None;
}
let name = args.remove(0);
if !name.starts_with("--") {
panic!("Expected option beginning with '--', not {}", name)
}
if args.is_empty() || args[0].starts_with("--") {
Some(Box::new(NameOnlyVclArg::new(&name)))
} else {
let value = args.remove(0);
Some(Box::new(NameValueVclArg::new(&name, &value)))
}
}
}

impl CommandLineVcl {
pub fn new(mut args: Vec<String>) -> CommandLineVcl {
args.remove(0); // remove command
Expand Down Expand Up @@ -334,7 +406,7 @@ impl EnvironmentVcl {
.collect();
let mut vcl_args: Vec<Box<dyn VclArg>> = vec![];
for (upper_name, value) in std::env::vars() {
if (upper_name.len() < 5) || (&upper_name[0..5] != "MASQ_") {
if (upper_name.len() < 5) || (&upper_name[0..5] != "MASQ_") || (value == *"") {
continue;
}
let lower_name = str::replace(&upper_name[5..].to_lowercase(), "_", "-");
Expand All @@ -347,10 +419,19 @@ impl EnvironmentVcl {
}
}

#[derive(Debug)]
pub struct ConfigFileVcl {
vcl_args: Vec<Box<dyn VclArg>>,
}

impl Clone for ConfigFileVcl {
fn clone(&self) -> Self {
ConfigFileVcl {
vcl_args: self.vcl_args.iter().map(|arg| arg.dup()).collect(),
}
}
}

impl VirtualCommandLine for ConfigFileVcl {
fn vcl_args(&self) -> Vec<&dyn VclArg> {
vcl_args_to_vcl_args(&self.vcl_args)
Expand Down Expand Up @@ -491,8 +572,14 @@ fn append<T>(ts: Vec<T>, t: T) -> Vec<T> {

#[cfg(not(feature = "no_test_share"))]
impl<'a> MultiConfig<'a> {
pub fn new_test_only(arg_matches: ArgMatches<'a>) -> Self {
Self { arg_matches }
pub fn new_test_only(
arg_matches: ArgMatches<'a>,
computed_value_names: HashSet<String>,
) -> Self {
Self {
arg_matches,
computed_value_names,
}
}
}

Expand All @@ -504,6 +591,7 @@ pub mod tests {
use clap::Arg;
use std::fs::File;
use std::io::Write;
use std::ops::Deref;

#[test]
fn config_file_vcl_error_displays_open_error() {
Expand Down Expand Up @@ -949,6 +1037,40 @@ pub mod tests {
assert_eq!(subject.args(), command_line);
}

#[test]
fn command_line_vcl_return_value_from_vcl_args_by_name() {
let command_line: Vec<String> = vec![
"",
"--first-value",
"/nonexistent/directory",
"--takes_no_value",
"--other_takes_no_value",
]
.into_iter()
.map(|s| s.to_string())
.collect();

let subject = CommandLineVcl::new(command_line.clone());
let existing_value = match subject
.vcl_args
.iter()
.find(|vcl_arg_box| vcl_arg_box.deref().name() == "--first-value")
{
Some(vcl_arg_box) => vcl_arg_box.deref().value_opt(),
None => None,
};
let non_existing_value = match subject
.vcl_args
.iter()
.find(|vcl_arg_box| vcl_arg_box.deref().name() == "--takes_no_value")
{
Some(vcl_arg_box) => vcl_arg_box.deref().value_opt(),
None => None,
};
assert_eq!(existing_value.unwrap(), "/nonexistent/directory");
assert_eq!(non_existing_value, None);
}

#[test]
#[should_panic(expected = "Expected option beginning with '--', not value")]
fn command_line_vcl_panics_when_given_value_without_name() {
Expand All @@ -963,12 +1085,19 @@ pub mod tests {
#[test]
fn environment_vcl_works() {
let _guard = EnvironmentGuard::new();
let schema = App::new("test").arg(
Arg::with_name("numeric-arg")
.long("numeric-arg")
.takes_value(true),
);
let schema = App::new("test")
.arg(
Arg::with_name("numeric-arg")
.long("numeric-arg")
.takes_value(true),
)
.arg(
Arg::with_name("empty-arg")
.long("empty-arg")
.takes_value(true),
);
std::env::set_var("MASQ_NUMERIC_ARG", "47");
std::env::set_var("MASQ_EMPTY_ARG", "");

let subject = EnvironmentVcl::new(&schema);

Expand Down
11 changes: 7 additions & 4 deletions node/src/daemon/setup_reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::node_configurator::unprivileged_parse_args_configuration::{
UnprivilegedParseArgsConfigurationDaoReal,
};
use crate::node_configurator::{
data_directory_from_context, determine_fundamentals, DirsWrapper, DirsWrapperReal,
data_directory_from_context, determine_user_specific_data, DirsWrapper, DirsWrapperReal,
};
use crate::sub_lib::accountant::PaymentThresholds as PaymentThresholdsFromAccountant;
use crate::sub_lib::accountant::DEFAULT_SCAN_INTERVALS;
Expand Down Expand Up @@ -495,9 +495,12 @@ impl SetupReporterReal {
Some(command_line) => command_line,
None => vec![],
};
let (config_file_path, user_specified, _data_directory, _real_user) =
determine_fundamentals(dirs_wrapper, &app, &command_line)?;
let config_file_vcl = match ConfigFileVcl::new(&config_file_path, user_specified) {
let user_specific_data =
determine_user_specific_data(dirs_wrapper, &app, &command_line)?;
let config_file_vcl = match ConfigFileVcl::new(
&user_specific_data.config_file,
user_specific_data.config_file_spec,
) {
Ok(cfv) => cfv,
Err(e) => return Err(ConfiguratorError::required("config-file", &e.to_string())),
};
Expand Down
Loading
Loading