Adds flag to publish to allow clearing on migration conflict. (#3601)

# Description of Changes

This PR modifies the `--delete-data` flag on `spacetime publish` and
adds the `--delete-data` flag on `spacetime dev`.

In particular instead of `--delete-data` being a boolean, it is now a an
enum:

- `always` -> corresponds to the old value of `true`
- `never` -> corresponds to the old value of `false`
- `on-conflict` -> clears the database, but only if publishing would
have required a manual migration

This flag does NOT change any behavior about prompting users to confirm
if they want to delete the data. Users will still be prompted to confirm
UNLESS they pass the separate `--yes` flag.

`spacetime dev` gets the same `--delete-data` flag. The default value of
`never` is equivalent to the existing behavior. `spacetime dev`
continues to publish with `--yes` just as before. This behavior is
unchanged.

# API and ABI breaking changes

Adds the flags specified above. This is NOT a breaking change to the
CLI. Passing `--delete-data` is the equivalent of
`--delete-data=always`.

This IS technically a breaking change to the `pre_publish` route. As far
as I'm aware this is *only* used by our CLI however.

> IMPORTANT SIDE NOTE: I would argue that `--break-clients` should
really be renamed to `--yes-break-clients` because it actually behaves
like the `--yes` force flag, but only for a subset of the user prompts.
I have not made this change because it would be a breaking change, but
if the reviewers agree, I will make this change.

# Expected complexity level and risk

2, Very small change, but if we get it wrong users could accidentally
lose data. I would ask reviewers to think about ways that users might
accidentally pass `--delete-data --yes`.

# Testing

- [ ] I have not yet tested manually.

---------

Signed-off-by: Tyler Cloutier <[email protected]>
Co-authored-by: Zeke Foppa <[email protected]>
Co-authored-by: Zeke Foppa <[email protected]>
Co-authored-by: John Detter <[email protected]>

Author: 3 people

crates/bindings/README.md

@@ -662,7 +662,7 @@ The following changes are forbidden without a manual migration:
 - ❌ **Changing whether a table is used for [scheduling](#scheduled-reducers).** <!-- TODO: update this if we ever actually implement it... -->
 - ❌ **Adding `#[unique]` or `#[primary_key]` constraints.** This could result in existing tables being in an invalid state.
 
-Currently, manual migration support is limited. The `spacetime publish --clear-database <DATABASE_IDENTITY>` command can be used to **COMPLETELY DELETE** and reinitialize your database, but naturally it should be used with EXTREME CAUTION.
+Currently, manual migration support is limited. The `spacetime publish --delete-data <DATABASE_IDENTITY>` command can be used to **COMPLETELY DELETE** and reinitialize your database, but naturally it should be used with EXTREME CAUTION.
 
 [macro library]: https://github.com/clockworklabs/SpacetimeDB/tree/master/crates/bindings-macro
 [module library]: https://github.com/clockworklabs/SpacetimeDB/tree/master/crates/lib

crates/cli/src/common_args.rs

@@ -1,5 +1,12 @@
-use clap::Arg;
 use clap::ArgAction::SetTrue;
+use clap::{value_parser, Arg, ValueEnum};
+
+#[derive(Copy, Clone, Debug, ValueEnum, PartialEq)]
+pub enum ClearMode {
+    Always,     // parses as "always"
+    OnConflict, // parses as "on-conflict"
+    Never,      // parses as "never"
+}
 
 pub fn server() -> Arg {
     Arg::new("server")
@@ -37,3 +44,20 @@ pub fn confirmed() -> Arg {
         .action(SetTrue)
         .help("Instruct the server to deliver only updates of confirmed transactions")
 }
+
+pub fn clear_database() -> Arg {
+    Arg::new("clear-database")
+        .long("delete-data")
+        .alias("clear-database")
+        .short('c')
+        .num_args(0..=1)
+        .value_parser(value_parser!(ClearMode))
+        // Because we have a default value for this flag, invocations can be ambiguous between
+        //passing a value to this flag, vs using the default value and passing an anonymous arg
+        // to the rest of the command. Adding `require_equals` resolves this ambiguity.
+        .require_equals(true)
+        .default_missing_value("always")
+        .help(
+            "When publishing to an existing database identity, first DESTROY all data associated with the module. With 'on-conflict': only when breaking schema changes occur."
+        )
+}

crates/cli/src/subcommands/dev.rs

@@ -1,3 +1,4 @@
+use crate::common_args::ClearMode;
 use crate::config::Config;
 use crate::generate::Language;
 use crate::subcommands::init;
@@ -71,6 +72,7 @@ pub fn cli() -> Command {
         )
         .arg(common_args::server().help("The nickname, host name or URL of the server to publish to"))
         .arg(common_args::yes())
+        .arg(common_args::clear_database())
 }
 
 #[derive(Deserialize)]
@@ -89,6 +91,10 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
     let spacetimedb_project_path = args.get_one::<PathBuf>("module-project-path").unwrap();
     let module_bindings_path = args.get_one::<PathBuf>("module-bindings-path").unwrap();
     let client_language = args.get_one::<Language>("client-lang");
+    let clear_database = args
+        .get_one::<ClearMode>("clear-database")
+        .copied()
+        .unwrap_or(ClearMode::Never);
     let force = args.get_flag("force");
 
     // If you don't specify a server, we default to your default server
@@ -236,6 +242,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
         &database_name,
         client_language,
         resolved_server,
+        clear_database,
     )
     .await?;
 
@@ -284,6 +291,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
                 &database_name,
                 client_language,
                 resolved_server,
+                clear_database,
             )
             .await
             {
@@ -339,6 +347,7 @@ fn upsert_env_db_names_and_hosts(env_path: &Path, server_host_url: &str, databas
     Ok(())
 }
 
+#[allow(clippy::too_many_arguments)]
 async fn generate_build_and_publish(
     config: &Config,
     project_dir: &Path,
@@ -347,6 +356,7 @@ async fn generate_build_and_publish(
     database_name: &str,
     client_language: Option<&Language>,
     server: &str,
+    clear_database: ClearMode,
 ) -> Result<(), anyhow::Error> {
     let module_language = detect_module_language(spacetimedb_dir)?;
     let client_language = client_language.unwrap_or(match module_language {
@@ -394,7 +404,20 @@ async fn generate_build_and_publish(
 
     let project_path_str = spacetimedb_dir.to_str().unwrap();
 
-    let mut publish_args = vec!["publish", database_name, "--project-path", project_path_str, "--yes"];
+    let clear_flag = match clear_database {
+        ClearMode::Always => "always",
+        ClearMode::Never => "never",
+        ClearMode::OnConflict => "on-conflict",
+    };
+    let mut publish_args = vec![
+        "publish",
+        database_name,
+        "--project-path",
+        project_path_str,
+        "--yes",
+        "--delete-data",
+        clear_flag,
+    ];
     publish_args.extend_from_slice(&["--server", server]);
 
     let publish_cmd = publish::cli();

crates/cli/src/subcommands/publish.rs

@@ -8,6 +8,7 @@ use spacetimedb_client_api_messages::name::{DatabaseNameError, PrePublishResult,
 use std::path::PathBuf;
 use std::{env, fs};
 
+use crate::common_args::ClearMode;
 use crate::config::Config;
 use crate::util::{add_auth_header_opt, get_auth_header, AuthHeader, ResponseExt};
 use crate::util::{decode_identity, y_or_n};
@@ -17,12 +18,8 @@ pub fn cli() -> clap::Command {
     clap::Command::new("publish")
         .about("Create and update a SpacetimeDB database")
         .arg(
-            Arg::new("clear_database")
-                .long("delete-data")
-                .short('c')
-                .action(SetTrue)
+            common_args::clear_database()
                 .requires("name|identity")
-                .help("When publishing to an existing database identity, first DESTROY all data associated with the module"),
         )
         .arg(
             Arg::new("build_options")
@@ -71,7 +68,7 @@ pub fn cli() -> clap::Command {
             Arg::new("break_clients")
                 .long("break-clients")
                 .action(SetTrue)
-                .help("Allow breaking changes when publishing to an existing database identity. This will break existing clients.")
+                .help("Allow breaking changes when publishing to an existing database identity. This will force publish even if it will break existing clients, but will NOT force publish if it would cause deletion of any data in the database. See --yes and --delete-data for details.")
         )
         .arg(
             common_args::anonymous()
@@ -109,15 +106,18 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
     let server = args.get_one::<String>("server").map(|s| s.as_str());
     let name_or_identity = args.get_one::<String>("name|identity");
     let path_to_project = args.get_one::<PathBuf>("project_path").unwrap();
-    let clear_database = args.get_flag("clear_database");
+    let clear_database = args
+        .get_one::<ClearMode>("clear-database")
+        .copied()
+        .unwrap_or(ClearMode::Never);
     let force = args.get_flag("force");
     let anon_identity = args.get_flag("anon_identity");
     let wasm_file = args.get_one::<PathBuf>("wasm_file");
     let js_file = args.get_one::<PathBuf>("js_file");
     let database_host = config.get_host_url(server)?;
     let build_options = args.get_one::<String>("build_options").unwrap();
     let num_replicas = args.get_one::<u8>("num_replicas");
-    let break_clients_flag = args.get_flag("break_clients");
+    let force_break_clients = args.get_flag("break_clients");
     let parent = args.get_one::<String>("parent");
 
     // If the user didn't specify an identity and we didn't specify an anonymous identity, then
@@ -175,7 +175,7 @@ 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 {
+        if clear_database != ClearMode::Always {
             builder = apply_pre_publish_if_needed(
                 builder,
                 &client,
@@ -184,7 +184,9 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
                 host_type,
                 &program_bytes,
                 &auth_header,
-                break_clients_flag,
+                clear_database,
+                force_break_clients,
+                force,
             )
             .await?;
         }
@@ -194,7 +196,7 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
         client.post(format!("{database_host}/v1/database"))
     };
 
-    if clear_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.",
@@ -336,25 +338,55 @@ async fn apply_pre_publish_if_needed(
     host_type: &str,
     program_bytes: &[u8],
     auth_header: &AuthHeader,
-    break_clients_flag: bool,
+    clear_database: ClearMode,
+    force_break_clients: bool,
+    force: bool,
 ) -> Result<reqwest::RequestBuilder, anyhow::Error> {
-    if let Some(pre) = call_pre_publish(client, base_url, domain, host_type, program_bytes, auth_header).await? {
-        println!("{}", pre.migrate_plan);
-
-        if pre.break_clients
-            && !y_or_n(
-                break_clients_flag,
-                "The above changes will BREAK existing clients. Do you want to proceed?",
-            )?
-        {
-            println!("Aborting");
-            // Early exit: return an error or a special signal. Here we bail out by returning Err.
-            anyhow::bail!("Publishing aborted by user");
+    if let Some(pre) = call_pre_publish(
+        client,
+        base_url,
+        &domain.to_string(),
+        host_type,
+        program_bytes,
+        auth_header,
+    )
+    .await?
+    {
+        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.");
+                }
+            }
+            PrePublishResult::AutoMigrate(auto) => {
+                println!("{}", auto.migrate_plan);
+                // We only arrive here if you have not specified ClearMode::Always AND there was no
+                // conflict that required manual migration.
+                if auto.break_clients
+                    && !y_or_n(
+                        force_break_clients || force,
+                        "The above changes will BREAK existing clients. Do you want to proceed?",
+                    )?
+                {
+                    println!("Aborting");
+                    // Early exit: return an error or a special signal. Here we bail out by returning Err.
+                    anyhow::bail!("Publishing aborted by user");
+                }
+                builder = builder
+                    .query(&[("token", auto.token)])
+                    .query(&[("policy", "BreakClients")]);
+            }
         }
-
-        builder = builder
-            .query(&[("token", pre.token)])
-            .query(&[("policy", "BreakClients")]);
     }
 
     Ok(builder)
@@ -369,7 +401,7 @@ async fn call_pre_publish(
     auth_header: &AuthHeader,
 ) -> Result<Option<PrePublishResult>, anyhow::Error> {
     let mut builder = client.post(format!("{database_host}/v1/database/{domain}/pre_publish"));
-    let style = pretty_print_style_from_env();
+    let style: PrettyPrintStyle = pretty_print_style_from_env();
     builder = builder
         .query(&[("pretty_print_style", style)])
         .query(&[("host_type", host_type)]);

crates/client-api-messages/src/name.rs

@@ -121,12 +121,23 @@ pub enum PrettyPrintStyle {
 }
 
 #[derive(serde::Serialize, serde::Deserialize, Debug)]
-pub struct PrePublishResult {
+pub enum PrePublishResult {
+    AutoMigrate(PrePublishAutoMigrateResult),
+    ManualMigrate(PrePublishManualMigrateResult),
+}
+
+#[derive(serde::Serialize, serde::Deserialize, Debug)]
+pub struct PrePublishAutoMigrateResult {
     pub migrate_plan: Box<str>,
     pub break_clients: bool,
     pub token: spacetimedb_lib::Hash,
 }
 
+#[derive(serde::Serialize, serde::Deserialize, Debug)]
+pub struct PrePublishManualMigrateResult {
+    pub reason: String,
+}
+
 #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
 pub enum DnsLookupResponse {
     /// The lookup was successful and the domain and identity are returned.

crates/client-api/src/routes/database.rs

@@ -33,7 +33,8 @@ use spacetimedb::identity::Identity;
 use spacetimedb::messages::control_db::{Database, HostType};
 use spacetimedb_client_api_messages::http::SqlStmtResult;
 use spacetimedb_client_api_messages::name::{
-    self, DatabaseName, DomainName, MigrationPolicy, PrePublishResult, PrettyPrintStyle, PublishOp, PublishResult,
+    self, DatabaseName, DomainName, MigrationPolicy, PrePublishAutoMigrateResult, PrePublishManualMigrateResult,
+    PrePublishResult, PrettyPrintStyle, PublishOp, PublishResult,
 };
 use spacetimedb_lib::db::raw_def::v9::RawModuleDefV9;
 use spacetimedb_lib::{sats, AlgebraicValue, Hash, ProductValue, Timestamp};
@@ -959,17 +960,17 @@ pub async fn pre_publish<S: NodeDelegate + ControlStateDelegate + Authorization>
             }
             .hash();
 
-            Ok(PrePublishResult {
+            Ok(PrePublishResult::AutoMigrate(PrePublishAutoMigrateResult {
                 token,
                 migrate_plan: plan,
                 break_clients: breaks_client,
-            })
+            }))
+        }
+        MigratePlanResult::AutoMigrationError(e) => {
+            Ok(PrePublishResult::ManualMigrate(PrePublishManualMigrateResult {
+                reason: e.to_string(),
+            }))
         }
-        MigratePlanResult::AutoMigrationError(e) => Err((
-            StatusCode::BAD_REQUEST,
-            format!("Automatic migration is not possible: {e}"),
-        )
-            .into()),
     }
     .map(axum::Json)
 }

docs/docs/06-Server Module Languages/04-csharp-reference.md

@@ -1079,7 +1079,7 @@ The following changes are forbidden without a manual migration:
 - ❌ **Changing whether a table is used for [scheduling](#scheduled-reducers).** <!-- TODO: update this if we ever actually implement it... -->
 - ❌ **Adding `[Unique]` or `[PrimaryKey]` constraints.** This could result in existing tables being in an invalid state.
 
-Currently, manual migration support is limited. The `spacetime publish --clear-database <DATABASE_IDENTITY>` command can be used to **COMPLETELY DELETE** and reinitialize your database, but naturally it should be used with EXTREME CAUTION.
+Currently, manual migration support is limited. The `spacetime publish --delete-data <DATABASE_IDENTITY>` command can be used to **COMPLETELY DELETE** and reinitialize your database, but naturally it should be used with EXTREME CAUTION.
 
 ## Other infrastructure
 

docs/docs/06-Server Module Languages/06-Typescript-reference.md

@@ -644,7 +644,7 @@ The following deletes all data stored in the database.
 To fully reset your database and clear all data, run:
 
 ```bash
-spacetime publish --clear-database <DATABASE_NAME>
+spacetime publish --delete-data <DATABASE_NAME>
 # or
 spacetime publish -c <DATABASE_NAME>
 ```

smoketests/tests/modules.py

@@ -65,7 +65,7 @@ class UpdateModule(Smoketest):
 
 
     def test_module_update(self):
-        """Test publishing a module without the --clear-database option"""
+        """Test publishing a module without the --delete-data option"""
 
         name = random_string()
 
@@ -88,7 +88,7 @@ def test_module_update(self):
         self.write_module_code(self.MODULE_CODE_B)
         with self.assertRaises(CalledProcessError) as cm:
             self.publish_module(name, clear=False)
-        self.assertIn("Error: Pre-publish check failed", cm.exception.stderr)
+        self.assertIn("Error: Aborting because publishing would require manual migration", cm.exception.stderr)
 
         # Check that the old module is still running by calling say_hello
         self.call("say_hello")

smoketests/tests/permissions.py

@@ -54,10 +54,9 @@ def test_publish(self):
         self.new_identity()
 
         with self.assertRaises(Exception):
-            # TODO: This raises for the wrong reason - `--clear-database` doesn't exist anymore!
-            self.spacetime("publish", self.database_identity, "--project-path", self.project_path, "--clear-database", "--yes")
+            self.spacetime("publish", self.database_identity, "--project-path", self.project_path, "--delete-data", "--yes")
 
-        # Check that this holds without `--clear-database`, too.
+        # Check that this holds without `--delete-data`, too.
         with self.assertRaises(Exception):
             self.spacetime("publish", self.database_identity, "--project-path", self.project_path, "--yes")