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.

Sign up for GitHub

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

refactor: Use PathBuf for paths in flag parsing and whitelists #3955

Merged
merged 5 commits into from
Feb 11, 2020
Merged

refactor: Use PathBuf for paths in flag parsing and whitelists #3955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb5ce24
Use PathBuf for DenoSubcommand::Bundle's out_file
nayeemrmn Feb 10, 2020
b2565f1
Use PathBuf for DenoSubcommand::Format's files
nayeemrmn Feb 10, 2020
2075fb4
Use PathBuf for DenoSubcommand::Install's dir
nayeemrmn Feb 10, 2020
45dc156
Use PathBuf for read/write whitelists
nayeemrmn Feb 10, 2020
3f6db61
Lint
nayeemrmn Feb 10, 2020
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
6 changes: 3 additions & 3 deletions cli/compilers/ts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ fn req(
request_type: msg::CompilerRequestType,
root_names: Vec<String>,
compiler_config: CompilerConfig,
out_file: Option<String>,
out_file: Option<PathBuf>,
target: &str,
bundle: bool,
) -> Buf {
Expand Down Expand Up @@ -271,7 +271,7 @@ impl TsCompiler {
&self,
global_state: GlobalState,
module_name: String,
out_file: Option<String>,
out_file: Option<PathBuf>,
) -> Result<(), ErrBox> {
debug!(
"Invoking the compiler to bundle. module_name: {}",
Expand Down Expand Up @@ -743,7 +743,7 @@ mod tests {
.bundle_async(
state.clone(),
module_name,
Some(String::from("$deno$/bundle.js")),
Some(PathBuf::from("$deno$/bundle.js")),
)
.await;
assert!(result.is_ok());
Expand Down
81 changes: 39 additions & 42 deletions cli/flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use clap::ArgMatches;
use clap::SubCommand;
use log::Level;
use std::collections::HashSet;
use std::path::Path;
use std::path::{Path, PathBuf};

/// Creates vector of strings, Vec<String>
macro_rules! svec {
Expand Down Expand Up @@ -35,7 +35,7 @@ const TEST_RUNNER_URL: &str = std_url!("testing/runner.ts");
pub enum DenoSubcommand {
Bundle {
source_file: String,
out_file: Option<String>,
out_file: Option<PathBuf>,
},
Completions {
buf: Box<[u8]>,
Expand All @@ -48,14 +48,14 @@ pub enum DenoSubcommand {
},
Format {
check: bool,
files: Option<Vec<String>>,
files: Option<Vec<PathBuf>>,
},
Help,
Info {
file: Option<String>,
},
Install {
dir: Option<String>,
dir: Option<PathBuf>,
exe_name: String,
module_url: String,
args: Vec<String>,
Expand Down Expand Up @@ -87,10 +87,10 @@ pub struct DenoFlags {
pub config_path: Option<String>,
pub import_map_path: Option<String>,
pub allow_read: bool,
pub read_whitelist: Vec<String>,
pub read_whitelist: Vec<PathBuf>,
pub cache_blacklist: Vec<String>,
pub allow_write: bool,
pub write_whitelist: Vec<String>,
pub write_whitelist: Vec<PathBuf>,
pub allow_net: bool,
pub net_whitelist: Vec<String>,
pub allow_env: bool,
Expand All @@ -107,14 +107,22 @@ pub struct DenoFlags {
pub lock_write: bool,
}

fn join_paths(whitelist: &[PathBuf], d: &str) -> String {
whitelist
.iter()
.map(|path| path.to_str().unwrap().to_string())
.collect::<Vec<String>>()
.join(d)
}

impl DenoFlags {
/// Return list of permission arguments that are equivalent
/// to the ones used to create `self`.
pub fn to_permission_args(&self) -> Vec<String> {
let mut args = vec![];

if !self.read_whitelist.is_empty() {
let s = format!("--allow-read={}", self.read_whitelist.join(","));
let s = format!("--allow-read={}", join_paths(&self.read_whitelist, ","));
args.push(s);
}

Expand All @@ -123,7 +131,8 @@ impl DenoFlags {
}

if !self.write_whitelist.is_empty() {
let s = format!("--allow-write={}", self.write_whitelist.join(","));
let s =
format!("--allow-write={}", join_paths(&self.write_whitelist, ","));
args.push(s);
}

Expand Down Expand Up @@ -297,7 +306,7 @@ fn types_parse(flags: &mut DenoFlags, _matches: &clap::ArgMatches) {
fn fmt_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
let maybe_files = match matches.values_of("files") {
Some(f) => {
let files: Vec<String> = f.map(String::from).collect();
let files: Vec<PathBuf> = f.map(PathBuf::from).collect();
Some(files)
}
None => None,
Expand All @@ -316,7 +325,7 @@ fn install_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {

let dir = if matches.is_present("dir") {
let install_dir = matches.value_of("dir").unwrap();
Some(install_dir.to_string())
Some(PathBuf::from(install_dir))
} else {
None
};
Expand Down Expand Up @@ -347,7 +356,7 @@ fn bundle_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {

let out_file = if let Some(out_file) = matches.value_of("out_file") {
flags.allow_write = true;
Some(out_file.to_string())
Some(PathBuf::from(out_file))
} else {
None
};
Expand Down Expand Up @@ -428,16 +437,10 @@ fn lock_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
}
}

fn resolve_fs_whitelist(whitelist: &[String]) -> Vec<String> {
fn resolve_fs_whitelist(whitelist: &[PathBuf]) -> Vec<PathBuf> {
whitelist
.iter()
.map(|raw_path| {
resolve_from_cwd(Path::new(&raw_path))
.unwrap()
.to_str()
.unwrap()
.to_owned()
})
.map(|raw_path| resolve_from_cwd(Path::new(&raw_path)).unwrap())
.collect()
}

Expand Down Expand Up @@ -998,8 +1001,8 @@ fn permission_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
if matches.is_present("allow-read") {
if matches.value_of("allow-read").is_some() {
let read_wl = matches.values_of("allow-read").unwrap();
let raw_read_whitelist: Vec<String> =
read_wl.map(std::string::ToString::to_string).collect();
let raw_read_whitelist: Vec<PathBuf> =
read_wl.map(PathBuf::from).collect();
flags.read_whitelist = resolve_fs_whitelist(&raw_read_whitelist);
debug!("read whitelist: {:#?}", &flags.read_whitelist);
} else {
Expand All @@ -1009,10 +1012,9 @@ fn permission_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
if matches.is_present("allow-write") {
if matches.value_of("allow-write").is_some() {
let write_wl = matches.values_of("allow-write").unwrap();
let raw_write_whitelist: Vec<String> =
write_wl.map(std::string::ToString::to_string).collect();
flags.write_whitelist =
resolve_fs_whitelist(raw_write_whitelist.as_slice());
let raw_write_whitelist: Vec<PathBuf> =
write_wl.map(PathBuf::from).collect();
flags.write_whitelist = resolve_fs_whitelist(&raw_write_whitelist);
debug!("write whitelist: {:#?}", &flags.write_whitelist);
} else {
flags.allow_write = true;
Expand Down Expand Up @@ -1376,7 +1378,10 @@ mod tests {
DenoFlags {
subcommand: DenoSubcommand::Format {
check: false,
files: Some(svec!["script_1.ts", "script_2.ts"])
files: Some(vec![
PathBuf::from("script_1.ts"),
PathBuf::from("script_2.ts")
])
},
..DenoFlags::default()
}
Expand Down Expand Up @@ -1544,23 +1549,19 @@ mod tests {
#[test]
fn allow_read_whitelist() {
use tempfile::TempDir;
let temp_dir = TempDir::new().expect("tempdir fail");
let temp_dir_path = temp_dir.path().to_str().unwrap();
let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf();

let r = flags_from_vec_safe(svec![
"deno",
"run",
format!("--allow-read=.,{}", &temp_dir_path),
format!("--allow-read=.,{}", temp_dir.to_str().unwrap()),
"script.ts"
]);
assert_eq!(
r.unwrap(),
DenoFlags {
allow_read: false,
read_whitelist: svec![
current_dir().unwrap().to_str().unwrap().to_owned(),
&temp_dir_path
],
read_whitelist: vec![current_dir().unwrap(), temp_dir],
subcommand: DenoSubcommand::Run {
script: "script.ts".to_string(),
},
Expand All @@ -1572,23 +1573,19 @@ mod tests {
#[test]
fn allow_write_whitelist() {
use tempfile::TempDir;
let temp_dir = TempDir::new().expect("tempdir fail");
let temp_dir_path = temp_dir.path().to_str().unwrap();
let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf();

let r = flags_from_vec_safe(svec![
"deno",
"run",
format!("--allow-write=.,{}", &temp_dir_path),
format!("--allow-write=.,{}", temp_dir.to_str().unwrap()),
"script.ts"
]);
assert_eq!(
r.unwrap(),
DenoFlags {
allow_write: false,
write_whitelist: svec![
current_dir().unwrap().to_str().unwrap().to_owned(),
&temp_dir_path
],
write_whitelist: vec![current_dir().unwrap(), temp_dir],
subcommand: DenoSubcommand::Run {
script: "script.ts".to_string(),
},
Expand Down Expand Up @@ -1677,7 +1674,7 @@ mod tests {
DenoFlags {
subcommand: DenoSubcommand::Bundle {
source_file: "source.ts".to_string(),
out_file: Some("bundle.js".to_string()),
out_file: Some(PathBuf::from("bundle.js")),
},
allow_write: true,
..DenoFlags::default()
Expand Down Expand Up @@ -1868,7 +1865,7 @@ mod tests {
r.unwrap(),
DenoFlags {
subcommand: DenoSubcommand::Install {
dir: Some("/usr/local/bin".to_string()),
dir: Some(PathBuf::from("/usr/local/bin")),
exe_name: "file_server".to_string(),
module_url: "https://deno.land/std/http/file_server.ts".to_string(),
args: svec!["arg1", "arg2"],
Expand Down
10 changes: 5 additions & 5 deletions cli/fmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@ fn format_source_files(
);
}

fn get_matching_files(glob_paths: Vec<String>) -> Vec<PathBuf> {
fn get_matching_files(glob_paths: Vec<PathBuf>) -> Vec<PathBuf> {
let mut target_files = Vec::with_capacity(128);

for path in glob_paths {
let files = glob::glob(&path)
let files = glob::glob(&path.to_str().unwrap())
.expect("Failed to execute glob.")
.filter_map(Result::ok);
target_files.extend(files);
Expand All @@ -165,14 +165,14 @@ fn get_matching_files(glob_paths: Vec<String>) -> Vec<PathBuf> {
/// First argument supports globs, and if it is `None`
/// then the current directory is recursively walked.
pub fn format_files(
maybe_files: Option<Vec<String>>,
maybe_files: Option<Vec<PathBuf>>,
check: bool,
) -> Result<(), ErrBox> {
// TODO: improve glob to look for tsx?/jsx? files only
let glob_paths = maybe_files.unwrap_or_else(|| vec!["**/*".to_string()]);
let glob_paths = maybe_files.unwrap_or_else(|| vec![PathBuf::from("**/*")]);
Copy link
Member

Choose a reason for hiding this comment

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

This seems odd/wrong PathBuf::from("**/*") ...

Copy link
Member

@bartlomieju bartlomieju Feb 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @ry - it'd be better to rename field on Subcommand::Format to globs and keep Vec<String> type.

Otherwise +1 from me, stricter types are always good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I've seen it's normal for glob patterns to be stored as "paths". They have distinctly separated segments (since any special syntax is limited to a segment), absoluteness, parent paths, it's useful to join() them, normalize() them, etc. Are you sure this is a problem?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense, I've seen glob/globset crate do that. LGTM


for glob_path in glob_paths.iter() {
if glob_path == "-" {
if glob_path.to_str().unwrap() == "-" {
// If the only given path is '-', format stdin.
if glob_paths.len() == 1 {
format_stdin(check);
Expand Down
10 changes: 5 additions & 5 deletions cli/installer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,14 @@ fn get_installer_dir() -> Result<PathBuf, Error> {

pub fn install(
flags: DenoFlags,
installation_dir: Option<String>,
installation_dir: Option<PathBuf>,
exec_name: &str,
module_url: &str,
args: Vec<String>,
force: bool,
) -> Result<(), Error> {
let installation_dir = if let Some(dir) = installation_dir {
PathBuf::from(dir).canonicalize()?
dir.canonicalize()?
} else {
get_installer_dir()?
};
Expand Down Expand Up @@ -245,7 +245,7 @@ mod tests {
let temp_dir = TempDir::new().expect("tempdir fail");
install(
DenoFlags::default(),
Some(temp_dir.path().to_string_lossy().to_string()),
Some(temp_dir.path().to_path_buf()),
"echo_test",
"http://localhost:4545/cli/tests/echo_server.ts",
vec![],
Expand Down Expand Up @@ -274,7 +274,7 @@ mod tests {
allow_read: true,
..DenoFlags::default()
},
Some(temp_dir.path().to_string_lossy().to_string()),
Some(temp_dir.path().to_path_buf()),
"echo_test",
"http://localhost:4545/cli/tests/echo_server.ts",
vec!["--foobar".to_string()],
Expand All @@ -301,7 +301,7 @@ mod tests {

install(
DenoFlags::default(),
Some(temp_dir.path().to_string_lossy().to_string()),
Some(temp_dir.path().to_path_buf()),
"echo_test",
&local_module_str,
vec![],
Expand Down
7 changes: 4 additions & 3 deletions cli/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ use log::Level;
use log::Metadata;
use log::Record;
use std::env;
use std::path::PathBuf;

static LOGGER: Logger = Logger;

Expand Down Expand Up @@ -258,7 +259,7 @@ async fn info_command(flags: DenoFlags, file: Option<String>) {

async fn install_command(
flags: DenoFlags,
dir: Option<String>,
dir: Option<PathBuf>,
exe_name: String,
module_url: String,
args: Vec<String>,
Expand Down Expand Up @@ -331,7 +332,7 @@ async fn eval_command(flags: DenoFlags, code: String) {
async fn bundle_command(
flags: DenoFlags,
source_file: String,
out_file: Option<String>,
out_file: Option<PathBuf>,
) {
debug!(">>>>> bundle_async START");
let source_file_specifier =
Expand Down Expand Up @@ -404,7 +405,7 @@ async fn run_script(flags: DenoFlags, script: String) {
js_check(worker.execute("window.dispatchEvent(new Event('unload'))"));
}

async fn fmt_command(files: Option<Vec<String>>, check: bool) {
async fn fmt_command(files: Option<Vec<PathBuf>>, check: bool) {
if let Err(err) = fmt::format_files(files, check) {
print_err_and_exit(err);
}
Expand Down
Loading