Fixes issues with --delete-data=on-conflict (#3730)

# Description of Changes

Fixes #3729

I genuinely don't know what came over me.

# API and ABI breaking changes

None

# Expected complexity level and risk

1.5 very straightforward but not strictly trivial

# Testing
Adds automated integration tests (written in Rust and run with `cargo
test`, although note this comment from @matklad about integration tests
for the future
https://internals.rust-lang.org/t/running-test-crates-in-parallel/15639/2):

- [x] Can publish an updated module if no migration is required
- [x] Can publish an updated module if auto-migration is required (with
the yes-break flag true/false)
- [x] Cannot publish if a manual migration is required
- [x] Can publish if a manual migration is required but the user
specified `--delete-data`
- [x] Can publish if a manual migration is required by the user
specified `--delete-data=on-conflict`
- [x] No data deletion occurs if no migration is required and
`--delete-data=on-conflict` is specified

---------

Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
Co-authored-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
Co-authored-by: Zeke Foppa <bfops@users.noreply.github.com>
Co-authored-by: Phoebe Goldman <phoebe@clockworklabs.io>
Co-authored-by: John Detter <4099508+jdetter@users.noreply.github.com>

Author: 4 people

Cargo.lock

@@ -86,6 +86,11 @@ notify.workspace = true
 
 [dev-dependencies]
 pretty_assertions.workspace = true
+fs_extra.workspace = true
+assert_cmd = "2"
+predicates = "3"
+portpicker = "0.1"
+reqwest = { workspace = true, features = ["blocking", "json"] }
 
 [target.'cfg(not(target_env = "msvc"))'.dependencies]
 tikv-jemallocator = { workspace = true }

crates/cli/Cargo.toml

@@ -22,6 +22,16 @@ pub fn cli() -> clap::Command {
                 .default_value("src")
                 .help("The directory to lint for nonfunctional print statements. If set to the empty string, skips linting.")
         )
+        .arg(
+            // TODO: Make this into --extra-build-args (or something similar) that will get passed along to the language's compiler.
+            Arg::new("features")
+                .long("features")
+                .value_parser(clap::value_parser!(OsString))
+                .required(false)
+                .help("Additional features to pass to the build process (e.g. `--features feature1,feature2` for Rust modules).")
+                // We're hiding this because we think it deserves a refactor first (see the TODO above)
+                .hide(true)
+        )
         .arg(
             Arg::new("debug")
                 .long("debug")
@@ -33,6 +43,7 @@ pub fn cli() -> clap::Command {
 
 pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(PathBuf, &'static str), anyhow::Error> {
     let project_path = args.get_one::<PathBuf>("project_path").unwrap();
+    let features = args.get_one::<OsString>("features");
     let lint_dir = args.get_one::<OsString>("lint_dir").unwrap();
     let lint_dir = if lint_dir.is_empty() {
         None
@@ -56,7 +67,7 @@ pub async fn exec(_config: Config, args: &ArgMatches) -> Result<(PathBuf, &'stat
         ));
     }
 
-    let result = crate::tasks::build(project_path, lint_dir.as_deref(), build_debug)?;
+    let result = crate::tasks::build(project_path, lint_dir.as_deref(), build_debug, features)?;
     println!("Build finished successfully.");
 
     Ok(result)

crates/cli/src/subcommands/build.rs

@@ -94,7 +94,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
     let clear_database = args
         .get_one::<ClearMode>("clear-database")
         .copied()
-        .unwrap_or(ClearMode::Never);
+        .unwrap_or(ClearMode::OnConflict);
     let force = args.get_flag("force");
 
     // If you don't specify a server, we default to your default server
@@ -388,7 +388,7 @@ async fn generate_build_and_publish(
 
     println!("{}", "Building...".cyan());
     let (_path_to_program, _host_type) =
-        tasks::build(spacetimedb_dir, Some(Path::new("src")), false).context("Failed to build project")?;
+        tasks::build(spacetimedb_dir, Some(Path::new("src")), false, None).context("Failed to build project")?;
     println!("{}", "Build complete!".green());
 
     println!("{}", "Generating module bindings...".cyan());
@@ -413,15 +413,14 @@ async fn generate_build_and_publish(
         ClearMode::OnConflict => "on-conflict",
     };
     let mut publish_args = vec![
-        "publish",
-        database_name,
-        "--project-path",
-        project_path_str,
-        "--yes",
-        "--delete-data",
-        clear_flag,
+        "publish".to_string(),
+        database_name.to_string(),
+        "--project-path".to_string(),
+        project_path_str.to_string(),
+        "--yes".to_string(),
+        format!("--delete-data={}", clear_flag),
     ];
-    publish_args.extend_from_slice(&["--server", server]);
+    publish_args.extend_from_slice(&["--server".to_string(), server.to_string()]);
 
     let publish_cmd = publish::cli();
     let publish_matches = publish_cmd

crates/cli/src/subcommands/dev.rs

@@ -102,6 +102,26 @@ i.e. only lowercase ASCII letters and numbers, separated by dashes."),
         .after_help("Run `spacetime help publish` for more detailed information.")
 }
 
+fn confirm_and_clear(
+    name_or_identity: &str,
+    force: bool,
+    mut builder: reqwest::RequestBuilder,
+) -> Result<reqwest::RequestBuilder, anyhow::Error> {
+    println!(
+        "This will DESTROY the current {} module, and ALL corresponding data.",
+        name_or_identity
+    );
+    if !y_or_n(
+        force,
+        format!("Are you sure you want to proceed? [deleting {}]", name_or_identity).as_str(),
+    )? {
+        anyhow::bail!("Aborted.");
+    }
+
+    builder = builder.query(&[("clear", true)]);
+    Ok(builder)
+}
+
 pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
     let server = args.get_one::<String>("server").map(|s| s.as_str());
     let name_or_identity = args.get_one::<String>("name|identity");
@@ -175,11 +195,15 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
         let domain = percent_encoding::percent_encode(name_or_identity.as_bytes(), encode_set);
         let mut builder = client.put(format!("{database_host}/v1/database/{domain}"));
 
-        if clear_database != ClearMode::Always {
+        // note that this only happens in the case where we've passed a `name_or_identity`, but that's required if we pass `--clear-database`.
+        if clear_database == ClearMode::Always {
+            builder = confirm_and_clear(name_or_identity, force, builder)?;
+        } else {
             builder = apply_pre_publish_if_needed(
                 builder,
                 &client,
                 &database_host,
+                name_or_identity,
                 &domain.to_string(),
                 host_type,
                 &program_bytes,
@@ -196,25 +220,6 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
         client.post(format!("{database_host}/v1/database"))
     };
 
-    if clear_database == ClearMode::Always || clear_database == ClearMode::OnConflict {
-        // Note: `name_or_identity` should be set, because it is `required` in the CLI arg config.
-        println!(
-            "This will DESTROY the current {} module, and ALL corresponding data.",
-            name_or_identity.unwrap()
-        );
-        if !y_or_n(
-            force,
-            format!(
-                "Are you sure you want to proceed? [deleting {}]",
-                name_or_identity.unwrap()
-            )
-            .as_str(),
-        )? {
-            println!("Aborting");
-            return Ok(());
-        }
-        builder = builder.query(&[("clear", true)]);
-    }
     if let Some(n) = num_replicas {
         eprintln!("WARNING: Use of unstable option `--num-replicas`.\n");
         builder = builder.query(&[("num_replicas", *n)]);
@@ -334,6 +339,7 @@ async fn apply_pre_publish_if_needed(
     mut builder: reqwest::RequestBuilder,
     client: &reqwest::Client,
     base_url: &str,
+    name_or_identity: &str,
     domain: &String,
     host_type: &str,
     program_bytes: &[u8],
@@ -342,6 +348,9 @@ async fn apply_pre_publish_if_needed(
     force_break_clients: bool,
     force: bool,
 ) -> Result<reqwest::RequestBuilder, anyhow::Error> {
+    // The caller enforces this
+    assert!(clear_database != ClearMode::Always);
+
     if let Some(pre) = call_pre_publish(
         client,
         base_url,
@@ -354,19 +363,15 @@ async fn apply_pre_publish_if_needed(
     {
         match pre {
             PrePublishResult::ManualMigrate(manual) => {
-                if clear_database == ClearMode::OnConflict {
-                    println!("{}", manual.reason);
-                    println!("Proceeding with database clear due to --delete-data=on-conflict.");
-                }
                 if clear_database == ClearMode::Never {
                     println!("{}", manual.reason);
                     println!("Aborting publish due to required manual migration.");
                     anyhow::bail!("Aborting because publishing would require manual migration or deletion of data and --delete-data was not specified.");
                 }
-                if clear_database == ClearMode::Always {
-                    println!("{}", manual.reason);
-                    println!("Proceeding with database clear due to --delete-data=always.");
-                }
+                println!("{}", manual.reason);
+                println!("Proceeding with database clear due to --delete-data=on-conflict.");
+
+                builder = confirm_and_clear(name_or_identity, force, builder)?;
             }
             PrePublishResult::AutoMigrate(auto) => {
                 println!("{}", auto.migrate_plan);

crates/cli/src/subcommands/publish.rs

@@ -13,10 +13,14 @@ pub fn build(
     project_path: &Path,
     lint_dir: Option<&Path>,
     build_debug: bool,
+    features: Option<&std::ffi::OsString>,
 ) -> anyhow::Result<(PathBuf, &'static str)> {
     let lang = util::detect_module_language(project_path)?;
+    if features.is_some() && lang != ModuleLanguage::Rust {
+        anyhow::bail!("The --features option is only supported for Rust modules.");
+    }
     let output_path = match lang {
-        ModuleLanguage::Rust => build_rust(project_path, lint_dir, build_debug),
+        ModuleLanguage::Rust => build_rust(project_path, features, lint_dir, build_debug),
         ModuleLanguage::Csharp => build_csharp(project_path, build_debug),
         ModuleLanguage::Javascript => build_javascript(project_path, build_debug),
     }?;