Skip to content

Commit e35fcdd

Browse files
Ludv1gLclaude
andcommitted
feat: Allow dropping empty tables during auto-migration
Previously, removing a table from the schema always required a manual migration (--delete-data), which nukes the entire database. Now, empty tables can be dropped seamlessly during publish. - Add RemoveTable variant to AutoMigrateStep (schema layer) - Remove AutoMigrateError::RemoveTable (no longer an error) - Filter sub-object diffs (indexes, constraints, sequences) for removed tables so drop_table handles cascade - Validate emptiness at execution time via table_row_count_mut (O(1)) - Non-empty tables fail with actionable error: "Clear the table's rows (e.g. via a reducer) before removing it from your schema" - Add format_remove_table to MigrationFormatter trait + termcolor impl - Update smoke test: removing empty table now succeeds without flags Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b391d73 commit e35fcdd

5 files changed

Lines changed: 187 additions & 29 deletions

File tree

crates/core/src/db/update.rs

Lines changed: 83 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,74 @@ 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 _ = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?;
458+
459+
assert!(stdb.table_id_from_name_mut(&tx, "droppable")?.is_none());
460+
Ok(())
461+
}
462+
463+
#[test]
464+
fn remove_nonempty_table_fails() -> anyhow::Result<()> {
465+
let auth_ctx = AuthCtx::for_testing();
466+
let stdb = TestDB::durable()?;
467+
468+
let old = single_table_module();
469+
let new = empty_module();
470+
471+
let mut tx = begin_mut_tx(&stdb);
472+
for def in old.tables() {
473+
create_table_from_def(&stdb, &mut tx, &old, def)?;
474+
}
475+
let table_id = stdb
476+
.table_id_from_name_mut(&tx, "droppable")?
477+
.expect("table should exist");
478+
insert(&stdb, &mut tx, table_id, &product![42u64])?;
479+
stdb.commit_tx(tx)?;
480+
481+
let mut tx = begin_mut_tx(&stdb);
482+
let plan = ponder_migrate(&old, &new)?;
483+
let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger);
484+
let err = result.err().expect("removing a non-empty table should fail");
485+
assert!(
486+
err.to_string().contains("table contains data"),
487+
"error should mention that the table contains data, got: {err}"
488+
);
489+
Ok(())
490+
}
408491
}

crates/schema/src/auto_migrate.rs

Lines changed: 87 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,10 @@ pub enum AutoMigrateStep<'def> {
256256
/// Remove a row-level security query.
257257
RemoveRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),
258258

259+
/// Remove an empty table and all its sub-objects (indexes, constraints, sequences).
260+
/// Validated at execution time: fails if the table contains data.
261+
RemoveTable(<TableDef as ModuleDefLookup>::Key<'def>),
262+
259263
/// Change the column types of a table, in a layout compatible way.
260264
///
261265
/// This should be done before any new indices are added.
@@ -406,9 +410,6 @@ pub enum AutoMigrateError {
406410
#[error("Changing a unique constraint {constraint} requires a manual migration")]
407411
ChangeUniqueConstraint { constraint: RawIdentifier },
408412

409-
#[error("Removing the table {table} requires a manual migration")]
410-
RemoveTable { table: Identifier },
411-
412413
#[error("Changing the table type of table {table} from {type1:?} to {type2:?} requires a manual migration")]
413414
ChangeTableType {
414415
table: Identifier,
@@ -452,17 +453,22 @@ pub fn ponder_auto_migrate<'def>(old: &'def ModuleDef, new: &'def ModuleDef) ->
452453
let views_ok = auto_migrate_views(&mut plan);
453454
let tables_ok = auto_migrate_tables(&mut plan);
454455

455-
// Our diffing algorithm will detect added constraints / indexes / sequences in new tables, we use this to filter those out.
456-
// They're handled by adding the root table.
457-
let new_tables: HashSet<&Identifier> = diff(plan.old, plan.new, ModuleDef::tables)
458-
.filter_map(|diff| match diff {
459-
Diff::Add { new } => Some(&new.name),
460-
_ => None,
461-
})
462-
.collect();
463-
let indexes_ok = auto_migrate_indexes(&mut plan, &new_tables);
464-
let sequences_ok = auto_migrate_sequences(&mut plan, &new_tables);
465-
let constraints_ok = auto_migrate_constraints(&mut plan, &new_tables);
456+
// Filter out sub-objects of added/removed tables — they're handled by AddTable/RemoveTable.
457+
let (new_tables, removed_tables): (HashSet<&Identifier>, HashSet<&Identifier>) =
458+
diff(plan.old, plan.new, ModuleDef::tables).fold(
459+
(HashSet::new(), HashSet::new()),
460+
|(mut added, mut removed), d| {
461+
match d {
462+
Diff::Add { new } => { added.insert(&new.name); }
463+
Diff::Remove { old } => { removed.insert(&old.name); }
464+
Diff::MaybeChange { .. } => {}
465+
}
466+
(added, removed)
467+
},
468+
);
469+
let indexes_ok = auto_migrate_indexes(&mut plan, &new_tables, &removed_tables);
470+
let sequences_ok = auto_migrate_sequences(&mut plan, &new_tables, &removed_tables);
471+
let constraints_ok = auto_migrate_constraints(&mut plan, &new_tables, &removed_tables);
466472
// IMPORTANT: RLS auto-migrate steps must come last,
467473
// since they assume that any schema changes, like adding or dropping tables,
468474
// have already been reflected in the database state.
@@ -630,11 +636,10 @@ fn auto_migrate_tables(plan: &mut AutoMigratePlan<'_>) -> Result<()> {
630636
plan.steps.push(AutoMigrateStep::AddTable(new.key()));
631637
Ok(())
632638
}
633-
// TODO: When we remove tables, we should also remove their dependencies, including row-level security.
634-
Diff::Remove { old } => Err(AutoMigrateError::RemoveTable {
635-
table: old.name.clone(),
639+
Diff::Remove { old } => {
640+
plan.steps.push(AutoMigrateStep::RemoveTable(old.key()));
641+
Ok(())
636642
}
637-
.into()),
638643
Diff::MaybeChange { old, new } => auto_migrate_table(plan, old, new),
639644
}
640645
})
@@ -927,7 +932,7 @@ fn ensure_old_ty_upgradable_to_new(
927932
}
928933
}
929934

930-
fn auto_migrate_indexes(plan: &mut AutoMigratePlan<'_>, new_tables: &HashSet<&Identifier>) -> Result<()> {
935+
fn auto_migrate_indexes(plan: &mut AutoMigratePlan<'_>, new_tables: &HashSet<&Identifier>, removed_tables: &HashSet<&Identifier>) -> Result<()> {
931936
diff(plan.old, plan.new, ModuleDef::indexes)
932937
.map(|index_diff| -> Result<()> {
933938
match index_diff {
@@ -938,6 +943,9 @@ fn auto_migrate_indexes(plan: &mut AutoMigratePlan<'_>, new_tables: &HashSet<&Id
938943
Ok(())
939944
}
940945
Diff::Remove { old } => {
946+
if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) {
947+
return Ok(()); // handled by RemoveTable
948+
}
941949
plan.steps.push(AutoMigrateStep::RemoveIndex(old.key()));
942950
Ok(())
943951
}
@@ -962,7 +970,7 @@ fn auto_migrate_indexes(plan: &mut AutoMigratePlan<'_>, new_tables: &HashSet<&Id
962970
.collect_all_errors()
963971
}
964972

965-
fn auto_migrate_sequences(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Identifier>) -> Result<()> {
973+
fn auto_migrate_sequences(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Identifier>, removed_tables: &HashSet<&Identifier>) -> Result<()> {
966974
diff(plan.old, plan.new, ModuleDef::sequences)
967975
.map(|sequence_diff| -> Result<()> {
968976
match sequence_diff {
@@ -975,6 +983,9 @@ fn auto_migrate_sequences(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Iden
975983
Ok(())
976984
}
977985
Diff::Remove { old } => {
986+
if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) {
987+
return Ok(()); // handled by RemoveTable
988+
}
978989
plan.steps.push(AutoMigrateStep::RemoveSequence(old.key()));
979990
Ok(())
980991
}
@@ -993,7 +1004,7 @@ fn auto_migrate_sequences(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Iden
9931004
.collect_all_errors()
9941005
}
9951006

996-
fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Identifier>) -> Result<()> {
1007+
fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Identifier>, removed_tables: &HashSet<&Identifier>) -> Result<()> {
9971008
diff(plan.old, plan.new, ModuleDef::constraints)
9981009
.map(|constraint_diff| -> Result<()> {
9991010
match constraint_diff {
@@ -1010,6 +1021,9 @@ fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Id
10101021
}
10111022
}
10121023
Diff::Remove { old } => {
1024+
if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) {
1025+
return Ok(()); // handled by RemoveTable
1026+
}
10131027
plan.steps.push(AutoMigrateStep::RemoveConstraint(old.key()));
10141028
Ok(())
10151029
}
@@ -1503,7 +1517,7 @@ mod tests {
15031517
let result = ponder_auto_migrate(&old_def, &new_def);
15041518

15051519
let apples = expect_identifier("Apples");
1506-
let bananas = expect_identifier("Bananas");
1520+
let _bananas = expect_identifier("Bananas");
15071521

15081522
let apples_name_unique_constraint = "Apples_name_key";
15091523

@@ -1711,10 +1725,8 @@ mod tests {
17111725
AutoMigrateError::ChangeTableType { table, type1, type2 } => table == &apples && type1 == &TableType::User && type2 == &TableType::System
17121726
);
17131727

1714-
expect_error_matching!(
1715-
result,
1716-
AutoMigrateError::RemoveTable { table } => table == &bananas
1717-
);
1728+
// Note: RemoveTable is no longer an error — removing tables is now allowed
1729+
// for empty tables; the emptiness check happens at execution time in update.rs.
17181730

17191731
let apples_id_index = "Apples_id_idx_btree";
17201732
let accessor_old = expect_identifier("id_index");
@@ -2399,4 +2411,53 @@ mod tests {
23992411

24002412
ponder_auto_migrate(&old, &new).expect("same event flag should succeed");
24012413
}
2414+
2415+
#[test]
2416+
fn remove_table_produces_step() {
2417+
let old = create_module_def(|builder| {
2418+
builder
2419+
.build_table_with_new_type("Keep", ProductType::from([("id", AlgebraicType::U64)]), true)
2420+
.with_access(TableAccess::Public)
2421+
.finish();
2422+
builder
2423+
.build_table_with_new_type("Drop", ProductType::from([("id", AlgebraicType::U64)]), true)
2424+
.with_access(TableAccess::Public)
2425+
.finish();
2426+
});
2427+
let new = create_module_def(|builder| {
2428+
builder
2429+
.build_table_with_new_type("Keep", ProductType::from([("id", AlgebraicType::U64)]), true)
2430+
.with_access(TableAccess::Public)
2431+
.finish();
2432+
});
2433+
2434+
let plan = ponder_auto_migrate(&old, &new).expect("removing a table should produce a valid plan");
2435+
assert!(
2436+
plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveTable(name) if &name[..] == "Drop")),
2437+
"plan should contain a RemoveTable step for 'Drop'"
2438+
);
2439+
}
2440+
2441+
#[test]
2442+
fn remove_table_does_not_produce_orphan_sub_object_steps() {
2443+
let old = create_module_def(|builder| {
2444+
builder
2445+
.build_table_with_new_type("Drop", ProductType::from([("id", AlgebraicType::U64)]), true)
2446+
.with_unique_constraint(0)
2447+
.with_index(btree(0), "Drop_id_idx")
2448+
.with_access(TableAccess::Public)
2449+
.finish();
2450+
});
2451+
let new = create_module_def(|_builder| {});
2452+
2453+
let plan = ponder_auto_migrate(&old, &new).expect("removing a table should produce a valid plan");
2454+
assert!(
2455+
!plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveIndex(_))),
2456+
"plan should not contain orphan RemoveIndex steps for a removed table"
2457+
);
2458+
assert!(
2459+
!plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveConstraint(_))),
2460+
"plan should not contain orphan RemoveConstraint steps for a removed table"
2461+
);
2462+
}
24022463
}

crates/schema/src/auto_migrate/formatter.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ fn format_step<F: MigrationFormatter>(
4242
// So we must recompute it and send any updates to clients.
4343
// No need to include this step in the formatted plan.
4444
AutoMigrateStep::UpdateView(_) => Ok(()),
45+
AutoMigrateStep::RemoveTable(t) => {
46+
let table_def: &TableDef = plan.old.expect_lookup(*t);
47+
f.format_remove_table(&table_def.name)
48+
}
4549
AutoMigrateStep::AddTable(t) => {
4650
let table_info = extract_table_info(*t, plan)?;
4751
f.format_add_table(&table_info)
@@ -140,6 +144,7 @@ pub enum Action {
140144
pub trait MigrationFormatter {
141145
fn format_header(&mut self) -> io::Result<()>;
142146
fn format_add_table(&mut self, table_info: &TableInfo) -> io::Result<()>;
147+
fn format_remove_table(&mut self, table_name: &Identifier) -> io::Result<()>;
143148
fn format_view(&mut self, view_info: &ViewInfo, action: Action) -> io::Result<()>;
144149
fn format_index(&mut self, index_info: &IndexInfo, action: Action) -> io::Result<()>;
145150
fn format_constraint(&mut self, constraint_info: &ConstraintInfo, action: Action) -> io::Result<()>;

crates/schema/src/auto_migrate/termcolor_formatter.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use spacetimedb_sats::algebraic_type::fmt::fmt_algebraic_type;
66
use termcolor::{Buffer, Color, ColorChoice, ColorSpec, WriteColor};
77

88
use crate::auto_migrate::formatter::ViewInfo;
9+
use crate::identifier::Identifier;
910

1011
use super::formatter::{
1112
AccessChangeInfo, Action, ColumnChange, ColumnChanges, ConstraintInfo, IndexInfo, MigrationFormatter, NewColumns,
@@ -229,6 +230,14 @@ impl MigrationFormatter for TermColorFormatter {
229230
self.write_line("")
230231
}
231232

233+
fn format_remove_table(&mut self, table_name: &Identifier) -> io::Result<()> {
234+
self.write_action_prefix(&Action::Removed)?;
235+
self.buffer.write_all(b" table: ")?;
236+
self.write_colored(table_name, Some(self.colors.table_name), true)?;
237+
self.buffer.write_all(b"\n")?;
238+
self.write_line("")
239+
}
240+
232241
fn format_view(&mut self, view: &ViewInfo, action: Action) -> io::Result<()> {
233242
self.write_indent()?;
234243
self.buffer.write_all("▸ ".to_string().as_bytes())?;

crates/smoketests/tests/smoketests/cli/publish.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,11 @@ fn cli_can_publish_automigration_change_with_delete_data_always_and_yes_break_cl
166166
}
167167

168168
#[test]
169-
fn cli_cannot_publish_breaking_change_without_flag() {
169+
fn cli_can_publish_remove_empty_table() {
170170
migration_test(
171-
"breaking-change-test",
171+
"remove-empty-table-test",
172172
&["--build-options=--features test-remove-table"],
173-
false,
173+
true,
174174
);
175175
}
176176

0 commit comments

Comments
 (0)