Skip to content

Commit e8b34f7

Browse files
Ludv1gLclaudeclockwork-labs-bot
authored
feat: Allow dropping empty tables during auto-migration (#4593)
## Summary - Removing a table from the schema previously always triggered `AutoMigrateError::RemoveTable`, forcing `--delete-data` which **nukes the entire database** - Now, empty tables can be dropped seamlessly during `spacetime publish` - Non-empty tables fail with an actionable error guiding the user to clear rows first ## Changes - **`auto_migrate.rs`**: Replace hard `RemoveTable` error with `AutoMigrateStep::RemoveTable`. Compute `removed_tables` set (same pattern as `new_tables`) and pass to `auto_migrate_indexes`/`auto_migrate_sequences`/`auto_migrate_constraints` to skip sub-object diffs for removed tables (cascade handled by `drop_table`) - **`update.rs`**: Execute `RemoveTable` step — O(1) emptiness check via `table_row_count_mut`, then `drop_table`. Fails with clear message if table has data - **`formatter.rs`** / **`termcolor_formatter.rs`**: Add `format_remove_table` to `MigrationFormatter` trait + implementation - **`publish.rs` (smoketests)**: Update existing test — removing empty table now succeeds without flags ## Safety - **Transaction safety**: Emptiness check and drop run in the same `MutTx` — no window for concurrent inserts - **Cascade**: `drop_table` already handles removing all indexes, constraints, and sequences for the table - **Sub-object filtering**: Indexes/constraints/sequences belonging to removed tables are filtered from their respective diffs, preventing orphan `RemoveIndex`/`RemoveConstraint`/`RemoveSequence` steps - **Rollback**: If the emptiness check fails, the entire migration aborts before any changes are applied ## Example error output (non-empty table) ``` Cannot remove table `MyTable`: table contains data. Clear the table's rows (e.g. via a reducer) before removing it from your schema. ``` ## Test plan - [x] `cargo check -p spacetimedb-schema -p spacetimedb-core` passes - [x] All 14 `auto_migrate` schema tests pass (2 new: `remove_table_produces_step`, `remove_table_does_not_produce_orphan_sub_object_steps`) - [x] All 3 `update` execution tests pass (2 new: `remove_empty_table_succeeds`, `remove_nonempty_table_fails`) - [x] Updated existing `auto_migration_errors` test (no longer expects `RemoveTable` error) - [x] Updated smoke test: `cli_can_publish_remove_empty_table` expects success 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: clockwork-labs-bot <clockwork-labs-bot@users.noreply.github.com>
1 parent fbec276 commit e8b34f7

File tree

4 files changed

+237
-42
lines changed

4 files changed

+237
-42
lines changed

crates/core/src/db/update.rs

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,19 @@ fn auto_migrate_database(
141141

142142
for step in plan.steps {
143143
match step {
144+
spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveTable(table_name) => {
145+
let table_id = stdb.table_id_from_name_mut(tx, table_name)?.unwrap();
146+
147+
if stdb.table_row_count_mut(tx, table_id).unwrap_or(0) > 0 {
148+
anyhow::bail!(
149+
"Cannot remove table `{table_name}`: table contains data. \
150+
Clear the table's rows (e.g. via a reducer) before removing it from your schema."
151+
);
152+
}
153+
154+
log!(logger, "Dropping table `{table_name}`");
155+
stdb.drop_table(tx, table_id)?;
156+
}
144157
spacetimedb_schema::auto_migrate::AutoMigrateStep::AddTable(table_name) => {
145158
let table_def: &TableDef = plan.new.expect_lookup(table_name);
146159

@@ -405,4 +418,92 @@ mod test {
405418

406419
Ok(())
407420
}
421+
422+
fn empty_module() -> ModuleDef {
423+
RawModuleDefV9Builder::new()
424+
.finish()
425+
.try_into()
426+
.expect("empty module should be valid")
427+
}
428+
429+
fn single_table_module() -> ModuleDef {
430+
let mut builder = RawModuleDefV9Builder::new();
431+
builder
432+
.build_table_with_new_type("droppable", [("id", U64)], true)
433+
.with_access(TableAccess::Public)
434+
.finish();
435+
builder
436+
.finish()
437+
.try_into()
438+
.expect("should be a valid module definition")
439+
}
440+
441+
#[test]
442+
fn remove_empty_table_succeeds() -> anyhow::Result<()> {
443+
let auth_ctx = AuthCtx::for_testing();
444+
let stdb = TestDB::durable()?;
445+
446+
let old = single_table_module();
447+
let new = empty_module();
448+
449+
let mut tx = begin_mut_tx(&stdb);
450+
for def in old.tables() {
451+
create_table_from_def(&stdb, &mut tx, &old, def)?;
452+
}
453+
stdb.commit_tx(tx)?;
454+
455+
let mut tx = begin_mut_tx(&stdb);
456+
let plan = ponder_migrate(&old, &new)?;
457+
let res = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?;
458+
459+
assert!(
460+
matches!(res, UpdateResult::RequiresClientDisconnect),
461+
"removing a table should disconnect clients"
462+
);
463+
assert!(stdb.table_id_from_name_mut(&tx, "droppable")?.is_none());
464+
// `drop_table` records a `TableRemoved` schema change for the subscription system.
465+
assert!(
466+
tx.pending_schema_changes()
467+
.iter()
468+
.any(|c| matches!(c, PendingSchemaChange::TableRemoved(..))),
469+
"dropping a table should produce a TableRemoved pending schema change: {:?}",
470+
tx.pending_schema_changes()
471+
);
472+
Ok(())
473+
}
474+
475+
#[test]
476+
fn remove_nonempty_table_fails() -> anyhow::Result<()> {
477+
let auth_ctx = AuthCtx::for_testing();
478+
let stdb = TestDB::durable()?;
479+
480+
let old = single_table_module();
481+
let new = empty_module();
482+
483+
let mut tx = begin_mut_tx(&stdb);
484+
for def in old.tables() {
485+
create_table_from_def(&stdb, &mut tx, &old, def)?;
486+
}
487+
let table_id = stdb
488+
.table_id_from_name_mut(&tx, "droppable")?
489+
.expect("table should exist");
490+
insert(&stdb, &mut tx, table_id, &product![42u64])?;
491+
stdb.commit_tx(tx)?;
492+
493+
let mut tx = begin_mut_tx(&stdb);
494+
let plan = ponder_migrate(&old, &new)?;
495+
let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger);
496+
let err = result.err().expect("removing a non-empty table should fail");
497+
assert!(
498+
err.to_string().contains("table contains data"),
499+
"error should mention that the table contains data, got: {err}"
500+
);
501+
// The migration failed, so no schema changes should be pending.
502+
assert!(
503+
tx.pending_schema_changes().is_empty(),
504+
"failed migration should leave no pending schema changes: {:?}",
505+
tx.pending_schema_changes()
506+
);
507+
Ok(())
508+
}
408509
}

0 commit comments

Comments
 (0)