From 5451653b7e8acea34fbff336dadb2f4be88cd19d Mon Sep 17 00:00:00 2001 From: Fantix King Date: Mon, 15 Jul 2024 18:00:26 -0400 Subject: [PATCH] Properly fix overwriting dump file 1. Fix to always overwrite targets in /dev/* 2. Add a warning for the overwrite behavior change 3. Add a test for piping through stdin/out --- src/commands/dump.rs | 86 +++++++++++++++++++++++++------------- src/commands/parser.rs | 8 ++-- tests/func/dump_restore.rs | 54 ++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 33 deletions(-) diff --git a/src/commands/dump.rs b/src/commands/dump.rs index 2fcc96004..9c840e424 100644 --- a/src/commands/dump.rs +++ b/src/commands/dump.rs @@ -25,39 +25,60 @@ pub struct Guard { } impl Guard { - async fn open(filename: &Path, overwrite_existing: bool) -> anyhow::Result<(Output, Guard)> { + async fn open( + filename: &Path, + overwrite_existing: Option, + ) -> anyhow::Result<(Output, Guard)> { if filename.to_str() == Some("-") { Ok((Box::new(io::stdout()), Guard { filenames: None })) - } else if cfg!(windows) || filename.starts_with("/dev/") || filename.file_name().is_none() { - let file = OpenOptions::new() - .write(true) - .create(overwrite_existing) - .create_new(!overwrite_existing) - .truncate(overwrite_existing) - .open(&filename) + } else if filename.starts_with("/dev/") { + let file = fs::File::create(&filename) .await .context(filename.display().to_string())?; Ok((Box::new(file), Guard { filenames: None })) } else { - if !overwrite_existing && fs::metadata(&filename).await.is_ok() { - anyhow::bail!( - "failed: target file already exists. Specify --overwrite-existing to replace." - ) - } - // Create .~.tmp file path, first remove if already existing - let tmp_path = filename.with_file_name(tmp_file_name(filename)); - if fs::metadata(&tmp_path).await.is_ok() { - fs::remove_file(&tmp_path).await.ok(); + let overwrite_existing = overwrite_existing.unwrap_or_else(|| { + log::warn!( + "In the next EdgeDB CLI release, the dump behavior will \ + change to not overwrite the target file by default. For \ + compatibility, please specify --overwrite-existing to \ + preserve the current behavior, or --overwrite-existing=false \ + to adopt the new behavior." + ); + true + }); + if cfg!(windows) || filename.file_name().is_none() { + let file = OpenOptions::new() + .write(true) + .create(overwrite_existing) + .create_new(!overwrite_existing) + .truncate(overwrite_existing) + .open(&filename) + .await + .context(filename.display().to_string())?; + Ok((Box::new(file), Guard { filenames: None })) + } else { + if !overwrite_existing && fs::metadata(&filename).await.is_ok() { + anyhow::bail!( + "failed: target file already exists. \ + Specify --overwrite-existing to replace." + ) + } + // Create .~.tmp file path, first remove if already existing + let tmp_path = filename.with_file_name(tmp_file_name(filename)); + if fs::metadata(&tmp_path).await.is_ok() { + fs::remove_file(&tmp_path).await.ok(); + } + let tmp_file = fs::File::create(&tmp_path) + .await + .context(tmp_path.display().to_string())?; + Ok(( + Box::new(tmp_file), + Guard { + filenames: Some((tmp_path, filename.to_owned(), overwrite_existing)), + }, + )) } - let tmp_file = fs::File::create(&tmp_path) - .await - .context(tmp_path.display().to_string())?; - Ok(( - Box::new(tmp_file), - Guard { - filenames: Some((tmp_path, filename.to_owned(), overwrite_existing)), - }, - )) } } @@ -114,7 +135,7 @@ async fn dump_db( _options: &Options, filename: &Path, mut include_secrets: bool, - overwrite_existing: bool, + overwrite_existing: Option, ) -> Result<(), anyhow::Error> { if cli.get_version().await?.specific() < "4.0-alpha.2".parse().unwrap() { include_secrets = false; @@ -189,7 +210,7 @@ pub async fn dump_all( fs::create_dir_all(dir).await?; - let (mut init, guard) = Guard::open(&dir.join("init.edgeql"), true).await?; + let (mut init, guard) = Guard::open(&dir.join("init.edgeql"), Some(true)).await?; if !config.trim().is_empty() { init.write_all(b"# DESCRIBE SYSTEM CONFIG\n").await?; init.write_all(config.as_bytes()).await?; @@ -207,7 +228,14 @@ pub async fn dump_all( match conn_params.branch(database)?.connect().await { Ok(mut db_conn) => { let filename = dir.join(&(urlencoding::encode(database) + ".dump")[..]); - dump_db(&mut db_conn, options, &filename, include_secrets, true).await?; + dump_db( + &mut db_conn, + options, + &filename, + include_secrets, + Some(true), + ) + .await?; } Err(err) => { if let Some(e) = err.downcast_ref::() { diff --git a/src/commands/parser.rs b/src/commands/parser.rs index a6d7a682a..108610df9 100644 --- a/src/commands/parser.rs +++ b/src/commands/parser.rs @@ -1,6 +1,6 @@ use std::path::PathBuf; -use clap::ValueHint; +use clap::{ArgAction, ValueHint}; use crate::migrations::options::{Migrate, Migration}; use crate::options::ConnectionOptions; @@ -411,9 +411,9 @@ pub struct Dump { pub format: Option, /// Used to automatically overwrite existing files of the same name. Defaults - /// to `true`. - #[arg(long, default_value = "true")] - pub overwrite_existing: bool, + /// to `true` for now but will default to `false` in the next release. + #[arg(long, default_missing_value = "true", num_args = 0..=1, action = ArgAction::Set)] + pub overwrite_existing: Option, } #[derive(clap::Args, Clone, Debug)] diff --git a/tests/func/dump_restore.rs b/tests/func/dump_restore.rs index f112b7153..c314c195b 100644 --- a/tests/func/dump_restore.rs +++ b/tests/func/dump_restore.rs @@ -115,3 +115,57 @@ fn dump_restore_all() { new_instance.0.stop(); println!("query"); } + +#[!cfg(target_os = "windows")] +#[test] +fn dump_restore_dev() { + println!("before"); + SERVER + .admin_cmd() + .arg("database") + .arg("create") + .arg("dump_03") + .assert() + .success(); + println!("dbcreated"); + SERVER + .database_cmd("dump_03") + .arg("query") + .arg("CREATE TYPE Hello { CREATE REQUIRED PROPERTY name -> str; }") + .arg("INSERT Hello { name := 'world' }") + .assert() + .success(); + println!("Created"); + let dumped_data = SERVER + .admin_cmd() + .arg("dump") + .arg("/dev/stdout") + .assert() + .success() + .get_output(); + println!("dumped"); + SERVER + .admin_cmd() + .arg("database") + .arg("create") + .arg("restore_03") + .assert() + .success(); + println!("created2"); + SERVER + .database_cmd("restore_03") + .arg("restore") + .arg("/dev/stdin") + .write_stdin(&dumped_data.stdout) + .assert() + .success(); + println!("restored"); + SERVER + .database_cmd("restore_03") + .arg("query") + .arg("SELECT Hello.name") + .assert() + .success() + .stdout("\"world\"\n"); + println!("query"); +}