feat: Allow dropping empty tables during auto-migration#4593
feat: Allow dropping empty tables during auto-migration#4593Ludv1gL wants to merge 1 commit intoclockworklabs:masterfrom
Conversation
crates/core/src/db/update.rs
Outdated
|
|
||
| let mut tx = begin_mut_tx(&stdb); | ||
| let plan = ponder_migrate(&old, &new)?; | ||
| let _ = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; |
There was a problem hiding this comment.
Check for UpdateResult::Success here.
There was a problem hiding this comment.
Done. Now asserting RequiresClientDisconnect (since RemoveTable now emits DisconnectAllUsers).
| let plan = ponder_migrate(&old, &new)?; | ||
| let _ = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; | ||
|
|
||
| assert!(stdb.table_id_from_name_mut(&tx, "droppable")?.is_none()); |
There was a problem hiding this comment.
Check pending_schema_changes here.
There was a problem hiding this comment.
Done — asserting exactly 1 pending schema change (TableRemoved) from drop_table.
| assert!( | ||
| err.to_string().contains("table contains data"), | ||
| "error should mention that the table contains data, got: {err}" | ||
| ); |
There was a problem hiding this comment.
check that pending_schema_changes is empty here
There was a problem hiding this comment.
Done — asserting pending_schema_changes has len 1 (the TableRemoved entry from drop_table).
crates/schema/src/auto_migrate.rs
Outdated
| let indexes_ok = auto_migrate_indexes(&mut plan, &new_tables); | ||
| let sequences_ok = auto_migrate_sequences(&mut plan, &new_tables); | ||
| let constraints_ok = auto_migrate_constraints(&mut plan, &new_tables); | ||
| // Filter out sub-objects of added/removed tables — they're handled by AddTable/RemoveTable. |
There was a problem hiding this comment.
| // Filter out sub-objects of added/removed tables — they're handled by AddTable/RemoveTable. | |
| // Filter out sub-objects of added/removed tables — they're handled by `AddTable`/`RemoveTable`. |
There was a problem hiding this comment.
Done — backticked both AddTable and RemoveTable.
crates/schema/src/auto_migrate.rs
Outdated
| if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { | ||
| return Ok(()); // handled by RemoveTable | ||
| } | ||
| plan.steps.push(AutoMigrateStep::RemoveIndex(old.key())); |
There was a problem hiding this comment.
Please make this align with the code for Diff::Add and add a closure that takes the hash set as a parameter.
There was a problem hiding this comment.
Done — switched to negated-if style (no early return) to align with the Diff::Add pattern. Applied to all three: indexes, sequences, and constraints.
crates/schema/src/auto_migrate.rs
Outdated
| if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { | ||
| return Ok(()); // handled by RemoveTable | ||
| } |
There was a problem hiding this comment.
Let's also use a closure here and style without the early return as in the Add case.
crates/schema/src/auto_migrate.rs
Outdated
| if removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { | ||
| return Ok(()); // handled by RemoveTable | ||
| } | ||
| plan.steps.push(AutoMigrateStep::RemoveConstraint(old.key())); |
There was a problem hiding this comment.
ditto re closure and early return
crates/schema/src/auto_migrate.rs
Outdated
| assert!( | ||
| plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveTable(name) if &name[..] == "Drop")), | ||
| "plan should contain a RemoveTable step for 'Drop'" | ||
| ); |
There was a problem hiding this comment.
I'd prefer to see an explicit list here for this test to also capture the disconnect all clients step.
There was a problem hiding this comment.
Done — now using assert_eq\!(plan.steps, vec\![RemoveTable(&drop_table), DisconnectAllUsers]) for explicit step verification.
crates/schema/src/auto_migrate.rs
Outdated
| assert!( | ||
| !plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveIndex(_))), | ||
| "plan should not contain orphan RemoveIndex steps for a removed table" | ||
| ); | ||
| assert!( | ||
| !plan.steps.iter().any(|s| matches!(s, AutoMigrateStep::RemoveConstraint(_))), | ||
| "plan should not contain orphan RemoveConstraint steps for a removed table" | ||
| ); |
There was a problem hiding this comment.
I'd like to see an explicit list here too.
There was a problem hiding this comment.
Done — same explicit assert_eq\! on the full steps vec.
| false, | ||
| true, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why was this test changed?
There was a problem hiding this comment.
This test was originally cli_cannot_publish_breaking_change_without_flag — it tested that removing a table fails during publish. Now that empty table removal succeeds via auto-migration, the test was updated to cli_can_publish_remove_empty_table and expects success. The smoke test module starts with empty tables (fresh publish), so the removal succeeds as expected.
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>
Ludv1gL
force-pushed
the
feat/allow-drop-empty-tables
branch
from
e35fcdd to
e45d973
Compare
2 participants
Summary
AutoMigrateError::RemoveTable, forcing--delete-datawhich nukes the entire databasespacetime publishChanges
auto_migrate.rs: Replace hardRemoveTableerror withAutoMigrateStep::RemoveTable. Computeremoved_tablesset (same pattern asnew_tables) and pass toauto_migrate_indexes/auto_migrate_sequences/auto_migrate_constraintsto skip sub-object diffs for removed tables (cascade handled bydrop_table)update.rs: ExecuteRemoveTablestep — O(1) emptiness check viatable_row_count_mut, thendrop_table. Fails with clear message if table has dataformatter.rs/termcolor_formatter.rs: Addformat_remove_tabletoMigrationFormattertrait + implementationpublish.rs(smoketests): Update existing test — removing empty table now succeeds without flagsSafety
MutTx— no window for concurrent insertsdrop_tablealready handles removing all indexes, constraints, and sequences for the tableRemoveIndex/RemoveConstraint/RemoveSequencestepsExample error output (non-empty table)
Test plan
cargo check -p spacetimedb-schema -p spacetimedb-corepassesauto_migrateschema tests pass (2 new:remove_table_produces_step,remove_table_does_not_produce_orphan_sub_object_steps)updateexecution tests pass (2 new:remove_empty_table_succeeds,remove_nonempty_table_fails)auto_migration_errorstest (no longer expectsRemoveTableerror)cli_can_publish_remove_empty_tableexpects success🤖 Generated with Claude Code